Incorrect rgb to hue convertion

Question

I'm trying to convert a rgb color to hsl in Java, i have searched for many codes that explain how you convert rgb to hsl, i now have saturation and lightness working, but the hue value is incorrect

I am now trying to convert rgb to hsl and then back.

the rgb values i am using are

red: 54 green: 43 blue: 21

The hsl values i get are

hue: 260 saturation: 44 lightness: 15

I tried to convert the rgb values to hsl at https://www.rapidtables.com/convert/color/rgb-to-hsl.html

The values i get there are

hue: 40 saturation: 44.0 lightness: 14.7

Does anyone know what i'm doing wrong in converting rgb to hsl? Here is my code

public static Map<String, Integer> rgbToHsl(Integer red, Integer green, Integer blue){
        Float redDouble = ((float)red) / 255.0f;
        Float greenDouble = ((float)green) / 255.0f;
        Float blueDouble = ((float)blue) / 255.0f;

        Float max = Math.max(Math.max(redDouble, greenDouble), blueDouble);
        Float min = Math.min(Math.min(redDouble, greenDouble), blueDouble);

        Float chroma = max - min;
        Float hue = chroma == 0.0f ? 0.0f : 
            (max == redDouble ? (greenDouble - blueDouble) / chroma : 
            (max == greenDouble ? 2f + (blueDouble - redDouble) / chroma : 
            4f + (redDouble - greenDouble) / chroma));

        Float lightness = (max + min) * 0.5f;
        Float saturation = chroma == 0.0f ? 0.0f : (lightness > 0.5f ? chroma / (2.0f - max - min) : chroma / (max + min));

        return Map.ofEntries(
            Map.entry("hue", (int) Math.round(hue * 60)),
            Map.entry("saturation", (int) Math.round(saturation * 100)),
            Map.entry("lightness", (int) Math.round(lightness * 100))
        );
    }

Maybe, first things first. What are all the boxed `Float`s good for? — Andrey Tyukin, Dec 12 '20 at 20:15
I remember now, i was first using doubles and then it gave an error if i did not convert them to doubles — Jaron, Dec 12 '20 at 20:22
Maybe comparing your implementation to [this implementation from Apache Commons](https://commons.apache.org/proper/commons-imaging/apidocs/src-html/org/apache/commons/imaging/color/ColorConversions.html#line.285) could be useful. Even better: just use some standard library where this is already implemented. — Andrey Tyukin, Dec 12 '20 at 20:24
Check out [HSL Color](https://tips4java.wordpress.com/2009/07/05/hsl-color/) for a reusable class. — camickr, Dec 12 '20 at 20:25
Regarding `Float`: you are not using the primitive floating point type `float`, you are using the boxed `Float` objects. Not only does it cause a performance penalty, but it also makes the behavior of `==` less obvious. For example, you are comparing them with `==`, which can results in all kind of "fun" results, like `Float a = new Float(0.0); Float b = new Float(0.0); System.out.println(a == b);` giving `false`. — Andrey Tyukin, Dec 12 '20 at 20:31
Either that, or using `.equals(...)` in positions where the behavior of `==` is not obvious. For example, I could imagine that `max == redDouble` and `max == greenDouble` are always giving `false`. — Andrey Tyukin, Dec 12 '20 at 20:36

sunrise · Answer 1 · 2020-12-12T20:32:29.417

0

For the hue value if red is the maximum, you forgot to convert it by modulus 6.

According to what IntelliJ is showing me, it doesn't believe max == redDouble even though their printed values look identical. So, your nested logic for the hue is calculating the wrong part. I would suggest you write some logic to figure out whether you're looking for white, red, green or blue as a string, then use a switch block with the new colour string as your trigger to decide which value to return. This will be longer, but probably more readable as well. Although I am a fan of the ternary operator, nesting them can be muddling.

edited Dec 12 '20 at 20:32

answered Dec 12 '20 at 20:18

sunrise

404
2
9

Thank you for answering, this is part of the problem, but i am still getting the same results, so red was not the maximum in this case – Jaron Dec 12 '20 at 20:24
I just noticed! The logic is favouring the blue response even though red is bigger. Back in a moment. – sunrise Dec 12 '20 at 20:28
I found out by the one of the answers above that using boxed floats was the problem, thank you for answering – Jaron Dec 12 '20 at 20:37
Yeah, that's a somewhat different approach and probably a neater solution. Thank you for the brain exercise, glad you got it sorted :) – sunrise Dec 12 '20 at 20:38
No problem, i'm happy that it all works now and that red gets calculated the right way as well – Jaron Dec 12 '20 at 20:40

Andrey Tyukin · Accepted Answer · 2020-12-12T20:45:21.113

When you use boxed Floats everywhere, the Math.max(Math.max(a, b), c) will unbox the arguments a, b and c, then perform the computation, then box them back into a Float.

The result will be a new object, unequal to all three a, b and c.

Therefore, the identity comparisons max == redDouble and max == greenDouble will always be false.

Eliminate the boxed types, use floats everywhere, it's both faster and clearer.

Even better: never use either == or equals on any kind of floating-point values. See, for example, how here additional boolean flags were used. Booleans are not susceptible to tiny rounding errors.

Incorrect rgb to hue convertion

2 Answers2