Color Coding for Terrain Defense In Unit Help & Unit Sidebar Inconsistent (GNA #24382)

[wesnoth-bugs]

Original submission by sigurdfdragon on 2016-02-03

This has been around since at least 1.10

Not 100% sure if this should be consider a bug or feature request.

The color for terrain defense on the map and the unit sidebar show a progression from red -> orange -> yellow -> green with different shades every 10%. These are different than the Unit Help Listing which uses red for 0-10%, yellow for 20-30%, white for 40-50%, and green for 60%-100%

I think it should be consistent if possible.

(Reproduced on Win 7 x64)
Release: 1.13.2+dev
Priority: 5 - Normal
Severity: 3 - Normal

[Wedge009]

I wonder if this is a deliberate design decision - the colours in Help are saturated while the ones in the side-bar appear to be more muted. If deliberate, then it's an Enhancement; if not, then it's a Bug.

[Wedge009]

@sigurdfdragon Were you doing any work related to this?

[sigurdfdragon]

@Wedge009 no

[Wedge009]

Defence in side-bar:

wesnoth/src/reports.cpp

Lines 581 to 582 in a8c96ba

	color_t color = game_config::red_to_green(def);
	str << span_color(color) << def << '%' << naps;

Defence in help:

wesnoth/src/help/help_topic_generators.cpp

Lines 749 to 761 in a8c96ba

	std::string color;
	if (m.defense <= 10) {
	color = "red";
	} else if (m.defense <= 30) {
	color = "yellow";
	} else if (m.defense <= 50) {
	color = "white";
	} else {
	color = "green";
	}
	std::stringstream str;
	str << "<format>color=" << color << " text='"<< m.defense << "%'</format>";

At first I thought I could just copy the red_to_green() reference to the help code but it looks like it doesn't accept anything but actual colour strings in the <format>color= section. Is this a gui1 vs gui2 limitation?

[jostephd]

format will accept #RRGGBB color strings, plus a few hardcoded ones. See:

wesnoth/src/help/help_text_area.cpp

Line 143 in f05f811

TRY(format);

wesnoth/src/help/help_text_area.cpp

Line 295 in f05f811

color_t color = help::string_to_color(cfg["color"]);

wesnoth/src/help/help_impl.cpp

Lines 1249 to 1271 in f05f811

	color_t string_to_color(const std::string &cmp_str)
	{
	if (cmp_str == "green") {
	return font::GOOD_COLOR;
	}
	if (cmp_str == "red") {
	return font::BAD_COLOR;
	}
	if (cmp_str == "black") {
	return font::BLACK_COLOR;
	}
	if (cmp_str == "yellow") {
	return font::YELLOW_COLOR;
	}
	if (cmp_str == "white") {
	return font::BIGMAP_COLOR;
	}
	// a #rrggbb color in pango format.
	if (*cmp_str.c_str() == '#' && cmp_str.size() == 7) {
	return color_t::from_argb_bytes(strtoul(cmp_str.c_str() + 1, nullptr, 16));
	}
	return font::NORMAL_COLOR;
	}

So, we could use red_to_green in the help; we'll just need to serialize the color_t it returns to a #RRGGBB string. A little roundabout, but it'll work.

I wonder if there are any such changes we should make. We use percentages for several different things: 1) terrain defense 2) resistance/vulnerability 3) chance to hit 4) time of day bonus 5) stats dialog. They appear in several contexts: in the help, in the sidebar, in the attack dialog / damage calculations dialog, on the hex under the mouse. Does any of these need to be changed, too?

(I know I changed one of these: I made the "damage against" sidebar tooltip use red_to_green, rather than the trichromatic behavior of master. But I didn't do a complete audit of all of them, to make sure each of them is internally consistent, and not likely to be confused with any of the others (primarily for the first three).)

[jostephd]

Sorry, I think I wasn't very clear. I'm not saying to hold forward progress or anything. I'm just trying to say that we use percentages in a lot of places and (1) maybe there are other inconsistencies similar to this one, (2) we should be conscious - in general, not specifically in this particular change - of the risk of conflating different kinds of percentages, for example, of accidentally writing a UI that makes the player compare apples to oranges, terrain defense values to chance to hit values.

Edit: For resistance/vulnerability specifically, the sidebar, help topic generator, and a few other places all use the resistance_color helper function 👍

[Wedge009]

I tried using color_t::to_hex_string() previously (and this includes the leading # symbol) before and tried it again today but I must be using it incorrectly.

Regarding the general consideration of usage of percentages that you pointed out, if we find them then it's worth resolving. But for the purposes of this particular report, I was just focusing on the Help vs side-bar defence percentage colouring.

[jostephd]

I tried using color_t::to_hex_string() previously (and this includes the leading # symbol) before and tried it again today but I must be using it incorrectly.

I assume the single quotes got you? Having to worry about serialization like that is not best practice, normally one uses a function that adds any needed quoting automatically, but this bit of code hasn't been written this way in the first place.

proof of concept

diff --git a/src/help/help_topic_generators.cpp b/src/help/help_topic_generators.cpp
index 33c0a6416bf..8176b30c115 100644
--- a/src/help/help_topic_generators.cpp
+++ b/src/help/help_topic_generators.cpp
@@ -748,19 +748,10 @@ std::string unit_topic_generator::operator()() const {
 				font::line_width(m.name, normal_font_size) + (high_res ? 32 : 16) );
 
 			//defense  -  range: +10 % .. +70 %
-			std::string color;
-			if (m.defense <= 10) {
-				color = "red";
-			} else if (m.defense <= 30) {
-				color = "yellow";
-			} else if (m.defense <= 50) {
-				color = "white";
-			} else {
-				color = "green";
-			}
+			std::string color = game_config::red_to_green(m.defense).to_hex_string();
 
 			std::stringstream str;
-			str << "<format>color=" << color << " text='"<< m.defense << "%'</format>";
+			str << "<format>color='" << color << "' text='"<< m.defense << "%'</format>";
 			std::string markup = str.str();
 			str.str(clear_stringstream);
 			str << m.defense << "%";

screenshot

[Wedge009]

...you've basically done what I tried to do. Yes, I didn't realise the quotes were necessary to enable the hex string format.

Do you object to this change? Do you want to commit this change or should I do it?

[jostephd]

The quotes were necessary to let the # be parsed into the value. It's not something special about <format> and hex strings; it's probably something in the code that parses help tags into config objects. I expect same will be true with <italic>text='#foo'</italic>, it won't work without the quotes.

As to the patch, before you commit it, I suggest to also pass false to red_to_green's second parameter. That changes the result to:

screenshot

Which, personally, I find easier to read. It's already used by the code that draws terrain defense in the hex under the mouse when ordering a unit to move.

[Wedge009]

I see only a very subtle difference between the two. This is text... and yet you want to specify the for_text parameter to be false? Whoever designed the red_to_green function must have intended the default true parameter be be left as true for text colouring. What comment should I put if you want it switched to false?

[jostephd]

// Passing false to select the more saturated red-to-green scale. As I said, other places use it for text too. And I suppose we could change the comment at the declaration of red_to_green itself to say the name of the parameter is misleading.

[jostephd]

Thanks!

[Wedge009]

Rather, thanks for your advice about the quotes.