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

[Jens Beyer]

This is not a merge-request per se, but more a request to review the code, and to find some way to create the necessary media files.

With this branch merged, the game displays the building icons in player color if available.
It uses playercolor mask files (menu_pc.png next to their menu.png files). Unfortunately, the mask files for the menu.png files are not available yet.
I added a rough playercolor image (a force-resize to the correct 30x30px) for the atlantean coal mine as proof of concept (you can check it ingame, best use a different player than player1 because of the blue player color of player1...)
The game can live without player color mask files, as you see (only the atlantean coal mine is there, the other files are missing). But now we need someone who is able to create the correct playercolor mask files where neccesary. Problem is that most animation frames are not of same width and height, but the menu icons are exactly 30x30, so simply force-resizing would probably lead to some undesired display effects.

[SirVer (sirver)]

I do not agree to this design. Especially since it needs new data files.

I suggest using code I added in https://code.launchpad.net/~widelands-dev/widelands/spritemaps/+merge/211225 which will be merged soonish (hopefully). I suggest using animation.representative_image together with image_transformations.h, resize to create the images shown in the menu. Maybe we can get rid of the menu.png altogether (which would be super awesome).

g_gr->animations().get_animation(world.get_bob_descr(j)->main_animation())
    .representative_image(RGBColor(0, 0, 0));

What do you think?

review: Needs Fixing

[Jens Beyer (qcumber-some)]

Thanks for the review and the suggestion.

Now that I have something working, it is easier for me to discuss it, so please be patient with me ;-)
I did not have a look into your branch yet, but from what I think I understand, you want to use a "representative image" of the animation, which is one, possibly selected, frame of the animation, and use it for the menus instead of menu.png/menu_pc.png.

I agree this would be awesome, and I think I could implement it (technologically).
And I will certainly go for this as a test (one can only learn from it).

But the animations are all at a very different size ranging from higher-than-wide to wider-than-high, while the menu items are at a fixed size. I can not really imagine having the build icons in the build menu at different sizes looking nicely, and I certainly don't want to force-resize them to equal-ratio 30x30px (as an example). What would be the correct approach to that?

[SirVer (sirver)]

I think resizing with keeping the aspect ratio is the way to go. The menu pictures we have right now have been created in the same way. Basically something like:

double ratio = 32. / std::max(image_w, image_h);
resize(image_w * ratio, image_h * ratio);

[Jens Beyer (qcumber-some)]

Setting rejected, I will push another merge request

=== modified file 'src/logic/building.cc'
--- src/logic/building.cc	2014-03-04 13:24:58 +0000
+++ src/logic/building.cc	2014-03-15 19:51:27 +0000
@@ -21,6 +21,7 @@
 
 #include <cstdio>
 #include <sstream>
+#include <fstream>
 
 #include <boost/foreach.hpp>
 
@@ -30,6 +31,7 @@
 #include "graphic/font_handler.h"
 #include "graphic/font_handler1.h"
 #include "graphic/graphic.h"
+#include "graphic/image_transformations.h"
 #include "graphic/rendertarget.h"
 #include "io/filesystem/filesystem.h"
 #include "io/filesystem/layered_filesystem.h"
@@ -62,6 +64,7 @@
 	m_tribe         (_descr),
 	m_buildable     (true),
 	m_buildicon     (nullptr),
+	m_buildiconmask     (nullptr),
 	m_size          (BaseImmovable::SMALL),
 	m_mine          (false),
 	m_port          (false),
