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.
_______________________________________________
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
