ajalt / colormath

Multiplatform Kotlin color conversion and manipulation
https://ajalt.github.io/colormath/
MIT License
308 stars 21 forks source link

Conversion from RGB to HSV result in a NaN hue #25

Closed ggrell closed 2 years ago

ggrell commented 2 years ago

I'm using your library to build an HSV style color picker which inputs and outputs RGB colors, thus the need for conversions. I'm running into an issue where converting a valid RGB color to HSV results in a NaN for the hue (RGB("#A9A9A9").toHSV() results in HSV(h=NaN, s=0.0, v=0.6627451, alpha=1.0). If I check the colormath color converter on the website, it converts it to HSV 0, 0, 0.66275.

Debugging the code, I see down in RGB.srgbHueMinMaxChroma calculations, min == max, so chroma is assigned 0 and thus h is set to NaN.

Is this a bug? or should I assume if a value is NaN to make it 0f

This is running on an Android emulator, Kotlin 1.5.31.

ggrell commented 2 years ago

I happened upon the answer in the class comments for HSV, where it says NaN hue indicates monochrome.

ajalt commented 2 years ago

That's correct, monochrome colors don't have a hue, so colormath uses NaN in that case.

Let me know if there's somewhere in the docs that you looked that didn't mention NaN hues, or if there are any extensions we could add that would make dealing with undefined hues easier.

ggrell commented 2 years ago

Thanks for the quick response! I'm curious about the decision to use NaN, as I'm finding that if the user picks a monochrome color, I have to add a bunch of special-case code checking if hue.isNan() then pretend it's 0f. I can add something like fun HSV.safeH get() = if (h.isNaN()) 0f else h to compensate. Even RGB conversion runs into issues (which I need for creating the color picker gradients) when h is NaN.

EDIT: For example, user enters hex ffffff, and internally the color picker stores everything as HSV, so it's HSV(h=NaN, s=1.0, v=1.0, alpha=1.0), but then I need to convert to RGB to render the swatch, so .toSRGB() returns RGB(r=NaN, g=NaN, b=NaN, alpha=1.0, space=sRGB). So even if NaN might be logically correct, it tends to break the developer usability and requires adding special case handling for the monochrome situation.

EDIT: This is what I ended up with: fun HSV.toSafeHSV(): HSV = if (h.isNaN()) HSV(0f, s, v, alpha) else this and call it anywhere it's used for UI rendering or conversion to RGB

ajalt commented 2 years ago

Monochromatic colors don't have a defined hue. Assigning one arbitrarily may be desirable for some applications, like your color picker, but causes problems in others.

For example: 0° hue in HSV is red. So if try to draw a gradient in HSV from white to blue, if use 0° hue for white, you'll see that the gradient will travel through red and magenta (or green, depending on the direction around the hue circle it travels), rather than just changing the lightness like you'd want:

HSV(0, 0, 1).interpolate(HSV(235, 1, 1), .5)
// #7d4080, dark magenta

HSV(NaN, 0, 1).interpolate(HSV(235, 1, 1), .5)
// #404580, dark blue

So we need a separate representation for undefined hues. Colormath uses NaN for this, since it makes it easier to write generic map functions on colors without performance penalty of boxing into Float?.

As for your example, HSV.toSRGB() should never result in NaN values in the RGB instance. Can you share the actual values you are using that caused that? RGB("#ffffff").toHSV().toSRGB().toHex() == "#ffffff". Certainly all hex values should round-trip.

I'll definitely add an extension like you suggest.

ggrell commented 2 years ago

Thank you, I now better appreciate the complexities of color math! And thanks again for building this great library.