@@ -135,7 +138,9 @@
 	if (m_buildable || m_enhanced_building) {
 		//  get build icon
 		m_buildicon_fname  = directory;
+		m_buildiconmask_fname  = directory;
 		m_buildicon_fname += "/menu.png";
+		m_buildiconmask_fname += "/menu_pc.png";
 
 		//  build animation
 		if (Section * const build_s = prof.get_section("build")) {
@@ -162,7 +167,9 @@
 	} else if (m_global) {
 		//  get build icon for global buildings (for statistics window)
 		m_buildicon_fname  = directory;
+		m_buildiconmask_fname  = directory;
 		m_buildicon_fname += "/menu.png";
+		m_buildiconmask_fname += "/menu_pc.png";
 	}
 
 	{ //  parse basic animation data
@@ -239,6 +246,13 @@
 {
 	if (m_buildicon_fname.size())
 		m_buildicon = g_gr->images().get(m_buildicon_fname);
+	if (m_buildiconmask_fname.size())
+	{
+		//TODO remove this when all menu_pc.png files are available
+		std::ifstream i(m_buildiconmask_fname);
+		if (i.good())
+			m_buildiconmask = g_gr->images().get(m_buildiconmask_fname);
+	}
 }
 
 /*
 
=== modified file 'src/logic/building.h'
--- src/logic/building.h	2014-02-22 18:04:02 +0000
+++ src/logic/building.h	2014-03-15 19:51:27 +0000
@@ -92,6 +92,7 @@
 	 */
 	const Buildcost & returned_wares_enhanced() const {return m_return_enhanced;}
 	const Image* get_buildicon() const {return m_buildicon;}
+	const Image* get_buildiconmask() const {return m_buildiconmask;}
 	int32_t get_size() const {return m_size;}
 	bool get_ismine() const {return m_mine;}
 	bool get_isport() const {return m_port;}
@@ -144,7 +145,9 @@
 	Buildcost     m_enhance_cost;     // cost for enhancing
 	Buildcost     m_return_enhanced;   // Returned ware for dismantling an enhanced building
 	const Image*     m_buildicon;       // if buildable: picture in the build dialog
+	const Image*     m_buildiconmask;       // if buildable: picture in the build dialog
 	std::string   m_buildicon_fname; // filename for this icon
+	std::string   m_buildiconmask_fname; // filename for this icon
 	int32_t       m_size;            // size of the building
 	bool          m_mine;
 	bool          m_port;
 
=== modified file 'src/wui/fieldaction.cc'
--- src/wui/fieldaction.cc	2014-03-07 19:12:24 +0000
+++ src/wui/fieldaction.cc	2014-03-15 19:51:27 +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"
@@ -69,7 +70,7 @@
 	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);
+	void add(Widelands::Building_Index::value_t, Widelands::Player *);
 
 private:
 	void clickslot(int32_t idx);
@@ -102,12 +103,24 @@
 Add a new building to the list of buildable buildings
 ===============
 */
-void BuildGrid::add(Widelands::Building_Index::value_t id)
+void BuildGrid::add(Widelands::Building_Index::value_t id, Widelands::Player * player)
 {
 	const Widelands::Building_Descr & descr =
 		*m_tribe.get_building_descr(Widelands::Building_Index(id));
+	const Image * icon = descr.get_buildicon();
+	const Image * mask = descr.get_buildiconmask();
+	const Image * pcicon;
+	if (mask == nullptr)
+	{
+		pcicon = icon;
+
+	}
+	else
+	{
+		pcicon = ImageTransformations::player_colored(player->get_playercolor(), icon, mask);
+	}
 	UI::Icon_Grid::add
-		(descr.name(), descr.get_buildicon(),
+		(descr.name(), pcicon,
 		 reinterpret_cast<void *>(id),
 		 descr.descname() + "<br><font size=11>" + _("Construction costs:") + "</font><br>" +
 			waremap_to_richtext(m_tribe, descr.buildcost()));
@@ -568,7 +581,7 @@
 		}
 
 		// Add it to the grid
-		(*ppgrid)->add(id.value());
+		(*ppgrid)->add(id.value(), m_plr);
 	}
 
 	// Add all necessary tabs
 
=== added file 'tribes/atlanteans/coalmine/menu_pc.png'
 Binary files tribes/atlanteans/coalmine/menu_pc.png	1970-01-01 00:00:00 +0000 and tribes/atlanteans/coalmine/menu_pc.png	2014-03-15 19:51:27 +0000 differ