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

[SirVer]

Fixes the display of the buildhelp.

This got broken in r7688 by changing the callback function of the OverlayManager to return a bool instead of a NodeCap.

Fixes the buildhelp.

[GunChleoc (gunchleoc)]

Still needs testing, but code LGTM.

review: Approve

[bunnybot (widelandsofficial)]

Hi, I am bunnybot (https://github.com/widelands/bunnybot).

I am keeping the source branch lp:~widelands-dev/widelands/fix_overlays mirrored to https://github.com/widelands/widelands/tree/_widelands_dev_widelands_fix_overlays

You can give me commands by starting a line with @bunnybot <command>. I understand:
 merge: Merges the source branch into the target branch, closing the merge proposal. I will use the proposed commit message if it is set.

[kaputtnik (franku)]

Solved buildhelp during game play, but in editor there is something wrong:

1. When trying to place resources on mountains, the red small house is displayed on the mountains
2. When trying to place fish resource in editor the small red house is displayed on water

review: Needs Fixing

[bunnybot (widelandsofficial)]

Travis build 190 has changed state to: passed. Details: https://travis-ci.org/widelands/widelands/builds/100389397.

[SirVer (sirver)]

Should be all good now. I did some mild refactorings during bug tracking too. PTAL.

review: Needs Resubmitting

[GunChleoc (gunchleoc)]

Code still LGTM.

[kaputtnik (franku)]

Works now as expected :-)

The only thing is what Tino already mentioned: A resource could not removed if one choose another resource and hold down Shift while clicking. See https://code.launchpad.net/~janosch-peters/widelands/editor-remove-invalid-resources/+merge/281462/comments/713355

I don't know how it was before, but this is a inconsistent behavior to immovables. Immovables could ever be removed from the map regardless of the kind of actual immovable. F.e. trees could be removed if a ruin is selected by holding down Shift. So we might want to make a new bug about this?

Another side note: For resource water there is only one image in the repo (precisely there are two images: water.png and water4.png which are exactly the same). For other resources there are 4 images used, representing the amount of resource placed on the map. Is this intended or are the small images for water just missing?

Approve functionality for the build help.

review: Approve

[kaputtnik (franku)]

Dammn... two times reread but still ambiguous...please replace:

" F.e. trees could be removed if a ruin is selected by holding down Shift."
with
" F.e. trees could be removed if a ruin is selected and hold down Shift."

[GunChleoc (gunchleoc)]

I think water is just there or not, and it doesn't deplete - we only have 1 resource indicator for it too.

Please open a new bug for the resource removing thing.

[TiborB (tiborb95)]

> A resource could not removed if one choose another resource and hold down Shift while clicking.

This is old behavior, and yes it is inconsistency (between underground resources and removables)...

[TiborB (tiborb95)]

correction: ...immovables - of course

[xxx-deleted (janosch-peters-deactivatedaccount)]

Tho code look good, sorry for introducing this bug.

About the resource-editing thing: Strictly speaking I think the behaviour is consistent.

The analogous feature of "remove immovable" in terms of resources is "set resource to 0". This works even if you have a different resource selected. The decrease/increase resource has no equivalent.

After a little bit of testing I also think the current increase/decrease behaviour makes sense. Imaging you have several resource sitting next to another. If you have a bigger tool size and start to reduce resources, it should only affect the resource you have selected. If we change the behaviour to allow decreasing any resource, at least in my scenario this will lead to unexpected results.

review: Approve

[SirVer (sirver)]

> Tho code look good, sorry for introducing this bug.

no worries, changing code introduces bugs, that is the way of life.

> I think water is just there or not, and it doesn't deplete - we only have 1 resource indicator for it too.

that is not correct. Water is not handled special in any way in the code, it is just another resource. The 'percentage found when depleted' is just set higher for wells.
I think I was just too lazy to make more than one water resource indicator back in the day, there is really no reason why there is only one that I can think of.

thanks for reviewing/testing. It seems like everybody is happy with this branch now, the open discussion items have nothing to do with the changes in this particular branch. So gonna merge now.

@bunnybot merge

[kaputtnik (franku)]

Yes, you are right :-)

I didn't understand the post from GunChleoc. My question was just if there are images for water resource missing which indicates less amount of water on the map.

[GunChleoc (gunchleoc)]

From what Sirver said, it sounds like I am wrong and they are missing. So is a resource indicator for little water as opposed to a lot of water. Would you like fixing that for us?

