ColorPicker: unable to update uint32_t color when saturation is zero #4014

New issue

Closed

opened 2021-04-06 11:25:37 +00:00 by harry75369 · 5 comments

harry75369 commented

2021-04-06 11:25:37 +00:00

(Migrated from github.com)

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();
  }

**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** ![Peek 2021-04-06 19-04](https://user-images.githubusercontent.com/1146007/113702988-d098ab00-970c-11eb-9b92-7f320b53c8cb.gif) **Standalone, minimal, complete and verifiable example:** ```cpp 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 commented

2021-04-06 11:44:44 +00:00

(Migrated from github.com)

Further investigation shows the problem may be due to this line: ocornut/imgui@58075c4414/imgui_widgets.cpp (L5087)

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.

Further investigation shows the problem may be due to this line: https://github.com/ocornut/imgui/blob/58075c4414b985b352d10718b02a8c43f25efd7c/imgui_widgets.cpp#L5087 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. ![Peek 2021-04-06 19-04 2](https://user-images.githubusercontent.com/1146007/113705848-83b6d380-9710-11eb-84ee-944b382d23f1.gif)

harry75369 commented

2021-04-06 12:07:19 +00:00

(Migrated from github.com)

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?

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 commented

2021-04-06 14:22:12 +00:00

(Migrated from github.com)

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 commented

2021-04-08 03:23:06 +00:00

(Migrated from github.com)

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...

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 commented

2021-09-21 13:15:48 +00:00

(Migrated from github.com)

Should now be fixed by 30546bc0e7

Should now be fixed by 30546bc0e7bccbebda0e328780e48bc95af3ae2c