Merge lp:~widelands-dev/widelands/bug-1727673 into lp:widelands

[TiborB]

This bug is situation when colonization ship is going to land a builder, but another builder is already on the port constructionsite (I dont know how it is possible) and request is thus nullptr. Sometimes it crashes.
The solution is to leave the builder on the ship.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2716. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/293854904.
Appveyor build 2529. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1727673-2529.

[GunChleoc (gunchleoc)]

Code LGTM, not tested.

What happens to the builder on the ship, will he be transported back and unloaded at a warehouse?

[TiborB (tiborb95)]

This is exactly the same situation as when port constructionsite disappears during unloading of wares/worker. So yes..

[GunChleoc (gunchleoc)]

I did some testing now. I don't get a crash in trunk, but the builder would disappear, while this branch will keep the extra builder on the Ship and return him to a port. So, definitely an improvement! I'll assume that the crash is fixed too and close the bug.

@bunnybot merge

[TiborB (tiborb95)]

BTW, can you tell where the first builder came from?

[GunChleoc (gunchleoc)]

I tested with a game I created myself, by building an expedition port on my own island and quickly connecting a road to a nearby warehouse. Two Lagoons is a good map for that.

=== modified file 'src/logic/map_objects/tribes/ship.cc'
--- src/logic/map_objects/tribes/ship.cc	2017-08-20 08:34:02 +0000
+++ src/logic/map_objects/tribes/ship.cc	2017-10-27 19:33:48 +0000
@@ -612,6 +612,10 @@
 	case ShipStates::kExpeditionColonizing: {
 		assert(!expedition_->seen_port_buildspaces.empty());
 		BaseImmovable* baim = map[expedition_->seen_port_buildspaces.front()].get_immovable();
+		// Following is a preparation for very rare situation, when colonizing port already have a
+		// worker (bug-1727673)
+		// We leave the worker on the ship then
+		bool leftover_builder = false;
 		if (baim) {
 			upcast(ConstructionSite, cs, baim);
 
@@ -644,6 +648,16 @@
 					break;
 				} else {
 					assert(worker);
+					// If constructionsite does not need worker anymore, we must leave it on the ship.
+					// Also we presume that he is on position 0
+					if (cs->get_builder_request() == nullptr) {
+						log("%2d: WARNING: Colonizing ship %s cannot unload the worker to new port at "
+						    "%3dx%3d because the request is no longer active\n",
+						    get_owner()->player_number(), shipname_.c_str(), cs->get_position().x,
+						    cs->get_position().y);
+						leftover_builder = true;
+						break;  // no more unloading (builder shoud be on position 0)
+					}
 					worker->set_economy(nullptr);
 					worker->set_location(cs);
 					worker->set_position(game, cs->get_position());
@@ -661,8 +675,8 @@
 			send_signal(game, "cancel_expedition");
 		}
 
-		if (items_.empty() || !baim) {            // we are done, either way
-			ship_state_ = ShipStates::kTransport;  // That's it, expedition finished
+		if (items_.empty() || !baim || leftover_builder) {  // we are done, either way
+			ship_state_ = ShipStates::kTransport;            // That's it, expedition finished
 
 			// Bring us back into a fleet and a economy.
 			init_fleet(game);
@@ -1002,13 +1016,14 @@
 	Bob::log_general_info(egbase);
 
 	molog("Ship belongs to fleet: %u\n destination: %s\n lastdock: %s\n",
-	      fleet_ ? fleet_->serial() : 0, (destination_.is_set()) ?
-	                                        (boost::format("%u (%d x %d)") % destination_.serial() %
-	                                         destination_.get(egbase)->get_positions(egbase)[0].x %
-	                                         destination_.get(egbase)->get_positions(egbase)[0].y)
-	                                           .str()
-	                                           .c_str() :
-	                                        "-",
+	      fleet_ ? fleet_->serial() : 0,
+	      (destination_.is_set()) ?
+	         (boost::format("%u (%d x %d)") % destination_.serial() %
+	          destination_.get(egbase)->get_positions(egbase)[0].x %
+	          destination_.get(egbase)->get_positions(egbase)[0].y)
+	            .str()
+	            .c_str() :
+	         "-",
 	      (lastdock_.is_set()) ?
 	         (boost::format("%u (%d x %d)") % lastdock_.serial() %
 	          lastdock_.get(egbase)->get_positions(egbase)[0].x %