=== modified file 'src/editor/tools/editor_decrease_resources_tool.cc'
--- src/editor/tools/editor_decrease_resources_tool.cc	2016-01-04 20:50:19 +0000
+++ src/editor/tools/editor_decrease_resources_tool.cc	2016-01-07 09:30:32 +0000
@@ -56,10 +56,10 @@
 			map->is_resource_valid(world, mr.location(), args->cur_res)) {
 			args->orgResT.push_back(mr.location().field->get_resources());
 			args->orgRes.push_back(mr.location().field->get_resources_amount());
-			EditorSetResourcesTool::set_res_and_overlay(world, amount, args->cur_res, &mr, args, map);
+			EditorSetResourcesTool::set_res_and_overlay(
+			   world, amount, args->cur_res, mr.location(), map);
 		}
 
-
 	} while (mr.advance(*map));
 	return mr.radius();
 }

=== modified file 'src/editor/tools/editor_increase_resources_tool.cc'
--- src/editor/tools/editor_increase_resources_tool.cc	2016-01-04 20:50:19 +0000
+++ src/editor/tools/editor_increase_resources_tool.cc	2016-01-07 09:30:32 +0000
@@ -54,10 +54,9 @@
 				map->is_resource_valid(world, mr.location(), args->cur_res)) {
 			args->orgResT.push_back(mr.location().field->get_resources());
 			args->orgRes.push_back(mr.location().field->get_resources_amount());
-			EditorSetResourcesTool::set_res_and_overlay(world, amount, args->cur_res, &mr, args, map);
+			EditorSetResourcesTool::set_res_and_overlay(
+			   world, amount, args->cur_res, mr.location(), map);
 		}
-
-
 	} while (mr.advance(*map));
 	return mr.radius();
 }

=== modified file 'src/editor/tools/editor_set_resources_tool.cc'
--- src/editor/tools/editor_set_resources_tool.cc	2016-01-04 20:50:19 +0000
+++ src/editor/tools/editor_set_resources_tool.cc	2016-01-07 09:30:32 +0000
@@ -52,10 +52,8 @@
 		if (map->is_resource_valid(world, mr.location(), args->cur_res)) {
 			args->orgResT.push_back(mr.location().field->get_resources());
 			args->orgRes.push_back(mr.location().field->get_resources_amount());
-			set_res_and_overlay(world, amount, args->cur_res, &mr, args, map);
+			set_res_and_overlay(world, amount, args->cur_res, mr.location(), map);
 		}
-
-
 	} while (mr.advance(*map));
 	return mr.radius();
 }
