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

Proposed by GunChleoc on 2018-08-12
Status: Merged
Merged at revision: 8800
Proposed branch: lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe
Merge into: lp:widelands
Diff against target: 139 lines (+25/-16)
5 files modified
src/editor/editorinteractive.cc (+5/-2)
src/editor/map_generator.cc (+2/-2)
src/editor/ui_menus/player_menu.cc (+10/-9)
src/editor/ui_menus/player_menu.h (+0/-2)
src/ui_fsmenu/launch_spg.cc (+8/-1)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe
Reviewer Review Type Date Requested Status
Klaus Halfmann compile, review, test, Approve on 2018-08-26
Notabilis 2018-08-12 Needs Fixing on 2018-08-24
GunChleoc Resubmit on 2018-08-24
Review via email: mp+352943@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
bunnybot (widelandsofficial) wrote :

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) wrote :

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
8777. By GunChleoc on 2018-08-24

If player tribe is empty in map, set random player in singleplayer game setup.

8778. By GunChleoc on 2018-08-24

Map Editors now can choose "Random" tribe in the player menu. Removed superfluous player loading from editorinteractive.cc.

8779. By GunChleoc on 2018-08-24

Merged trunk.

GunChleoc (gunchleoc) wrote :

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: Resubmit
Notabilis (notabilis27) wrote :

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) wrote :

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
8780. By GunChleoc on 2018-08-26

Fixed assert failure in editor player menu. The random map generator now assigns the empty tribe.

8781. By GunChleoc on 2018-08-26

Asserts allow empty tribe in editor player menu.

8782. By GunChleoc on 2018-08-26

Merged trunk.

GunChleoc (gunchleoc) wrote :

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

ypopezios (ypopezios) wrote :

Added inline comment.

Klaus Halfmann (klaus-halfmann) wrote :

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) wrote :

@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) wrote :

@bunnybot merge

bunnybot (widelandsofficial) wrote :

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) wrote :

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.

8783. By GunChleoc on 2018-08-28

Unbreak the test suite.

8784. By GunChleoc on 2018-08-28

Merged trunk.

GunChleoc (gunchleoc) wrote :

@bunnybot merge

bunnybot (widelandsofficial) wrote :

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.

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 2018-07-20 08:42:23 +0000
+++ src/editor/editorinteractive.cc 2018-08-28 08:07:51 +0000
@@ -177,10 +177,13 @@
177 load_all_tribes(&egbase(), &loader_ui);177 load_all_tribes(&egbase(), &loader_ui);
178178
179 // Create the players. TODO(SirVer): this must be managed better179 // Create the players. TODO(SirVer): this must be managed better
180 // TODO(GunChleoc): Ugly - we only need this for the test suite right now
180 loader_ui.step(_("Creating players"));181 loader_ui.step(_("Creating players"));
181 iterate_player_numbers(p, map->get_nrplayers()) {182 iterate_player_numbers(p, map->get_nrplayers()) {
182 egbase().add_player(183 if (!map->get_scenario_player_tribe(p).empty()) {
183 p, 0, map->get_scenario_player_tribe(p), map->get_scenario_player_name(p));184 egbase().add_player(
185 p, 0, map->get_scenario_player_tribe(p), map->get_scenario_player_name(p));
186 }
184 }187 }
185188
186 ml->load_map_complete(egbase(), Widelands::MapLoader::LoadType::kEditor);189 ml->load_map_complete(egbase(), Widelands::MapLoader::LoadType::kEditor);
187190
=== 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 @@
28#include "logic/editor_game_base.h"28#include "logic/editor_game_base.h"
29#include "logic/findnode.h"29#include "logic/findnode.h"
30#include "logic/map.h"30#include "logic/map.h"
31#include "logic/map_objects/tribes/tribe_basic_info.h"
31#include "logic/map_objects/world/map_gen.h"32#include "logic/map_objects/world/map_gen.h"
32#include "logic/map_objects/world/world.h"33#include "logic/map_objects/world/world.h"
33#include "scripting/lua_interface.h"34#include "scripting/lua_interface.h"
@@ -661,7 +662,6 @@
661 // Care about players and place their start positions662 // Care about players and place their start positions
662 map_.set_nrplayers(map_info_.numPlayers);663 map_.set_nrplayers(map_info_.numPlayers);
663 assert(map_info_.numPlayers >= 1);664 assert(map_info_.numPlayers >= 1);
664 const std::string tribe = map_.get_scenario_player_tribe(1);
665 const std::string ai = map_.get_scenario_player_ai(1);665 const std::string ai = map_.get_scenario_player_ai(1);
666 FindNodeSize functor(FindNodeSize::sizeBig);666 FindNodeSize functor(FindNodeSize::sizeBig);
667 Coords playerstart(Coords::null());667 Coords playerstart(Coords::null());
@@ -714,7 +714,7 @@
714 for (PlayerNumber n = 1; n <= map_info_.numPlayers; ++n) {714 for (PlayerNumber n = 1; n <= map_info_.numPlayers; ++n) {
715 // Set scenario information - needed even if it's not a scenario715 // Set scenario information - needed even if it's not a scenario
716 map_.set_scenario_player_name(n, _("Random Player"));716 map_.set_scenario_player_name(n, _("Random Player"));
717 map_.set_scenario_player_tribe(n, tribe);717 map_.set_scenario_player_tribe(n, "");
718 map_.set_scenario_player_ai(n, ai);718 map_.set_scenario_player_ai(n, ai);
719 map_.set_scenario_player_closeable(n, false);719 map_.set_scenario_player_closeable(n, false);
720720
721721
=== 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 @@
129 24,129 24,
130 _("Number of players"),130 _("Number of players"),
131 UI::DropdownType::kTextual,131 UI::DropdownType::kTextual,
132 UI::PanelStyle::kWui),132 UI::PanelStyle::kWui) {
133 default_tribe_(Widelands::get_all_tribenames().front()) {
134 box_.set_size(100, 100); // Prevent assert failures133 box_.set_size(100, 100); // Prevent assert failures
135 box_.add(&no_of_players_, UI::Box::Resizing::kFullSize);134 box_.add(&no_of_players_, UI::Box::Resizing::kFullSize);
136 box_.add_space(2 * kMargin);135 box_.add_space(2 * kMargin);
@@ -146,7 +145,7 @@
146 mutable_map->set_scenario_player_closeable(1, false);145 mutable_map->set_scenario_player_closeable(1, false);
147 /** TRANSLATORS: Default player name, e.g. Player 1 */146 /** TRANSLATORS: Default player name, e.g. Player 1 */
148 mutable_map->set_scenario_player_name(1, (boost::format(_("Player %u")) % 1).str());147 mutable_map->set_scenario_player_name(1, (boost::format(_("Player %u")) % 1).str());
149 mutable_map->set_scenario_player_tribe(1, default_tribe_);148 mutable_map->set_scenario_player_tribe(1, "");
150 eia().set_need_save(true);149 eia().set_need_save(true);
151 }150 }
152151
@@ -179,11 +178,13 @@
179 plr_tribe->add(_(tribeinfo.descname), tribeinfo.name,178 plr_tribe->add(_(tribeinfo.descname), tribeinfo.name,
180 g_gr->images().get(tribeinfo.icon), false, tribeinfo.tooltip);179 g_gr->images().get(tribeinfo.icon), false, tribeinfo.tooltip);
181 }180 }
181 plr_tribe->add(pgettext("tribe", "Random"), "",
182 g_gr->images().get("images/ui_fsmenu/random.png"), false,
183 _("The tribe will be selected at random"));
182 }184 }
183 const std::string player_scenario_tribe =185
184 map_has_player ? map.get_scenario_player_tribe(p) : default_tribe_;186 plr_tribe->select((p < map.get_nrplayers() && Widelands::tribe_exists(map.get_scenario_player_tribe(p))) ? map.get_scenario_player_tribe(p) :
185 plr_tribe->select(Widelands::tribe_exists(player_scenario_tribe) ? player_scenario_tribe :187 "");
186 default_tribe_);
187 plr_tribe->selected.connect(188 plr_tribe->selected.connect(
188 boost::bind(&EditorPlayerMenu::player_tribe_clicked, boost::ref(*this), p - 1));189 boost::bind(&EditorPlayerMenu::player_tribe_clicked, boost::ref(*this), p - 1));
189 row->add(plr_tribe);190 row->add(plr_tribe);
@@ -268,7 +269,7 @@
268 rows_.at(pn - 1)->name->set_text(name);269 rows_.at(pn - 1)->name->set_text(name);
269270
270 const std::string& tribename = rows_.at(pn - 1)->tribe->get_selected();271 const std::string& tribename = rows_.at(pn - 1)->tribe->get_selected();
271 assert(Widelands::tribe_exists(tribename));272 assert(tribename.empty() || Widelands::tribe_exists(tribename));
272 map->set_scenario_player_tribe(pn, tribename);273 map->set_scenario_player_tribe(pn, tribename);
273 rows_.at(pn - 1)->box->set_visible(true);274 rows_.at(pn - 1)->box->set_visible(true);
274 }275 }
@@ -292,7 +293,7 @@
292293
293void EditorPlayerMenu::player_tribe_clicked(size_t row) {294void EditorPlayerMenu::player_tribe_clicked(size_t row) {
294 const std::string& tribename = rows_.at(row)->tribe->get_selected();295 const std::string& tribename = rows_.at(row)->tribe->get_selected();
295 assert(Widelands::tribe_exists(tribename));296 assert(tribename.empty() || Widelands::tribe_exists(tribename));
296 EditorInteractive& menu = eia();297 EditorInteractive& menu = eia();
297 menu.egbase().mutable_map()->set_scenario_player_tribe(row + 1, tribename);298 menu.egbase().mutable_map()->set_scenario_player_tribe(row + 1, tribename);
298 menu.set_need_save(true);299 menu.set_need_save(true);
299300
=== 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 @@
70 UI::Box box_;70 UI::Box box_;
71 UI::Dropdown<uintptr_t> no_of_players_;71 UI::Dropdown<uintptr_t> no_of_players_;
72 std::vector<std::unique_ptr<PlayerEditRow>> rows_;72 std::vector<std::unique_ptr<PlayerEditRow>> rows_;
73
74 const std::string default_tribe_;
75};73};
7674
77#endif // end of include guard: WL_EDITOR_UI_MENUS_PLAYER_MENU_H75#endif // end of include guard: WL_EDITOR_UI_MENUS_PLAYER_MENU_H
7876
=== 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 @@
283 Widelands::PlayerNumber const nrplayers = map.get_nrplayers();283 Widelands::PlayerNumber const nrplayers = map.get_nrplayers();
284 for (uint8_t i = 0; i < nrplayers; ++i) {284 for (uint8_t i = 0; i < nrplayers; ++i) {
285 settings_->set_player_name(i, map.get_scenario_player_name(i + 1));285 settings_->set_player_name(i, map.get_scenario_player_name(i + 1));
286 settings_->set_player_tribe(i, map.get_scenario_player_tribe(i + 1));286 const std::string playertribe = map.get_scenario_player_tribe(i + 1);
287 if (playertribe.empty()) {
288 // Set tribe selection to random
289 settings_->set_player_tribe(i, "", true);
290 } else {
291 // Set tribe selection from map
292 settings_->set_player_tribe(i, playertribe);
293 }
287 }294 }
288}295}
289296