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);
_______________________________________________
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
