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

[GunChleoc]

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

[kaputtnik (franku)]

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

[GunChleoc (gunchleoc)]

Oops, should be fixed now.

[kaputtnik (franku)]

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

[kaputtnik (franku)]

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.

[bunnybot (widelandsofficial)]

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.

[GunChleoc (gunchleoc)]

> 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.

[bunnybot (widelandsofficial)]

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.

[kaputtnik (franku)]

I think it works now :-)

review: Approve (testing)

[Miroslav Remák (miroslavr256)]

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

[GunChleoc (gunchleoc)]

I like that much better, thank you!

@bunnybot merge

=== 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 @@
 
 	Widelands::Map & map = egbase().map();
 
-	// TODO(unknown): get rid of cleanup_for_load, it tends to be very messy
-	// Instead, delete and re-create the egbase.
-	egbase().cleanup_for_load();
+	cleanup_for_load();
 
 	std::unique_ptr<Widelands::MapLoader> ml(map.get_correct_loader(filename));
 	if (!ml.get())
@@ -245,6 +243,16 @@
 	map_changed(MapWas::kReplaced);
 }
 
+void EditorInteractive::cleanup_for_load()
+{
+	// TODO(unknown): get rid of cleanup_for_load, it tends to be very messy
+	// Instead, delete and re-create the egbase.
+	egbase().cleanup_for_load();
+
+	// Select a tool that doesn't care about map changes
+	mutable_field_overlay_manager()->register_overlay_callback_function(nullptr);
+}
+
 
 /// Called just before the editor starts, after postload, init and gfxload.
 void EditorInteractive::start() {

=== 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 @@
 	static void run_editor(const std::string & filename, const std::string& script_to_run);
 
 	void load(const std::string & filename);
+	void cleanup_for_load() override;
 
 	// leaf functions from base class
 	void start() override;

=== 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 @@
 	// Area around already placed players
 	Widelands::PlayerNumber const nr_players = map.get_nrplayers();
 	for (Widelands::PlayerNumber p = 1, last = current_player_ - 1;; ++p) {
-		for (; p <= last; ++p)
-			if (Widelands::Coords const sp = map.get_starting_pos(p))
-				if (map.calc_distance(sp, c) < MIN_PLACE_AROUND_PLAYERS)
+		for (; p <= last; ++p) {
+			if (Widelands::Coords const sp = map.get_starting_pos(p)) {
+				if (map.calc_distance(sp, c) < MIN_PLACE_AROUND_PLAYERS) {
 					return 0;
-		if (last == nr_players)
+				}
+			}
+		}
+		if (last == nr_players) {
 			break;
+		}
 		last = nr_players;
 	}
 
@@ -80,7 +84,7 @@
 EditorSetStartingPosTool::EditorSetStartingPosTool()
 	: EditorTool(*this, *this, false), current_sel_pic_(nullptr)
 {
-	current_player_ = 0;
+	current_player_ = 1;
 	fsel_picsname_ = "images/players/fsel_editor_set_player_01_pos.png";
 }
 

=== 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 @@
 
 	loader_ui.step(_("Creating empty map…"));
 
-	egbase.cleanup_for_load();
+	parent.cleanup_for_load();
 
 	map.create_empty_map(
 				egbase.world(),

=== 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 @@
 	Widelands::Map              & map    = egbase.map();
 	UI::ProgressWindow loader_ui;
 
-	egbase.cleanup_for_load();
+	eia.cleanup_for_load();
 
 	UniqueRandomMapInfo map_info;
 	set_map_info(map_info);