[Widelands-dev] [Merge] lp:~widelands-dev/widelands/frisian_music into lp:widelands

2019-03-01 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 4546. State: failed. Details: 
https://travis-ci.org/widelands/widelands/builds/499966184.
Appveyor build 4333. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_frisian_music-4333.
-- 
https://code.launchpad.net/~widelands-dev/widelands/frisian_music/+merge/350429
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/frisian_music into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

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

2019-03-01 Thread GunChleoc 
GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1818073-worker 
into lp:widelands.

Commit message:
Reinstate mining behavior from r8823.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1818073 in widelands: "changes to worker.cc erroneously reverted"
  https://bugs.launchpad.net/widelands/+bug/1818073

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands.
=== modified file 'src/logic/map_objects/tribes/worker.cc'
--- src/logic/map_objects/tribes/worker.cc	2019-02-27 17:19:00 +
+++ src/logic/map_objects/tribes/worker.cc	2019-03-01 12:23:04 +
@@ -138,15 +138,15 @@
 		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)
+		// 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 && amount <= 2) {
 			totalchance += 6;
-		else if (amount <= 4)
+		} else if (amount <= 4) {
 			totalchance += 4;
-		else if (amount <= 6)
+		} else if (amount <= 6) {
 			totalchance += 2;
+		}
 	} while (mr.advance(*map));
 
 	if (totalres == 0) {

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread hessenfarmer 
Review: Needs Fixing

>From my understanding this code results in a penalty of 4 instead of 0 for 
>totally depleted or wrong fields.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Toni Förster 
Review: Approve

@hessenfarmer how do you come to that conclusion? 8*0 is 0 not 4
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread hessenfarmer 
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
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Toni Förster 
stupid me...
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Toni Förster 
Took care of it in r9003.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818227-replay-desync into lp:widelands

2019-03-01 Thread GunChleoc 
GunChleoc has proposed merging 
lp:~widelands-dev/widelands/bug-1818227-replay-desync into lp:widelands.

Commit message:
Move running of win condition scripts back to Game::init_newgame to fix 
desyncing replays.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1818227 in widelands: "Replays desync on replay start"
  https://bugs.launchpad.net/widelands/+bug/1818227

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1818227-replay-desync/+merge/363871
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1818227-replay-desync into lp:widelands.
=== modified file 'src/logic/game.cc'
--- src/logic/game.cc	2019-02-26 05:56:01 +
+++ src/logic/game.cc	2019-03-01 16:40:58 +
@@ -314,21 +314,10 @@
 
 	// Check for win_conditions
 	if (!settings.scenario) {
-		win_condition_script_ = settings.win_condition_script;
-		std::unique_ptr table(lua().run_script(win_condition_script_));
+		std::unique_ptr table(lua().run_script(settings.win_condition_script));
 		table->do_not_warn_about_unaccessed_keys();
 		win_condition_displayname_ = table->get_string("name");
-		// We run the actual win condition from InteractiveGameBase::start() to prevent a pure black
-		// screen while the game is being started - we can display a message there.
-	} else {
-		win_condition_displayname_ = "Scenario";
-	}
-}
 
-void Game::run_win_condition() {
-	if (!win_condition_script_.empty()) {
-		std::unique_ptr table(lua().run_script(win_condition_script_));
-		table->do_not_warn_about_unaccessed_keys();
 		// Run separate initialization function if it is there.
 		if (table->has_key("init")) {
 			std::unique_ptr cr = table->get_coroutine("init");
@@ -336,7 +325,8 @@
 		}
 		std::unique_ptr cr = table->get_coroutine("func");
 		enqueue_command(new CmdLuaCoroutine(get_gametime() + 100, std::move(cr)));
-		win_condition_script_ = "";
+	} else {
+		win_condition_displayname_ = "Scenario";
 	}
 }
 

