widelands-dev team mailing list archive

[Benedikt Straub <benedikt-straub@xxxxxx>]

Benedikt Straub has proposed merging lp:~widelands-dev/widelands/bug-1827033-shipping-algorithm into lp:widelands.

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

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1827033 in widelands: "Neptuns Revenge: Assert is_path_favourable,fleet.cc,764. bzr9088"
  https://bugs.launchpad.net/widelands/+bug/1827033

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1827033-shipping-algorithm/+merge/366959

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...
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1827033-shipping-algorithm into lp:widelands.

=== modified file 'src/economy/portdock.cc'
--- src/economy/portdock.cc	2019-04-24 15:11:23 +0000
+++ src/economy/portdock.cc	2019-05-05 12:42:44 +0000
@@ -368,6 +368,17 @@
 		}
 	}
 
+	// 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)) {
@@ -378,16 +389,11 @@
 		}
 	}
 
-	// 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 +408,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-03 08:01:07 +0000
+++ src/logic/map_objects/tribes/ship.cc	2019-05-05 12:42:44 +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);