Merge lp:~widelands-dev/widelands/toggle_immovables into lp:widelands

[SirVer]

Make display of bobs and immovables toggable.

Hotkeys are CTRL-<1-4>.

[toptopple (7010622-q-deactivatedaccount)]

Very nice! I don't see a technical problem which hasn't been there in the unmodified program. :)
When I look into the specs of this task, however, there is no mention of the Animals class. So with your implementation we actually are dealing with an over-fulfillment of the task.

When I was designing the task, I didn't think of the Animals as something different to Immovables, probably because they don't move in the editor and better handling of the editor was my only purpose. We have to re-think. Anyways, short-key 'a' was targeted for the Animals tool and hence is placed falsely here (for the VIEW). I surmise we should discuss this matter in the task thread first and then continue here.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2537. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/264379653.
Appveyor build 2360. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_toggle_immovables-2360.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2537. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/264379653.
Appveyor build 2360. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_toggle_immovables-2360.

[GunChleoc (gunchleoc)]

Code LTGM. Let's wait with merging until we have decided on the hotkeys.

review: Approve

[SirVer (sirver)]

I changed the hotkeys to use CTRL 1 to 4, doubling CTRL 1 with SPACE (which is a slight ward). But CTRL 1-4 for the layers feels really good and quick. What do you think?

[kaputtnik (franku)]

Didn't test it yet, but i think the order should be the same as we have icons in the tools menu?

That is from left to right:
- ..
- Immovables (here: 3), suggestion: 2
- Animals (here: 4), suggestion: 3
- Resources (here: 2), suggestion: 4
- ..

So there a little dependency with the visual representation and one could may better remember which hotkey toggles which overlay without always trying around.

[kaputtnik (franku)]

Ah, forgotton:

If the hotkey for buildhelp is changed in editor, will it still be 'space' for playing games?

[SirVer (sirver)]

> Didn't test it yet, but i think the order should be the same as we have icons in the tools menu?

true that. done.

> If the hotkey for buildhelp is changed in editor, will it still be 'space' for playing games?

It is not changed. It is duplicated - space still works. But for symmetry I added CTRL 1. That is what I meant with a slight ward. It feels really good using the for hotkeys will making a map though

[kaputtnik (franku)]

Thanks for clarification :-)

[GunChleoc (gunchleoc)]

I removed a blank line - xgettext can only pick up comments that are immediately above the gettext invocation, so a blank space might keep it from picking up the comment.

Code still LGTM, will do a bit of testing once the compiler is finished.

[GunChleoc (gunchleoc)]

Tested and working :)

@bunnybot merge

=== added file 'data/images/wui/menus/menu_toggle_bobs.png'
Binary files data/images/wui/menus/menu_toggle_bobs.png	1970-01-01 00:00:00 +0000 and data/images/wui/menus/menu_toggle_bobs.png	2017-08-16 05:34:38 +0000 differ
=== added file 'data/images/wui/menus/menu_toggle_immovables.png'
Binary files data/images/wui/menus/menu_toggle_immovables.png	1970-01-01 00:00:00 +0000 and data/images/wui/menus/menu_toggle_immovables.png	2017-08-16 05:34:38 +0000 differ
=== modified file 'data/scripting/editor/editor_controls.lua'
--- data/scripting/editor/editor_controls.lua	2017-08-14 09:06:04 +0000
+++ data/scripting/editor/editor_controls.lua	2017-08-16 05:34:38 +0000
@@ -21,10 +21,16 @@
             dl(help_format_hotkey("T"), _"Toggle tools menu") ..
             toggle_minimap_hotkey ..
             toggle_building_spaces_hotkey ..
+            -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
+            dl(help_format_hotkey("Ctrl + 1"), _"Toggle building spaces") ..
+            -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
+            dl(help_format_hotkey("Ctrl + 2"), _"Toggle immovables display") ..
+            -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
+            dl(help_format_hotkey("Ctrl + 3"), _"Toggle animals display") ..
             -- TRANSLATORS: This is an access key combination. The hotkey is 'p'
+            dl(help_format_hotkey("Ctrl + 4"), _"Toggle resources display") ..
+            -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
             dl(help_format_hotkey("P"), _"Toggle player menu") ..
-            -- TRANSLATORS: This is an access key combination. The hotkey is 'q'
-            dl(help_format_hotkey("Q"), _"Toggle resources display") ..
             -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
             dl(help_format_hotkey("Ctrl + Z"), _"Undo") ..
             -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.

=== modified file 'src/editor/editorinteractive.cc'
--- src/editor/editorinteractive.cc	2017-08-14 09:06:04 +0000
+++ src/editor/editorinteractive.cc	2017-08-16 05:34:38 +0000
@@ -118,6 +118,14 @@
 	toggle_buildhelp_ = add_toolbar_button(
 	   "wui/menus/menu_toggle_buildhelp", "buildhelp", _("Show Building Spaces (on/off)"));
 	toggle_buildhelp_->sigclicked.connect(boost::bind(&EditorInteractive::toggle_buildhelp, this));
+	toggle_immovables_ = add_toolbar_button(
+	   "wui/menus/menu_toggle_immovables", "immovables", _("Show Immovables (on/off)"));
+	toggle_immovables_->set_perm_pressed(true);
+	toggle_immovables_->sigclicked.connect([this]() { toggle_immovables(); });
+	toggle_bobs_ = add_toolbar_button(
+	   "wui/menus/menu_toggle_bobs", "animals", _("Show Animals (on/off)"));
+	toggle_bobs_->set_perm_pressed(true);
+	toggle_bobs_->sigclicked.connect([this]() { toggle_bobs(); });
 	toggle_resources_ = add_toolbar_button(
 	   "wui/menus/menu_toggle_resources", "resources", _("Show Resources (on/off)"));
 	toggle_resources_->set_perm_pressed(true);
@@ -339,54 +347,69 @@
 	toggle_resources_->set_perm_pressed(value);
 }
 