=== modified file 'src/logic/game.h'
--- src/logic/game.h	2019-02-24 22:50:04 +
+++ src/logic/game.h	2019-03-01 16:40:58 +
@@ -177,9 +177,6 @@
 	void init_newgame(UI::ProgressWindow* loader_ui, const GameSettings&);
 	void init_savegame(UI::ProgressWindow* loader_ui, const GameSettings&);
 
-	/// Run the win condition that is defined by win_condition_script_
-	void run_win_condition();
-
 	enum StartGameType { NewSPScenario, NewNonScenario, Loaded, NewMPScenario };
 
 	bool run(UI::ProgressWindow* loader_ui,
@@ -407,7 +404,6 @@
 
 	/// For save games and statistics generation
 	std::string win_condition_displayname_;
-	std::string win_condition_script_;
 	bool replay_;
 };
 

=== modified file 'src/wui/interactive_gamebase.cc'
--- src/wui/interactive_gamebase.cc	2019-02-27 17:19:00 +
+++ src/wui/interactive_gamebase.cc	2019-03-01 16:40:58 +
@@ -156,8 +156,6 @@
 }
 
 void InteractiveGameBase::start() {
-	game().run_win_condition();
-
 	// Multiplayer games don't save the view position, so we go to the starting position instead
 	if (is_multiplayer()) {
 		Widelands::PlayerNumber pln = player_number();

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread GunChleoc 
Review: Needs Fixing

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

Diff comments:

> === modified file 'src/logic/map_objects/tribes/worker.cc'
> --- src/logic/map_objects/tribes/worker.cc2019-02-27 17:19:00 +
> +++ src/logic/map_objects/tribes/worker.cc2019-03-01 16:32:50 +
> @@ -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)
> + // 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) {
> + totalchance += 0;

This line does not do anything whatsoever. x + 0 = x.

> + } else if (amount <= 2) {
>   totalchance += 6;
> - else if (amount <= 4)
> + } else if (amount <= 4) {
>   totalchance += 4;
> - else if (amount <= 6)
> + } else if (amount <= 6) {
>   totalchance += 2;
> + }
>   } while (mr.advance(*map));
>  
>   if (totalres == 0) {


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread 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.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Toni Förster 
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;
}
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Benedikt Straub 
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;
}
}
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818227-replay-desync into lp:widelands

2019-03-01 Thread Toni Förster 
Review: Needs Fixing

Now we have the problem with 4 Minutes of writing scripting data for big maps 
like 'Magic Mountain', again.

Writing Scripting Data ... took 116673ms
.
.
.
Writing Scripting Data ... took 97252ms

-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818227-replay-desync/+merge/363871
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818227-replay-desync.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Toni Förster 
Or we simply add an else see r9004.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Benedikt Straub 
Setting it to a fixed value won't work, because we iterate through all 
reachable fields and sum up the individual values.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Toni Förster 
Are you talking about r9004? Where do I set what to a fixed value?


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Toni Förster 
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;
}
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Benedikt Straub 
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).
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Toni Förster 
I'm terrible sorry. I should have stayed AFK today :-) Did what you suggested 
Benedikt.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818227-replay-desync into lp:widelands

2019-03-01 Thread kaputtnik 
Please revert the changes made in r8993, otherwise i fear we will never get a 
new official release.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818227-replay-desync/+merge/363871
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818227-replay-desync.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818073-worker into lp:widelands

2019-03-01 Thread Benedikt Straub 
Review: Approve code

No problem. I find small reviews without any issues rather boring ;)
This should work as desired now.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818227-replay-desync into lp:widelands

2019-03-01 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 4550. State: errored. Details: 
https://travis-ci.org/widelands/widelands/builds/500454380.
Appveyor build 4337. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1818227_replay_desync-4337.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818227-replay-desync/+merge/363871
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818227-replay-desync.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

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

2019-03-01 Thread bunnybot 
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.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1818073-worker/+merge/363851
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1818073-worker.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp