ColorPicker: unable to update uint32_t color when saturation is zero

[harry75369]

Version/Branch of Dear ImGui:

Dear ImGui 1.80 (18000)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=201703
define: __linux__
define: __GNUC__=10
--------------------------------
io.BackendPlatformName: imgui_impl_sdl
io.BackendRendererName: imgui_impl_opengl3
io.ConfigFlags: 0x00000020
 NoMouseCursorChange
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x0000000E
 HasMouseCursors
 HasSetMousePos
 RendererHasVtxOffset
--------------------------------
io.Fonts: 1 fonts, Flags: 0x00000000, TexSize: 512,1024
io.DisplaySize: 2000.00,1600.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 16.00,16.00
style.WindowBorderSize: 1.00
style.FramePadding: 8.00,6.00
style.FrameRounding: 0.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 16.00,8.00
style.ItemInnerSpacing: 8.00,8.00

My Issue/Question:

When change a color with saturation==0 (which is converted from uint32_t), in color picker in RGB mode, clicking on the hue bar will temporarily change the color but the color is immediately reverted to the original.

The return value of ImGui::ColorEdit4 is true, however, the color value is almost identical to the original. See code below.

If the saturation is non-zero, the color can be changed successfully.

But the expected behaviour should be correctly changing the color no matter what the original color value is.

Screenshots/Video

Standalone, minimal, complete and verifiable example:

  void showColorTest()
  {
    ImGui::Begin("Color Test", nullptr, ImGuiWindowFlags_AlwaysAutoResize);
  
    static uint32_t ccc = 0xFFD8D8D8;
    ImVec4 color = ImGui::ColorConvertU32ToFloat4(ccc);
    auto flags = ImGuiColorEditFlags_AlphaPreview | ImGuiColorEditFlags_AlphaBar | ImGuiColorEditFlags_NoLabel;
    printf("original color: (%.6f, %.6f, %.6f, %.6f)\n", color.w, color.x, color.y, color.z);
  
    if (ImGui::ColorEdit4("ColorTest", (float*)&color, flags))
    {
      printf("changed color: (%.6f, %.6f, %.6f, %.6f)\n", color.w, color.x, color.y, color.z);
      ccc = ImGui::ColorConvertFloat4ToU32(color);
    }
    
    ImGui::End();
  }

[harry75369]

Further investigation shows the problem may be due to this line:

imgui/imgui_widgets.cpp

Line 5087 in 58075c4

ColorConvertHSVtoRGB(H >= 1.0f ? H - 10 * 1e-6f : H, S > 0.0f ? S : 10 * 1e-6f, V > 0.0f ? V : 1e-6f, col[0], col[1], col[2]);

Clicking the hue bar will give an H. However, no matter what value H is, this function always set RGB value (very close) to (v,v,v).

My current solution is to set fallback saturation value to 50 * 1e-4 (rather than 10 * 1e-6) when it is zero. This gives an updated color value with S=1(/255). However, the indicator in the vertical hur bar will just jump round, which is the case for any color with S=1.

[harry75369]

I think changing the underlying color storage from uint32_t to float4 is a better solution for me. This can avoid the precision lost when converting colors.

But when uint32_t color is a must, can the color picker GUI be designed to avoid various jumping back/around?

[ocornut]

We already have mitigation code in the picker to temporarily preserve varying Hue while Saturation is 0.0f, but somehow this code doesn't work on a round-trip through int32 packing. So I would say on our end it is a bug that this mitigation code doesn't work and we could try to keep it working in that case.

[harry75369]

The reason why round-trip through int32 packing is not working is because the precision of int32 is not enough for editing Hue with zero Saturation when int32 is seen as RGB and converting to/from HSV must be performed. The edition (to RGB space) in HSV space is not uniformly equal under different saturation.

Say we are editing color 0xD8D8D8, which is RGB(216, 216, 216)/HSV(0,0,216). If we change H to 100, HSV(100,0,216) stills corresponds to RGB(216,216,216) and thus color is not changed. However, if color is stored in float4, HSV(100,0,216) will change float4 value, even RGB (converted from float4) still looks the same.

So just not use color picker to edit in32 color and the trouble is gone...

[ocornut]

Should now be fixed by 30546bc