Added inline comments concerning the NOCOM comments.
Diff comments:
>
> === modified file 'src/economy/portdock.cc'
> --- src/economy/portdock.cc 2018-04-16 07:03:12 +0000
> +++ src/economy/portdock.cc 2018-09-04 18:46:42 +0000
> @@ -281,44 +286,70 @@
> }
> }
>
> -void PortDock::update_shippingitem(Game& game,
> std::vector<ShippingItem>::iterator it) {
> +void PortDock::update_shippingitem(Game& game,
> std::list<ShippingItem>::iterator it) {
> it->update_destination(game, *this);
>
> - PortDock* dst = it->get_destination(game);
> + const PortDock* dst = it->get_destination(game);
> assert(dst != this);
>
> // Destination might have vanished or be in another economy altogether.
> if (dst && dst->get_economy() == get_economy()) {
> - set_need_ship(game, true);
> + if (ships_coming_ <= 0) {
> + set_need_ship(game, true);
> + }
> } else {
> it->set_location(game, warehouse_);
> it->end_shipping(game);
> *it = waiting_.back();
> waiting_.pop_back();
> -
> - if (waiting_.empty())
> - set_need_ship(game, false);
> - }
> -}
> -
> -/**
> - * A ship has arrived at the dock. Clear all items designated for this dock,
> - * and load the ship.
> + }
> +}
> +
> +/**
> + * Receive shipping item from unloading ship.
> + * Called by ship code.
> + */
> +void PortDock::shipping_item_arrived(Game& game, ShippingItem& si) {
> + si.set_location(game, warehouse_);
> + si.end_shipping(game);
> +}
> +
> +/**
> + * Receive shipping item from departing ship.
> + * Called by ship code.
> + */
> +void PortDock::shipping_item_returned(Game& game, ShippingItem& si) {
> + si.set_location(game, this);
> + waiting_.push_back(si);
> +}
> +
> +/**
> + * A ship changed destination and is now coming to the dock. Increase
> counter for need_ship.
> + */
> +void PortDock::ship_coming(bool affirmative) {
> + if (affirmative) {
> + ++ships_coming_;
> + } else {
> + // Max used for compatibility with old savegames
> + // NOCOM We should shift the savegame compatibility into
> PortDock::Loader::load it at all possible.
> + // Increment kCurrentPacketVersion and write separate loading
> code for both versions.
> + // Which case is for compatibility, and which case is the
> actual case that we want now?
> + ships_coming_ = std::max(0, ships_coming_ - 1);
The current line is for the compatibility case. The line for the actual case
would be simply "--ships_coming_".
> + }
> +}
> +
> +/**
> + * A ship has arrived at the dock. Set its next destination and load it
> accordingly.
> */
> void PortDock::ship_arrived(Game& game, Ship& ship) {
> - std::vector<ShippingItem> items_brought_by_ship;
> - ship.withdraw_items(game, *this, items_brought_by_ship);
> -
> - for (ShippingItem& shipping_item : items_brought_by_ship) {
> - shipping_item.set_location(game, warehouse_);
> - shipping_item.end_shipping(game);
> - }
> + ship_coming(false);
>
> if (expedition_ready_) {
> assert(expedition_bootstrap_ != nullptr);
>
> // Only use an empty ship.
> if (ship.get_nritems() < 1) {
> + ship.set_destination(nullptr);
> // Load the ship
> std::vector<Worker*> workers;
> std::vector<WareInstance*> wares;
> @@ -337,46 +368,71 @@
> // The expedition goods are now on the ship, so from
> now on it is independent from the port
> // and thus we switch the port to normal, so we could
> even start a new expedition,
> cancel_expedition(game);
> - return fleet_->update(game);
> + fleet_->update(game);
> + return;
> }
> }
>
> - if (ship.get_nritems() < ship.descr().get_capacity() &&
> !waiting_.empty()) {
> - uint32_t nrload =
> - std::min<uint32_t>(waiting_.size(),
> ship.descr().get_capacity() - ship.get_nritems());
> -
> - while (nrload--) {
> - // Check if the item has still a valid destination
> - if (waiting_.back().get_destination(game)) {
> - // Destination is valid, so we load the item
> onto the ship
> - ship.add_item(game, waiting_.back());
> + // 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);
> + 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();
> +
> + // Firstly load the items which go to chosen destination, while also
> checking for items with invalid destination
> + // NOCOM I made this change for performance reasons - please
> double-check that I didn't accidentally change the semantics.
> + auto si_it = waiting_.begin();
> + while (si_it != waiting_.end()) {
> + const PortDock* itemdest = si_it->get_destination(game);
> + if (itemdest) { // valid dest
> + if (remaining_capacity == 0) {
> + ++si_it; // keep the item here
> } else {
> - // The item has no valid destination anymore,
> so we just carry it
> - // back in the warehouse
> - waiting_.back().set_location(game, warehouse_);
> - waiting_.back().end_shipping(game);
> - }
> - waiting_.pop_back();
> - }
> -
> - if (waiting_.empty()) {
> - set_need_ship(game, false);
> - }
> - }
> -
> - fleet_->update(game);
> + if (itemdest == next_port) { // the item goes
> to chosen destination
> + ship.add_item(game, *si_it); // load it
> + si_it = waiting_.erase(si_it);
> + --remaining_capacity;
> + } else { // different destination
> + ++si_it; // keep it here for now
> + }
> + }
> + } else { // invalid destination
> + // carry the item back into the warehouse
> + si_it->set_location(game, warehouse_);
> + si_it->end_shipping(game);
> + ++si_it;
I'm not familiar with C++ iterators. The goal is to iterate over waiting items,
removing some of them. Items with invalid destination should be removed from
the waiting list, so "++si_it" should probably change into "si_it =
waiting_.erase(si_it)" .
> + }
> + }
> +
> + if (remaining_capacity > 0) { // there is still capacity
> + // Load any items favored by the chosen destination
> + si_it = waiting_.begin();
> + while (si_it != waiting_.end() && remaining_capacity > 0) {
> + assert(si_it->get_destination(game) != nullptr);
> + if (fleet_->is_path_favourable(*this, *next_port,
> *si_it->get_destination(game))) {
> + ship.add_item(game, *si_it);
> + si_it = waiting_.erase(si_it);
> + --remaining_capacity;
> + } else { // item is not favored by the chosen
> destination
> + // spare it from getting trapped inside the
> wrong ship
> + ++si_it;
> + }
> + }
> + }
> + ship.set_destination(next_port);
> + set_need_ship(game, !waiting_.empty());
> }
>
> void PortDock::set_need_ship(Game& game, bool need) {
> - molog("set_need_ship(%s)\n", need ? "true" : "false");
> -
> - if (need == need_ship_)
> - return;
> -
> - need_ship_ = need;
> -
> - if (fleet_) {
> - molog("... trigger fleet update\n");
> + if (need && fleet_) {
> + molog("trigger fleet update\n");
> fleet_->update(game);
> }
> }
--
https://code.launchpad.net/~widelands-dev/widelands/ship_scheduling_2/+merge/354019
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/ship_scheduling_2 into lp:widelands.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
