Merge lp:~widelands-dev/widelands/bug-1573968-new-map-crash into lp:widelands

Proposed by GunChleoc
Status: Merged
Merged at revision: 7987
Proposed branch: lp:~widelands-dev/widelands/bug-1573968-new-map-crash
Merge into: lp:widelands
Diff against target: 103 lines (+23/-10)
5 files modified
src/editor/editorinteractive.cc (+11/-3)
src/editor/editorinteractive.h (+1/-0)
src/editor/tools/set_starting_pos_tool.cc (+9/-5)
src/editor/ui_menus/main_menu_new_map.cc (+1/-1)
src/editor/ui_menus/main_menu_random_map.cc (+1/-1)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1573968-new-map-crash
Reviewer Review Type Date Requested Status
kaputtnik (community) testing Approve
Review via email: mp+292712@code.launchpad.net

Commit message

Select info tool before creating new maps. This fixes a crash with the player tool.

To post a comment you must log in.
Revision history for this message
kaputtnik (franku) wrote :

Now i get a crash when

1. Start editor
2. Place a player
3. Save the map
4. Load the previous saved map

Result:
widelands: /home/kaputtnik/widelands-repo/bug-1573968-new-map-crash/src/logic/map.h:205: Widelands::Coords Widelands::Map::get_starting_pos(Widelands::PlayerNumber) const: Zusicherung »1 <= p && p <= get_nrplayers()« nicht erfüllt.

review: Needs Fixing
Revision history for this message
GunChleoc (gunchleoc) wrote :

Oops, should be fixed now.

Revision history for this message
kaputtnik (franku) wrote :

I got other crashs of the same type, but couldn't reproduce it with explicit steps. Sometimes with the steps described earlier, sometimes when a map was loaded and then a new map is created ... feels randomly :-(

Are there no initial data for the editor to which it can be reset before a map is loaded or a new map is going to be created? It looks always that there is some data from the previous map when creating a new map or loading a map...

review: Needs Fixing
Revision history for this message
kaputtnik (franku) wrote :

I think it's the buildhelp overlay what's causing this. I can reproduce this now as follows:

1. Start editor
2. Deactivate buildhelp
3. Place player (the buildhelp should normally be activated, but it isn't)
4. Save map
5. Load previous saved map

Crash.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 1054. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/125345011.
Appveyor build 885. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1573968_new_map_crash-885.

Revision history for this message
GunChleoc (gunchleoc) wrote :

> Are there no initial data for the editor to which it can be reset before a map
> is loaded or a new map is going to be created? It looks always that there is
> some data from the previous map when creating a new map or loading a map...

There is already a TODO comment in the codebase for that. I am thinking of starting the next release cycle by going through TODO comments to clean stuff up.

I can't reproduce the crash now (maybe it doesn't happen often enough), but I have now added the same fix when loading a map. I think that should take care of it.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 1063. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/125501343.
Appveyor build 894. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1573968_new_map_crash-894.

Revision history for this message
kaputtnik (franku) wrote :

I think it works now :-)

review: Approve (testing)
Revision history for this message
Miroslav Remák (miroslavr256) wrote :

I think selecting info tool in EditorInteractive::cleanup_for_load is unnecessary and does not fix the problem at its root. I propose:
 - changing 'current_player_ = 0;' to 'current_player_ = 1;' in EditorSetStartingPosTool's constructor
 - replacing 'select_tool(tools()->info, EditorTool::First);' with 'mutable_field_overlay_manager()->register_overlay_callback_function(nullptr);' in EditorInteractive::cleanup_for_load

Revision history for this message
GunChleoc (gunchleoc) wrote :

I like that much better, thank you!

