widelands-dev team mailing list archive

[ypopezios <ypopezios@xxxxxxxx>]

Replied to inline comments. Will update the code soon.

Diff comments:

> === modified file 'src/economy/flag.cc'
> --- src/economy/flag.cc	2018-07-11 08:32:54 +0000
> +++ src/economy/flag.cc	2018-07-30 09:36:58 +0000
> @@ -505,14 +576,43 @@
>  	        sizeof(wares_[0]) * (ware_filled_ - best_index));
>  
>  	ware->set_location(game, nullptr);
> -
> -	// wake up capacity wait queue
> -	wake_up_capacity_queue(game);
> -
>  	return ware;
>  }
>  
>  /**
> + * Called by carrier code to notify waiting carriers
> + * which may be interested in the new state of this flag.
> + */
> +void Flag::ware_departing(Game& game) {
> +	// Wake up one sleeper from the capacity queue.
> +	while (!capacity_wait_.empty()) {
> +		Worker* const w = capacity_wait_[0].get(game); // NOCOM consider using a deque

Yes please.

> +		capacity_wait_.erase(capacity_wait_.begin());
> +		if (w && w->wakeup_flag_capacity(game, *this)) {
> +			return;
> +		}
> +	}
> +
> +	// Consider pending wares of neighboring flags.
> +	for (int32_t dir = 1; dir <= WalkingDir::LAST_DIRECTION; ++dir) {
> +		Road* const road = get_road(dir);
> +		if (!road) {
> +			continue;
> +		}
> +
> +		Flag* other = &road->get_flag(Road::FlagEnd);
> +		if (other == this) {
> +			other = &road->get_flag(Road::FlagStart);
> +		}
> +
> +		PendingWare* pw = other->get_ware_for_flag(*this, kPendingOnly);
> +		if (pw && road->notify_ware(game, *other)) {
> +			pw->pending = false;
> +		}
> +	}
> +}
> +
> +/**
>   * Accelerate potential promotion of roads adjacent to a newly promoted road.
>   */
>  void Flag::propagate_promoted_road(Road* const promoted_road) {
> @@ -681,6 +779,72 @@
>  }
>  
>  /**
> + * Called by neighboring flags, before agreeing for a carrier
> + * to take one of their wares heading to this flag.
> + * \return true/allow on low congestion-risk.
> + */
> +bool Flag::allow_ware_from_flag(WareInstance& ware, Flag& flag) {
> +	// avoid iteration for the easy cases
> +	if (ware_filled_ < ware_capacity_ - 2) {
> +		return true;
> +	}
> +
> +	DescriptionIndex const descr_index = ware.descr_index();
> +	bool has_swappable = false;
> +	for (int i = 0; i < ware_filled_; ++i) {
> +		PendingWare& pw = wares_[i];
> +		if (pw.pending && pw.nextstep == &flag) {
> +			has_swappable = true;
> +		} else if (pw.ware->descr_index() == descr_index) {
> +			return false;
> +		}
> +	}
> +	return (ware_filled_ < ware_capacity_ || has_swappable) ? true : false;
> +}
> +
> +/**
> + * Called when a ware is trying to reach this flag through the provided road,
> + * having just arrived to the provided flag.
> + * Swaps pending wares if possible. Otherwise,
> + * asks road for carrier on low congestion-risk.
> + * \return false if the ware is not immediately served.
> + */
> +bool Flag::update_ware_from_flag(Game& game, PendingWare& pw1, Road& road, Flag& flag) {
> +	WareInstance& w1 = *pw1.ware;
> +	DescriptionIndex const w1_descr_index = w1.descr_index();
> +	bool has_same_ware = false;
> +	bool has_swappable = false;
> +	for (int i = 0; i < ware_filled_; ++i) {
> +		PendingWare& pw2 = wares_[i];
> +		WareInstance& w2 = *pw2.ware;
> +		if (w2.descr_index() == w1_descr_index) {
> +			if (pw2.nextstep == &flag) {
> +				// swap pending wares remotely
> +				init_ware(game, w1, pw2);
> +				flag.init_ware(game, w2, pw1);
> +				w1.update(game);
> +				w2.update(game);
> +				return true;
> +			}
> +
> +			has_same_ware = true;
> +		} else if (pw2.pending && pw2.nextstep == &flag) {
> +			has_swappable = true;
> +		}
> +	}
> +
> +	// ask road for carrier on low congestion-risk
> +	if (ware_filled_ < ware_capacity_ - 2 ||
> +		(!has_same_ware && (ware_filled_ < ware_capacity_ || has_swappable))) {

I don't think so. We may be able to unify this code in a future branch, after wares get stripped off their code, in favour to flags.

> +		if (road.notify_ware(game, flag)) {
> +			pw1.pending = false;
> +			return true;
> +		}
> +	}
> +	return false;
> +}
> +
> +/**
>   * Called whenever a road gets broken or split.
>   * Make sure all wares on this flag are rerouted if necessary.
>   *
> 
> === modified file 'src/logic/map_objects/tribes/carrier.h'
> --- src/logic/map_objects/tribes/carrier.h	2018-04-15 06:13:09 +0000
> +++ src/logic/map_objects/tribes/carrier.h	2018-07-30 09:36:58 +0000
> @@ -77,17 +78,14 @@
>  	static Task const taskTransport;
>  
>  	void deliver_to_building(Game&, State&);
> -	void pickup_from_flag(Game&, State&);
> -	void drop_ware(Game&, State&);
> -	void enter_building(Game&, State&);
> -	bool swap_or_wait(Game&, State&);
>  
> -	/// -1: no ware acked; 0/1: acked ware for start/end flag of road
>  	// This should be an enum, but this clutters the code with too many casts
> -	static const int32_t NOONE = -1;
> -	static const int32_t START_FLAG = 0;
> -	static const int32_t END_FLAG = 1;
> -	int32_t promised_pickup_to_;
> +	static const int32_t INIT = -3; // ready to undertake or resume operations

All these are possible values of operation_. I'm not familiar with C++ flavours. If you can make this an enum or even avoid some casts, go on. The only arithmetic is a "^ 1" to swap between START_FLAG and END_FLAG, which can be easily reworked.

> +	static const int32_t WAIT = -2; // waiting for flag capacity
> +	static const int32_t NO_OPERATION = -1; // idling
> +	static const int32_t START_FLAG = 0; // serving start flag of road
> +	static const int32_t END_FLAG = 1; // serving end flag of road
> +	int32_t operation_;
>  
>  	// saving and loading
>  protected:


-- 
https://code.launchpad.net/~widelands-dev/widelands/congestion2/+merge/348525
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/congestion2.