Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/inputwarequeue_display into lp:widelands
@bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/inputwarequeue_display. ___ 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/inputwarequeue_display into lp:widelands
Review: Approve I can't reproduce my bug any longer either, so as far as I am concerned this can go in. Thanks! -- https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/inputwarequeue_display. ___ 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/inputwarequeue_display into lp:widelands
Implemented your suggestion. WaresQueue returns only a variable, but WorkersQueue calls std::vector::size(), so this is more efficient. -- https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/inputwarequeue_display 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/inputwarequeue_display into lp:widelands
1 small comment for performance
Diff comments:
> === modified file 'src/economy/input_queue.cc'
> --- src/economy/input_queue.cc2018-04-07 16:59:00 +
> +++ src/economy/input_queue.cc2018-07-22 09:47:47 +
> @@ -120,6 +120,13 @@
> update();
> }
>
> +uint32_t InputQueue::get_missing() const {
> + if (get_filled() >= max_fill_ || request_ == nullptr ||
> !request_->is_open()) {
How is get_filled() calculated? It it's more than just returning a variable, it
will be more efficient to have const auto currently_filed = get_filled(); at
the top.
> + return 0;
> + }
> + return max_fill_ - get_filled() - std::min(max_fill_,
> request_->get_num_transfers());
> +}
> +
> constexpr uint16_t kCurrentPacketVersion = 3;
>
> void InputQueue::read(FileRead& fr, Game& game, MapObjectLoader& mol) {
--
https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/inputwarequeue_display 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/inputwarequeue_display into lp:widelands
I think I found it. request_->get_num_transfers() is actually the right function here, but merely passing through it´s return value isn´t enough. I made InputQueue::get_missing() more intelligent to account for capacity and existing inputs itself. I can´t reproduce the bug anymore now. I also added an assert to check that the calculations at least make sense. -- https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/inputwarequeue_display 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/inputwarequeue_display into lp:widelands
I´ll look into that bug… it can´t be the fault of get_num_transfers() because although I provide that function, I don´t use it for the calculations, so the problem is probably that request_->get_open_count() doesn´t do what it should… -- https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/inputwarequeue_display 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/inputwarequeue_display into lp:widelands
Thanks, it seems much more logical now. Mostly, that is, sometimes the "dark" section changes its size based on the current max-amount (i.e., 2 cloth requested -> 2 dark; 3 cloth requested -> only 1 dark). Might be correct inside the code based on how requests are calculated, guess you can't do much about that. I think coal is fine, wheat and cloth are a bit hard to differ but in my opinion it is good enough. You can see the difference, so I think you can leave it as it is now. An unfortunate bug I noticed: I have 1 cloth in my whole economy (based on ware statistics). With two shipyards each requesting 2 cloth, all the 2*2=4 cloth is displayed as "on their way" which just can't be the case. In the end, only 1 cloth is delivered while the rest is still reported as "on their way". I guess request_->get_num_transfers() should have better been called request_->get_num_requests() ? I haven't looked in the code, maybe there is some method which really returns the number of "wares on their way to fulfill this request". :-/ -- https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/inputwarequeue_display 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/inputwarequeue_display into lp:widelands
The bug was that request_->get_open_count() also seems to count wares that could be brought to the building at once if suddenly needed. Fixed this now by defining in the GUI that all wares beyond the player-defined limit are shown as missing unless they´re present. >From left to right in the queue: Wares in the building (normal); wares on >their way (dark); missing completely (light). I reset the transparency for >missing wares to the trunk value. Please also check wares that are already more or less monochrome (like coal, wheat, cloth), do you think the three states are distinguishable enough? -- https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/inputwarequeue_display 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/inputwarequeue_display into lp:widelands
Good feature, I like it. The structure of the code looks good, but there might be a bug in the calculations. When testing I reduced the maximum amount of requested wares for a building. It seems as if the number of darker shadow (the wares on their way?) is always the number of wares I reduced it by (e.g., building can store 10 logs, I clicked "store less" twice, now there are two darker shadows and 8 lighter ones). Not quite sure whether the darker ones really are the requested ones, but it looks strange either way. From an UI perspective I would prefer the icon-order "wares currently stored", "wares on their way" (darker/more visible), "wares (possibly requested but) not on their way" (lighter/more transparent). Also, I would prefer to not make the "lighter shadows" more transparent that they are in trunk. For some wares they are pretty hard to see and recognize at it is (blackwood, I think) and it becomes even more difficult with this branch. -- https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/inputwarequeue_display 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
