[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1782593-segfault-random-map into lp:widelands
Notabilis has proposed merging
lp:~widelands-dev/widelands/bug-1782593-segfault-random-map into lp:widelands.
Commit message:
Fixing failed assert in random map generator and fixing bug in code for
selection of start position.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1782593 in widelands: "Segfault in random map generator"
https://bugs.launchpad.net/widelands/+bug/1782593
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1782593-segfault-random-map/+merge/350384
Fixing the broken assert was a simple case of too late initialization.
The fixed bug regarding the starting position was an unrelated bug which
resulted in the map generator nearly always failing to find a valid start
position for a player. Due to the wrong initialization of the min_distance the
following comparison always failed to accept one of the valid positions as
start position.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1782593-segfault-random-map into lp:widelands.
=== modified file 'src/editor/map_generator.cc'
--- src/editor/map_generator.cc 2018-05-11 04:48:10 +
+++ src/editor/map_generator.cc 2018-07-21 08:03:23 +
@@ -659,9 +659,10 @@
map_.recalc_whole_map(egbase_.world());
// Care about players and place their start positions
+ map_.set_nrplayers(map_info_.numPlayers);
+ assert(map_info_.numPlayers >= 1);
const std::string tribe = map_.get_scenario_player_tribe(1);
const std::string ai = map_.get_scenario_player_ai(1);
- map_.set_nrplayers(map_info_.numPlayers);
FindNodeSize functor(FindNodeSize::sizeBig);
Coords playerstart(Coords::null());
@@ -754,7 +755,7 @@
map_.find_fields(Area(map_.get_fcoords(playerstart), 20), &coords, functor);
// Take the nearest ones
- uint32_t min_distance = 0;
+ uint32_t min_distance = std::numeric_limits::max();
Coords coords2;
for (uint16_t i = 0; i < coords.size(); ++i) {
uint32_t test = map_.calc_distance(coords[i], playerstart);
___
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-1782593-segfault-random-map into lp:widelands
LGTM :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1782593-segfault-random-map/+merge/350384 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1782593-segfault-random-map 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-1782593-segfault-random-map into lp:widelands
So bug 1580892 (Random Map Generator doesn't always find viable player starting positions) is also fixed with this branch? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1782593-segfault-random-map/+merge/350384 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1782593-segfault-random-map 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-1782593-segfault-random-map into lp:widelands
Thanks for the review and for looking up that bug. It is not totally fixed. In current trunk it nearly never finds a valid position, while with this branch it has a fair chance to assign valid start positions. Its still random based and can fail but it fails much less frequent that before (tested with ... uh ... around two created maps). I guess with the current design it is pretty much impossible to always guarantee viable start positions without major changes to the map generator. As far as I am concerned, we could mark the bug (partially) fixed and reopen it if someone complains. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1782593-segfault-random-map/+merge/350384 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1782593-segfault-random-map 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/bug-1782593-segfault-random-map into lp:widelands
Continuous integration builds have changed state: Travis build 3698. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/406542858. Appveyor build 3497. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1782593_segfault_random_map-3497. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1782593-segfault-random-map/+merge/350384 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1782593-segfault-random-map 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/bug-1782593-segfault-random-map into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1782593-segfault-random-map into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1782593-segfault-random-map/+merge/350384 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1782593-segfault-random-map 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/remove-savegame-compatibility-after-economy-change into lp:widelands
Continuous integration builds have changed state: Travis build 3699. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/406553761. Appveyor build 3498. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_remove_savegame_compatibility_after_economy_change-3498. -- 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
[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
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
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
[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
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