+void EditorInteractive::toggle_immovables() {
+	const bool value = !draw_immovables();
+	set_draw_immovables(value);
+	toggle_immovables_->set_perm_pressed(value);
+}
+
+void EditorInteractive::toggle_bobs() {
+	const bool value = !draw_bobs();
+	set_draw_bobs(value);
+	toggle_bobs_->set_perm_pressed(value);
+}
+
 bool EditorInteractive::handle_key(bool const down, SDL_Keysym const code) {
-	bool handled = InteractiveBase::handle_key(down, code);
-
 	if (down) {
-		// only on down events
-
 		switch (code.sym) {
 		// Sel radius
 		case SDLK_1:
-			set_sel_radius_and_update_menu(0);
-			handled = true;
-			break;
+			if (code.mod & (KMOD_CTRL)) {
+				toggle_buildhelp();
+			} else {
+				set_sel_radius_and_update_menu(0);
+			}
+			return true;
 		case SDLK_2:
-			set_sel_radius_and_update_menu(1);
-			handled = true;
-			break;
+			if (code.mod & (KMOD_CTRL)) {
+				toggle_immovables();
+			} else {
+				set_sel_radius_and_update_menu(1);
+			}
+			return true;
 		case SDLK_3:
-			set_sel_radius_and_update_menu(2);
-			handled = true;
-			break;
+			if (code.mod & (KMOD_CTRL)) {
+				toggle_bobs();
+			} else {
+				set_sel_radius_and_update_menu(2);
+			}
+			return true;
 		case SDLK_4:
-			set_sel_radius_and_update_menu(3);
-			handled = true;
-			break;
+			if (code.mod & (KMOD_CTRL)) {
+				toggle_resources();
+			} else {
+				set_sel_radius_and_update_menu(3);
+			}
+			return true;
 		case SDLK_5:
 			set_sel_radius_and_update_menu(4);
-			handled = true;
-			break;
+			return true;
 		case SDLK_6:
 			set_sel_radius_and_update_menu(5);
-			handled = true;
-			break;
+			return true;
 		case SDLK_7:
 			set_sel_radius_and_update_menu(6);
-			handled = true;
-			break;
+			return true;
 		case SDLK_8:
 			set_sel_radius_and_update_menu(7);
-			handled = true;
-			break;
+			return true;
 		case SDLK_9:
 			set_sel_radius_and_update_menu(8);
-			handled = true;
-			break;
+			return true;
 		case SDLK_0:
 			if (!(code.mod & KMOD_CTRL)) {
 				set_sel_radius_and_update_menu(9);
-				handled = true;
+				return true;
 			}
 			break;
 
@@ -394,8 +417,7 @@
 		case SDLK_RSHIFT:
 			if (tools_->use_tool == EditorTool::First)
 				select_tool(tools_->current(), EditorTool::Second);
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_LCTRL:
 		case SDLK_RCTRL:
@@ -405,80 +427,62 @@
 		case SDLK_MODE:
 			if (tools_->use_tool == EditorTool::First)
 				select_tool(tools_->current(), EditorTool::Third);
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_SPACE:
 			toggle_buildhelp();
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_c:
 			set_display_flag(
 			   InteractiveBase::dfShowCensus, !get_display_flag(InteractiveBase::dfShowCensus));
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_h:
 			mainmenu_.toggle();
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_i:
 			select_tool(tools_->info, EditorTool::First);
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_l:
 			if (code.mod & (KMOD_LCTRL | KMOD_RCTRL))
 				new MainMenuLoadMap(*this);
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_m:
 			minimap_registry().toggle();
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_p:
 			playermenu_.toggle();
-			handled = true;
-			break;
-
-		case SDLK_q:
-			toggle_resources();
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_s:
 			if (code.mod & (KMOD_LCTRL | KMOD_RCTRL))
 				new MainMenuSaveMap(*this);
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_t:
 			toolmenu_.toggle();
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_y:
 			if (code.mod & (KMOD_LCTRL | KMOD_RCTRL))
 				history_->redo_action(egbase().world());
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_z:
 			if ((code.mod & (KMOD_LCTRL | KMOD_RCTRL)) && (code.mod & (KMOD_LSHIFT | KMOD_RSHIFT)))
 				history_->redo_action(egbase().world());
 			else if (code.mod & (KMOD_LCTRL | KMOD_RCTRL))
 				history_->undo_action(egbase().world());
-			handled = true;
-			break;
+			return true;
 
 		case SDLK_F1:
 			helpmenu_.toggle();
-			handled = true;
-			break;
+			return true;
 
 		default:
 			break;
@@ -496,13 +500,12 @@
 		case SDLK_MODE:
 			if (tools_->use_tool != EditorTool::First)
 				select_tool(tools_->current(), EditorTool::First);
-			handled = true;
-			break;
+			return true;
 		default:
 			break;
 		}
 	}
-	return handled;
+	return InteractiveBase::handle_key(down, code);
 }
 
 void EditorInteractive::select_tool(EditorTool& primary, EditorTool::ToolIndex const which) {

=== modified file 'src/editor/editorinteractive.h'
--- src/editor/editorinteractive.h	2017-08-14 09:06:04 +0000
+++ src/editor/editorinteractive.h	2017-08-16 05:34:38 +0000
@@ -157,6 +157,8 @@
 	void on_buildhelp_changed(const bool value) override;
 
 	void toggle_resources();
+	void toggle_immovables();
+	void toggle_bobs();
 
 	//  state variables
 	bool need_save_;
@@ -181,6 +183,8 @@
 
 	UI::Button* toggle_buildhelp_;
 	UI::Button* toggle_resources_;
+	UI::Button* toggle_immovables_;
+	UI::Button* toggle_bobs_;
 	UI::Button* undo_;
 	UI::Button* redo_;
 

=== modified file 'src/graphic/game_renderer.cc'
--- src/graphic/game_renderer.cc	2017-08-13 20:47:55 +0000
+++ src/graphic/game_renderer.cc	2017-08-16 05:34:38 +0000
@@ -96,7 +96,7 @@
 	return brightness / 255.;
 }
 
-void draw_objects_for_visible_field(const EditorGameBase& egbase,
+void draw_immovables_for_visible_field(const EditorGameBase& egbase,
                                     const FieldsToDraw::Field& field,
                                     const float zoom,
                                     const TextToDraw text_to_draw,
@@ -111,10 +111,17 @@
 			draw_text_for_this_immovable =
 			   static_cast<TextToDraw>(draw_text_for_this_immovable & ~TextToDraw::kStatistics);
 		}
-
 		imm->draw(
 		   egbase.get_gametime(), draw_text_for_this_immovable, field.rendertarget_pixel, zoom, dst);
 	}
+}
+
+void draw_bobs_for_visible_field(const EditorGameBase& egbase,
+                                    const FieldsToDraw::Field& field,
+                                    const float zoom,
+                                    const TextToDraw text_to_draw,
+                                    const Player* player,
+                                    RenderTarget* dst) {
 	for (Bob* bob = field.fcoords.field->get_first_bob(); bob; bob = bob->get_next_bob()) {
 		TextToDraw draw_text_for_this_bob = text_to_draw;
 		const Player* owner = bob->get_owner();
@@ -127,7 +134,7 @@
 	}
 }
 
