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

[Jens Beyer]

Ok, new try for the player-colored build-icons. Now with the code merged by the sprite-maps branch.

We now have one main advantage - we really do not need all those menu.png for the buildings anymore.

But there are two disadvantages - some of the icons are really barely distinguishable from the background, like the Barbarian Well; and the details of the large buildings somehow distract the eye in the menu, but this could also only be me.

I can imagine making the menu slightly bigger allowing for bigger buttons (like 50x50px). But the problem with the Well persists.

Before merging, we need to remove all menu.png in the tribes (not done yet, I just want code and design review; if you approve, I will remove those files and the sources related to them).

[SirVer (sirver)]

looks good and works great. I changed the code to use get_animation("idle") instead of main_animation() - which should probably be removed anyways. I also dropped a NOCOM(#qcs) for you.

I would also suggest doing a side by side comparison between the new menu and the old to see if quality has decreased before submitting.

When you remove the menu.png, keep in mind that they sometimes are used as idle animation too - so you'll need to check the conf files real quick too.

I suggest making the images bigger - resolutions have changed over the years. But that can be done separately if you prefer.

[Jens Beyer (qcumber-some)]

I fixed the NOCOM.
But there is one problem which was not present yesterday: The playercolors in the building menu are completely wrong. I remember that it definitely worked. What could have caused this?

[Jens Beyer (qcumber-some)]

I am not sure why, but the playercolors in the build menu do not work on the branch, even with trunk merged; but merging this branch into trunk makes the playercolor work in the build menu.
Now I am totally confused.

[SirVer (sirver)]

const Image& anim_frame = g_gr->animations().get_animation(descr.get_animation("idle"))
.representative_image(RGBColor(0, 0, 0));

well, here was the problem. You construct an image with playercolor black, always. I changed it to use the true player color.

While doing that I also checked the differences in the resized image - and they look awful. The menu.png images are much prettier - especially noticable in the big buildings. I think the zoomSurface method is not cutting it really - we need something fancier that does proper filtering. The plus side is that we then can get rid of the SDLgfx dependency which we only use for resizing. The downside is that we need to add another library or roll our own - keep in mind that we also need to upscale sometimes (menus). I think this is really important, but can be done after this is merged - however if this is merged, it needs to be done and soonish.

This looks sophisticated:
http://www.codeproject.com/Articles/11143/Image-Resizing-outperform-GDI

This should give some ideas:
http://stackoverflow.com/questions/3086770/what-algorithms-to-use-for-image-downsizing

[SirVer (sirver)]

And there is a bilinear interpolation ready to be copy&pasted: http://www.scratchapixel.com/lessons/3d-advanced-lessons/interpolation/bilinear-interpolation/

That is maybe already good enough.

[SirVer (sirver)]

I looked around the SDLgfx source code real quick and saw that it provides a shrinking method that gives better quality then just using zoom to shrink. I added a quick hack to shrink the image to ballpark the size, then zoomSurface to get it correct. It still looks worse than menu.png, but much better than before. I think we can ship this and see if somebody complains. Or we enhance the image rescaling later anyways.

[SirVer (sirver)]

ping?

[Jens Beyer (qcumber-some)]

Sorry, got distracted by 'the other life'.

I say this is good enough to go on trunk.

Few words:
1) We can not get rid yet of the menu.png files as they are used at other places (Editor, Messages...). Those should be targetted next.
2) I agree that I can live with the current state of the menu images on this branch. Though I did not have a look at them with very low resolution, rather the opposite ;-) But I trust your eyes more than mine.
3) Maybe we should file two new bugs then after merging this: Get rid of menu.png files, and find a better scaling algorithm.

Unfortunately I can not contribute to widelands development for the next few weeks.

[SirVer (sirver)]

Merged.

=== modified file 'src/constants.h'
--- src/constants.h	2013-10-14 07:20:46 +0000
+++ src/constants.h	2014-03-18 19:28:13 +0000
@@ -38,6 +38,9 @@
 #define TEXTURE_HEIGHT TEXTURE_WIDTH
 //@}
 
+//sizes for the images in the build menu (containing building icons)
+#define BUILDMENU_IMAGE_SIZE 30. // used for width and height
+
 #define XRES 800 ///< Fullscreen Menu Width
 #define YRES 600 ///< Fullscreen Menu Height
 
 
