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

[GunChleoc]

Reinstate mining behavior from r8823.

[hessenfarmer (stephan-lutz)]

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

review: Needs Fixing

[Toni Förster (stonerl)]

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

review: Approve

[hessenfarmer (stephan-lutz)]

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

[Toni Förster (stonerl)]

stupid me...

[Toni Förster (stonerl)]

Took care of it in r9003.

[GunChleoc (gunchleoc)]

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

review: Needs Fixing

[GunChleoc (gunchleoc)]

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.

[Toni Förster (stonerl)]

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;
}

[Benedikt Straub (nordfriese)]

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;
    }
}

[Toni Förster (stonerl)]

Or we simply add an else see r9004.

[Benedikt Straub (nordfriese)]

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

[Toni Förster (stonerl)]

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

[Toni Förster (stonerl)]

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;
}

[Benedikt Straub (nordfriese)]

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).

[Toni Förster (stonerl)]

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

[Benedikt Straub (nordfriese)]

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

review: Approve (code)

[bunnybot (widelandsofficial)]

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.

[GunChleoc (gunchleoc)]

Thanks for fixing

@bunnybot merge

review: Approve

[bunnybot (widelandsofficial)]

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.

[Toni Förster (stonerl)]

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?

[GunChleoc (gunchleoc)]

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.

[Toni Förster (stonerl)]

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

@bunnybot merge force

=== modified file 'src/logic/map_objects/tribes/worker.cc'
--- src/logic/map_objects/tribes/worker.cc	2019-02-27 17:19:00 +0000
+++ src/logic/map_objects/tribes/worker.cc	2019-03-02 11:21:24 +0000
@@ -138,15 +138,17 @@
 		totalchance += 8 * amount;
 
 		// Add penalty for fields that are running out
-		if (amount == 0)
-			// we already know it's completely empty, so punish is less
-			totalchance += 1;
-		else if (amount <= 2)
-			totalchance += 6;
-		else if (amount <= 4)
-			totalchance += 4;
-		else if (amount <= 6)
-			totalchance += 2;
+		// Except for totally depleted fields or wrong ressource fields
+		// if we already know there is no ressource (left) we won't mine there
+		if (amount > 0) {
+			if (amount <= 2) {
+				totalchance += 6;
+			} else if (amount <= 4) {
+				totalchance += 4;
+			} else if (amount <= 6) {
+				totalchance += 2;
+			}
+		}
 	} while (mr.advance(*map));
 
 	if (totalres == 0) {