Fix: HSV color picker resetting hue and value
AbandonedPublic
Actions

Authored by Juanfran Matheu (jfmatheu) on Oct 28 2020, 12:28 PM.

Details

Reviewers

Summary

There is a bug - probably there for ages - that is specially notable in Square SV color picker If you want to give it a try.

If you...

Have the picker cursor over left or bottom edge... you can't change the Hue value cause it is reset to 0 no matter what.
Have the Hue in other value than 0 (default), if you move picker cursor to left (Saturation = 0) it will reset Hue to 0 and if you move it to the bottom edge (Value = 0) it will reset both Hue and Saturation to 0.

See video to see the evident issues causing an annoying UX:

Why is this caused?

I debugged a little and HSV and conversion to RGB is right, all that process looks right I guess BUT there is a design issue in the color pickers, that is, if each RGB component is equal, it completely lose which color tone/hue it was, so in color conversion from RGB to HSV, hue takes value of 0.

What this patch make?

This patch change the minimun clamp value when calculing relative position within color picker box/rect from 0 to a number close to 0, this is a quick workaround that solves the mentioned issues.
The ideal would be to solve the issue from root, but I'm not too confident in that area for required changes and is a little messy but as short term fix that can be reverted later easily if root problem is solved I believe is worth it in exhange of what it solves, improving UX a lot.

Diff Detail

Event Timeline

Juanfran Matheu (jfmatheu) requested review of this revision.Oct 28 2020, 12:28 PM

Juanfran Matheu (jfmatheu) created this revision.

This preventing completely de-saturated colors from being picked by storing some color in the RGB, which I think solves the problem at the wrong level.

The UI already has logic to keep the current hue even when de-saturated, it's not working when the color picker is embedded directly in the UI (it works in popups).

We could update the color picker from the old HSV, to prevent the hue from being lost, from a quick test it works for this case but fails when the color picker is set to HV+S, so this needs some more investigation.

diff --git a/source/blender/editors/interface/interface.c b/source/blender/editors/interface/interface.c
index 659ebfa4a39..97b564743c6 100644
--- a/source/blender/editors/interface/interface.c
+++ b/source/blender/editors/interface/interface.c
@@ -896,6 +896,14 @@ static bool ui_but_update_from_old_block(const bContext *C,
     return false;
   }
 
+  if (oldbut->type == but->type) {
+    if (ELEM(oldbut->type, UI_BTYPE_HSVCUBE)) {
+      const ColorPicker *cpicker_old = oldbut->custom_data;
+      ColorPicker *cpicker = but->custom_data;
+      copy_v3_v3(cpicker->color_data, cpicker_old->color_data);
+    }
+  }
+
   if (oldbut->active) {
     /* Move button over from oldblock to new block. */
     BLI_remlink(&oldblock->buttons, oldbut);

This revision now requires changes to proceed.Oct 29 2020, 5:54 AM

Yeah, it's a quick hack but not the ideal solution (from root problem).

Your test seems like a nice start-point to solve the issue from right level.
I hope to have more time in next days to dig into it so I can help somehow in that investigation about HV+S case..

Thanks for your time!

Juanfran Matheu (jfmatheu) abandoned this revision.Jan 7 2021, 5:29 PM

Campbell Barton (campbellbarton) mentioned this in T88261: Colorpicker: Hue is not saved if Saturation is on 0.May 17 2021, 10:42 AM