Incorrect RGB to HSB color implementation

[another_commander]

I believe our current OOColor class methods for providing HSB values from RGB input are problematic. This became evident during the testing of the latest atmosphere color changes, where certain input colors are for various reasons converted to HSB and back to RGB inside our planet setup methods. The OOColor class methods affected are -(void) getHue:(float *)hue saturation:(float *)saturation brightness:(float *)brightness alpha:(float *)alpha, -(float) brightnessComponent, -(float) saturationComponent and -(float) hueComponent.

To see the problem, just launch from Lave using the latest trunk and with the console invoke the planet's airColor with system.mainPlanet.airColor. Although the planetinfo key sets it to blue ("0 0 1 1"), the response you will get is [0, 0, 0.5, 1] - a darker blue. Other colors will yield even weirder responses.

After looking at this a bit, I think the issue is with the conversion from rgb to hsb itself. The b (brightness) component calculation seems to be particularly incorrect. After some research, I would propose the following changes to fix this, but would appreciate some more testing before committing it.

Here is the patch:

Code: Select all

diff --git a/src/Core/OOColor.m b/src/Core/OOColor.m
index 346f36b4..27e40309 100644
--- a/src/Core/OOColor.m
+++ b/src/Core/OOColor.m
@@ -400,26 +400,25 @@ MA 02110-1301, USA.
 {
 	float maxrgb = (rgba[0] > rgba[1])? ((rgba[0] > rgba[2])? rgba[0]:rgba[2]):((rgba[1] > rgba[2])? rgba[1]:rgba[2]);
 	float minrgb = (rgba[0] < rgba[1])? ((rgba[0] < rgba[2])? rgba[0]:rgba[2]):((rgba[1] < rgba[2])? rgba[1]:rgba[2]);
-	if (maxrgb == minrgb)
-	{
-		return 0.0f;
-	}
 	float delta = maxrgb - minrgb;
+	float fRed = rgba[0], fGreen = rgba[1], fBlue = rgba[2];
 	float hue = 0.0f;
-	if (rgba[0] == maxrgb)
+	if (maxrgb == fRed && fGreen >= fBlue)
+	{
+		hue = 60.0f * (fGreen - fBlue) / delta;
+	}
+	else if (maxrgb == fRed && fGreen < fBlue)
 	{
-		hue = (rgba[1] - rgba[2]) / delta;
+		hue = 60.0f * (fGreen - fBlue) / delta + 360.0f;
 	}
-	else if (rgba[1] == maxrgb)
+	else if (maxrgb == fGreen)
 	{
-		hue = 2.0f + (rgba[2] - rgba[0]) / delta;
+		hue = 60.0f * (fBlue - fRed) / delta + 120.0f;
 	}
-	else if (rgba[2] == maxrgb)
+	else if (maxrgb == fBlue)
 	{
-		hue = 4.0f + (rgba[0] - rgba[1]) / delta;
+		hue = 60.0f * (fRed - fGreen) / delta + 240.0f;
 	}
-	hue *= 60.0f;
-	while (hue < 0.0f) hue += 360.0f;
 	return hue;
 }
 
@@ -427,17 +426,13 @@ MA 02110-1301, USA.
 {
 	float maxrgb = (rgba[0] > rgba[1])? ((rgba[0] > rgba[2])? rgba[0]:rgba[2]):((rgba[1] > rgba[2])? rgba[1]:rgba[2]);
 	float minrgb = (rgba[0] < rgba[1])? ((rgba[0] < rgba[2])? rgba[0]:rgba[2]):((rgba[1] < rgba[2])? rgba[1]:rgba[2]);
-	float brightness = 0.5f * (maxrgb + minrgb);
-	if (maxrgb == minrgb)  return 0.0f;
-	float delta = maxrgb - minrgb;
-	return (brightness <= 0.5f) ? (delta / (maxrgb + minrgb)) : (delta / (2.0f - (maxrgb + minrgb)));
+	return maxrgb == 0.0f ? 0.0f : (1.0f - (minrgb / maxrgb));
 }
 
 - (float) brightnessComponent
 {
 	float maxrgb = (rgba[0] > rgba[1])? ((rgba[0] > rgba[2])? rgba[0]:rgba[2]):((rgba[1] > rgba[2])? rgba[1]:rgba[2]);
-	float minrgb = (rgba[0] < rgba[1])? ((rgba[0] < rgba[2])? rgba[0]:rgba[2]):((rgba[1] < rgba[2])? rgba[1]:rgba[2]);
-	return 0.5f * (maxrgb + minrgb);
+	return maxrgb;
 }
 
 - (void) getHue:(float *)hue saturation:(float *)saturation brightness:(float *)brightness alpha:(float *)alpha