-void draw_objects_for_formerly_visible_field(const FieldsToDraw::Field& field,
+void draw_immovables_for_formerly_visible_field(const FieldsToDraw::Field& field,
                                              const Player::Field& player_field,
                                              const float zoom,
                                              RenderTarget* dst) {
@@ -209,6 +216,8 @@
                   const FieldsToDraw& fields_to_draw,
                   const Player* player,
                   const TextToDraw text_to_draw,
+						const GameRenderer::DrawImmovables& draw_immovables,
+						const GameRenderer::DrawBobs& draw_bobs,
                   RenderTarget* dst) {
 	for (size_t current_index = 0; current_index < fields_to_draw.size(); ++current_index) {
 		const FieldsToDraw::Field& field = fields_to_draw.at(current_index);
@@ -220,7 +229,7 @@
 		const FieldsToDraw::Field& bln = fields_to_draw.at(field.bln_index);
 		const FieldsToDraw::Field& brn = fields_to_draw.at(field.brn_index);
 
-		if (field.is_border) {
+		if (field.is_border && draw_immovables == GameRenderer::DrawImmovables::kYes) {
 			assert(field.owner != nullptr);
 			uint32_t const anim_idx = field.owner->tribe().frontier_animation();
 			if (field.vision) {
@@ -237,14 +246,19 @@
 		}
 
 		if (1 < field.vision) {  // Render stuff that belongs to the node.
-			draw_objects_for_visible_field(egbase, field, zoom, text_to_draw, player, dst);
-		} else if (field.vision == 1) {
+			if (draw_immovables == GameRenderer::DrawImmovables::kYes) {
+				draw_immovables_for_visible_field(egbase, field, zoom, text_to_draw, player, dst);
+			}
+			if (draw_bobs == GameRenderer::DrawBobs::kYes) {
+				draw_bobs_for_visible_field(egbase, field, zoom, text_to_draw, player, dst);
+			}
+		} else if (field.vision == 1 && draw_immovables == GameRenderer::DrawImmovables::kYes) {
 			// We never show census or statistics for objects in the fog.
 			assert(player != nullptr);
 			const Map& map = egbase.map();
 			const Player::Field& player_field =
 			   player->fields()[map.get_index(field.fcoords, map.get_width())];
-			draw_objects_for_formerly_visible_field(field, player_field, zoom, dst);
+			draw_immovables_for_formerly_visible_field(field, player_field, zoom, dst);
 		}
 
 		egbase.get_ibase()->field_overlay_manager().foreach_overlay(
@@ -271,21 +285,25 @@
                              const Widelands::Player& player,
                              const Overlays& overlays,
                              RenderTarget* dst) {
-	draw(egbase, viewpoint, zoom, overlays, &player, dst);
+	draw(egbase, viewpoint, zoom, overlays, DrawImmovables::kYes, DrawBobs::kYes, &player, dst);
 }
 
 void GameRenderer::rendermap(const Widelands::EditorGameBase& egbase,
                              const Vector2f& viewpoint,
                              const float zoom,
                              const Overlays& overlays,
+                             const DrawImmovables& draw_immovables,
+                             const DrawBobs& draw_bobs,
                              RenderTarget* dst) {
-	draw(egbase, viewpoint, zoom, overlays, nullptr, dst);
+	draw(egbase, viewpoint, zoom, overlays, draw_immovables, draw_bobs, nullptr, dst);
 }
 
 void GameRenderer::draw(const EditorGameBase& egbase,
                         const Vector2f& viewpoint,
                         const float zoom,
                         const Overlays& overlays,
+                        const DrawImmovables& draw_immovables,
+                        const DrawBobs& draw_bobs,
                         const Player* player,
                         RenderTarget* dst) {
 	assert(viewpoint.x >= 0);  // divisions involving negative numbers are bad
@@ -410,5 +428,6 @@
 	i.program_id = RenderQueue::Program::kTerrainRoad;
 	RenderQueue::instance().enqueue(i);
 
-	draw_objects(egbase, scale, fields_to_draw_, player, overlays.text_to_draw, dst);
+	draw_objects(egbase, scale, fields_to_draw_, player, overlays.text_to_draw, draw_immovables,
+	             draw_bobs, dst);
 }

=== modified file 'src/graphic/game_renderer.h'
--- src/graphic/game_renderer.h	2017-08-12 20:13:24 +0000
+++ src/graphic/game_renderer.h	2017-08-16 05:34:38 +0000
@@ -43,6 +43,9 @@
 		std::map<Widelands::Coords, uint8_t> road_building_preview;
 	};
 
+	enum class DrawImmovables { kNo, kYes };
+	enum class DrawBobs { kNo, kYes };
+
 	GameRenderer();
 	~GameRenderer();
 
@@ -57,11 +60,14 @@
 	               RenderTarget* dst);
 
 	// Renders the map from an omniscient perspective. This is used
-	// for spectators, players that see all, and in the editor.
+	// for spectators, players that see all, and in the editor. Only in the editor we allow toggling
+	// of immovables and bobs.
 	void rendermap(const Widelands::EditorGameBase& egbase,
 	               const Vector2f& viewpoint,
 	               float scale,
 	               const Overlays& overlays,
+						const DrawImmovables& draw_immovables,
+						const DrawBobs& draw_bobs,
 	               RenderTarget* dst);
 
 private:
@@ -71,6 +77,8 @@
 	          const Vector2f& viewpoint,
 	          float scale,
 	          const Overlays& overlays,
+	          const DrawImmovables& draw_immovables,
+	          const DrawBobs& draw_bobs,
 	          const Widelands::Player* player,
 	          RenderTarget* dst);
 

=== modified file 'src/wui/interactive_base.cc'
--- src/wui/interactive_base.cc	2017-08-15 07:47:21 +0000
+++ src/wui/interactive_base.cc	2017-08-16 05:34:38 +0000
@@ -93,6 +93,8 @@
      lastframe_(SDL_GetTicks()),
      frametime_(0),
      avg_usframetime_(0),
+	  draw_immovables_(true),
+	  draw_bobs_(true),
      road_buildhelp_overlay_jobid_(0),
      buildroad_(nullptr),
      road_build_player_(0),
@@ -223,6 +225,22 @@
 	on_buildhelp_changed(t);
 }
 
+bool InteractiveBase::draw_bobs() const {
+	return draw_bobs_;
+}
+
+void InteractiveBase::set_draw_bobs(const bool value) {
+	draw_bobs_ = value;
+}
+
+bool InteractiveBase::draw_immovables() const {
+	return draw_immovables_;
+}
+
+void InteractiveBase::set_draw_immovables(const bool value) {
+	draw_immovables_ = value;
+}
+
 void InteractiveBase::toggle_buildhelp() {
 	show_buildhelp(!field_overlay_manager_->buildhelp());
 }

=== modified file 'src/wui/interactive_base.h'
--- src/wui/interactive_base.h	2017-08-12 08:19:27 +0000
+++ src/wui/interactive_base.h	2017-08-16 05:34:38 +0000
@@ -100,6 +100,14 @@
 	// Sets if the buildhelp should be displayed. Will also call on_buildhelp_changed().
 	void show_buildhelp(bool t);
 
+	// Returns true if bobs or immovables should be rendered.
+	bool draw_bobs() const;
+	bool draw_immovables() const;
+
+	// Sets if bobs or immovables should be rendered.
+	void set_draw_bobs(bool value);
+	void set_draw_immovables(bool value);
+
 	/**
 	 * sel_triangles determines whether the mouse pointer selects triangles.
 	 * (False meas that it selects nodes.)
@@ -256,6 +264,8 @@
 	uint32_t lastframe_;        //  system time (milliseconds)
 	uint32_t frametime_;        //  in millseconds
 	uint32_t avg_usframetime_;  //  in microseconds!
+	bool draw_immovables_;
+	bool draw_bobs_;
 
 	FieldOverlayManager::OverlayId road_buildhelp_overlay_jobid_;
 	Widelands::CoordPath* buildroad_;  //  path for the new road

=== modified file 'src/wui/mapview.cc'
--- src/wui/mapview.cc	2017-08-12 20:13:24 +0000
+++ src/wui/mapview.cc	2017-08-16 05:34:38 +0000
@@ -407,8 +407,13 @@
 		renderer_->rendermap(
 		   egbase, view_.viewpoint, view_.zoom, interactive_player->player(), overlays, &dst);
 	} else {
+		const auto draw_immovables = intbase().draw_immovables() ?
+		                                  GameRenderer::DrawImmovables::kYes :
+		                                  GameRenderer::DrawImmovables::kNo;
+		const auto draw_bobs =
+		   intbase().draw_bobs() ? GameRenderer::DrawBobs::kYes : GameRenderer::DrawBobs::kNo;
 		const GameRenderer::Overlays overlays{static_cast<TextToDraw>(text_to_draw), {}};
-		renderer_->rendermap(egbase, view_.viewpoint, view_.zoom, overlays, &dst);
+		renderer_->rendermap(egbase, view_.viewpoint, view_.zoom, overlays, draw_immovables, draw_bobs, &dst);
 	}
 }