@bunnybot merge

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/editor/editorinteractive.cc'
--- src/editor/editorinteractive.cc 2016-04-14 16:23:39 +0000
+++ src/editor/editorinteractive.cc 2016-05-04 06:29:55 +0000
@@ -214,9 +214,7 @@
214214
215 Widelands::Map & map = egbase().map();215 Widelands::Map & map = egbase().map();
216216
217 // TODO(unknown): get rid of cleanup_for_load, it tends to be very messy217 cleanup_for_load();
218 // Instead, delete and re-create the egbase.
219 egbase().cleanup_for_load();
220218
221 std::unique_ptr<Widelands::MapLoader> ml(map.get_correct_loader(filename));219 std::unique_ptr<Widelands::MapLoader> ml(map.get_correct_loader(filename));
222 if (!ml.get())220 if (!ml.get())
@@ -245,6 +243,16 @@
245 map_changed(MapWas::kReplaced);243 map_changed(MapWas::kReplaced);
246}244}
247245
246void EditorInteractive::cleanup_for_load()
247{
248 // TODO(unknown): get rid of cleanup_for_load, it tends to be very messy
249 // Instead, delete and re-create the egbase.
250 egbase().cleanup_for_load();
251
252 // Select a tool that doesn't care about map changes
253 mutable_field_overlay_manager()->register_overlay_callback_function(nullptr);
254}
255
248256
249/// Called just before the editor starts, after postload, init and gfxload.257/// Called just before the editor starts, after postload, init and gfxload.
250void EditorInteractive::start() {258void EditorInteractive::start() {
251259
=== modified file 'src/editor/editorinteractive.h'
--- src/editor/editorinteractive.h 2016-04-14 16:23:39 +0000
+++ src/editor/editorinteractive.h 2016-05-04 06:29:55 +0000
@@ -92,6 +92,7 @@
92 static void run_editor(const std::string & filename, const std::string& script_to_run);92 static void run_editor(const std::string & filename, const std::string& script_to_run);
9393
94 void load(const std::string & filename);94 void load(const std::string & filename);
95 void cleanup_for_load() override;
9596
96 // leaf functions from base class97 // leaf functions from base class
97 void start() override;98 void start() override;
9899
=== modified file 'src/editor/tools/set_starting_pos_tool.cc'
--- src/editor/tools/set_starting_pos_tool.cc 2016-04-06 09:23:04 +0000
+++ src/editor/tools/set_starting_pos_tool.cc 2016-05-04 06:29:55 +0000
@@ -60,12 +60,16 @@
60 // Area around already placed players60 // Area around already placed players
61 Widelands::PlayerNumber const nr_players = map.get_nrplayers();61 Widelands::PlayerNumber const nr_players = map.get_nrplayers();
62 for (Widelands::PlayerNumber p = 1, last = current_player_ - 1;; ++p) {62 for (Widelands::PlayerNumber p = 1, last = current_player_ - 1;; ++p) {
63 for (; p <= last; ++p)63 for (; p <= last; ++p) {
64 if (Widelands::Coords const sp = map.get_starting_pos(p))64 if (Widelands::Coords const sp = map.get_starting_pos(p)) {
65 if (map.calc_distance(sp, c) < MIN_PLACE_AROUND_PLAYERS)65 if (map.calc_distance(sp, c) < MIN_PLACE_AROUND_PLAYERS) {
66 return 0;66 return 0;
67 if (last == nr_players)67 }
68 }
69 }
70 if (last == nr_players) {
68 break;71 break;
72 }
69 last = nr_players;73 last = nr_players;
70 }74 }
7175
@@ -80,7 +84,7 @@
80EditorSetStartingPosTool::EditorSetStartingPosTool()84EditorSetStartingPosTool::EditorSetStartingPosTool()
81 : EditorTool(*this, *this, false), current_sel_pic_(nullptr)85 : EditorTool(*this, *this, false), current_sel_pic_(nullptr)
82{86{
83 current_player_ = 0;87 current_player_ = 1;
84 fsel_picsname_ = "images/players/fsel_editor_set_player_01_pos.png";88 fsel_picsname_ = "images/players/fsel_editor_set_player_01_pos.png";
85}89}
8690
8791
=== modified file 'src/editor/ui_menus/main_menu_new_map.cc'
--- src/editor/ui_menus/main_menu_new_map.cc 2016-04-06 09:23:04 +0000
+++ src/editor/ui_menus/main_menu_new_map.cc 2016-05-04 06:29:55 +0000
@@ -121,7 +121,7 @@
121121
122 loader_ui.step(_("Creating empty map…"));122 loader_ui.step(_("Creating empty map…"));
123123
124 egbase.cleanup_for_load();124 parent.cleanup_for_load();
125125
126 map.create_empty_map(126 map.create_empty_map(
127 egbase.world(),127 egbase.world(),
128128
=== modified file 'src/editor/ui_menus/main_menu_random_map.cc'
--- src/editor/ui_menus/main_menu_random_map.cc 2016-04-23 13:33:53 +0000
+++ src/editor/ui_menus/main_menu_random_map.cc 2016-05-04 06:29:55 +0000
@@ -432,7 +432,7 @@
432 Widelands::Map & map = egbase.map();432 Widelands::Map & map = egbase.map();
433 UI::ProgressWindow loader_ui;433 UI::ProgressWindow loader_ui;
434434
435 egbase.cleanup_for_load();435 eia.cleanup_for_load();
436436
437 UniqueRandomMapInfo map_info;437 UniqueRandomMapInfo map_info;
438 set_map_info(map_info);438 set_map_info(map_info);

Subscribers

People subscribed via source and target branches

to status/vote changes: