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
1=== modified file 'src/editor/editorinteractive.cc'
2--- src/editor/editorinteractive.cc 2018-07-20 08:42:23 +0000
3+++ src/editor/editorinteractive.cc 2018-08-28 08:07:51 +0000
4@@ -177,10 +177,13 @@
5 load_all_tribes(&egbase(), &loader_ui);
6
7 // Create the players. TODO(SirVer): this must be managed better
8+ // TODO(GunChleoc): Ugly - we only need this for the test suite right now
9 loader_ui.step(_("Creating players"));
10 iterate_player_numbers(p, map->get_nrplayers()) {
11- egbase().add_player(
12- p, 0, map->get_scenario_player_tribe(p), map->get_scenario_player_name(p));
13+ if (!map->get_scenario_player_tribe(p).empty()) {
14+ egbase().add_player(
15+ p, 0, map->get_scenario_player_tribe(p), map->get_scenario_player_name(p));
16+ }
17 }
18
19 ml->load_map_complete(egbase(), Widelands::MapLoader::LoadType::kEditor);
20
21=== modified file 'src/editor/map_generator.cc'
22--- src/editor/map_generator.cc 2018-07-21 07:53:42 +0000
23+++ src/editor/map_generator.cc 2018-08-28 08:07:51 +0000
24@@ -28,6 +28,7 @@
25 #include "logic/editor_game_base.h"
26 #include "logic/findnode.h"
27 #include "logic/map.h"
28+#include "logic/map_objects/tribes/tribe_basic_info.h"
29 #include "logic/map_objects/world/map_gen.h"
30 #include "logic/map_objects/world/world.h"
31 #include "scripting/lua_interface.h"
32@@ -661,7 +662,6 @@
33 // Care about players and place their start positions
34 map_.set_nrplayers(map_info_.numPlayers);
35 assert(map_info_.numPlayers >= 1);
36- const std::string tribe = map_.get_scenario_player_tribe(1);
37 const std::string ai = map_.get_scenario_player_ai(1);
38 FindNodeSize functor(FindNodeSize::sizeBig);
39 Coords playerstart(Coords::null());
40@@ -714,7 +714,7 @@
41 for (PlayerNumber n = 1; n <= map_info_.numPlayers; ++n) {
42 // Set scenario information - needed even if it's not a scenario
43 map_.set_scenario_player_name(n, _("Random Player"));
44- map_.set_scenario_player_tribe(n, tribe);
45+ map_.set_scenario_player_tribe(n, "");
46 map_.set_scenario_player_ai(n, ai);
47 map_.set_scenario_player_closeable(n, false);
48
49
50=== modified file 'src/editor/ui_menus/player_menu.cc'
51--- src/editor/ui_menus/player_menu.cc 2018-07-13 11:23:52 +0000
52+++ src/editor/ui_menus/player_menu.cc 2018-08-28 08:07:51 +0000
53@@ -129,8 +129,7 @@
54 24,
55 _("Number of players"),
56 UI::DropdownType::kTextual,
57- UI::PanelStyle::kWui),
58- default_tribe_(Widelands::get_all_tribenames().front()) {
59+ UI::PanelStyle::kWui) {
60 box_.set_size(100, 100); // Prevent assert failures
61 box_.add(&no_of_players_, UI::Box::Resizing::kFullSize);
62 box_.add_space(2 * kMargin);
63@@ -146,7 +145,7 @@
64 mutable_map->set_scenario_player_closeable(1, false);
65 /** TRANSLATORS: Default player name, e.g. Player 1 */
66 mutable_map->set_scenario_player_name(1, (boost::format(_("Player %u")) % 1).str());
67- mutable_map->set_scenario_player_tribe(1, default_tribe_);
68+ mutable_map->set_scenario_player_tribe(1, "");
69 eia().set_need_save(true);
70 }
71
72@@ -179,11 +178,13 @@
73 plr_tribe->add(_(tribeinfo.descname), tribeinfo.name,
74 g_gr->images().get(tribeinfo.icon), false, tribeinfo.tooltip);
75 }
76+ plr_tribe->add(pgettext("tribe", "Random"), "",
77+ g_gr->images().get("images/ui_fsmenu/random.png"), false,
78+ _("The tribe will be selected at random"));
79 }
80- const std::string player_scenario_tribe =
81- map_has_player ? map.get_scenario_player_tribe(p) : default_tribe_;
82- plr_tribe->select(Widelands::tribe_exists(player_scenario_tribe) ? player_scenario_tribe :
83- default_tribe_);
84+
85+ plr_tribe->select((p < map.get_nrplayers() && Widelands::tribe_exists(map.get_scenario_player_tribe(p))) ? map.get_scenario_player_tribe(p) :
86+ "");
87 plr_tribe->selected.connect(
88 boost::bind(&EditorPlayerMenu::player_tribe_clicked, boost::ref(*this), p - 1));
89 row->add(plr_tribe);
90@@ -268,7 +269,7 @@
91 rows_.at(pn - 1)->name->set_text(name);
92
93 const std::string& tribename = rows_.at(pn - 1)->tribe->get_selected();
94- assert(Widelands::tribe_exists(tribename));
95+ assert(tribename.empty() || Widelands::tribe_exists(tribename));
96 map->set_scenario_player_tribe(pn, tribename);
97 rows_.at(pn - 1)->box->set_visible(true);
98 }
99@@ -292,7 +293,7 @@
100
101 void EditorPlayerMenu::player_tribe_clicked(size_t row) {
102 const std::string& tribename = rows_.at(row)->tribe->get_selected();
103- assert(Widelands::tribe_exists(tribename));
104+ assert(tribename.empty() || Widelands::tribe_exists(tribename));
105 EditorInteractive& menu = eia();
106 menu.egbase().mutable_map()->set_scenario_player_tribe(row + 1, tribename);
107 menu.set_need_save(true);
108
109=== modified file 'src/editor/ui_menus/player_menu.h'
110--- src/editor/ui_menus/player_menu.h 2018-07-08 07:57:22 +0000
111+++ src/editor/ui_menus/player_menu.h 2018-08-28 08:07:51 +0000
112@@ -70,8 +70,6 @@
113 UI::Box box_;
114 UI::Dropdown<uintptr_t> no_of_players_;
115 std::vector<std::unique_ptr<PlayerEditRow>> rows_;
116-
117- const std::string default_tribe_;
118 };
119
120 #endif // end of include guard: WL_EDITOR_UI_MENUS_PLAYER_MENU_H
121
122=== modified file 'src/ui_fsmenu/launch_spg.cc'
123--- src/ui_fsmenu/launch_spg.cc 2018-07-07 20:22:12 +0000
124+++ src/ui_fsmenu/launch_spg.cc 2018-08-28 08:07:51 +0000
125@@ -283,7 +283,14 @@
126 Widelands::PlayerNumber const nrplayers = map.get_nrplayers();
127 for (uint8_t i = 0; i < nrplayers; ++i) {
128 settings_->set_player_name(i, map.get_scenario_player_name(i + 1));
129- settings_->set_player_tribe(i, map.get_scenario_player_tribe(i + 1));
130+ const std::string playertribe = map.get_scenario_player_tribe(i + 1);
131+ if (playertribe.empty()) {
132+ // Set tribe selection to random
133+ settings_->set_player_tribe(i, "", true);
134+ } else {
135+ // Set tribe selection from map
136+ settings_->set_player_tribe(i, playertribe);
137+ }
138 }
139 }
140