widelands-dev team mailing list archive
-
widelands-dev team
-
Mailing list archive
-
Message #13766
Re: [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Code review done, not tested yet.
Please let me know if you have any questions :)
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
> @@ -312,6 +312,22 @@
> }
>
> /**
> + * Called by workers wanting to drop a ware to their building's flag.
> + * \return true/allow on low congestion-risk.
> +*/
> +bool Flag::has_capacity_for_ware(WareInstance& ware) const {
> + // avoid iteration for the easy cases
> + if (ware_filled_ < ware_capacity_ - 2) return true;
Please always add {} after if and for statements etc - also for everything below. Omitting them can cause bugs. I know we still have a lot of those missing in the code base, but we shouldn't add to the problem when we edit/add code.
> + if (ware_filled_ >= ware_capacity_) return false;
> +
> + DescriptionIndex const descr_index = ware.descr_index();
> + for (int i = 0; i < ware_filled_; ++i) {
> + if (wares_[i].ware->descr_index() == descr_index) return false;
> + }
> + return true;
> +}
> +
> +/**
> * Returns true if the flag can hold more wares.
> */
> bool Flag::has_capacity() const {
> @@ -362,29 +381,22 @@
> }
>
> ware.set_location(egbase, this);
> -
> - if (upcast(Game, game, &egbase))
> - ware.update(*game); // will call call_carrier() if necessary
> }
>
> /**
> - * \return true if an ware is currently waiting for a carrier to the given Flag.
> + * \return a ware currently waiting for a carrier to the given destflag.
> *
> * \note Due to fetch_from_flag() semantics, this function makes no sense
> * for a building destination.
> */
> -bool Flag::has_pending_ware(Game&, Flag& dest) {
> +Flag::PendingWare* Flag::has_pending_ware_for_flag(Flag& destflag) {
The function name doesn't fit well with the return type any more - call it get_pending_ware_for_flag?
> for (int32_t i = 0; i < ware_filled_; ++i) {
> - if (!wares_[i].pending)
> - continue;
> -
> - if (wares_[i].nextstep != &dest)
> - continue;
> -
> - return true;
> + PendingWare* pw = &wares_[i];
> + if (pw->pending && pw->nextstep == &destflag &&
> + destflag.allow_ware_from_flag(*pw->ware, *this)) return pw;
> }
>
> - return false;
> + return nullptr;
> }
>
> /**
> @@ -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 use Widelands::INVALID_INDEX to signal invalid wares. We are tying to get away from pure integers for flags like this.
> +*/
> +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
Please make -2 a named constexpr
> +*/
> +int32_t Flag::find_swapable_ware(WareInstance& ware, Flag& destflag) {
swapable -> swappable
> + 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);
We're accessing the front of capacity_wait_ a lot - maybe using a deque would be more efficient? http://www.cplusplus.com/reference/deque/deque/
> + 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) {
We can use the LAST_DIRECTION constant here
> + 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
> */
> @@ -634,6 +671,70 @@
> }
>
> /**
> + * 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_swapable = false;
swapable -> swappable
> + for (int i = 0; i < ware_filled_; ++i) {
> + PendingWare& pw = wares_[i];
> + if (pw.pending && pw.nextstep == &flag) {
> + has_swapable = true;
> + } else if (pw.ware->descr_index() == descr_index) {
> + return false;
> + }
> + }
> + return (ware_filled_ < ware_capacity_ || has_swapable) ? 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_swapable = false;
swapable -> swappable
> + 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_swapable = true;
> + }
> + }
> +
> + // ask road for carrier on low congestion-risk
> + if (ware_filled_ < ware_capacity_ - 2 ||
> + (!has_same_ware && (ware_filled_ < ware_capacity_ || has_swapable))) {
> + 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.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 rename NOP - I have no idea what it means. How about calling it "IDLE"?
> 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)
Does this fit one of the named constants from carrier.h?
> // 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;
Is there a reason you can't #include "economy/flag.h" here?
>
> class CarrierDescr : public WorkerDescr {
> public:
--
https://code.launchpad.net/~widelands-dev/widelands/congestion2/+merge/348525
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/congestion2 into lp:widelands.
References
