Merge lp:~widelands-dev/widelands/bug-1827033-shipping-algorithm into lp:widelands

[Benedikt Straub]

Fix an assert fail because of nullptr destinations in the shipping algorithm

I have a feeling that this only masks the real issue. Portdock code first removes items that don´t have a portdock as destination and implicitly re-adds them at once. This causes *nullptr to be passed as an argument, resulting in the assert fail.
This branch simply prevents this situation by skipping something that doesn´t work anyway, so it shouldn´t have undesired side-effects. Someone who knows (or wrote) that code should check why the unallowed elements are adressed to a portdock when they should not be and therefore re-add themselves...

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4896. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/528437763.
Appveyor build 4677. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1827033_shipping_algorithm-4677.

[kaputtnik (franku)]

I am in favor to revert the changes that caused this failure... although the 'old' shipping algorithm isn't perfect.

[Benedikt Straub (nordfriese)]

Apart from this bug, which happens quite often, no unusual behaviour was observed in shipping so far. I would vote to merge this symptomatic fix, and if more problems are found, to revert or rewrite the algorithm.

[kaputtnik (franku)]

> no unusual behaviour was observed in shipping so far.

I think it is very unusual that a ship navigate to a port, unload all wares and immediately load the same wares again to navigate the wares to the destination port. In the end this means the time a ware is transported will grow with the number of ports.

Shouldn't the question be: Does the new shipping algorithm add any features?

But i agree that this bug should fixed very soon. Test games on seafaring maps makes no fun until this is fixed.

[GunChleoc (gunchleoc)]

I have added a tiny code style tweak. I'll let some AIs dish it out on this branch now.

Sorry for dragging my heels on this one.

review: Approve

[GunChleoc (gunchleoc)]

> I think it is very unusual that a ship navigate to a port, unload all wares and immediately load the same wares again to navigate the wares to the destination port. In the end this means the time a ware is transported will grow with the number of ports.

This should definitely be fixed too

> Shouldn't the question be: Does the new shipping algorithm add any features?

The goal was to make ship transportation more efficient overall

[Benedikt Straub (nordfriese)]

> I think it is very unusual that a ship navigate to a port, unload all wares and immediately load the same wares again to navigate the wares to the destination port.

Wasn´t aware of that one yet, but that has to be fixed of course.
I think this won´t be possible without rewriting most of the shipping algorithm due to the sometimes strange and potentially very buggy codeflow. I will try that out soon.
This branch should be considered a provisional fix to make seafaring games on debug builds possible again without a crash every two minutes, and hopefully I can provide a more stable algorithm (with a different approach) to replace it sometime soon.

[kaputtnik (franku)]

This behavior applies also for workers (soldiers), as mentioned in the bug report at #3 (4. paragraph) and comment #6 (replay).

Lets have this then and create another bug report?

[GunChleoc (gunchleoc)]

@bunnybot merge

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 5062. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/537402325.
Appveyor build 4842. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1827033_shipping_algorithm-4842.

=== modified file 'src/economy/portdock.cc'
--- src/economy/portdock.cc	2019-04-24 15:11:23 +0000
+++ src/economy/portdock.cc	2019-05-26 10:11:35 +0000
@@ -368,26 +368,34 @@
 		}
 	}
 
+	// Decide where the arrived ship will go next
+	PortDock* next_port = fleet_->find_next_dest(game, ship, *this);
+	if (next_port) {
+		// Unload any wares/workers onboard the departing ship which are not favored by next dest
+		ship.unload_unfit_items(game, *this, *next_port);
+	}
+#ifndef NDEBUG
+	else {
+		assert(ship.get_nritems() == 0);
+	}
+#endif
+
 	// Check for items with invalid destination. TODO(ypopezios): Prevent invalid destinations
 	for (auto si_it = waiting_.begin(); si_it != waiting_.end(); ++si_it) {
 		if (!si_it->get_destination(game)) {
 			// Invalid destination. Carry the item back into the warehouse
+			// TODO(Nordfriese): This readds the invalid item to the list
 			si_it->set_location(game, warehouse_);
 			si_it->end_shipping(game);
 			si_it = waiting_.erase(si_it);
 		}
 	}
 
-	// Decide where the arrived ship will go next
-	PortDock* next_port = fleet_->find_next_dest(game, ship, *this);
 	if (!next_port) {
-		ship.set_destination(next_port);
+		ship.set_destination(nullptr);
 		return;  // no need to load anything
 	}
 
-	// Unload any wares/workers onboard the departing ship which are not favored by next dest
-	ship.unload_unfit_items(game, *this, *next_port);
-
 	// Then load the remaining capacity of the departing ship with relevant items
 	uint32_t remaining_capacity = ship.descr().get_capacity() - ship.get_nritems();
 
@@ -402,7 +410,8 @@
 
 	// Then load any wares/workers favored by the chosen destination
 	for (auto si_it = waiting_.begin(); si_it != waiting_.end() && remaining_capacity > 0; ++si_it) {
-		if (fleet_->is_path_favourable(*this, *next_port, *si_it->get_destination(game))) {
+		const PortDock* dest = si_it->get_destination(game);
+		if (dest && fleet_->is_path_favourable(*this, *next_port, *dest)) {
 			ship.add_item(game, *si_it);
 			si_it = waiting_.erase(si_it);
 			--remaining_capacity;

=== modified file 'src/logic/map_objects/tribes/ship.cc'
--- src/logic/map_objects/tribes/ship.cc	2019-05-11 13:48:12 +0000
+++ src/logic/map_objects/tribes/ship.cc	2019-05-26 10:11:35 +0000
@@ -788,7 +788,8 @@
 void Ship::unload_unfit_items(Game& game, PortDock& here, const PortDock& nextdest) {
 	size_t dst = 0;
 	for (ShippingItem& si : items_) {
-		if (fleet_->is_path_favourable(here, nextdest, *si.get_destination(game))) {
+		const PortDock* dest = si.get_destination(game);
+		if (dest && fleet_->is_path_favourable(here, nextdest, *dest)) {
 			items_[dst++] = si;
 		} else {
 			here.shipping_item_returned(game, si);