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

[SirVer]

Make resources toggable in the editor.

- Changes drawing of resources to use a callback instead of a vector of objects which saves some code and might be slightly more efficient.

[SirVer (sirver)]

Might also fix bug 1696064, at least I could not repro it with this branch.

Bug 1649958 contains more discussion about this feature, for example I did not add a hotkey yet (which should be simple to do), because I am unsure about the current consensus.

Not drawing map objects is not part of this branch, but should be relatively simple to implement.

[SirVer (sirver)]

Oh, and this is diffbased on https://code.launchpad.net/~widelands-dev/widelands/buildhelp_overlay which must be merged first.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2534. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/264163602.
Appveyor build 2358. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_toggle_resources-2358.

[toptopple (7010622-q-deactivatedaccount)]

Thanks for taking this up, SirVer!

Looks good to me on first sight (not extensively tested) with one exception:
-- issue: the resource editing tool has to be deactivated while the RESOURCE VIEW is switched OFF. Deactivated means it must not be possible to create or remove any resources on the map.

Extended:
-- the 'q' key is targeted as a shortcut for the resource view button

[SirVer (sirver)]

> the 'q' key is targeted as a shortcut for the resource view button

done.

> issue: the resource editing tool has to be deactivated while the RESOURCE VIEW is switched OFF. Deactivated means it must not be possible to create or remove any resources on the map.

I disagree with this design for two reasons:

1) precedence: right now it is already possible to change stuff with UI disabled. For example you can change terrain and this changes the buildhelp even if it is toggled off. You can also add/remove port spaces without the buildhelp being enabled.

2) mental model: right now, the editor is simple: you have exactly one tool selected and you can toggle some visualization (buildhelp) on and off. Both are independent. Now we add a new toggle for another visualization (resources), but it is another toggle, not bound to any of the other state.

If you now make some tools dependent on which visualization is toggled on you get a decision tree that the user has to understand first. I argue this is more complex in code and more complex to use.

[toptopple (7010622-q-deactivatedaccount)]

I assume people, including me, can live with such arrangement as you propose. I find it not so convincing, however. While I see your point, I don't quite agree that the current state of the Buildhelp view and related tools has to remain as is.

If I had to decide I probably would prefer the WYSIWIG principle and regard it undesirable you have feasibility to perform editions which you are not visibly aware of, or made aware of, so to speak. So I would agree the edition principles should hold strong for all similar tools, but I rather suggest to investigate to change the Buildhelp settings than allow for invisible editions for other tools.

The modifications of terrain can be ignored as Buildhelp display can be defined as derivation from the current terrain shape, so it's not really that you create or remove Buildhelp patterns. In that there is no tool involved. Only thing relevant are edifice creation or Portspace creation and these would have to be changed and made dependent on Buildhelp visibility. On a sidenote: once Portspaces are indicated by Anchors, as is planned for the future, the Buildhelp arguments falls aside for this tool.

[SirVer (sirver)]

>>> from Gun
I think we could further simplify FieldOverlayManager::get_overlays by making kLevelForBuildHelp part of the OverlayLevel enum class, something like this:

 auto it = overlays_.lower_bound(c);
 while (it != overlays_.end() && it->first == c) {
            if (buildhelp_ && it->second.level == OverlayLevel::kLevelForBuildHelp) {
                result->emplace_back(buildhelp_infos_[get_buildhelp_overlay(c)]);
            } else {
  result->emplace_back(it->second.pic, it->second.hotspot);
  ++it;
            }
 }

We should then be able to use a range-bases for loop too?
<<<

OverlayLevel is part of the public API of this class and kLevelForBuildHelp should never be used when 'register_overlay' is called. I therefore opted to not make it part of the enum class to make misuse harder.

Also, you cannot range base over all values in an enum class easily [1] and since no overlay will ever be inserted into 'overlays_' where 'it->second.level == OverlayLevel::kLevelForBuildHelp' - since buildhelp is handled differently from all other overlays - the suggested loop simplification unfortunately does not work.

