Information is lost on a round trip through ColorPicker4 for applications that store colors as HSV #2383

New issue

Closed

opened 2019-02-25 23:06:21 +00:00 by haldean · 3 comments

haldean commented

2019-02-25 23:06:21 +00:00

(Migrated from github.com)

Version/Branch of Dear ImGui:

Version: 1.68
Branch: I believe all, but definitely master and docking

Back-end/Renderer/Compiler/OS

Back-ends: all
Compiler: all
Operating System: all

My Issue/Question:

Applications that store colors as HSV values have a hard time using ColorPicker and ColorEdit correctly, because they have to take the round trip of HSV -> RGB -> HSV. The map from HSV to RGB has two singularities: when S is 0 (grayscale values, where hue doesn't matter) and when V is 0 (black, hue and saturation both don't matter).

Applications that store color values in HSV would like to maintain the H and S values when V goes to zero, or the H value when S goes to zero, even though the RGB color is the same. Otherwise, you get odd behavior where fading the saturation from a positive value down to zero and then back up will change the hue of the color you were editing.

From looking at the code of ColorPicker4, it looks like internally the first thing that the picker does is convert from RGB to HSV, and then it uses those HSV values almost everywhere when drawing the UI. I believe this issue could be fixed by adding a ColorPicker flag that specifies that the input float[3] of color is already in HSV format. This is because the problem is not really with ColorPicker, but with the need for using ColorConvertHSVtoRGB and ColorConvertRGBtoHSV, which are inherently lossy. If ColorPicker took and returned HSV values, then it would have the necessary information to maintain the state of hue and saturation when saturation or value went to zero.

Unfortunately ImGuiColorEditFlags_HSV is already taken, but maybe the flag could be named ImGuiColorEditFlags_HSV_IO, to mark that input and output color data are in HSV, or something similar? I'm happy to provide a PR to add a flag like this, but I wanted to make sure this sounded like a good approach before I started coding. Either way, I'm happy to do the work to get native HSV I/O support into ColorPicker; if you're willing to accept a patch, let me know what you'd like to see!

