Merge lp:~widelands-dev/widelands/bug-1378801 into lp:widelands

[GunChleoc]

Fixed bug with mountains value in editor random map menu

[TiborB (tiborb95)]

Open the new random map dialog window and increase the Wasteland to 30% - there is mathematical problem, perhaps overflow (underflow?)...

review: Needs Fixing

[GunChleoc (gunchleoc)]

I knew this was too easy *lol

[TiborB (tiborb95)]

Hi, I was curious and came up with another solution - see revision 7205. But review and probably some polish is still neeeded.
Also, it would be reasonable and possible to add arrows (increase/decrease) also to 'mountains'

[GunChleoc (gunchleoc)]

Thanks, that gave me a baseline to work from.

-1 to adding buttons for the mountain value. In another game, this kind of thing confused the hell out of me, because I never knew which other values would change when I pressed a button. This way, the user knows that the mountains are the first value that gets changed when you change one of the other values. Until you hit 0% / 100% anyway.

[TiborB (tiborb95)]

you are welcome!

But I still believe that a change in one parameter should invoke
adjustments in all other parameters. But I do not insist.

And I am really interesting in your solution. To me this is a
mathematical/programming challange. :)

2014-10-10 9:56 GMT+02:00 GunChleoc <email address hidden>:

> Thanks, that gave me a baseline to work from.
>
> -1 to adding buttons for the mountain value. In another game, this kind of
> thing confused the hell out of me, because I never knew which other values
> would change when I pressed a button. This way, the user knows that the
> mountains are the first value that gets changed when you change one of the
> other values. Until you hit 0% / 100% anyway.
> --
>
> https://code.launchpad.net/~widelands-dev/widelands/bug-1378801/+merge/237599
> You are reviewing the proposed merge of
> lp:~widelands-dev/widelands/bug-1378801 into lp:widelands.
>

[GunChleoc (gunchleoc)]

Yep, we have 2 different approaches: Yours is to distribute evenly, which makes for elegant code and nice mathematics. Mine is to change one value and then take care of the out of range issues afterwards. I also made sure that values get changed in steps of 5%, and this has the consequence that at most 2 values change at the same time, and I arranged it from bottom to top. I think that's just easier to control for the user. IMO it can get very fiddly if everything keeps changing at once and you want to set something specific.

[SirVer (sirver)]

Added a bunch of comments in the review.

review: Needs Fixing

[TiborB (tiborb95)]

Compilation fails here:

[ 59%] Building CXX object src/editor/CMakeFiles/editor.dir/ui_menus/editor_main_menu.cc.o
In file included from /var/widelands/BZR/bug-1378801/src/ui_basic/panel.h:32:0,
                 from /var/widelands/BZR/bug-1378801/src/ui_basic/button.h:28,
                 from /var/widelands/BZR/bug-1378801/src/editor/ui_menus/editor_main_menu.h:23,
                 from /var/widelands/BZR/bug-1378801/src/editor/ui_menus/editor_main_menu.cc:20:
/var/widelands/BZR/bug-1378801/src/base/macros.h:70:11: error: expected unqualified-id before ‘const’
  TypeName(const TypeName&) = delete; ^
/var/widelands/BZR/bug-1378801/src/editor/ui_menus/editor_main_menu_random_map.h:106:2: note: in expansion of macro ‘DISALLOW_COPY_AND_ASSIGN’
  DISALLOW_COPY_AND_ASSIGN(WorldDescription);
  ^
/var/widelands/BZR/bug-1378801/src/base/macros.h:70:11: error: expected ‘)’ before ‘const’
  TypeName(const TypeName&) = delete; ^
/var/widelands/BZR/bug-1378801/src/editor/ui_menus/editor_main_menu_random_map.h:106:2: note: in expansion of macro ‘DISALLOW_COPY_AND_ASSIGN’
  DISALLOW_COPY_AND_ASSIGN(WorldDescription);
  ^