I am not super happy with how this class looks and agree with your push to make it simpler. Unfortunately the correct way I see for this would be to make buildhelp not be special - which would mean that the class will require much more memory which is also not super desirable. Alternatively we could scratch the multimap and instead have callbacks for each OverlayLevel and calculate which overlays are required each frame on the fly. But that is also a pretty deep change. I think for now I am satisfied with having pushed the class a bit towards simplification.

[1] https://stackoverflow.com/questions/8498300/allow-for-range-based-for-with-enum-classes

[SirVer (sirver)]

At #12: I think we just disagree here. I prefer a simple mental model, you prefer a tool that always shows what is currently changing. I would then argue: what happens for undo-redo, would this toggle resources on and off? Also, tools like Gimp or photoshop also support my position [1]: If you make a layer invisible in gimp and draw into the image, the layer is modified though there is no visual feedback. I also argue that your implementation is harder to get right because it has more cross-dependencies between tools and UI.

For now I suggest we submit this as is and see if people actually have trouble with the simple mental model - we can always revisit in the future.

[1] https://www.youtube.com/watch?v=heFsYfQjOKg

[toptopple (7010622-q-deactivatedaccount)]

Ok, I don't think I have any personal problem here! ;)

Are you going to work on the second view filter as well? This should switch all objects of the "Immovable" class/tool. And independent thereof: can you implement or make a suggestion for the Immovable View switch button?

[SirVer (sirver)]

> Are you going to work on the second view filter as well? This should switch all objects of the "Immovable" class/tool.

This and independent toggling of animals/bobs is done in https://code.launchpad.net/~widelands-dev/widelands/toggle_immovables/+merge/328977.

> And independent thereof: can you implement or make a suggestion for the Immovable View switch button?

Not sure I understand the question. Are you talking about the button graphic? I am not an artist, I simply copied the icons for the corresponding tool menus. Together with the grouping of the buttons into toggable UI things I think that is sufficiently understandable.

[GunChleoc (gunchleoc)]

> OverlayLevel is part of the public API of this class and kLevelForBuildHelp should never be used when 'register_overlay' is called. I therefore opted to not make it part of the enum class to make misuse harder.

This has convinced me, plus that we really don't want to use more memory. Let's keep it as it is then.

Regarding the overlay on/off thing, how about switching it on when the tool gets activated? IMO it might be annoying with the increase/decrease tool if the user had to remember to switch it on in order to see what they are doing.

I agree that deactivating the tool when the overlay is switched off would be confusing for the user and cause some hair-pulling - reactions would probably be "Where on Earth has my bl** tool gone?"

Code LGTM, I'll do a bit of testing.

review: Approve

[GunChleoc (gunchleoc)]

Testing works. I'll leave it up to you if you want to real with the auto-switchon in this merge request or in the follow-up branch.

[SirVer (sirver)]

> Regarding the overlay on/off thing, how about switching it on when the tool gets activated?

I do not like this sort of covenience magic. It has the ability to annoy users just as much as help them. If we design the hotkeys correctly it will be cheap to toggle the overlays on and off, so this convenience will not add much.

[SirVer (sirver)]

@bunnybot merge

[GunChleoc (gunchleoc)]

OK, let's leave this as it is then.

=== added file 'data/images/wui/menus/menu_toggle_resources.png'
Binary files data/images/wui/menus/menu_toggle_resources.png	1970-01-01 00:00:00 +0000 and data/images/wui/menus/menu_toggle_resources.png	2017-08-15 09:35:42 +0000 differ
=== modified file 'data/scripting/editor/editor_controls.lua'
--- data/scripting/editor/editor_controls.lua	2016-10-01 07:45:42 +0000
+++ data/scripting/editor/editor_controls.lua	2017-08-15 09:35:42 +0000
@@ -23,6 +23,8 @@
             toggle_building_spaces_hotkey ..
             -- TRANSLATORS: This is an access key combination. The hotkey is 'p'
             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-13 18:38:27 +0000
+++ src/editor/editorinteractive.cc	2017-08-15 09:35:42 +0000
@@ -115,16 +115,22 @@
 
 	toolbar_.add_space(15);
 
-	add_toolbar_button(
-	   "wui/menus/menu_toggle_minimap", "minimap", _("Minimap"), &minimap_registry(), true);
-	minimap_registry().open_window = [this] { toggle_minimap(); };
-
 	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));
