Merge lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe into lp:widelands

[GunChleoc]

Empty player tribes are interpreted as random tribe. Random Map Generator now assigns a random tribe to all players.

- Fix crash when loading a map in the editor where there is a player with no
  tribe assigned: player loading removed, because it's unused in the editor
  anyway
- Map Editors now can choose "Random" tribe in the player menu
- If player tribe is empty in map, set random player in singleplayer game
  setup

I decided not to bother with any UI changes at this point - we have more important bugs to fix for Build 20.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3777. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/415165379.
Appveyor build 3576. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1783878_editor_random_map_tribe-3576.

[Notabilis (notabilis27)]

Code is looking okay and the editor no longer crashes. Two observations:

- When loading the old/broken maps, we still get the "Tribe not known" message. Maybe only display a "note:" message and select a default tribe in that case? Having the wrong tribe is in most cases probably preferable to not being able to use the map at all. Unknown tribes in maps could also happen when "backporting" maps from (newer) game versions where more/custom tribes are available.

- When loading the "broken" map in the editor and saving again the map is still broken. This one is actually a bit strange, shouldn't the change in editorinteractive.cc avoid this as well?

review: Approve

[GunChleoc (gunchleoc)]

Actually, the players loaded into the editor were never used, so I kicked them out. I changed the semantics so that an empty tribe means "pick at random."

review: Needs Resubmitting

[Notabilis (notabilis27)]

I like the proposed change. However, when starting the editor and clicking the "Players" button I get an exception:

