Merge lp:~widelands-dev/widelands/bug-1624216-horsepocalypse into lp:widelands

[GunChleoc]

When a warehouse is destroyed, limit maximum number of fleeing units per unit type to 500.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1581. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/174007207.
Appveyor build 1419. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1624216_horsepocalypse-1419.

[Klaus Halfmann (klaus-halfmann)]

Code looks good to me, no Idea if that inline comment will help.

[GunChleoc (gunchleoc)]

Looks like your inline comment wasn't saved :(

[xxx-deleted (janosch-peters-deactivatedaccount)]

It could be coded a bit more elegantly(see my comments), but apart from that it LGTM. However, I did not test it because I dont know a straight forward way to get a warehous with 500+ workers.

[GunChleoc (gunchleoc)]

Thanks for the review, the code looks much better now :)

There is a savegame attached to the bug that you can use for testing - attack the headquarters to the north.

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

HTTP Error 500: Internal Server Error

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1607. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/175655541.
Appveyor build 1445. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1624216_horsepocalypse-1445.

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

('The read operation timed out',)

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1607. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/175655541.
Appveyor build 1445. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1624216_horsepocalypse-1445.

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

('The read operation timed out',)

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1607. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/175655541.
Appveyor build 1445. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1624216_horsepocalypse-1445.

[Notabilis (notabilis27)]

Assuming this branch waits for a tester: Code looks good to me and testing the provided savegame with trunk and this branch it seems to work as planned.

[GunChleoc (gunchleoc)]

Thanks!

@bunnybot merge

=== modified file 'src/logic/map_objects/tribes/warehouse.cc'
--- src/logic/map_objects/tribes/warehouse.cc	2016-11-25 09:37:24 +0000
+++ src/logic/map_objects/tribes/warehouse.cc	2016-12-04 13:14:16 +0000
@@ -53,6 +53,7 @@
 namespace {
 
 static const uint32_t WORKER_WITHOUT_COST_SPAWN_INTERVAL = 2500;
+constexpr int kFleeingUnitsCap = 500;
 
 // Goes through the list and removes all workers that are no longer in the
 // game.
@@ -529,24 +530,23 @@
 		portdock_ = nullptr;
 	}
 
-	// This will empty the stock and launch all workers including incorporated
-	// ones.
+	// This will launch all workers including incorporated ones up to kFleeingUnitsCap and then empty
+	// the stock.
 	if (upcast(Game, game, &egbase)) {
 		const WareList& workers = get_workers();
 		for (DescriptionIndex id = 0; id < workers.get_nrwareids(); ++id) {
-			const uint32_t stock = workers.stock(id);
-			// Separate behaviour for the case of loading the game
-			// (which does save/destroy/reload) and simply destroying ingame
+			// If the game is running, have the workers flee the warehouse.
 			if (game->is_loaded()) {
-				// This game is really running
-				for (uint32_t i = 0; i < stock; ++i) {
+				// We have kFleeingUnitsCap to make sure that we won't flood the map with carriers etc.
+				Quantity stock = workers.stock(id);
+				for (Quantity i = 0; i < stock && i < kFleeingUnitsCap; ++i) {
 					launch_worker(*game, id, Requirements()).start_task_leavebuilding(*game, true);
 				}
-				assert(!incorporated_workers_.count(id) || incorporated_workers_[id].empty());
-			} else {
-				// We are in the load-game sequence...
-				remove_workers(id, stock);
 			}
+			// Make sure that all workers are gone
+			remove_workers(id, workers.stock(id));
+			assert(!game->is_loaded() ||
+			       (!incorporated_workers_.count(id) || incorporated_workers_[id].empty()));
 		}
 	}
 	incorporated_workers_.clear();