-
-	reset_zoom_ = add_toolbar_button("wui/menus/menu_reset_zoom", "reset_zoom", _("Reset zoom"));
-	reset_zoom_->sigclicked.connect([this] {
+	toggle_resources_ = add_toolbar_button(
+	   "wui/menus/menu_toggle_resources", "resources", _("Show Resources (on/off)"));
+	toggle_resources_->set_perm_pressed(true);
+	toggle_resources_->sigclicked.connect([this]() { toggle_resources(); });
+
+	toolbar_.add_space(15);
+
+	add_toolbar_button(
+	   "wui/menus/menu_toggle_minimap", "minimap", _("Minimap"), &minimap_registry(), true);
+	minimap_registry().open_window = [this] { toggle_minimap(); };
+
+	auto zoom = add_toolbar_button("wui/menus/menu_reset_zoom", "reset_zoom", _("Reset zoom"));
+	zoom->sigclicked.connect([this] {
 		zoom_around(1.f, Vector2f(get_w() / 2.f, get_h() / 2.f), MapView::Transition::Smooth);
 	});
 
@@ -326,6 +332,13 @@
 	toggle_buildhelp_->set_perm_pressed(value);
 }
 
+void EditorInteractive::toggle_resources() {
+	auto* overlay_manager = mutable_field_overlay_manager();
+	const bool value = !overlay_manager->is_enabled(OverlayLevel::kResource);
+	overlay_manager->set_enabled(OverlayLevel::kResource, value);
+	toggle_resources_->set_perm_pressed(value);
+}
+
 bool EditorInteractive::handle_key(bool const down, SDL_Keysym const code) {
 	bool handled = InteractiveBase::handle_key(down, code);
 
@@ -416,22 +429,27 @@
 			handled = true;
 			break;
 
+		case SDLK_l:
+			if (code.mod & (KMOD_LCTRL | KMOD_RCTRL))
+				new MainMenuLoadMap(*this);
+			handled = true;
+			break;
+
 		case SDLK_m:
 			minimap_registry().toggle();
 			handled = true;
 			break;
 
-		case SDLK_l:
-			if (code.mod & (KMOD_LCTRL | KMOD_RCTRL))
-				new MainMenuLoadMap(*this);
-			handled = true;
-			break;
-
 		case SDLK_p:
 			playermenu_.toggle();
 			handled = true;
 			break;
 
+		case SDLK_q:
+			toggle_resources();
+			handled = true;
+			break;
+
 		case SDLK_s:
 			if (code.mod & (KMOD_LCTRL | KMOD_RCTRL))
 				new MainMenuSaveMap(*this);
@@ -443,6 +461,12 @@
 			handled = true;
 			break;
 
+		case SDLK_y:
+			if (code.mod & (KMOD_LCTRL | KMOD_RCTRL))
+				history_->redo_action(egbase().world());
+			handled = true;
+			break;
+
 		case SDLK_z:
 			if ((code.mod & (KMOD_LCTRL | KMOD_RCTRL)) && (code.mod & (KMOD_LSHIFT | KMOD_RSHIFT)))
 				history_->redo_action(egbase().world());
@@ -451,12 +475,6 @@
 			handled = true;
 			break;
 
-		case SDLK_y:
-			if (code.mod & (KMOD_LCTRL | KMOD_RCTRL))
-				history_->redo_action(egbase().world());
-			handled = true;
-			break;
-
 		case SDLK_F1:
 			helpmenu_.toggle();
 			handled = true;

=== modified file 'src/editor/editorinteractive.h'
--- src/editor/editorinteractive.h	2017-06-24 08:47:46 +0000
+++ src/editor/editorinteractive.h	2017-08-15 09:35:42 +0000
@@ -156,6 +156,8 @@
 
 	void on_buildhelp_changed(const bool value) override;
 
+	void toggle_resources();
+
 	//  state variables
 	bool need_save_;
 	std::vector<PlayerReferences> player_tribe_references_;
@@ -178,7 +180,7 @@
 	UI::UniqueWindow::Registry helpmenu_;
 
 	UI::Button* toggle_buildhelp_;
-	UI::Button* reset_zoom_;
+	UI::Button* toggle_resources_;
 	UI::Button* undo_;
 	UI::Button* redo_;
 

=== modified file 'src/graphic/game_renderer.cc'
--- src/graphic/game_renderer.cc	2017-08-13 18:02:53 +0000
+++ src/graphic/game_renderer.cc	2017-08-15 09:35:42 +0000
@@ -210,7 +210,6 @@
                   const Player* player,
                   const TextToDraw text_to_draw,
                   RenderTarget* dst) {
-	std::vector<FieldOverlayManager::OverlayInfo> overlay_info;
 	for (size_t current_index = 0; current_index < fields_to_draw.size(); ++current_index) {
 		const FieldsToDraw::Field& field = fields_to_draw.at(current_index);
 		if (!field.all_neighbors_valid()) {
@@ -248,18 +247,13 @@
 			draw_objects_for_formerly_visible_field(field, player_field, zoom, dst);
 		}
 
-		const FieldOverlayManager& overlay_manager = egbase.get_ibase()->field_overlay_manager();
-		{
-			overlay_info.clear();
-			overlay_manager.get_overlays(field.fcoords, &overlay_info);
-			for (const auto& overlay : overlay_info) {
-				dst->blitrect_scale(
-				   Rectf(field.rendertarget_pixel - overlay.hotspot.cast<float>() * zoom,
-				         overlay.pic->width() * zoom, overlay.pic->height() * zoom),
-				   overlay.pic, Recti(0, 0, overlay.pic->width(), overlay.pic->height()), 1.f,
-				   BlendMode::UseAlpha);
-			}
-		}
+		egbase.get_ibase()->field_overlay_manager().foreach_overlay(
+		   field.fcoords, [dst, &field, zoom](const Image* pic, const Vector2i& hotspot) {
+			   dst->blitrect_scale(Rectf(field.rendertarget_pixel - hotspot.cast<float>() * zoom,
+			                             pic->width() * zoom, pic->height() * zoom),
+			                       pic, Recti(0, 0, pic->width(), pic->height()), 1.f,
+			                       BlendMode::UseAlpha);
+			});
 	}
 }
 

=== modified file 'src/wui/field_overlay_manager.cc'
--- src/wui/field_overlay_manager.cc	2017-08-15 07:47:21 +0000
+++ src/wui/field_overlay_manager.cc	2017-08-15 09:35:42 +0000
@@ -26,12 +26,6 @@
 #include "graphic/graphic.h"
 #include "logic/field.h"
 
-namespace {
-
-constexpr int kLevelForBuildHelp = 5;
-
-}  // namespace
-
 FieldOverlayManager::FieldOverlayManager() : current_overlay_id_(0) {
 	OverlayInfo* buildhelp_info = buildhelp_infos_;
 	const char* filenames[] = {"images/wui/overlays/set_flag.png", "images/wui/overlays/small.png",
@@ -64,28 +58,6 @@
 	buildhelp_ = value;
 }
 
-void FieldOverlayManager::get_overlays(const Widelands::FCoords& c,
-                                       std::vector<OverlayInfo>* result) const {
-	auto it = overlays_.lower_bound(c);
-	while (it != overlays_.end() && it->first == c &&
-	       static_cast<int>(it->second.level) <= kLevelForBuildHelp) {
-		result->emplace_back(it->second.pic, it->second.hotspot);
-		++it;
-	}
-
-	if (buildhelp_) {
-		int buildhelp_overlay_index = get_buildhelp_overlay(c);
-		if (buildhelp_overlay_index < Widelands::Field::Buildhelp_None) {
-			result->emplace_back(buildhelp_infos_[buildhelp_overlay_index]);
-		}
-	}
-
-	while (it != overlays_.end() && it->first == c) {
-		result->emplace_back(it->second.pic, it->second.hotspot);
-		++it;
-	}
-}
-
 int FieldOverlayManager::get_buildhelp_overlay(const Widelands::FCoords& fc) const {
 	Widelands::NodeCaps const caps =
 	   callback_ ? static_cast<Widelands::NodeCaps>(callback_(fc)) : fc.field->nodecaps();
@@ -195,3 +167,16 @@
 	++current_overlay_id_;
 	return current_overlay_id_;
 }
+
+bool FieldOverlayManager::is_enabled(const OverlayLevel& level) const {
+	return disabled_layers_.count(level) == 0;
+}
+
+void FieldOverlayManager::set_enabled(const OverlayLevel& level, const bool enabled) {
+	if (enabled) {
+		disabled_layers_.erase(level);
+	} else {
+		disabled_layers_.insert(level);
+	}
+}
+

=== modified file 'src/wui/field_overlay_manager.h'
--- src/wui/field_overlay_manager.h	2017-08-15 07:47:21 +0000
+++ src/wui/field_overlay_manager.h	2017-08-15 09:35:42 +0000
@@ -63,17 +63,6 @@
 	/// A unique id identifying a registered overlay.
 	using OverlayId = uint32_t;
 
-	/// A overlay as drawn onto the screen.
-	struct OverlayInfo {
-		OverlayInfo() = default;
-		OverlayInfo(const Image* init_pic, const Vector2i& init_hotspot)
-		   : pic(init_pic), hotspot(init_hotspot) {
-		}
-
-		const Image* pic;
-		Vector2i hotspot = Vector2i::zero();
-	};
-
 	/// A function returning Field::nodecaps() for the build overlay. This can be
 	/// registered to hide or change some of the nodecaps during rendering.
 	using CallbackFn = std::function<int32_t(const Widelands::FCoords& coordinates)>;
@@ -89,6 +78,10 @@
 	/// Register callback function.
 	void register_overlay_callback_function(CallbackFn function);
 
+	/// Like 'buildhelp', but for an individual layer.
+	bool is_enabled(const OverlayLevel& level) const;
+	void set_enabled(const OverlayLevel& level, bool value);
+
 	/// Get a unique, unused id that can be passed to register_overlay.
 	OverlayId next_overlay_id();
 
@@ -111,10 +104,44 @@
 	// TODO(sirver): It would be preferable to just delete and recreate the object.
 	void remove_all_overlays();
 
-	/// Returns the currently registered overlays and the buildhelp for a node.
-	void get_overlays(const Widelands::FCoords& c, std::vector<OverlayInfo>* result) const;
+	/// Calls 'func' for each of the the currently registered and enabled
+	/// overlays and the buildhelp.
+	template <typename T> void foreach_overlay(const Widelands::FCoords& c, T func) const {
+		auto it = overlays_.lower_bound(c);
+		while (it != overlays_.end() && it->first == c &&
+		       static_cast<int>(it->second.level) <= kLevelForBuildHelp) {
+			if (is_enabled(it->second.level)) {
+				func(it->second.pic, it->second.hotspot);
+			}
+			++it;
+		}
+
+		if (buildhelp_) {
+			int buildhelp_overlay_index = get_buildhelp_overlay(c);
+			if (buildhelp_overlay_index < Widelands::Field::Buildhelp_None) {
+				auto& overlay_info = buildhelp_infos_[buildhelp_overlay_index];
+				func(overlay_info.pic, overlay_info.hotspot);
+			}
+		}
+
+		while (it != overlays_.end() && it->first == c) {
+			if (is_enabled(it->second.level)) {
+				func(it->second.pic, it->second.hotspot);
+			}
+			++it;
+		}
+}
+
 
 private:
+	static constexpr int kLevelForBuildHelp = 5;
+
+	/// A overlay as drawn onto the screen.
+	struct OverlayInfo {
+		const Image* pic = nullptr;
+		Vector2i hotspot = Vector2i::zero();
+	};
+
 	struct RegisteredOverlays {
 		RegisteredOverlays(const OverlayId init_overlay_id,
 		                   const Image* init_pic,
@@ -137,6 +164,11 @@
 
 	OverlayInfo buildhelp_infos_[Widelands::Field::Buildhelp_None];
 	bool buildhelp_;
+	// We are inverting the logic here, since new layers are by default enabled
+	// and we only support to toggle some of them off. Otherwise whenever a new
+	// layer is added in 'OverlayLevel' we would also need to add it to the
+	// 'enabled_layers_' set on construction.
+	std::set<OverlayLevel> disabled_layers_;
 
 	// this callback is used to define where overlays are drawn.
 	CallbackFn callback_;