widelands: ../src/logic/map.cc:411: const string& Widelands::Map::get_scenario_player_tribe(Widelands::PlayerNumber) const: Assertion `p <= get_nrplayers()' failed.

Problem is the change in diff line 81, where p can be greater than the number of active players.

Since the editor-user has the option to do so now anyway: Select the "random" tribe in line 42 of the diff?

review: Needs Fixing

[Klaus Halfmann (klaus-halfmann)]

Checked out and compiled the branch, reproduced the bug on trunk.
(SEGV on unknown address 0x0000000000f0 ...)

This version loads the map without any hazzles.

* Bug: when creating a new Random Map (after loading one)
  the number of players is stuck at 2
  and cannot be increased unless in is decreased

* Bug(?): Creating a new Random map (after wokaround above) and then opeing the player
   dialog fails with an Assertion
   WidelandsMapLoader::load_map_complete() for 'Radom42' took 216ms
============== Generating Map ==============
ID: bw6g-v7c3-gxc2-sans-t4ys-muda
Random number: 42
Dimensions: 80 x 80
Players: 3
World: greenland
Resources: high
Land: 0,50 Water: 0,20 Wasteland: 0,10
Using Island Mode

WARNING: Could not find a suitable place for player 2
WARNING: Could not find a suitable place for player 3
atlanteans trainingsites: We have used up 100% on 2 sites, there are 0 without
barbarians trainingsites: We have used up 100% on 2 sites, there are 0 without
empire trainingsites: We have used up 100% on 3 sites, there are 0 without
frisians trainingsites: We have used up 100% on 2 sites, there are 0 without
Assertion failed: (p <= get_nrplayers()), function get_scenario_player_tribe, file .../bug-1783878_editor_random_map_tribe/src/logic/map.cc, line 411.
Abort trap: 6

Can be reproduced by recreating the map via the ID: bw6g-v7c3-gxc2-sans-t4ys-muda

Code looks ok for me, otherwise.

review: Needs Fixing

[GunChleoc (gunchleoc)]

Changes are done. Could you please retest to make sure that I haven't missed anything?

[ypopezios (ypopezios)]

Added inline comment.

[Klaus Halfmann (klaus-halfmann)]

Works for me,

I cannot judge mutch abouthe the code.

ypopezios: you are correct, but lets get rid of the bug first
and do the improvement in some second step.

Gun: will we need a resubmit before wer can merge this?

@bonnybot merge

review: Approve (compile, review, test,)

[GunChleoc (gunchleoc)]

@ypopezios: Good idea, but not as easy as it sounds - when set to "random", the tribe is already sneakily assigned in the background so that the user can choose an initialization ("Headquarters") etc. So, removing that extra variable has a lot of other implications in the code as well and is not trivial at all. So, definitely not for Build 20 ;)

[GunChleoc (gunchleoc)]

@bunnybot merge

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3841. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/420890791.
Appveyor build 3639. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1783878_editor_random_map_tribe-3639.

[bunnybot (widelandsofficial)]

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 3841. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/420890791.

[GunChleoc (gunchleoc)]

@bunnybot merge

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3850. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/421503400.
Appveyor build 3648. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1783878_editor_random_map_tribe-3648.

=== modified file 'src/editor/editorinteractive.cc'
--- src/editor/editorinteractive.cc	2018-07-20 08:42:23 +0000
+++ src/editor/editorinteractive.cc	2018-08-28 08:07:51 +0000
@@ -177,10 +177,13 @@
 	load_all_tribes(&egbase(), &loader_ui);
 
 	// Create the players. TODO(SirVer): this must be managed better
+	// TODO(GunChleoc): Ugly - we only need this for the test suite right now
 	loader_ui.step(_("Creating players"));
 	iterate_player_numbers(p, map->get_nrplayers()) {
-		egbase().add_player(
-		   p, 0, map->get_scenario_player_tribe(p), map->get_scenario_player_name(p));
+		if (!map->get_scenario_player_tribe(p).empty()) {
+			egbase().add_player(
+			   p, 0, map->get_scenario_player_tribe(p), map->get_scenario_player_name(p));
+		}
 	}
 
 	ml->load_map_complete(egbase(), Widelands::MapLoader::LoadType::kEditor);
 
=== modified file 'src/editor/map_generator.cc'
--- src/editor/map_generator.cc	2018-07-21 07:53:42 +0000
+++ src/editor/map_generator.cc	2018-08-28 08:07:51 +0000
@@ -28,6 +28,7 @@
 #include "logic/editor_game_base.h"
 #include "logic/findnode.h"
 #include "logic/map.h"
+#include "logic/map_objects/tribes/tribe_basic_info.h"
 #include "logic/map_objects/world/map_gen.h"
 #include "logic/map_objects/world/world.h"
 #include "scripting/lua_interface.h"
@@ -661,7 +662,6 @@
 	// 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);
 	FindNodeSize functor(FindNodeSize::sizeBig);
 	Coords playerstart(Coords::null());
@@ -714,7 +714,7 @@
 	for (PlayerNumber n = 1; n <= map_info_.numPlayers; ++n) {
 		// Set scenario information - needed even if it's not a scenario
 		map_.set_scenario_player_name(n, _("Random Player"));
-		map_.set_scenario_player_tribe(n, tribe);
+		map_.set_scenario_player_tribe(n, "");
 		map_.set_scenario_player_ai(n, ai);
 		map_.set_scenario_player_closeable(n, false);
 
 
=== modified file 'src/editor/ui_menus/player_menu.cc'
--- src/editor/ui_menus/player_menu.cc	2018-07-13 11:23:52 +0000
+++ src/editor/ui_menus/player_menu.cc	2018-08-28 08:07:51 +0000
@@ -129,8 +129,7 @@
                     24,
                     _("Number of players"),
                     UI::DropdownType::kTextual,
-                    UI::PanelStyle::kWui),
-     default_tribe_(Widelands::get_all_tribenames().front()) {
+                    UI::PanelStyle::kWui) {
 	box_.set_size(100, 100);  // Prevent assert failures
 	box_.add(&no_of_players_, UI::Box::Resizing::kFullSize);
 	box_.add_space(2 * kMargin);
@@ -146,7 +145,7 @@
 		mutable_map->set_scenario_player_closeable(1, false);
 		/** TRANSLATORS: Default player name, e.g. Player 1 */
 		mutable_map->set_scenario_player_name(1, (boost::format(_("Player %u")) % 1).str());
-		mutable_map->set_scenario_player_tribe(1, default_tribe_);
+		mutable_map->set_scenario_player_tribe(1, "");
 		eia().set_need_save(true);
 	}
 
@@ -179,11 +178,13 @@
 				plr_tribe->add(_(tribeinfo.descname), tribeinfo.name,
 				               g_gr->images().get(tribeinfo.icon), false, tribeinfo.tooltip);
 			}
+			plr_tribe->add(pgettext("tribe", "Random"),  "",
+							g_gr->images().get("images/ui_fsmenu/random.png"), false,
+							_("The tribe will be selected at random"));
 		}
-		const std::string player_scenario_tribe =
-		   map_has_player ? map.get_scenario_player_tribe(p) : default_tribe_;
-		plr_tribe->select(Widelands::tribe_exists(player_scenario_tribe) ? player_scenario_tribe :
-		                                                                   default_tribe_);
+
+		plr_tribe->select((p < map.get_nrplayers() && Widelands::tribe_exists(map.get_scenario_player_tribe(p))) ? map.get_scenario_player_tribe(p) :
+		                                                                   "");
 		plr_tribe->selected.connect(
 		   boost::bind(&EditorPlayerMenu::player_tribe_clicked, boost::ref(*this), p - 1));
 		row->add(plr_tribe);
@@ -268,7 +269,7 @@
 			rows_.at(pn - 1)->name->set_text(name);
 
 			const std::string& tribename = rows_.at(pn - 1)->tribe->get_selected();
-			assert(Widelands::tribe_exists(tribename));
+			assert(tribename.empty() || Widelands::tribe_exists(tribename));
 			map->set_scenario_player_tribe(pn, tribename);
 			rows_.at(pn - 1)->box->set_visible(true);
 		}
@@ -292,7 +293,7 @@
 
 void EditorPlayerMenu::player_tribe_clicked(size_t row) {
 	const std::string& tribename = rows_.at(row)->tribe->get_selected();
-	assert(Widelands::tribe_exists(tribename));
+	assert(tribename.empty() || Widelands::tribe_exists(tribename));
 	EditorInteractive& menu = eia();
 	menu.egbase().mutable_map()->set_scenario_player_tribe(row + 1, tribename);
 	menu.set_need_save(true);
 
=== modified file 'src/editor/ui_menus/player_menu.h'
--- src/editor/ui_menus/player_menu.h	2018-07-08 07:57:22 +0000
+++ src/editor/ui_menus/player_menu.h	2018-08-28 08:07:51 +0000
@@ -70,8 +70,6 @@
 	UI::Box box_;
 	UI::Dropdown<uintptr_t> no_of_players_;
 	std::vector<std::unique_ptr<PlayerEditRow>> rows_;
-
-	const std::string default_tribe_;
 };
 
 #endif  // end of include guard: WL_EDITOR_UI_MENUS_PLAYER_MENU_H
 
=== modified file 'src/ui_fsmenu/launch_spg.cc'
--- src/ui_fsmenu/launch_spg.cc	2018-07-07 20:22:12 +0000
+++ src/ui_fsmenu/launch_spg.cc	2018-08-28 08:07:51 +0000
@@ -283,7 +283,14 @@
 	Widelands::PlayerNumber const nrplayers = map.get_nrplayers();
 	for (uint8_t i = 0; i < nrplayers; ++i) {
 		settings_->set_player_name(i, map.get_scenario_player_name(i + 1));
-		settings_->set_player_tribe(i, map.get_scenario_player_tribe(i + 1));
+		const std::string playertribe = map.get_scenario_player_tribe(i + 1);
+		if (playertribe.empty()) {
+			// Set tribe selection to random
+			settings_->set_player_tribe(i, "", true);
+		} else {
+			// Set tribe selection from map
+			settings_->set_player_tribe(i, playertribe);
+		}
 	}
 }