Standalone, minimal, complete and verifiable example: (see https://github.com/ocornut/imgui/issues/2261)

A dumb example to show how a round-trip through ColorConvertHSVtoRGB and ColorConvertRGBtoHSV can muck up your colors:

    static ImVec4 hsv(0.5f, 1.0f, 1.0f, 1.0f);
    ImVec4 rgb(0, 0, 0, 1);

    ImGui::Begin("Colors");
    ImGui::DragFloat3("color-hsv", &hsv.x, 0.01f, 0.0f, 1.0f);

    ImGui::ColorConvertHSVtoRGB(hsv.x, hsv.y, hsv.z, rgb.x, rgb.y, rgb.z);
    ImGui::DragFloat3("color-rgb", &rgb.x, 0.01f, 0.0f, 1.0f);

    static bool showEdit = false;
    ImGui::Checkbox("show ColorEdit3", &showEdit);
    if (showEdit) {
        ImGui::ColorEdit3("color", &rgb.x);
        ImGui::ColorConvertRGBtoHSV(rgb.x, rgb.y, rgb.z, hsv.x, hsv.y, hsv.z);
    }
    ImGui::End();

Screenshots/Video

The hue starts at 0.5, but is reset to zero when the saturation is dropped to zero, because ColorConvertRGBtoHSV turns (1, 1, 1) into (0, 0, 1), instead of the (0.5, 0, 1) we would like to have. We wouldn't need to call the ColorConvert* functions if ColorEdit3 could take HSV values natively.

**Version/Branch of Dear ImGui:** Version: 1.68 Branch: I believe all, but definitely master and docking **Back-end/Renderer/Compiler/OS** Back-ends: all Compiler: all Operating System: all **My Issue/Question:** Applications that store colors as HSV values have a hard time using `ColorPicker` and `ColorEdit` correctly, because they have to take the round trip of HSV -> RGB -> HSV. The map from HSV to RGB has two singularities: when S is 0 (grayscale values, where hue doesn't matter) and when V is 0 (black, hue and saturation both don't matter). Applications that store color values in HSV would like to maintain the H and S values when V goes to zero, or the H value when S goes to zero, even though the RGB color is the same. Otherwise, you get odd behavior where fading the saturation from a positive value down to zero and then back up will change the hue of the color you were editing. From looking at the code of `ColorPicker4`, it looks like internally the first thing that the picker does is convert from RGB to HSV, and then it uses those HSV values almost everywhere when drawing the UI. I believe this issue could be fixed by adding a ColorPicker flag that specifies that the input `float[3]` of color is already in HSV format. This is because the problem is not really with `ColorPicker`, but with the need for using `ColorConvertHSVtoRGB` and `ColorConvertRGBtoHSV`, which are inherently lossy. If ColorPicker took and returned HSV values, then it would have the necessary information to maintain the state of hue and saturation when saturation or value went to zero. Unfortunately `ImGuiColorEditFlags_HSV` is already taken, but maybe the flag could be named `ImGuiColorEditFlags_HSV_IO`, to mark that input and output color data are in HSV, or something similar? I'm happy to provide a PR to add a flag like this, but I wanted to make sure this sounded like a good approach before I started coding. Either way, I'm happy to do the work to get native HSV I/O support into `ColorPicker`; if you're willing to accept a patch, let me know what you'd like to see! **Standalone, minimal, complete and verifiable example:** _(see https://github.com/ocornut/imgui/issues/2261)_ A dumb example to show how a round-trip through `ColorConvertHSVtoRGB` and `ColorConvertRGBtoHSV` can muck up your colors: ``` static ImVec4 hsv(0.5f, 1.0f, 1.0f, 1.0f); ImVec4 rgb(0, 0, 0, 1); ImGui::Begin("Colors"); ImGui::DragFloat3("color-hsv", &hsv.x, 0.01f, 0.0f, 1.0f); ImGui::ColorConvertHSVtoRGB(hsv.x, hsv.y, hsv.z, rgb.x, rgb.y, rgb.z); ImGui::DragFloat3("color-rgb", &rgb.x, 0.01f, 0.0f, 1.0f); static bool showEdit = false; ImGui::Checkbox("show ColorEdit3", &showEdit); if (showEdit) { ImGui::ColorEdit3("color", &rgb.x); ImGui::ColorConvertRGBtoHSV(rgb.x, rgb.y, rgb.z, hsv.x, hsv.y, hsv.z); } ImGui::End(); ``` **Screenshots/Video** The hue starts at 0.5, but is reset to zero when the saturation is dropped to zero, because `ColorConvertRGBtoHSV` turns `(1, 1, 1)` into `(0, 0, 1)`, instead of the `(0.5, 0, 1)` we would like to have. We wouldn't need to call the `ColorConvert*` functions if `ColorEdit3` could take HSV values natively. ![ezgif-1-0be233565035](https://user-images.githubusercontent.com/187501/53374690-e6bcc600-390d-11e9-9781-a38d40c979df.gif)

ocornut commented

2019-02-25 23:22:35 +00:00

(Migrated from github.com)

Hello,

Thanks for the detailed post. I agree with your suggestion and would very much appreciate a PR for it.

(
I suggest the flag could be called _InputHSV (corresponding _InputRGB may be added to make things cleared, as well as __InputDefault).

I also suggest __InputsMask (which is private) be renamed to __ShowMask. And we could later consider to rename _RGB/ _HSV/ _HEX to add the “Show” word in then (with redirecting enums for backward compatibility) but that could be done separately.
)

Thanks a lot!

Hello, Thanks for the detailed post. I agree with your suggestion and would very much appreciate a PR for it. ( I suggest the flag could be called _InputHSV (corresponding _InputRGB may be added to make things cleared, as well as __InputDefault). I also suggest __InputsMask (which is private) be renamed to __ShowMask. And we could later consider to rename _RGB/ _HSV/ _HEX to add the “Show” word in then (with redirecting enums for backward compatibility) but that could be done separately. ) Thanks a lot!

haldean commented

2019-02-25 23:25:23 +00:00

(Migrated from github.com)

Great! I'll start in on this and get you something in the next couple days. Thanks Omar!

ocornut commented

2019-03-05 10:11:36 +00:00

(Migrated from github.com)

Closing this as the PR itself is enough for tracking progress on this, Thanks again!