src/editor/CMakeFiles/editor.dir/build.make:537: recipe for target 'src/editor/CMakeFiles/editor.dir/ui_menus/editor_main_menu.cc.o' failed
make[2]: *** [src/editor/CMakeFiles/editor.dir/ui_menus/editor_main_menu.cc.o] Error 1
CMakeFiles/Makefile2:9236: recipe for target 'src/editor/CMakeFiles/editor.dir/all' failed
make[1]: *** [src/editor/CMakeFiles/editor.dir/all] Error 2
Makefile:127: recipe for target 'all' failed
make: *** [all] Error 2

[GunChleoc (gunchleoc)]

All fixed now.

BTW the compilation failed because the DISALLOW_COPY_AND_ASSIGN had the wrong object - SirVer doesn't have time to wait for the compiler when doing code reviews, so I always compile again when addressing his comments.

review: Needs Fixing

[GunChleoc (gunchleoc)]

OOps, I meant to resubmit

review: Needs Resubmitting

[SirVer (sirver)]

> SirVer doesn't have time to wait for the compiler when doing code reviews,

that is a poor excuse :(. Sorry for that.

Code lgtm now.

review: Approve

[GunChleoc (gunchleoc)]

No worries, I found it fast enough :)

Will merge

=== modified file 'src/editor/ui_menus/editor_main_menu_random_map.cc'
--- src/editor/ui_menus/editor_main_menu_random_map.cc	2014-10-08 13:57:52 +0000
+++ src/editor/ui_menus/editor_main_menu_random_map.cc	2014-10-12 06:27:37 +0000
@@ -49,7 +49,7 @@
 				  (parent.get_w() - 260) / 2,
 				  (parent.get_h() - 450) / 2,
 				  305,
