Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Color range picking #4131

Open
wants to merge 6 commits into
base: master
from

Conversation

@AI0867
Copy link
Member

commented Jun 21, 2019

Fixes a long-standing bug regarding color range picking, factors out the logic involved, adds interpolation and a red_grey_green function for when 50% is neutral, which is used in the statistics.

Possibly other color ranges should also move from picking to interpolation.

Various other places that do coloring:

  • unit::hp_color
  • unit::xp_color
  • attack_predictions::draw_hp_graph
  • probably more

Color choices could perhaps be improved, I picked them rather quickly.

AI0867 added some commits Jun 11, 2019

Fix color range bucket sizes.
There used to be N-1 buckets covering [0,99], with the last bucket containing only 100.
This has now been rescaled to N buckets covering [0,100].
Fix a floating-point edge-case
We're trying to do division + modulo.
floor + fmod can have inconsistent results around integers.
(floor rounding less than one unit up, with fmod returning slightly less than the divisor)
lround + remainder will return negative remainders when rounding up, which needs to be compensated for,
but is at least consistent.

Details at: https://stackoverflow.com/q/56580411/145413

@AI0867 AI0867 requested review from soliton- and jostephd Jun 21, 2019

@soliton-

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Code looks fine to me. I‘m not going to be able to test for another two weeks but I trust the code works as intended.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Need to add the new key (red_grey_green_scale) to the schema at data/schema/core/config.cfg.

@AI0867

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2019

I believe I did?

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

Oh huh, you're right, somehow I missed it. Sorry about that.

assert(!color_scale.empty());

value = utils::clamp(value, 0, 100);
const int idx = color_scale.size() * value / 101;

This comment has been minimized.

Copy link
@jostephd

jostephd Aug 10, 2019

Member

If color_scale.size() == 200 and value == 100, idx will work out to 198. Is that right, or should it give 199 in that case?

This only matters when color_scale.size > 101 so the color scales we actually use are unaffected.

@@ -41,6 +41,7 @@
unit_rgb=magenta
red_green_scale="e60000,ff0000,ff4000,ff8000,ffc000,ffff00,c0ff00,80ff00,40ff00,00ff00,00e600"
red_green_scale_text="c80000,dd0000,dd3700,dd6e00,dda500,dddd00,a5dd00,6edd00,37dd00,00dd00,00c800"
red_grey_green_scale="800000,d00000,d06a6a,d0d0d0,6ad06a,00d000,008000"

This comment has been minimized.

Copy link
@jostephd

jostephd Aug 10, 2019

Member

I tried to force one of my test cases to use this colorset:

2019-08-10-170143_255x526_scrot

Maybe it's just me, but the sequence looks like it's out of order: for example, -20 and -50 are very similar colors, but -40 is different than both of them.

For comparison, here's a screenshot of the standard red_green scale: https://forums.wesnoth.org/download/file.php?id=83893

This comment has been minimized.

Copy link
@AI0867

AI0867 Aug 12, 2019

Author Member

Ugh, that's exactly what a78466c was supposed to fix, and it works for all the testcases I produced locally. Is that testcase usable outside of your machine?

This comment has been minimized.

Copy link
@jostephd

jostephd Aug 12, 2019

Member

Sure, this should do it:

test case
diff --git a/data/scenario-test.cfg b/data/scenario-test.cfg
index 784f11193a6..bec9eb59fa3 100644
--- a/data/scenario-test.cfg
+++ b/data/scenario-test.cfg
@@ -4139,6 +4139,36 @@ unplagueable: $wml_unit.status.unplagueable"
             advance=false
         [/unstore_unit]
     [/event]
+
+#define JOSTEPH_TEST_DAMAGE_VERSUS
+foo#enddef
+
+#ifdef JOSTEPH_TEST_DAMAGE_VERSUS
+    [event]
+        name=prestart
+        {GENERIC_UNIT 2 "josteph custom unit id 00" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 01" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 02" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 03" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 04" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 05" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 06" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 07" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 08" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 09" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 10" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 11" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 12" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 13" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 14" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 15" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 16" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 17" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 18" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 19" 5 5}
+        {GENERIC_UNIT 2 "josteph custom unit id 20" 5 5}
+    [/event]
+#endif
 [/test]
 
 [units]
@@ -4151,6 +4181,47 @@ unplagueable: $wml_unit.status.unplagueable"
     [/unit_type]
 [/units]
 
+#ifdef JOSTEPH_TEST_DAMAGE_VERSUS
+[units]
+#define JOSTEPH_UT X
+    [unit_type]
+        id=josteph custom unit id {X}
+        name=_ "Custom " + {X}
+        [base_unit]
+            id=Orcish Grunt
+        [/base_unit]
+        hide_help=yes
+        do_not_list=yes
+        [resistance]
+            blade={X}0
+            electrical={X}0
+        [/resistance]
+    [/unit_type]
+#enddef
+    {JOSTEPH_UT 00}
+    {JOSTEPH_UT 01}
+    {JOSTEPH_UT 02}
+    {JOSTEPH_UT 03}
+    {JOSTEPH_UT 04}
+    {JOSTEPH_UT 05}
+    {JOSTEPH_UT 06}
+    {JOSTEPH_UT 07}
+    {JOSTEPH_UT 08}
+    {JOSTEPH_UT 09}
+    {JOSTEPH_UT 10}
+    {JOSTEPH_UT 11}
+    {JOSTEPH_UT 12}
+    {JOSTEPH_UT 13}
+    {JOSTEPH_UT 14}
+    {JOSTEPH_UT 15}
+    {JOSTEPH_UT 16}
+    {JOSTEPH_UT 17}
+    {JOSTEPH_UT 18}
+    {JOSTEPH_UT 19}
+    {JOSTEPH_UT 20}
+[/units]
+#endif
+
 [+language]
     range_very_long= _ "very long"
     type_electrical= _ "electrical"
diff --git a/src/game_config.cpp b/src/game_config.cpp
index 50d2d24260f..199905c77c5 100644
--- a/src/game_config.cpp
+++ b/src/game_config.cpp
@@ -553,9 +553,7 @@ const std::vector<color_t>& tc_info(const std::string& name)
 
 color_t red_to_green(int val, bool for_text)
 {
-	const std::vector<color_t>& color_scale = for_text ? red_green_scale_text : red_green_scale;
-
-	return utils::pick_color_range(color_scale, val);
+	return utils::interpolate_color_range(red_grey_green_scale, val);
 }
 
 color_t red_grey_green(int val)
diff --git a/src/reports.cpp b/src/reports.cpp
index deafbb16015..a9a4a73fb78 100644
--- a/src/reports.cpp
+++ b/src/reports.cpp
@@ -738,9 +738,8 @@ REPORT_GENERATOR(selected_unit_moves, rc)
 static inline const color_t attack_info_percent_color(int resistance)
 {
 	// Compare unit_helper::resistance_color()
-	if (resistance < 0) return font::BAD_COLOR;
-	if (resistance > 0) return font::GOOD_COLOR;
-	return font::YELLOW_COLOR;
+	const int value_in_0_to_200 = static_cast<int>((resistance+100)/2);
+	return game_config::red_to_green(value_in_0_to_200);
 }
 
 static int attack_info(reports::context & rc, const attack_type &at, config &res, const unit &u, const map_location &hex, const unit* sec_u = nullptr, const_attack_ptr sec_u_weapon = nullptr)

(By the way, I see now that I left out the + in [+units], but it seems to work anyway.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.