[Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/congestion2 into lp:widelands has been updated. Status: Needs review => Merged For more details, see: 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Continuous integration builds have changed state: Travis build 3762. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/413937893. Appveyor build 3561. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_congestion2-3561. -- 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/congestion2 into lp:widelands has been updated. Commit message changed to: Ware routing improvements to prevent road congestion. For more details, see: 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Review: Approve I had 8 AIs battle it out on The Nile for 1 hour gametime - I then ran into an AI bug which is not related to this branch. - Max productivity in this branch was 64, in trunk 74. However: - The less successful AIs had much higher productivity than on trunk, so the graphs were closer together - The AIs expanded faster in this branch So, definitely an improvement - let's have it :) Travis error is a timeout on one of the MacOS builds. @bunnybot merge force -- 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Continuous integration builds have changed state: Travis build 3761. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/413597619. Appveyor build 3560. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_congestion2-3560. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
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 +
> +++ src/economy/flag.cc 2018-07-30 09:36:58 +
> @@ -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 = >get_flag(Road::FlagEnd);
> + if (other == this) {
> + other = >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 == ) {
> + 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 == ) {
> + // 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 == ) {
> + 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.h2018-04-15 06:13:09 +
> +++
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Another round of code review done - just some small nits.
Diff comments:
> === modified file 'src/economy/flag.cc'
> --- src/economy/flag.cc 2018-07-11 08:32:54 +
> +++ src/economy/flag.cc 2018-07-30 09:36:58 +
> @@ -400,103 +450,124 @@
> #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 != )
> - 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
> + * to be picked by another carrier. Returns true if a 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 != )
> - continue;
> -
> - if (wares_[i].priority < lowest_prio) {
> - lowest_prio = wares_[i].priority;
> - i_pri = i;
> + PendingWare& pw = wares_[i];
> + if (!pw.pending && pw.nextstep == ) {
> + 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 != )
> - continue;
> -
> - // We prefer to retrieve wares that have already been acked
> - if (best_index < 0 || !wares_[i].pending)
> - best_index = i;
> - }
> -
> - if (best_index < 0)
> + * 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 (carrier will take it)
> + * or kNotFoundAppropriate (carrier will leave empty-handed)
> + */
> +int32_t Flag::find_pending_ware(PlayerImmovable& dest) {
> + int32_t highest_pri = -1;
> + int32_t best_index = kNotFoundAppropriate;
> + bool ware_pended = false;
> +
> + for (int32_t i = 0; i < ware_filled_; ++i) {
> + PendingWare& pw = wares_[i];
> + if (pw.nextstep != ) {
> + 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 ( != building_ &&
> +
>
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Continuous integration builds have changed state: Travis build 3737. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/409791669. Appveyor build 3537. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_congestion2-3537. -- 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Continuous integration builds have changed state: Travis build 3727. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/408616293. Appveyor build 3527. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_congestion2-3527. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Replied to your comments.
Diff comments:
> === modified file 'src/economy/flag.cc'
> --- src/economy/flag.cc 2018-04-07 16:59:00 +
> +++ src/economy/flag.cc 2018-06-26 21:40:51 +
> @@ -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 != )
> - 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 != )
> - continue;
> -
> - if (wares_[i].priority < lowest_prio) {
> - lowest_prio = wares_[i].priority;
> - i_pri = i;
> + PendingWare& pw = wares_[i];
> + if (!pw.pending && pw.nextstep == ) {
> + 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 != )
> - 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 != ) 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 ( != building_ &&
> +
> !dynamic_cast(dest).allow_ware_from_flag(*pw.ware, *this)) {
> + continue;
> + }
> +
> + if (pw.priority > highest_pri) {
> + highest_pri = pw.priority;
> + best_index = i;
> + }
> + }
> +
>
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Replied to inline comments. Will update the code soon.
Diff comments:
> === modified file 'src/economy/flag.cc'
> --- src/economy/flag.cc 2018-04-07 16:59:00 +
> +++ src/economy/flag.cc 2018-06-26 21:40:51 +
> @@ -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 != )
> - 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 != )
> - continue;
> -
> - if (wares_[i].priority < lowest_prio) {
> - lowest_prio = wares_[i].priority;
> - i_pri = i;
> + PendingWare& pw = wares_[i];
> + if (!pw.pending && pw.nextstep == ) {
> + 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 != )
> - 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
-1 here doesn't signal invalid wares, it instead indicates one of the cases of
no index, so it needs a different name. As about not using an integer, refer to
next comment.
> +*/
> +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 != ) 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 ( != building_ &&
> +
> !dynamic_cast(dest).allow_ware_from_flag(*pw.ware, *this)) {
> + continue;
> + }
> +
> +
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
@klaus I suggest you don't spend your energy with this before I have a review of the code myself, cause I haven't looked at it after the two merges took place. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Mhh, thhis diff shows some Merge conflicts <<
But I do not seem them in the code?
I get lost in tat code change, sorry.
That complexity in Road and Carrier gives me a headache.
Ill give it another try with the debugger tomorrow.
Diff comments:
> === modified file 'src/economy/flag.cc'
> --- src/economy/flag.cc 2018-07-07 09:05:13 +
> +++ src/economy/flag.cc 2018-07-10 13:28:48 +
> @@ -394,101 +421,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 != )
> - 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 != )
> - continue;
> -
> - if (wares_[i].priority < lowest_prio) {
> - lowest_prio = wares_[i].priority;
> - i_pri = i;
> + PendingWare& pw = wares_[i];
> + if (!pw.pending && pw.nextstep == ) {
> + 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 != )
> - 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
> +*/
> +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 != ) 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 ( != building_ &&
> +
> !dynamic_cast(dest).allow_ware_from_flag(*pw.ware, *this)) {
> + continue;
> + }
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Uhhm, I opend a game I played with this branch with trunk and got InternetGaming: Game update on metaserver. ASAN:DEADLYSIGNAL = ==88925==ERROR: AddressSanitizer: BUS on unknown address 0x6118002f2ba8 (pc 0x0001098ce1c1 bp 0x7ffee6bd7390 sp 0x7ffee6bd7370 T0) #0 0x1098ce1c0 in Widelands::Road::get_flag(Widelands::Road::FlagId) const road.h:91 #1 0x109a7d217 in Widelands::Carrier::pickup_from_flag(Widelands::Game&, Widelands::Bob::State&) carrier.cc:253 #2 0x109a7c681 in Widelands::Carrier::transport_update(Widelands::Game&, Widelands::Bob::State&) carrier.cc:168 not sure where this comes from, will try this with this branch again. optically a carrier dropped a ware in the middle of a road and wen he went back I got that crash. Whatever we did this may break savegames. I will continue checking the code why this may happen. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
./regression_test.py -b ./widelands Ran 42 tests in 748.805s OK so far, will do more small improvements, later -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
I'd rather write a paper on theoretical aspects of Widelands in graph theory... And I'm not interested in publicity, thanks. I'm interested in testing though. You can test current code the way you said (specific congestion scenarios and strange maps) or you can wait to test after I update the code. Even watching the carriers and meditating on their behaviour under special circumstances can be helpful, since the preferable behaviour is not obvious. Thanks for your offer to help and thanks in advance for doing it. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
@ypopezios: OK, thanks for your hints, once you are done you should write a paper on pracital aspects of https://en.wikipedia.org/wiki/Graph_theory in Widelands ;-) Now, how can I practically help? I could try to reproduce on of the deadlocks/congestions we found so far and check, if your change will either fix or at least improve it. I can setup a debugger and check details of your code, if you wish. So please tell me how I can help and thanks for the plates your serve. (Your road promotion code made it alread to youtube https://www.youtube.com/watch?v=ZAe4zSCznRw=1s) Next time I should give credit to you. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
@klaus Road promotion was just the "appetizer". Congestion here is the "first plate". There will hopefully come a "main plate" as well, which will seriously improve routing performance. There will also come some other improvements related to economy. Concerning the description of this merge, reading the following opening post is a must: https://wl.widelands.org/forum/topic/4257/ I will address Gun's comments one of these days. I may still need some help with C++, but I will report that here. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Review: Needs Information ypopezios what is the purpose / descripton of this branch? You want to adress that "ai roads getting jammed a lot". You already improved road promotion, but this is not the case here? Does this inlcude the "swapping the routing" idea (which is really good!) Testing would check how somme AIs behave on this "concentric rings" and other "strange" maps :-) Do you have time to care for Guns (very valid) comments? If not I might pick up to lear more about that code. please respond. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Flag& otherflag = road.get_flag(static_cast(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.h2018-04-15 06:13:09 +
> +++ src/logic/map_objects/tribes/carrier.h2018-06-26 21:40:51 +
> @@ -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.
___
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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Took the freedom and merged trunk into this branch. Otherwise i couldn't compile... -- 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. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
Continuous integration builds have changed state: Travis build 3618. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/396890268. Appveyor build 3418. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_congestion2-3418. -- 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. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/congestion2 into lp:widelands
ypopezios has proposed merging lp:~widelands-dev/widelands/congestion2 into
lp:widelands.
Commit message:
Another attempt to get windows builds.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
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.
=== modified file 'src/economy/flag.cc'
--- src/economy/flag.cc 2018-04-07 16:59:00 +
+++ src/economy/flag.cc 2018-06-26 12:47:13 +
@@ -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;
+ 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 {
@@ -339,10 +355,13 @@
}
void Flag::add_ware(EditorGameBase& egbase, WareInstance& ware) {
-
assert(ware_filled_ < ware_capacity_);
+ init_ware(egbase, ware, wares_[ware_filled_++]);
+ if (upcast(Game, game, ))
+ ware.update(*game); // will call call_carrier() if necessary
+}
- PendingWare& pi = wares_[ware_filled_++];
+void Flag::init_ware(EditorGameBase& egbase, WareInstance& ware, PendingWare& pi) {
pi.ware =
pi.pending = false;
pi.nextstep = nullptr;
@@ -362,29 +381,22 @@
}
ware.set_location(egbase, this);
-
- if (upcast(Game, game, ))
- 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) {
for (int32_t i = 0; i < ware_filled_; ++i) {
- if (!wares_[i].pending)
- continue;
-
- if (wares_[i].nextstep != )
- continue;
-
- return true;
+ PendingWare* pw = _[i];
+ if (pw->pending && pw->nextstep == &&
+ 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 != )
- 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 != )
- continue;
-
- if (wares_[i].priority < lowest_prio) {
- lowest_prio = wares_[i].priority;
- i_pri = i;
+ PendingWare& pw = wares_[i];
+ if (!pw.pending && pw.nextstep == ) {
+ 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 tha