-				  490,
+				  500,
 				  _("New Random Map")),
    // TRANSLATORS: The next are world names for the random map generator.
 	m_world_descriptions(
@@ -70,6 +70,7 @@
 	m_waterval     = 20;
 	m_landval      = 60;
 	m_wastelandval = 0;
+	m_mountainsval = 100 - m_waterval - m_landval - m_wastelandval;
 	m_pn = 1;
 
 	// ---------- Random map number edit ----------
@@ -81,7 +82,7 @@
 		new UI::EditBox
 			(this,
 			 posx, posy,
-			 width, 20,
+			 width, height,
 			 g_gr->images().get("pics/but1.png"));
 	m_nrEditbox->changed.connect
 		(boost::bind(&MainMenuNewRandomMap::nr_edit_box_changed, this));
@@ -90,7 +91,7 @@
 	rng.rand();
 	m_mapNumber = rng.rand();
 	m_nrEditbox->set_text(std::to_string(static_cast<unsigned int>(m_mapNumber)));
-	posy += height + spacing + spacing + spacing;
+	posy += height + 3 * spacing;
 
 
 	// ---------- Width  ----------
@@ -106,7 +107,7 @@
 
 	UI::Button * widthupbtn = new UI::Button
 		(this, "width_up",
-		 get_inner_w() - spacing - 20, posy, 20, 20,
+		 get_inner_w() - spacing - height, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_up.png"));
 	widthupbtn->sigclicked.connect
@@ -114,27 +115,27 @@
 
 	UI::Button * widthdownbtn = new UI::Button
 		(this, "width_down",
-		 posx, posy, 20, 20,
+		 posx, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_down.png"));
 	widthdownbtn->sigclicked.connect
 		(boost::bind(&MainMenuNewRandomMap::button_clicked, this, ButtonId::MAP_W_MINUS));
 
-	m_width = new UI::Textarea(this, posx + spacing + 20, posy,
+	m_width = new UI::Textarea(this, posx + spacing + height, posy,
 										(boost::format(_("Width: %u"))
 										 % Widelands::MAP_DIMENSIONS[m_w]).str().c_str());
 
-	posy += 20 + spacing + spacing;
+	posy += height + 2 * spacing;
 
 	// ---------- Height  ----------
 
-	m_height = new UI::Textarea(this, posx + spacing + 20, posy,
+	m_height = new UI::Textarea(this, posx + spacing + height, posy,
 										 (boost::format(_("Height: %u"))
 										  % Widelands::MAP_DIMENSIONS[m_h]).str().c_str());
 
 	UI::Button * heightupbtn = new UI::Button
 		(this, "height_up",
-		 get_inner_w() - spacing - 20, posy, 20, 20,
+		 get_inner_w() - spacing - height, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_up.png"));
 	heightupbtn->sigclicked.connect
@@ -142,20 +143,20 @@
 
 	UI::Button * heightdownbtn = new UI::Button
 		(this, "height_down",
-		 posx, posy, 20, 20,
+		 posx, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_down.png"));
 	heightdownbtn->sigclicked.connect
 		(boost::bind(&MainMenuNewRandomMap::button_clicked, this, ButtonId::MAP_H_MINUS));
 
-	posy += 20 + spacing + spacing;
+	posy += height + 4 * spacing;
 
 
 	// ---------- Water -----------
 
 	UI::Button * waterupbtn = new UI::Button
 		(this, "water_up",
-		 get_inner_w() - spacing - 20, posy, 20, 20,
+		 get_inner_w() - spacing - height, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_up.png"));
 	waterupbtn->sigclicked.connect
@@ -163,16 +164,16 @@
 
 	UI::Button * waterdownbtn = new UI::Button
 		(this, "water_down",
-		 posx, posy, 20, 20,
+		 posx, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_down.png"));
 	waterdownbtn->sigclicked.connect
 		(boost::bind(&MainMenuNewRandomMap::button_clicked, this, ButtonId::WATER_MINUS));
 
-	m_water = new UI::Textarea(this, posx + spacing + 20, posy,
+	m_water = new UI::Textarea(this, posx + spacing + height, posy,
 										(boost::format(_("Water: %i %%")) % m_waterval).str().c_str());
 
-	posy += 20 + spacing + spacing;
+	posy += height + 2 * spacing;
 
 
 
@@ -180,7 +181,7 @@
 
 	UI::Button * landupbtn = new UI::Button
 		(this, "land_up",
-		 get_inner_w() - spacing - 20, posy, 20, 20,
+		 get_inner_w() - spacing - height, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_up.png"));
 	landupbtn->sigclicked.connect
@@ -188,16 +189,16 @@
 
 	UI::Button * landdownbtn = new UI::Button
 		(this, "land_down",
-		 posx, posy, 20, 20,
+		 posx, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_down.png"));
 	landdownbtn->sigclicked.connect
 		(boost::bind(&MainMenuNewRandomMap::button_clicked, this, ButtonId::LAND_MINUS));
 
-	m_land = new UI::Textarea(this, posx + spacing + 20, posy,
+	m_land = new UI::Textarea(this, posx + spacing + height, posy,
 									  (boost::format(_("Land: %i %%")) % m_landval).str().c_str());
 
-	posy += 20 + spacing + spacing;
+	posy += height + 2 * spacing;
 
 
 
@@ -205,7 +206,7 @@
 
 	UI::Button * wastelandupbtn = new UI::Button
 		(this, "wasteland_up",
-		 get_inner_w() - spacing - 20, posy, 20, 20,
+		 get_inner_w() - spacing - height, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_up.png"));
 	wastelandupbtn->sigclicked.connect
@@ -213,32 +214,30 @@
 
 	UI::Button * wastelanddownbtn = new UI::Button
 		(this, "wasteland_down",
-		 posx, posy, 20, 20,
+		 posx, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_down.png"));
 	wastelanddownbtn->sigclicked.connect
 		(boost::bind(&MainMenuNewRandomMap::button_clicked, this, ButtonId::WASTE_MINUS));
 
-	m_wasteland = new UI::Textarea(this, posx + spacing + 20, posy,
+	m_wasteland = new UI::Textarea(this, posx + spacing + height, posy,
 											 (boost::format(_("Wasteland: %i %%")) % m_wastelandval).str().c_str());
 
-	posy += 20 + spacing + spacing;
+	posy += height + 2 * spacing;
 
 
 
 	// ---------- Mountains -----------
 
-	m_mountains = new UI::Textarea(this, posx + spacing + 20, posy,
-											 (boost::format(_("Mountains: %i %%"))
-											  % (100 - m_waterval - m_landval - m_wastelandval)).str().c_str());
-
-	posy += 20 + spacing + spacing;
-
+	m_mountains = new UI::Textarea(this, posx + spacing + height, posy,
+											 (boost::format(_("Mountains: %i %%")) % m_mountainsval).str().c_str());
+
+	posy += height + 2 * spacing;
 
 
 	// ---------- Island mode ----------
 
-	Point pos(get_inner_w() - spacing - 20, posy);
+	Point pos(get_inner_w() - spacing - height, posy);
 	m_island_mode = new UI::Checkbox(this, pos);
 	m_island_mode->set_state(true);
 	m_island_mode->changed.connect
@@ -267,7 +266,7 @@
 		 m_res_amounts[m_res_amount].c_str());
 	m_res->sigclicked.connect(boost::bind(&MainMenuNewRandomMap::button_clicked, this, ButtonId::SWITCH_RES));
 
-	posy += height + spacing + spacing + spacing;
+	posy += height + 3 * spacing;
 
 	// ---------- Worlds ----------
 	m_world = new UI::Button
@@ -278,23 +277,23 @@
 	m_world->sigclicked.connect
 		(boost::bind(&MainMenuNewRandomMap::button_clicked, this, ButtonId::SWITCH_WORLD));
 
-	posy += height + spacing + spacing + spacing;
+	posy += height + 3 * spacing;
 
 	// ---------- Map ID String edit ----------
 
-	new UI::Textarea(this, posx + spacing + 20, posy, _("Map ID:"));
+	new UI::Textarea(this, posx, posy, _("Map ID:"));
 	posy += height + spacing;
 
 	m_idEditbox =
 		new UI::EditBox
 			(this,
 			 posx, posy,
-			 width, 20,
+			 width, height,
 			 g_gr->images().get("pics/but1.png"));
 	m_idEditbox->set_text("abcd-efgh-ijkl-mnop");
 	m_idEditbox->changed.connect
 		(boost::bind(&MainMenuNewRandomMap::id_edit_box_changed, this));
-	posy += height + spacing + spacing + spacing;
+	posy += height + 3 * spacing;
 
 
 
@@ -302,7 +301,7 @@
 
 	UI::Button * playerupbtn = new UI::Button
 		(this, "player_up",
-		 get_inner_w() - spacing - 20, posy, 20, 20,
+		 get_inner_w() - spacing - height, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_up.png"));
 	playerupbtn->sigclicked.connect
@@ -310,17 +309,17 @@
 
 	UI::Button * playerdownbtn = new UI::Button
 		(this, "player_down",
-		 posx, posy, 20, 20,
+		 posx, posy, height, height,
 		 g_gr->images().get("pics/but1.png"),
 		 g_gr->images().get("pics/scrollbar_down.png"));
 	playerdownbtn->sigclicked.connect
 		(boost::bind(&MainMenuNewRandomMap::button_clicked, this, ButtonId::PLAYER_MINUS));
 
-	m_players = new UI::Textarea(this, posx + spacing + 20, posy,
+	m_players = new UI::Textarea(this, posx + spacing + height, posy,
 										  (boost::format(_("Players: %u"))
 											% static_cast<unsigned int>(m_pn)).str().c_str());
 
-	posy += 20 + spacing + spacing;
+	posy += height + 2 * spacing;
 
 
 
@@ -370,22 +369,26 @@
 	case ButtonId::WATER_PLUS:
 		if (m_waterval < 60)
 			m_waterval += 5;
-		if (m_landval + m_waterval > 100)
-			m_landval -= 5;
+		normalize_landmass(n);
 		break;
 	case ButtonId::WATER_MINUS:
-		if (m_waterval > 0)
+		if (m_waterval >= 5)
 			m_waterval -= 5;
+		else
+			m_waterval = 0;
+		normalize_landmass(n);
 		break;
 	case ButtonId::LAND_PLUS:
 		if (m_landval < 100)
 			m_landval += 5;
-		if (m_waterval + m_landval > 100)
-			m_waterval -= 5;
+		normalize_landmass(n);
 		break;
 	case ButtonId::LAND_MINUS:
-		if (m_landval > 0)
+		if (m_landval >= 5)
 			m_landval -= 5;
+		else
+			m_landval = 0;
+		normalize_landmass(n);
 		break;
 	case ButtonId::SWITCH_WORLD:
 		++ m_current_world;
@@ -396,11 +399,15 @@
 		break;
 	case ButtonId::WASTE_PLUS:
 		if (m_wastelandval < 70)
-			m_wastelandval += 10;
+			m_wastelandval += 5;
+		normalize_landmass(n);
 		break;
 	case ButtonId::WASTE_MINUS:
-		if (m_wastelandval > 0)
-			m_wastelandval -= 10;
+		if (m_wastelandval >= 5)
+			m_wastelandval -= 5;
+		else
+			m_wastelandval = 0;
+		normalize_landmass(n);
 		break;
 	case ButtonId::SWITCH_RES:
 		++ m_res_amount;
@@ -418,24 +425,63 @@
 	if (m_h >= NUMBER_OF_MAP_DIMENSIONS) m_h = NUMBER_OF_MAP_DIMENSIONS - 1;
 
 	m_width ->set_text((boost::format(_("Width: %u")) % Widelands::MAP_DIMENSIONS[m_w]).str().c_str());
-
 	m_height->set_text((boost::format(_("Height: %u")) % Widelands::MAP_DIMENSIONS[m_h]).str().c_str());
-
 	m_water->set_text((boost::format(_("Water: %i %%")) % m_waterval).str().c_str());
-
 	m_land->set_text((boost::format(_("Land: %i %%")) % m_landval).str().c_str());
-
 	m_wasteland->set_text((boost::format(_("Wasteland: %i %%")) % m_wastelandval).str().c_str());
-
-	m_mountains->set_text((boost::format(_("Mountains: %i %%"))
-								  % (100 - m_waterval - m_landval)).str().c_str());
-
+	m_mountains->set_text((boost::format(_("Mountains: %i %%")) % m_mountainsval).str().c_str());
 	m_players->set_text((boost::format(_("Players: %u"))
 								% static_cast<unsigned int>(m_pn)).str().c_str());
 
 	nr_edit_box_changed();  // Update ID String
 }
 
+
+// If the the sum of our landmass is < 0% or > 100% change the mountain value.
+// If the mountain value gets out of range, change the other values.
+void MainMenuNewRandomMap::normalize_landmass(ButtonId clicked_button) {
+	int32_t sum_without_mountainsval = m_waterval + m_landval + m_wastelandval;
+
+	// Prefer changing mountainsval to keep consistency with old behaviour
+	while (sum_without_mountainsval + m_mountainsval > 100) {
+			m_mountainsval -= 1;
+	}
+	while (sum_without_mountainsval + m_mountainsval < 100) {
+			m_mountainsval += 1;
+	}
+
+	// Compensate if mountainsval got above 100% / below 0%
+	while (m_mountainsval < 0) {
+		if (clicked_button != ButtonId::WASTE_PLUS && m_wastelandval > 0) {
+			m_wastelandval -= 5;
+			m_mountainsval += 5;
+		}
+		if (m_mountainsval < 0 && clicked_button != ButtonId::LAND_PLUS && m_landval > 0) {
+			m_landval -= 5;
+			m_mountainsval += 5;
+		}
+		if (m_mountainsval < 0 && clicked_button != ButtonId::WATER_PLUS && m_waterval > 0) {
+			m_waterval -= 5;
+			m_mountainsval += 5;
+		}
+	}
+
+	while (m_mountainsval > 100) {
+		if (clicked_button != ButtonId::WASTE_MINUS && m_wastelandval < 100) {
+			m_wastelandval += 5;
+			m_mountainsval -= 5;
+		}
+		if (m_mountainsval > 100 && clicked_button != ButtonId::LAND_MINUS && m_landval < 100) {
+			m_landval += 5;
+			m_mountainsval -= 5;
+		}
+		if (m_mountainsval > 100 && clicked_button != ButtonId::WATER_MINUS && m_waterval < 100) {
+			m_waterval += 5;
+			m_mountainsval -= 5;
+		}
+	}
+}
+
 void MainMenuNewRandomMap::clicked_create_map() {
 	EditorInteractive & eia =
 		ref_cast<EditorInteractive, UI::Panel>(*get_parent());

=== modified file 'src/editor/ui_menus/editor_main_menu_random_map.h'
--- src/editor/ui_menus/editor_main_menu_random_map.h	2014-09-14 12:13:35 +0000
+++ src/editor/ui_menus/editor_main_menu_random_map.h	2014-10-12 06:27:37 +0000
@@ -22,6 +22,7 @@
 
 #include <vector>
 
+#include "base/macros.h"
 #include "ui_basic/checkbox.h"
 #include "ui_basic/editbox.h"
 #include "ui_basic/window.h"
@@ -68,6 +69,21 @@
 		std::string descrname;
 	};
 
+	void button_clicked(ButtonId);
+	void clicked_create_map();
+	void id_edit_box_changed();
+	void nr_edit_box_changed();
+
+	// Ensures that the sum of our landmass is >= 0% and <= 100%, and changes
+	// values as necessary.
+	// \param clicked_button: The button that was clicked to change the values.
+	//                        This function makes sure that after the normalization,
+	//                        the value related to the button will still be set
+	//                        as requested by the user.
+	void normalize_landmass(MainMenuNewRandomMap::ButtonId clicked_button);
+
+	void set_map_info(Widelands::UniqueRandomMapInfo & mapInfo) const;
+
 	const std::vector<WorldDescription> m_world_descriptions;
 	int m_current_world;
 	UI::Textarea * m_width, * m_height, * m_land;
@@ -76,7 +92,8 @@
 	UI::Button * m_world;
 	UI::Checkbox * m_island_mode;
 	UI::Button * m_goButton;
-	int32_t m_w, m_h, m_landval, m_waterval, m_wastelandval;
+	int32_t m_w, m_h;
+	int32_t m_landval, m_waterval, m_wastelandval, m_mountainsval;
 	uint8_t m_pn;
 	uint32_t m_mapNumber;
 	uint32_t m_res_amount;
@@ -85,12 +102,7 @@
 	UI::EditBox * m_nrEditbox;
 	UI::EditBox * m_idEditbox;
 
-	void button_clicked(ButtonId);
-	void clicked_create_map();
-	void id_edit_box_changed();
-	void nr_edit_box_changed();
-
-	void set_map_info(Widelands::UniqueRandomMapInfo & mapInfo) const;
+	DISALLOW_COPY_AND_ASSIGN(MainMenuNewRandomMap);
 };
 
 #endif  // end of include guard: WL_EDITOR_UI_MENUS_EDITOR_MAIN_MENU_RANDOM_MAP_H