[Widelands-dev] [Merge] lp:~widelands-dev/widelands/frisian-buildings into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/frisian-buildings into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/frisian-buildings/+merge/349856 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/frisian-buildings. ___ 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/frisian-buildings into lp:widelands
Continuous integration builds have changed state: Travis build 3709. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/406860500. Appveyor build 3508. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_frisian_buildings-3508. -- https://code.launchpad.net/~widelands-dev/widelands/frisian-buildings/+merge/349856 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/frisian-buildings. ___ 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
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/frisian_music into lp:widelands
I would like to play around with the sheet music a bit because I think some of the harmonies can be improved, and trying to explain it with words doesn't work. This will have to wait until August though. -- https://code.launchpad.net/~widelands-dev/widelands/frisian_music/+merge/350429 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/frisian_music 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/frisian_music into lp:widelands
Yep I composed that song, you can find it here as sheet music: https://musescore.com/user/18659851/scores/515845 -- https://code.launchpad.net/~widelands-dev/widelands/frisian_music/+merge/350429 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/frisian_music 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/bug-1736901-shift-market into lp:widelands
Review: Resubmit Sphinx is fixed, so this branch is ready now -- https://code.launchpad.net/~widelands-dev/widelands/bug-1736901-shift-market/+merge/349630 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1736901-shift-market. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/frisian_music into lp:widelands
Continuous integration builds have changed state: Travis build 3705. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/406816829. Appveyor build 3504. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_frisian_music-3504. -- https://code.launchpad.net/~widelands-dev/widelands/frisian_music/+merge/350429 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/frisian_music 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
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/frisian-buildings into lp:widelands
Review: Approve You have done a beautiful job on those buildings! :) There are 2 buildings that I'd still like to see changed in a follow-up branch though: The Port and the Tower. Their tower heights work really well as works of art, but they are obscuring the fields behind them, so it becomes a UI issue. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/frisian-buildings/+merge/349856 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/frisian-buildings. ___ 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/remove-savegame-compatibility-after-economy-change into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/remove-savegame-compatibility-after-economy-change into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/remove-savegame-compatibility-after-economy-change/+merge/349390 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/remove-savegame-compatibility-after-economy-change. ___ 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/remove-savegame-compatibility-after-economy-change into lp:widelands
@bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/remove-savegame-compatibility-after-economy-change/+merge/349390 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/remove-savegame-compatibility-after-economy-change. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/frisian_music into lp:widelands
Benedikt Straub has proposed merging lp:~widelands-dev/widelands/frisian_music into lp:widelands. Commit message: Includes Klaus Halfmann's Frisian-style music Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1780804 in widelands: "A song for the Frisisans" https://bugs.launchpad.net/widelands/+bug/1780804 For more details, see: https://code.launchpad.net/~widelands-dev/widelands/frisian_music/+merge/350429 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/frisian_music into lp:widelands. === added file 'data/music/ingame_27.ogg' Binary files data/music/ingame_27.ogg 1970-01-01 00:00:00 + and data/music/ingame_27.ogg 2018-07-22 12:32:51 + differ ___ 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