@@ -66,14 +64,12 @@
                                             EditorInteractive& /* parent */,
                                             EditorActionArgs* args,
 											Widelands::Map* map) {
-	OverlayManager & overlay_manager = map->overlay_manager();
 	Widelands::MapRegion<Widelands::Area<Widelands::FCoords> > mr
 	(*map,
 	 Widelands::Area<Widelands::FCoords>
 	 (map->get_fcoords(center.node), args->sel_radius));
 	std::list<uint8_t>::iterator ir = args->orgRes.begin(), it = args->orgResT.begin();
 	do {
-		int32_t res        = mr.location().field->get_resources();
 		int32_t amount     = *ir;
 		int32_t max_amount = world.get_resource(args->cur_res)->max_amount();
 
@@ -82,7 +78,7 @@
 		if (amount > max_amount)
 			amount = max_amount;
 
-		set_res_and_overlay(world, amount, *ir, &mr, args, map);
+		set_res_and_overlay(world, amount, *ir, mr.location(), map);
 
 		++ir;
 		++it;
@@ -102,31 +98,28 @@
 
 void EditorSetResourcesTool::set_res_and_overlay(const Widelands::World& world,
 											int32_t amount, uint8_t new_res,
-											Widelands::MapRegion<Widelands::Area<Widelands::FCoords> >* mr,
-											EditorActionArgs* args,
+											const Widelands::FCoords& fcoords,
 											Widelands::Map* map) {
-	int32_t old_res = mr->location().field->get_resources();
+	int32_t old_res = fcoords.field->get_resources();
 
 	//  Ok, we're doing something. First remove the current overlays.
 	if (old_res != Widelands::kNoResource) {
 		std::string str = world.get_resource(old_res)->get_editor_pic(
-				mr->location().field->get_resources_amount());
+				fcoords.field->get_resources_amount());
 		const Image* pic = g_gr->images().get(str);
-		map->overlay_manager().remove_overlay(mr->location(), pic);
+		map->overlay_manager().remove_overlay(fcoords, pic);
 	}
 
 	if (!amount) {
-		mr->location().field->set_resources(Widelands::kNoResource, 0);
-		mr->location().field->set_initial_res_amount(0);
+		fcoords.field->set_resources(Widelands::kNoResource, 0);
+		fcoords.field->set_initial_res_amount(0);
 	} else {
-		mr->location().field->set_resources(new_res, amount);
-		mr->location().field->set_initial_res_amount(amount);
+		fcoords.field->set_resources(new_res, amount);
+		fcoords.field->set_initial_res_amount(amount);
 		//  set new overlay
 		std::string str = world.get_resource(new_res)->get_editor_pic(amount);
 		const Image* pic = g_gr->images().get(str);
-		map->overlay_manager().register_overlay(mr->location(), pic, 4);
-		map->recalc_for_field_area(
-				world, Widelands::Area<Widelands::FCoords>(mr->location(), 0));
+		map->overlay_manager().register_overlay(fcoords, pic, 0);
+		map->recalc_for_field_area(world, Widelands::Area<Widelands::FCoords>(fcoords, 0));
 	}
 }
-

=== modified file 'src/editor/tools/editor_set_resources_tool.h'
--- src/editor/tools/editor_set_resources_tool.h	2016-01-04 20:50:19 +0000
+++ src/editor/tools/editor_set_resources_tool.h	2016-01-07 09:30:32 +0000
@@ -50,9 +50,11 @@
 	/**
 	 * Sets the resource amount and updates the overlay.
 	 */
-	static void set_res_and_overlay(const Widelands::World& world, int32_t amount, uint8_t resIx,
-			Widelands::MapRegion<Widelands::Area<Widelands::FCoords> > *mr,
-			EditorActionArgs* args, Widelands::Map* map);
+	static void set_res_and_overlay(const Widelands::World& world,
+	                                int32_t amount,
+	                                uint8_t resIx,
+	                                const Widelands::FCoords& fcoords,
+	                                Widelands::Map* map);
 
 	char const * get_sel_impl() const override {
 		return "pics/fsel_editor_set_resources.png";

=== modified file 'src/editor/tools/editor_set_starting_pos_tool.cc'
--- src/editor/tools/editor_set_starting_pos_tool.cc	2016-01-04 20:50:19 +0000
+++ src/editor/tools/editor_set_starting_pos_tool.cc	2016-01-07 09:30:32 +0000
@@ -95,7 +95,7 @@
 
 			//  add new overlay
 			overlay_manager.register_overlay
-			(center.node, pic, 8, Point(pic->width() / 2, STARTING_POS_HOTSPOT_Y));
+			(center.node, pic, 4, Point(pic->width() / 2, STARTING_POS_HOTSPOT_Y));
 
 			//  set new player pos
 			map->set_starting_pos(m_current_player, center.node);

=== modified file 'src/editor/ui_menus/editor_tool_change_resources_options_menu.cc'
--- src/editor/ui_menus/editor_tool_change_resources_options_menu.cc	2016-01-05 10:07:13 +0000
+++ src/editor/ui_menus/editor_tool_change_resources_options_menu.cc	2016-01-07 09:30:32 +0000
@@ -222,8 +222,12 @@
 	Widelands::EditorGameBase& egbase = eia().egbase();
 	Widelands::Map & map = egbase.map();
 	map.overlay_manager().register_overlay_callback_function(
-			[&] (const Widelands::TCoords<Widelands::FCoords>& coords)
-			{return map.is_resource_valid(egbase.world(), coords, resIx);});
+		[resIx, &map, &egbase](const Widelands::TCoords<Widelands::FCoords>& coords) -> uint32_t {
+			if (map.is_resource_valid(egbase.world(), coords, resIx)) {
+				return coords.field->nodecaps();
+			}
+			return 0;
+		});
 
 	map.recalc_whole_map(egbase.world());
 	select_correct_tool();

=== modified file 'src/wui/overlay_manager.cc'
--- src/wui/overlay_manager.cc	2015-12-30 21:47:08 +0000
+++ src/wui/overlay_manager.cc	2016-01-07 09:30:32 +0000
@@ -44,6 +44,7 @@
 	RegisteredOverlaysMap::const_iterator it = overlay_map.lower_bound(c);
 	while (it != overlay_map.end() && it->first == c && it->second.level <= MAX_OVERLAYS_PER_NODE)
 	{
+		assert(it->second.pic);
 		overlays[num_ret].pic = it->second.pic;
 		overlays[num_ret].hotspot = it->second.hotspot;
 		if (++num_ret == MAX_OVERLAYS_PER_NODE)

=== modified file 'src/wui/overlay_manager.h'
--- src/wui/overlay_manager.h	2016-01-03 21:51:01 +0000
+++ src/wui/overlay_manager.h	2016-01-07 09:30:32 +0000
@@ -65,8 +65,11 @@
 		Point hotspot;
 	};
 
+	// The function used to calculate overlays for the build help. The function
+	// needs to return the nodecaps of the field that correspond to the image
+	// that should be used.
 	using CallbackFn =
-		boost::function<bool(const Widelands::TCoords<Widelands::FCoords>& coordinates)>;
+		boost::function<int32_t(const Widelands::TCoords<Widelands::FCoords>& coordinates)>;
 
 	OverlayManager();