widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Replied to your comments.

Diff comments:

> === modified file 'src/economy/flag.cc'
> --- src/economy/flag.cc	2018-04-07 16:59:00 +0000
> +++ src/economy/flag.cc	2018-06-26 21:40:51 +0000
> @@ -394,101 +406,112 @@
>  #define MAX_TRANSFER_PRIORITY 16
>  
>  /**
> - * Called by carrier code to indicate that the carrier is moving to pick up an
> - * ware. Ware with highest transfer priority is chosen.
> - * \return true if an ware is actually waiting for the carrier.
> - */
> -bool Flag::ack_pickup(Game&, Flag& destflag) {
> -	int32_t highest_pri = -1;
> -	int32_t i_pri = -1;
> -
> -	for (int32_t i = 0; i < ware_filled_; ++i) {
> -		if (!wares_[i].pending)
> -			continue;
> -
> -		if (wares_[i].nextstep != &destflag)
> -			continue;
> -
> -		if (wares_[i].priority > highest_pri) {
> -			highest_pri = wares_[i].priority;
> -			i_pri = i;
> -
> -			// Increase ware priority, it matters only if the ware has to wait.
> -			if (wares_[i].priority < MAX_TRANSFER_PRIORITY)
> -				wares_[i].priority++;
> -		}
> -	}
> -
> -	if (i_pri >= 0) {
> -		wares_[i_pri].pending = false;
> -		return true;
> -	}
> -
> -	return false;
> -}
> -/**
>   * Called by the carriercode when the carrier is called away from his job
>   * but has acknowledged a ware before. This ware is then freed again
>   * to be picked by another carrier. Returns true if an ware was indeed
>   * made pending again
>   */
>  bool Flag::cancel_pickup(Game& game, Flag& destflag) {
> -	int32_t lowest_prio = MAX_TRANSFER_PRIORITY + 1;
> -	int32_t i_pri = -1;
> -
>  	for (int32_t i = 0; i < ware_filled_; ++i) {
> -		if (wares_[i].pending)
> -			continue;
> -
> -		if (wares_[i].nextstep != &destflag)
> -			continue;
> -
> -		if (wares_[i].priority < lowest_prio) {
> -			lowest_prio = wares_[i].priority;
> -			i_pri = i;
> +		PendingWare& pw = wares_[i];
> +		if (!pw.pending && pw.nextstep == &destflag) {
> +			pw.pending = true;
> +			pw.ware->update(game);  //  will call call_carrier() if necessary
> +			return true;
>  		}
>  	}
>  
> -	if (i_pri >= 0) {
> -		wares_[i_pri].pending = true;
> -		wares_[i_pri].ware->update(game);  //  will call call_carrier() if necessary
> -		return true;
> -	}
> -
>  	return false;
>  }
>  
>  /**
> - * Wake one sleeper from the capacity queue.
> -*/
> -void Flag::wake_up_capacity_queue(Game& game) {
> -	while (!capacity_wait_.empty()) {
> -		Worker* const w = capacity_wait_[0].get(game);
> -		capacity_wait_.erase(capacity_wait_.begin());
> -		if (w && w->wakeup_flag_capacity(game, *this))
> -			break;
> -	}
> -}
> -
> -/**
> - * Called by carrier code to retrieve one of the wares on the flag that is meant
> - * for that carrier.
> - *
> - * This function may return 0 even if \ref ack_pickup() has already been
> - * called successfully.
> -*/
> -WareInstance* Flag::fetch_pending_ware(Game& game, PlayerImmovable& dest) {
> -	int32_t best_index = -1;
> -
> -	for (int32_t i = 0; i < ware_filled_; ++i) {
> -		if (wares_[i].nextstep != &dest)
> -			continue;
> -
> -		// We prefer to retrieve wares that have already been acked
> -		if (best_index < 0 || !wares_[i].pending)
> -			best_index = i;
> -	}
> -
> + * Called by carrier code to find the best among the wares on this flag
> + * that are meant for the provided dest.
> + * \return index of found ware or -1 if not found appropriate

Please give it a name in any case.

> +*/
> +int32_t Flag::find_pending_ware(PlayerImmovable& dest) {
> +	int32_t highest_pri = -1;
> +	int32_t best_index = -1;
> +	bool ware_pended = false;
> +
> +	for (int32_t i = 0; i < ware_filled_; ++i) {
> +		PendingWare& pw = wares_[i];
> +		if (pw.nextstep != &dest) continue;
> +
> +		if (pw.priority < MAX_TRANSFER_PRIORITY) pw.priority++;
> +		// Release promised pickup, in case we find a preferable ware
> +		if (!ware_pended && !pw.pending) {
> +			ware_pended = pw.pending = true;
> +		}
> +
> +		// If dest is flag, we exclude wares that can stress it
> +		if (&dest != building_ &&
> +			!dynamic_cast<Flag&>(dest).allow_ware_from_flag(*pw.ware, *this)) {
> +			continue;
> +		}
> +
> +		if (pw.priority > highest_pri) {
> +			highest_pri = pw.priority;
> +			best_index = i;
> +		}
> +	}
> +
> +	return best_index;
> +}
> +
> +/**
> + * Like find_pending_ware above, but for carriers who are currently carrying a ware.
> + * \return -2 if denied drop

Assuming the datatype is int, the call looks like this:

constexpr int kSomeName = -2;

if it's declared within a class:

static constexpr int kSomeName = -2;

> +*/
> +int32_t Flag::find_swapable_ware(WareInstance& ware, Flag& destflag) {
> +	DescriptionIndex const descr_index = ware.descr_index();
> +	int32_t highest_pri = -1;
> +	int32_t best_index = -1;
> +	bool has_same_ware = false;
> +	bool has_allowed = false;
> +	bool ware_pended = false;
> +
> +	for (int32_t i = 0; i < ware_filled_; ++i) {
> +		PendingWare& pw = wares_[i];
> +		if (pw.nextstep != &destflag) {
> +			if (pw.ware->descr_index() == descr_index) has_same_ware = true;
> +			continue;
> +		}
> +
> +		if (pw.priority < MAX_TRANSFER_PRIORITY) pw.priority++;
> +		// Release promised pickup, in case we find a preferable ware
> +		if (!ware_pended && !pw.pending) {
> +			ware_pended = pw.pending = true;
> +		}
> +
> +		// We prefer to retrieve wares that won't stress the destflag
> +		if (destflag.allow_ware_from_flag(*pw.ware, *this)) {
> +			if (!has_allowed) {
> +				has_allowed = true;
> +				highest_pri = -1;
> +			}
> +		} else {
> +			if (has_allowed) continue;
> +		}
> +
> +		if (pw.priority > highest_pri) {
> +			highest_pri = pw.priority;
> +			best_index = i;
> +		}
> +	}
> +
> +	if (best_index > -1) {
> +		return (ware_filled_ > ware_capacity_ - 3 || has_allowed) ? best_index : -1;
> +	} else {
> +		return (ware_filled_ < ware_capacity_ - 2 ||
> +			(ware_filled_ < ware_capacity_ && !has_same_ware)) ? -1 : -2;
> +	}
> +}
> +
> +/**
> + * Called by carrier code to retrieve a ware found by the previous methods.
> +*/
> +WareInstance* Flag::fetch_pending_ware(Game& game, int32_t best_index) {
>  	if (best_index < 0)
>  		return nullptr;
>  
> @@ -499,14 +522,39 @@
>  	        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);

Maybe make a note of it in the code with NOCOM, so that we won't forget? I'd be happy to take care of it once this branch is more mature.

> +		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 <= 6; ++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->has_pending_ware_for_flag(*this);
> +		if (pw && road->notify_ware(game, *other)) {
> +			pw->pending = false;
> +		}
> +	}	
> +}
> +
> +/**
>   * Return a List of all the wares currently on this Flag. Do not rely
>   * the result value to stay valid and do not change them
>   */
> 
> === modified file 'src/logic/map_objects/tribes/carrier.cc'
> --- src/logic/map_objects/tribes/carrier.cc	2018-04-15 06:13:09 +0000
> +++ src/logic/map_objects/tribes/carrier.cc	2018-06-26 21:40:51 +0000
> @@ -77,14 +78,14 @@
>  		return pop_task(game);
>  	}
>  
> -	// Check for pending wares
> -	if (promised_pickup_to_ == NOONE)
> -		find_pending_ware(game);
> +	if (operation_ == INIT) {
> +		operation_ = find_source_flag(game);
> +	}
>  
> -	if (promised_pickup_to_ != NOONE) {
> +	if (operation_ > NOP) {

Please call it "NO_OPERATION" then - I expect I am not the only one who will have to look this up. Also, abbreviations are very bad of having multiple meanings, e.g. https://www.acronymfinder.com/NOP.html returns 47 entries ;)

>  		if (state.ivar1) {
>  			state.ivar1 = 0;
> -			return start_task_transport(game, promised_pickup_to_);
> +			return start_task_transport(game, operation_);
>  		} else {
>  			// Short delay before we move to pick up
>  			state.ivar1 = 1;
> @@ -155,35 +156,85 @@
>  		return pop_task(game);
>  	}
>  
> -	if (state.ivar1 == -1)
> +	int32_t const ivar1 = state.ivar1;
> +	if (ivar1 == -1)

I'd love to see a rework like that! I'm reluctant to touch that bit with a barge pole :(

>  		// If we're "in" the target building, special code applies
> -		deliver_to_building(game, state);
> -
> -	else if (!does_carry_ware())
> -		// If we don't carry something, walk to the flag
> -		pickup_from_flag(game, state);
> -
> -	else {
> -		Road& road = dynamic_cast<Road&>(*get_location(game));
> -		// If the ware should go to the building attached to our flag, walk
> -		// directly into said building
> -		Flag& flag = road.get_flag(static_cast<Road::FlagId>(state.ivar1 ^ 1));
> -
> -		WareInstance& ware = *get_carried_ware(game);
> -		assert(ware.get_location(game) == this);
> -
> +		return deliver_to_building(game, state);
> +
> +	WareInstance* ware = get_carried_ware(game);
> +	if (ware) {
> +		assert(ware->get_location(game) == this);
> +	}
> +
> +	Road& road = dynamic_cast<Road&>(*get_location(game));
> +	int32_t const dest = ware ? ivar1 ^ 1 : ivar1;
> +	Flag& flag = road.get_flag(static_cast<Road::FlagId>(dest));
> +
> +	if (ware) {
> +		// If the ware should go to the building attached to our flag,
> +		// walk directly into said building
>  		// A sanity check is necessary, in case the building has been destroyed
> -		PlayerImmovable* const next = ware.get_next_move_step(game);
> -
> -		if (next && next != &flag && &next->base_flag() == &flag)
> -			enter_building(game, state);
> -
> -		// If the flag is overloaded we are allowed to drop wares as
> -		// long as we can pick another up. Otherwise we have to wait.
> -		else if ((flag.has_capacity() || !swap_or_wait(game, state)) &&
> -		         !start_task_walktoflag(game, state.ivar1 ^ 1))
> -			// Drop the ware, possible exchanging it with another one
> -			drop_ware(game, state);
> +
> +		PlayerImmovable* next = ware->get_next_move_step(game);
> +		if (next && next != &flag && &next->base_flag() == &flag) {
> +			if (!start_task_walktoflag(game, dest)) {
> +				// Enter building
> +				state.ivar1 = -1;
> +				start_task_move(game, WALK_NW, descr().get_right_walk_anims(does_carry_ware()), true);
> +			}
> +			return;
> +		}
> +	}
> +
> +	if (!start_task_walktoflag(game, dest, operation_ == WAIT)) {
> +		// If the flag is overloaded we are allowed to drop wares,
> +		// as long as we can pick another up. Otherwise we have to wait.
> +
> +		Flag& otherflag = road.get_flag(static_cast<Road::FlagId>(dest ^ 1));
> +		int32_t otherware_idx = ware ? flag.find_swapable_ware(*ware, otherflag) :
> +										flag.find_pending_ware(otherflag);
> +		if (operation_ == WAIT) {
> +			if (otherware_idx < -1) {
> +				return start_task_waitforcapacity(game, flag); // join flag's wait queue
> +			} else {
> +				operation_ = dest ^ 1; // resume transport without joining flag's wait queue
> +				set_animation(game, descr().get_animation("idle"));
> +				return schedule_act(game, 20);
> +			}
> +		} else if (otherware_idx < -1) {
> +			operation_ = WAIT; // move one node away
> +			set_animation(game, descr().get_animation("idle"));
> +			return schedule_act(game, 20);
> +		}
> +
> +		WareInstance* otherware = flag.fetch_pending_ware(game, otherware_idx);
> +
> +		if (ware) {
> +			// Drop our ware
> +			flag.add_ware(game, *fetch_carried_ware(game));
> +		}
> +
> +		// Pick up new load, if any
> +		if (otherware) {
> +			set_carried_ware(game, otherware);
> +			flag.ware_departing(game);
> +
> +			operation_ = state.ivar1 = dest;
> +			set_animation(game, descr().get_animation("idle"));
> +			schedule_act(game, 20);
> +		} else {
> +			Flag::PendingWare* pw = otherflag.has_pending_ware_for_flag(flag);
> +			if (pw) {
> +				pw->pending = false;
> +
> +				operation_ = state.ivar1 = dest ^ 1;
> +				set_animation(game, descr().get_animation("idle"));
> +				schedule_act(game, 20);
> +			} else {
> +				operation_ = NOP;
> +				pop_task(game);
> +			}
> +		}
>  	}
>  }
>  
> 
> === 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-06-26 21:40:51 +0000
> @@ -24,6 +24,7 @@
>  #include "logic/map_objects/tribes/worker.h"
>  
>  namespace Widelands {
> +class PendingWare;

There is a lot of work needed on that front - will probably take me a few years to clean that all up. Let's leave it for now.

>  
>  class CarrierDescr : public WorkerDescr {
>  public:


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