=== modified file 'src/graphic/animation.h'
--- src/graphic/animation.h	2014-03-16 20:55:15 +0000
+++ src/graphic/animation.h	2014-03-18 19:28:13 +0000
@@ -63,9 +63,10 @@
 	/// so the caller has to adjust for the hotspot himself.
 	virtual const Point& hotspot() const = 0;
 
-	/// An image frame that shows a singular animation frame. This can be used in
-	/// the UI (e.g. buildingwindow to represent this image.
-	virtual const Image& representative_image(const RGBColor& clr) const = 0;
+	// An image frame that shows the first animation frame, colored using the
+	// 'player_clr'. This can be used in the UI (e.g. buildingwindow) to
+	// represent this image.
+	virtual const Image& representative_image(const RGBColor& player_clr) const = 0;
 
 	/// Blit the animation frame that should be displayed at the given time index
 	/// so that the given point is at the top left of the frame. Srcrc defines
 
=== modified file 'src/graphic/image_transformations.cc'
--- src/graphic/image_transformations.cc	2014-02-22 18:04:02 +0000
+++ src/graphic/image_transformations.cc	2014-03-18 19:28:13 +0000
@@ -97,9 +97,25 @@
 		srcsdl = extract_sdl_surface(*src, srcrect);
 	}
 
+	// If we actually shrink a surface, ballpark the zoom so that the shrinking
+	// effect is weakened.
+	int factor = 1;
+	while ((static_cast<double>(w) * factor / srcsdl->w) < 1. ||
+	       (static_cast<double>(h) * factor / srcsdl->h) < 1.) {
+		++factor;
+	}
+	if (factor > 2) {
+		SDL_Surface* temp = shrinkSurface(srcsdl, factor - 1, factor - 1);
+		if (free_source) {
+			SDL_FreeSurface(srcsdl);
+		}
+		srcsdl = temp;
+		free_source = true;
+	}
+
 	// Third step: perform the zoom and placement
-	SDL_Surface * zoomed = zoomSurface
-		(srcsdl, double(w) / srcsdl->w, double(h) / srcsdl->h, 1);
+	SDL_Surface* zoomed = zoomSurface(srcsdl, double(w) / srcsdl->w, double(h) / srcsdl->h, 1);
+
 	if (free_source)
 		SDL_FreeSurface(srcsdl);
 
 
=== modified file 'src/wui/fieldaction.cc'
--- src/wui/fieldaction.cc	2014-03-07 19:12:24 +0000
+++ src/wui/fieldaction.cc	2014-03-18 19:28:13 +0000
@@ -23,6 +23,7 @@
 #include "economy/flag.h"
 #include "economy/road.h"
 #include "graphic/graphic.h"
+#include "graphic/image_transformations.h"
 #include "i18n.h"
 #include "logic/attackable.h"
 #include "logic/cmd_queue.h"
@@ -59,15 +60,16 @@
 
 // The BuildGrid presents a selection of buildable buildings
 struct BuildGrid : public UI::Icon_Grid {
-	BuildGrid
-		(UI::Panel                    * parent,
-		 const Widelands::Tribe_Descr & tribe,
-		 int32_t x, int32_t y,
-		 int32_t cols);
+	BuildGrid(UI::Panel* parent,
+	          const RGBColor& player_color,
+	          const Widelands::Tribe_Descr& tribe,
+	          int32_t x,
+	          int32_t y,
+	          int32_t cols);
 
-	boost::signals2::signal<void (Widelands::Building_Index::value_t)> buildclicked;
-	boost::signals2::signal<void (Widelands::Building_Index::value_t)> buildmouseout;
-	boost::signals2::signal<void (Widelands::Building_Index::value_t)> buildmousein;
+	boost::signals2::signal<void(Widelands::Building_Index::value_t)> buildclicked;
+	boost::signals2::signal<void(Widelands::Building_Index::value_t)> buildmouseout;
+	boost::signals2::signal<void(Widelands::Building_Index::value_t)> buildmousein;
 
 	void add(Widelands::Building_Index::value_t);
 
@@ -77,26 +79,22 @@
 	void mouseinslot(int32_t idx);
 
 private:
-	const Widelands::Tribe_Descr & m_tribe;
+	const RGBColor player_color_;
+	const Widelands::Tribe_Descr& tribe_;
 };
 
