Merge lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

Proposed by GunChleoc
Status: Merged
Merged at revision: 9004
Proposed branch: lp:~widelands-dev/widelands/bug-1818073-worker
Merge into: lp:widelands
Diff against target: 30 lines (+11/-9)
1 file modified
src/logic/map_objects/tribes/worker.cc (+11/-9)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1818073-worker
Reviewer Review Type Date Requested Status
GunChleoc Approve
Benedikt Straub code Approve
Toni Förster Approve
hessenfarmer Needs Fixing
Review via email: mp+363851@code.launchpad.net

Commit message

Reinstate mining behavior from r8823.

To post a comment you must log in.
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

From my understanding this code results in a penalty of 4 instead of 0 for totally depleted or wrong fields.

review: Needs Fixing
Revision history for this message
Toni Förster (stonerl) wrote :

@hessenfarmer how do you come to that conclusion? 8*0 is 0 not 4

review: Approve
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

Because of the first else if statement, 0 is also less than 4. So we need to have 0 in an own if statement as it has been in rev8823 or we need the !0 in every if statement

Revision history for this message
Toni Förster (stonerl) wrote :

stupid me...

9003. By Toni Förster

penalty is 0 if amount is 0

Revision history for this message
Toni Förster (stonerl) wrote :

Took care of it in r9003.

Revision history for this message
GunChleoc (gunchleoc) wrote :

The fix in r9003 is wrong (see my in-line comment)

review: Needs Fixing
Revision history for this message
GunChleoc (gunchleoc) wrote :

P.S. the math is exactly the same, but adding 0 is really awkward code. So, my version was better while it does the exact same thing.

Revision history for this message
Toni Förster (stonerl) wrote :

We want it to be 0 right? I just reverted to how it was in r8823.

Yours did set it to 4 as hessenfarmer said, 4 ways to fix it:

(1)

if (amount != 0 && amount <= 2) {
    totalchance += 6;
} else if (amount != 0 && amount <= 4) {
    totalchance += 4;
} else if (amount != 0 && amount <= 6) {
    totalchance += 2;
}

(2)

if (amount == 0) {
    totalchance = totalchance;
} else if (amount <= 2) {
    totalchance += 6;
} else if (amount != 0 && amount <= 4) {
    totalchance += 4;
} else if (amount != 0 && amount <= 6) {
    totalchance += 2;
}

(3)
if (amount == 0) {
    totalchance = 0;
} else if (amount <= 2) {
    totalchance += 6;
} else if (amount != 0 && amount <= 4) {
    totalchance += 4;
} else if (amount != 0 && amount <= 6) {
    totalchance += 2;
}

(4)

if (amount == 0) {
    // don't do nothing
} else if (amount <= 2) {
    totalchance += 6;
} else if (amount != 0 && amount <= 4) {
    totalchance += 4;
} else if (amount != 0 && amount <= 6) {
    totalchance += 2;
}

Revision history for this message
Benedikt Straub (nordfriese) wrote :

All four are just bad coding style IMHO. Setting something to itself or doing += 0 is ugly, and empty conditional blocks and repetitive conditions should be avoided.

Suggestion:

if (amount > 0) {
    if (amount <= 2) {
        totalchance += 6;
    } else if (amount <= 4) {
        totalchance += 4;
    } else if (amount <= 6) {
        totalchance += 2;
    }
}

9004. By Toni Förster

don't do += 0

Revision history for this message
Toni Förster (stonerl) wrote :

Or we simply add an else see r9004.

Revision history for this message
Benedikt Straub (nordfriese) wrote :

Setting it to a fixed value won't work, because we iterate through all reachable fields and sum up the individual values.

Revision history for this message
Toni Förster (stonerl) wrote :

Are you talking about r9004? Where do I set what to a fixed value?

Revision history for this message
Toni Förster (stonerl) wrote :

Okay, today I'm especially stubborn.

if (amount == 0) {
    totalchance += 1;
} else if (amount <= 2) {
    totalchance += 6;
} else if (amount <= 4) {
    totalchance += 4;
} else if (amount <= 6) {
    totalchance += 2;
} else {
  totalchance += 8 * amount;
}

Revision history for this message
Benedikt Straub (nordfriese) wrote :

Diff line 18. totalchance is a global counter and must not be reset.

Also, you now add the (8 * amount) only if (amount > 6), whereas it was added for all values of amount before, changing the result for (0 < amount <= 6).

9005. By Toni Förster

use nordfrieses suggestion

Revision history for this message
Toni Förster (stonerl) wrote :

I'm terrible sorry. I should have stayed AFK today :-) Did what you suggested Benedikt.

Revision history for this message
Benedikt Straub (nordfriese) wrote :

No problem. I find small reviews without any issues rather boring ;)
This should work as desired now.

review: Approve (code)
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4553. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/500508131.
Appveyor build 4340. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1818073_worker-4340.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Thanks for fixing

@bunnybot merge

review: Approve
Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4553. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/500508131.

Revision history for this message
Toni Förster (stonerl) wrote :

Since bunnybot refused to merge, I had another Idea, currently we enter the if-clause for every amount > 0. But we only want it to enter when: 0 < amount <= 6:

if (amount > 0 && amount <= 6) {
 if (amount <= 2) {
  totalchance += 6;
 } else if (amount <= 4) {
  totalchance += 4;
 } else {
  totalchance += 2;
 }
}

Wouldn't this be better?

Revision history for this message
GunChleoc (gunchleoc) wrote :

Arithmetic comparisons are very fast - I don't know if we would gain any performance here really. We also get an additional comparison if amount <= 2, so theoretically we only gain 1 comparison on average here while making the code harder to read.

Revision history for this message
Toni Förster (stonerl) wrote :

Well, then we let it be as it is. :-)

@bunnybot merge force

9006. By Toni Förster

remove trailing whitespace

9007. By Toni Förster

revert accidentally added code changes

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/logic/map_objects/tribes/worker.cc'
2--- src/logic/map_objects/tribes/worker.cc 2019-02-27 17:19:00 +0000
3+++ src/logic/map_objects/tribes/worker.cc 2019-03-02 11:21:24 +0000
4@@ -138,15 +138,17 @@
5 totalchance += 8 * amount;
6
7 // Add penalty for fields that are running out
8- if (amount == 0)
9- // we already know it's completely empty, so punish is less
10- totalchance += 1;
11- else if (amount <= 2)
12- totalchance += 6;
13- else if (amount <= 4)
14- totalchance += 4;
15- else if (amount <= 6)
16- totalchance += 2;
17+ // Except for totally depleted fields or wrong ressource fields
18+ // if we already know there is no ressource (left) we won't mine there
19+ if (amount > 0) {
20+ if (amount <= 2) {
21+ totalchance += 6;
22+ } else if (amount <= 4) {
23+ totalchance += 4;
24+ } else if (amount <= 6) {
25+ totalchance += 2;
26+ }
27+ }
28 } while (mr.advance(*map));
29
30 if (totalres == 0) {

Subscribers

People subscribed via source and target branches

to status/vote changes: