Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1818494-reset-zoom-crash into lp:widelands
Wow, we should get the Map(s) this user created :-)
To reproduce the first problem we should edit the biggest maxium possible
map with as many objects a possible and zomm out and in like mad.
Never restart to get the overflow in that undo stack?.
An in a normal game this could happen, too?
Can we perhaps limit the maximum Zoom-factor to avoid this?
Some questions / comments inline.
Diff comments:
> === modified file 'src/editor/tools/history.cc'
> --- src/editor/tools/history.cc 2019-02-23 11:00:49 +
> +++ src/editor/tools/history.cc 2019-03-10 07:49:40 +
> @@ -115,6 +118,11 @@
> undo_stack_.push_front(ac);
> undo_button_.set_enabled(true);
> redo_button_.set_enabled(false);
> + if (undo_stack_.size() > kMaximumUndoActions) {
> + for (size_t i = 0; i < kTooManyUndoActionsDeleteBatch;
> ++i) {
> + undo_stack_.pop_back();
> + }
> + }
A stack has no bulk operation fo this?
> }
> return tool.handle_click(ind, world, center, parent, ac.args, &map);
> }
>
> === modified file 'src/graphic/gl/fields_to_draw.cc'
> --- src/graphic/gl/fields_to_draw.cc 2019-02-23 11:00:49 +
> +++ src/graphic/gl/fields_to_draw.cc 2019-03-10 07:49:40 +
> @@ -106,7 +106,17 @@
>
> w_ = max_fx_ - min_fx_ + 1;
This is FieldsToDraw::reset(), which is called when ctrl-0 or the Reset-zoom
Button is pressed.
If this correct we should spend some comments on this function and its
parameters.
> h_ = max_fy_ - min_fy_ + 1;
> - const size_t dimension = w_ * h_;
> + assert(w_ > 0);
> + assert(h_ > 0);
> +
> + // Ensure that there is enough memory for the resize operation
> + size_t dimension = w_ * h_;
> + const size_t max_dimension = fields_.max_size();
> + if (dimension > max_dimension) {
> + log("WARNING: Not enough memory allocated to redraw the whole
> map!\nWe recommend that you restart Widelands\n");
> + dimension = max_dimension;
> + }
> + // Now resize the vector
> if (fields_.size() != dimension) {
> fields_.resize(dimension);
> }
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1818494-reset-zoom-crash/+merge/364208
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1818494-reset-zoom-crash 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-1818494-reset-zoom-crash into lp:widelands
Review: Approve code reieww, compile, testing Found a (related?) Bug at #1819311 before I coould do the > 500 undos :-) OTOH from the code review it sould not harm. I will do more tests with this huge (nice) map, lets see what we will find. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1818494-reset-zoom-crash/+merge/364208 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1818494-reset-zoom-crash. ___ 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-website/update_third_party_apps into lp:widelands-website
Updating the packages by hand did the work... Sorry for the amazing amount of server errors :[] I didn't recognized that i had updated django-registration and started the website again, without merging this branch. -- https://code.launchpad.net/~widelands-dev/widelands-website/update_third_party_apps/+merge/364024 Your team Widelands Developers is subscribed to branch lp:widelands-website. ___ 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-website/update_third_party_apps into lp:widelands-website
The proposal to merge lp:~widelands-dev/widelands-website/update_third_party_apps into lp:widelands-website has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands-website/update_third_party_apps/+merge/364024 -- Your team Widelands Developers is subscribed to branch lp:widelands-website. ___ 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-1817607-macos-build into lp:widelands
Review: Approve Still works on my old Mac to build the nightlies. I did not yet achieve to build a newer version on my new mac, but I believe this to be an issue with my build environment. This is good to go, imho. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1817607-macos-build/+merge/364204 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1817607-macos-build. ___ 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/valgrind into lp:widelands
GunChleoc has proposed merging lp:~widelands-dev/widelands/valgrind into
lp:widelands.
Commit message:
Update Valgrind suppressions and fix uninitialized variables reported by
Valgrind Memcheck
- Uninitialized variable in default AI
- Uninitialized variables in Building Statistics
- Exclude more errors from zip filesystem, Eris and graphics drivers
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/valgrind/+merge/364213
I used Valgrind to have a look at memory errors caused by Widelands code. Found
a few uninitialized variables. Our code seems to be pretty clean already thanks
to ASan :)
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/valgrind into lp:widelands.
=== modified file 'src/ai/defaultai.cc'
--- src/ai/defaultai.cc 2019-02-23 11:00:49 +
+++ src/ai/defaultai.cc 2019-03-10 11:42:22 +
@@ -113,6 +113,7 @@
ts_in_const_count_(0),
ts_without_trainers_(0),
enemysites_check_delay_(30),
+ spots_(0),
resource_necessity_water_needed_(false),
highest_nonmil_prio_(0),
expedition_ship_(kNoShip) {
=== modified file 'src/wui/building_statistics_menu.cc'
--- src/wui/building_statistics_menu.cc 2019-02-23 11:00:49 +
+++ src/wui/building_statistics_menu.cc 2019-03-10 11:42:22 +
@@ -111,6 +111,11 @@
kButtonHeight,
"",
UI::Align::kRight),
+ current_building_type_(INVALID_INDEX),
+ last_building_index_(0),
+ last_building_type_(INVALID_INDEX),
+ lastupdate_(0),
+ was_minimized_(false),
low_production_(33),
has_selection_(false),
nr_building_types_(parent.egbase().tribes().nrbuildings()) {
=== modified file 'valgrind.supp'
--- valgrind.supp 2019-03-10 09:07:24 +
+++ valgrind.supp 2019-03-10 11:42:22 +
@@ -48,6 +48,13 @@
...
}
{
+ driver1
+ Memcheck:Addr1
+ ...
+ obj:*965_dri.so
+ ...
+}
+{
driver2
Memcheck:Addr2
...
@@ -69,6 +76,13 @@
...
}
{
+ driver16
+ Memcheck:Addr16
+ ...
+ obj:*i965_dri.so
+ ...
+}
+{
SDL2Leak
Memcheck:Leak
...
@@ -95,3 +109,39 @@
obj:*linux-gnu/lib*
...
}
+{
+ ZipWriteValue8
+ Memcheck:Value8
+ ...
+ fun:zipWriteInFileInZip
+ ...
+}
+{
+ ZipWriteCond
+ Memcheck:Cond
+ ...
+ fun:zipWriteInFileInZip
+ ...
+}
+{
+ ZipCloseCond
+ Memcheck:Cond
+ ...
+ fun:zipCloseFileInZip
+ ...
+}
+{
+ ZipCloseParam
+ Memcheck:Param
+ write(buf)
+ ...
+ fun:zipCloseFileInZip
+ ...
+}
+{
+ ErisLeak
+ Memcheck:Leak
+ ...
+ fun:l_alloc
+ ...
+}
___
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/valgrind into lp:widelands
Review: Approve code review Code looks fine, will not do any further tests. -- https://code.launchpad.net/~widelands-dev/widelands/valgrind/+merge/364213 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/valgrind. ___ 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-1818494-reset-zoom-crash into lp:widelands
Continuous integration builds have changed state: Travis build 4574. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/504234539. Appveyor build 4361. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1818494_reset_zoom_crash-4361. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1818494-reset-zoom-crash/+merge/364208 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1818494-reset-zoom-crash. ___ 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-1818494-reset-zoom-crash into lp:widelands
Thanks for the review!
@bunnybot merge
Diff comments:
> === modified file 'src/editor/tools/history.cc'
> --- src/editor/tools/history.cc 2019-02-23 11:00:49 +
> +++ src/editor/tools/history.cc 2019-03-10 07:49:40 +
> @@ -115,6 +118,11 @@
> undo_stack_.push_front(ac);
> undo_button_.set_enabled(true);
> redo_button_.set_enabled(false);
> + if (undo_stack_.size() > kMaximumUndoActions) {
> + for (size_t i = 0; i < kTooManyUndoActionsDeleteBatch;
> ++i) {
> + undo_stack_.pop_back();
> + }
> + }
Good point - there is: http://www.cplusplus.com/reference/deque/deque/erase/
This is non-trivial though, because it can't trivially move the contents. It's
also not exception-safe, so I'll leave things as is for now. I don't think that
we can gain much performance here anyway.
> }
> return tool.handle_click(ind, world, center, parent, ac.args, &map);
> }
>
> === modified file 'src/graphic/gl/fields_to_draw.cc'
> --- src/graphic/gl/fields_to_draw.cc 2019-02-23 11:00:49 +
> +++ src/graphic/gl/fields_to_draw.cc 2019-03-10 07:49:40 +
> @@ -106,7 +106,17 @@
>
> w_ = max_fx_ - min_fx_ + 1;
It is called every time the panel thinks, actually. It skips the vector resize
when the size has not changed, to that bis is triggered by any change in the
zoom factor, or when toggling fullscreen mode.
> h_ = max_fy_ - min_fy_ + 1;
> - const size_t dimension = w_ * h_;
> + assert(w_ > 0);
> + assert(h_ > 0);
> +
> + // Ensure that there is enough memory for the resize operation
> + size_t dimension = w_ * h_;
> + const size_t max_dimension = fields_.max_size();
> + if (dimension > max_dimension) {
> + log("WARNING: Not enough memory allocated to redraw the whole
> map!\nWe recommend that you restart Widelands\n");
> + dimension = max_dimension;
> + }
> + // Now resize the vector
> if (fields_.size() != dimension) {
> fields_.resize(dimension);
> }
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1818494-reset-zoom-crash/+merge/364208
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1818494-reset-zoom-crash.
___
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-1817607-macos-build into lp:widelands
Thanks for testing! @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1817607-macos-build/+merge/364204 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1817607-macos-build. ___ 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/valgrind into lp:widelands
I think I have tested this sufficiently. Thanks for the review @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/valgrind/+merge/364213 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/valgrind. ___ 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/speedup_saveloading into lp:widelands
Review: Needs Fixing travis Regressiontestsfail, though -- https://code.launchpad.net/~widelands-dev/widelands/speedup_saveloading/+merge/364205 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/speedup_saveloading. ___ 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-1818494-reset-zoom-crash into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1818494-reset-zoom-crash into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1818494-reset-zoom-crash/+merge/364208 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1818494-reset-zoom-crash. ___ 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-1817607-macos-build into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1817607-macos-build into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1817607-macos-build/+merge/364204 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1817607-macos-build. ___ 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/valgrind into lp:widelands
Continuous integration builds have changed state: Travis build 4576. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/504273300. Appveyor build 4363. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_valgrind-4363. -- https://code.launchpad.net/~widelands-dev/widelands/valgrind/+merge/364213 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/valgrind. ___ 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/valgrind into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/valgrind into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/valgrind/+merge/364213 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/valgrind. ___ 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/speedup_saveloading into lp:widelands
I forgot to adapt the tests to the new data types. Should be working now. -- https://code.launchpad.net/~widelands-dev/widelands/speedup_saveloading/+merge/364205 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/speedup_saveloading. ___ 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/speedup_saveloading into lp:widelands
Continuous integration builds have changed state: Travis build 4579. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/504410525. Appveyor build 4366. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_speedup_saveloading-4366. -- https://code.launchpad.net/~widelands-dev/widelands/speedup_saveloading/+merge/364205 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/speedup_saveloading. ___ 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/speedup_saveloading into lp:widelands
Review: Approve -- https://code.launchpad.net/~widelands-dev/widelands/speedup_saveloading/+merge/364205 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/speedup_saveloading. ___ 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/speedup_saveloading into lp:widelands
Thanks for the review! @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/speedup_saveloading/+merge/364205 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/speedup_saveloading. ___ 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/speedup_saveloading into lp:widelands
Here are my values from my old laptop. Seem this branch makes loading slower? Version bzr9012[speedup_saveloading] (Release) WidelandsMapLoader::load_map_complete() for 'Magic Mountain' took 5136ms GameLoader::load() took 11895ms MapSaver::save() for 'Magic Mountain' took 12376ms MapSaver::save() for 'Magic Mountain' took 12096ms GameSaver::save() took 13103ms WidelandsMapLoader::load_map_complete() for 'Magic Mountain' took 7798ms GameLoader::load() took 7921ms Version bzr9008[trunk] (Release) WidelandsMapLoader::load_map_complete() for 'Magic Mountain' took 5042ms GameLoader::load() took 12061ms MapSaver::save() for 'Magic Mountain' took 11165ms MapSaver::save() for 'Magic Mountain' took 11309ms GameSaver::save() took 12220ms WidelandsMapLoader::load_map_complete() for 'Magic Mountain' took 7495ms GameLoader::load() took 7617ms -- https://code.launchpad.net/~widelands-dev/widelands/speedup_saveloading/+merge/364205 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/speedup_saveloading. ___ 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