@@ -446,32 +441,33 @@ MA 02110-1301, USA.
 	
 	*alpha = rgba[3];
 	
-	int maxrgb = (rgba[0] > rgba[1])? ((rgba[0] > rgba[2])? 0:2):((rgba[1] > rgba[2])? 1:2);
-	int minrgb = (rgba[0] < rgba[1])? ((rgba[0] < rgba[2])? 0:2):((rgba[1] < rgba[2])? 1:2);
-	*brightness = 0.5f * (rgba[maxrgb] + rgba[minrgb]);
-	if (rgba[maxrgb] == rgba[minrgb])
+	float fRed = rgba[0], fGreen = rgba[1], fBlue = rgba[2];
+	float maxrgb = fmax(fRed, fmax(fGreen, fBlue));
+	float minrgb = fmin(fRed, fmin(fGreen, fBlue));
+	float delta = maxrgb - minrgb;
+	float h = 0.0f;
+	if (maxrgb == fRed && fGreen >= fBlue)
 	{
-		*saturation = 0.0f;
-		*hue = 0.0f;
-		return;
+		h = 60.0f * (fGreen - fBlue) / delta;
 	}
-	float delta = rgba[maxrgb] - rgba[minrgb];
-	*saturation = (*brightness <= 0.5f) ? (delta / (rgba[maxrgb] + rgba[minrgb])) : (delta / (2.0f - (rgba[maxrgb] + rgba[minrgb])));
-
-	if (maxrgb == 0)
+	else if (maxrgb == fRed && fGreen < fBlue)
 	{
-		*hue = (rgba[1] - rgba[2]) / delta;
+		h = 60.0f * (fGreen - fBlue) / delta + 360.0f;
 	}
-	else if (maxrgb == 1)
+	else if (maxrgb == fGreen)
 	{
-		*hue = 2.0f + (rgba[2] - rgba[0]) / delta;
+		h = 60.0f * (fBlue - fRed) / delta + 120.0f;
 	}
-	else if (maxrgb == 2)
+	else if (maxrgb == fBlue)
 	{
-		*hue = 4.0f + (rgba[0] - rgba[1]) / delta;
+		h = 60.0f * (fRed - fGreen) / delta + 240.0f;
 	}
-	*hue *= 60.0f;
-	while (*hue < 0.0f)  *hue += 360.0f;
+	
+	float s = (maxrgb == 0.0f) ? 0.0f : (1.0f - (minrgb / maxrgb));
+	
+	*hue = h;
+	*saturation = s;
+	*brightness = maxrgb;
 }
 

In my own tests I now get the correct colors when I run the above sequence. If there are no other issues during testing, I will send this patch over to github soon

[another_commander]

After some more digging, it looks like the RGB to HSB implementation we currently have is not to HSB, but to HSL colorspace. At least the brightness value is calculated the way the lightness value (the L in HSL) would be. Obviously, this creates havoc when trying to convert an RGB color to HSB and then back, because three different colorspaces are getting mixed up. The more I look at it, the more convinced I am becoming that the proposed patch must get checked in.

[Milo]

I am not familiar with the different color representations but I agree that the conversion methods should not change the values.

A consequence is that existing colors in the game that have been specified as RGB will visually change. This could be avoided by modifying the game files and adjusting the RGB values to what the incorrect conversion was interpreting them as.

But I am not sure we need to go that far. Why not just fix the conversion and then only adjust the actual RGB values configured in various places on a case by case basis if someone reports an issue.

I suppose OXPs will be affected by this also — in what ways can they provide RGB color specifications?

[another_commander]

Very few OXPs provide color specifications as HSB dictionaries (if any) and planet generation is the only case I know of in the core where HSB conversion is utilized. I think it will be a quite transparent change.

[Milo]

That's comforting. In any case, good job noticing this and figuring out how to fix it!

[another_commander]

Fix committed in revision 6109294.