widelands-dev team mailing list archive

[ypopezios <ypopezios@xxxxxxxx>]

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.