-
-BuildGrid::BuildGrid
-	(UI::Panel                    * parent,
-	 const Widelands::Tribe_Descr & tribe,
-	 int32_t x, int32_t y,
-	 int32_t                        cols)
-:
-	UI::Icon_Grid
-		(parent, x, y, BG_CELL_WIDTH, BG_CELL_HEIGHT, cols),
-	m_tribe(tribe)
+BuildGrid::BuildGrid(
+		UI::Panel* parent, const RGBColor& player_color, const Widelands::Tribe_Descr& tribe,
+		int32_t x, int32_t y, int32_t cols) :
+	UI::Icon_Grid(parent, x, y, BG_CELL_WIDTH, BG_CELL_HEIGHT, cols),
+	player_color_(player_color),
+	tribe_(tribe)
 {
 	clicked.connect(boost::bind(&BuildGrid::clickslot, this, _1));
 	mouseout.connect(boost::bind(&BuildGrid::mouseoutslot, this, _1));
 	mousein.connect(boost::bind(&BuildGrid::mouseinslot, this, _1));
 }
 
-
 /*
 ===============
 Add a new building to the list of buildable buildings
@@ -105,13 +103,18 @@
 void BuildGrid::add(Widelands::Building_Index::value_t id)
 {
 	const Widelands::Building_Descr & descr =
-		*m_tribe.get_building_descr(Widelands::Building_Index(id));
+		*tribe_.get_building_descr(Widelands::Building_Index(id));
+	const Image& anim_frame = g_gr->animations().get_animation(descr.get_animation("idle"))
+		.representative_image(player_color_);
+	const uint16_t image_w = anim_frame.width();
+	const uint16_t image_h = anim_frame.height();
+	double ratio = BUILDMENU_IMAGE_SIZE / std::max(image_w, image_h);
+	const Image* menu_image = ImageTransformations::resize(&anim_frame, image_w * ratio, image_h * ratio);
 	UI::Icon_Grid::add
-		(descr.name(), descr.get_buildicon(),
+		(descr.name(), menu_image,
 		 reinterpret_cast<void *>(id),
 		 descr.descname() + "<br><font size=11>" + _("Construction costs:") + "</font><br>" +
-			waremap_to_richtext(m_tribe, descr.buildcost()));
-;
+			waremap_to_richtext(tribe_, descr.buildcost()));
 }
 
 
@@ -181,7 +184,7 @@
 
 	void init();
 	void add_buttons_auto();
-	void add_buttons_build(int32_t buildcaps);
+	void add_buttons_build(int32_t buildcaps, const RGBColor& player_color);
 	void add_buttons_road(bool flag);
 	void add_buttons_attack();
 
@@ -396,11 +399,11 @@
 			const int32_t buildcaps = m_plr ? m_plr->get_buildcaps(m_node) : 0;
 
 			// Add house building
-			if
-				((buildcaps & Widelands::BUILDCAPS_SIZEMASK)
-				 ||
-				 (buildcaps & Widelands::BUILDCAPS_MINE))
-				add_buttons_build(buildcaps);
+			if ((buildcaps & Widelands::BUILDCAPS_SIZEMASK) ||
+			    (buildcaps & Widelands::BUILDCAPS_MINE)) {
+				assert(igbase->get_player());
+				add_buttons_build(buildcaps, igbase->get_player()->get_playercolor());
+			}
 
 			// Add build actions
 			if ((m_fastclick = buildcaps & Widelands::BUILDCAPS_FLAG))
@@ -508,7 +511,7 @@
 Add buttons for house building.
 ===============
 */
-void FieldActionWindow::add_buttons_build(int32_t buildcaps)
+void FieldActionWindow::add_buttons_build(int32_t buildcaps, const RGBColor& player_color)
 {
 	if (not m_plr)
 		return;
@@ -558,7 +561,7 @@
 
 		// Allocate the tab's grid if necessary
 		if (!*ppgrid) {
-			*ppgrid = new BuildGrid(&m_tabpanel, tribe, 0, 0, 5);
+			*ppgrid = new BuildGrid(&m_tabpanel, player_color, tribe, 0, 0, 5);
 			(*ppgrid)->buildclicked.connect(boost::bind(&FieldActionWindow::act_build, this, _1));
 			(*ppgrid)->buildmouseout.connect
 				(boost::bind(&FieldActionWindow::building_icon_mouse_out, this, _1));