[Widelands-dev] [Merge] lp:~widelands-dev/widelands/inputwarequeue_display into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/inputwarequeue_display into lp:widelands has been updated. Status: Needs review => Merged For more details, see: 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
@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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/inputwarequeue_display into lp:widelands
Continuous integration builds have changed state: Travis build 3702. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/406665001. Appveyor build 3501. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_inputwarequeue_display-3501. -- 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/inputwarequeue_display into lp:widelands
Benedikt Straub has proposed merging lp:~widelands-dev/widelands/inputwarequeue_display into lp:widelands. Commit message: Missing wares (and workers) are indicated differently in InputQueueDisplays if the ware is on the way to the building than if it's really missing Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1781732 in widelands: "Show missing wares differently if ware is on the way" https://bugs.launchpad.net/widelands/+bug/1781732 For more details, see: 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. === modified file 'src/economy/input_queue.cc' --- src/economy/input_queue.cc 2018-04-07 16:59:00 + +++ src/economy/input_queue.cc 2018-07-21 16:50:25 + @@ -120,6 +120,20 @@ update(); } +uint32_t InputQueue::get_coming() const { + if (request_ == nullptr || !request_->is_open()) { + return 0; + } + return request_->get_num_transfers(); +} + +uint32_t InputQueue::get_missing() const { + if (request_ == nullptr || !request_->is_open()) { + return 0; + } + return request_->get_open_count(); +} + constexpr uint16_t kCurrentPacketVersion = 3; void InputQueue::read(FileRead& fr, Game& game, MapObjectLoader& mol) { === modified file 'src/economy/input_queue.h' --- src/economy/input_queue.h 2018-04-07 16:59:00 + +++ src/economy/input_queue.h 2018-07-21 16:50:25 + @@ -102,6 +102,22 @@ virtual Quantity get_filled() const = 0; /** + * The amount of missing wares or workers which are currently being transported to this building. + * This might temporarily be larger than get_max_fill() but will + * be smaller than get_max_size(). + * @return The amount at this moment. + */ + uint32_t get_coming() const; + + /** + * The amount of missing wares or workers which are not currently being transported to this building. + * This might temporarily be larger than get_max_fill() but will + * be smaller than get_max_size(). + * @return The amount at this moment. + */ + uint32_t get_missing() const; + + /** * Clear the queue appropriately. * Implementing classes should call update() at the end to remove the request. */ === modified file 'src/wui/inputqueuedisplay.cc' --- src/wui/inputqueuedisplay.cc 2018-04-27 06:11:05 + +++ src/wui/inputqueuedisplay.cc 2018-07-21 16:50:25 + @@ -129,7 +129,8 @@ cache_max_fill_ = queue_->get_max_fill(); uint32_t nr_inputs_to_draw = std::min(queue_->get_filled(), cache_size_); - uint32_t nr_empty_to_draw = cache_size_ - nr_inputs_to_draw; + uint32_t nr_missing_to_draw = std::min(queue_->get_missing(), cache_size_ - nr_inputs_to_draw); + uint32_t nr_coming_to_draw = cache_size_ - nr_inputs_to_draw - nr_missing_to_draw; Vector2i point = Vector2i::zero(); point.x = Border + (show_only_ ? 0 : CellWidth + CellSpacing); @@ -139,10 +140,15 @@ dst.blitrect(Vector2i(point.x, point.y), icon_, Recti(0, 0, icon_->width(), icon_->height()), BlendMode::UseAlpha); } - for (; nr_empty_to_draw; --nr_empty_to_draw, point.x += CellWidth + CellSpacing) { - dst.blitrect_scale_monochrome(Rectf(point.x, point.y, icon_->width(), icon_->height()), icon_, - Recti(0, 0, icon_->width(), icon_->height()), - RGBAColor(166, 166, 166, 127)); + for (; nr_coming_to_draw; --nr_coming_to_draw, point.x += CellWidth + CellSpacing) { + dst.blitrect_scale_monochrome(Rectf(point.x, point.y, icon_->width(), icon_->height()), icon_, + Recti(0, 0, icon_->width(), icon_->height()), + RGBAColor(127, 127, 127, 191)); + } + for (; nr_missing_to_draw; --nr_missing_to_draw, point.x += CellWidth + CellSpacing) { + dst.blitrect_scale_monochrome(Rectf(point.x, point.y, icon_->width(), icon_->height()), icon_, + Recti(0, 0, icon_->width(), icon_->height()), + RGBAColor(191, 191, 191, 63)); } if (!show_only_) { ___ 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