Fix #2722 - ColorPicker / ColorEdit Widget Wrong Conversion from HSV to RGB

[rokups]

There already was a fix for hue wrapping around. It depended on g.ActiveId != 0 && !g.ActiveIdAllowOverlap condition for detecting changes through DragInt/DragFloat and it no longer worked. Fix is simply a bool ColorFixHueWrap in ImGuiContext which is set whenever DragInt/DragFloat has modified color values, thus 100% reliable now.

[ocornut]

Linking to #2722

[ocornut]

Thanks @rokups for the PR!

This doesn't seem to work on a raw ColorEdit4 used without the picker (right-click to change an RGB one to HSV) ?
(If the code gets moved to ColorEdit4 then there's a possibility that the ColorFixHueWrap field could even become a local variable?)

[ocornut]

Also pushed a change to ColorEdit4 now where Hue edition is disabled when Sat==0, which avoid seeing the Hue value bounce around:
0537ac0

[ocornut]

I might be wrong about it, but it seems also extremely flimsy that with or without this patch, Hue is preserved AT ALL when clicking on the Hue wheel of a color picker and after a one frame round trip.

It feels like the HSV>RGB and RGB>HSV calculation accidentally make this possible due to storing precision in floating point value, and this is very accidental !

Case in point if we perform a float4>u32>float4 round-trip in the main.cpp code:

if (ImGui::ColorEdit3("clear color", (float*)&clear_color)) // Edit 3 floats representing a color
   clear_color = ImGui::ColorConvertU32ToFloat4(ImGui::ColorConvertFloat4ToU32(clear_color));

Then what happens is that even though all the 0..255 or 3 decimal floating values apppears to be the exact same in the picker, Hue is now lost when clicking on the wheel.

Which bring us back to the original intent of #2722 which is that rather than benefiting from that accidental preservation of Hue which only works with floats, we'd want to also explcitely preserve Hue for a given widget.

We also want this preservation of Hue to be reliable so user can click on Hue wheel FIRST then Sat/Val triangle. However it is not relaliable in this order but only reliable in the opposite order.

[rokups]

Thanks for comments. Ill keep looking into this. It takes some effort wrapping my head around this. I almost have no idea what im doing

[ocornut]

The issue is unexpected confusing because of how HSV>RGB>HSV calculation somehow magically/unintentionally manage preserve a decent Hue value even after a round trip as long as full float precision is kept. If you set Sat==0 and click on the Hue wheel you'll notice the Hexadecimal and RGB values never changes, but the HSV does. That's a manifestation of that magical roundtrip :)

We don't need to investigate any of the HSV<>RGB roundtrip calculation, rather assume that this accidental restoration doesn't happen. While testing, having the data go through a ImU32 will enforce that and then we can have ColorEdit/ColorPicker carry on a Hue value associated to a given Float4.

[rokups]

So... Should i still go after approach you suggested on skype (saving hue/color/id in ImGuiContext and restoring it where appropriate)? I tested this approach and it "works" except for one glitch, but before i push this further there is something to consider. Saving hue works only for one widget. If multiple color widgets are visible then editing color in one widget would cause hue flip in others. It seems to me it is necessary to save hue for every widget individually. Sounds like a job for GetStateStorage(). Is that fine?

[ocornut]

I think it's ok to not save Hue for every widget, that would be going too far us overriding the assumption that user hold their data.

The purpose of saving this Hue is mostly to have it saved while editing a given value as we want to avoid that loss of Hue when dragging Sat.

There's still the "accidental" fallback that most people will use float4 for their underlying storage so that also mitigate the problem a little. There's really no 100% perfect solution to this problem (other than user saving HSV data instead of RGB data).

[rokups]

Another stab at this issue:

Issue is fixed by storing last active color picker color and last hue value when active color picker takes rgb as input. Then if current color picker color matches last active color - hue value will be restored. IDs are not used because ColorEdit4() and ColorWidget4() may call each other in hard-to-predict ways and they both push their own IDs on to the stack. We need hue restoration to happen in entire stack of these widgets if topmost widget used hue restoration. Since these widgets operate on exact same color value - color was chosen as a factor deciding which widgets should restore hue.

I was unable to make hue reset so far. Please let me know how to do it if you manage to. Also please let me know if lack of id checking should be changed. Aside from scanning entire id stack for last active color widget i am not sure how to handle that situation. And scanning entire id stack sounds too hacky.

[ocornut]

Merged, !