Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Color Widget's HSV mode doesn't update the hue properly when saturation is 0. #76968

Closed
MewPurPur opened this issue May 11, 2023 · 7 comments · Fixed by #77863
Closed

Color Widget's HSV mode doesn't update the hue properly when saturation is 0. #76968

MewPurPur opened this issue May 11, 2023 · 7 comments · Fixed by #77863
Milestone

Comments

@MewPurPur
Copy link
Contributor

MewPurPur commented May 11, 2023

Godot version

4.0.2

System information

Ubuntu 22.04.1 LTS

Issue description

image

Notice how the hue is on the blue color, but the saturation is insists on remaining red.

If I change the saturation a little bit, the correct value will apply. But if I adjust the value or the alpha, the blue hue will be punched out and become red again.

Steps to reproduce

Go to a ColorPicker in the editor (say, the modulate property of Node2D), open the color picker, and copy the above configuration in the HSV tab.

Minimal reproduction project

N/A

@KoBeWi
Copy link
Member

KoBeWi commented May 11, 2023

Slider colors are taken directly from the picker's color, not from other sliders. Hue is undefined if saturation is 0.

@dinoplane
Copy link
Contributor

dinoplane commented May 16, 2023

However, the hue seems to reset back to red when saturation is put to 0... I personally wouldn't want the hue value to change.

Edit: it seems to do this when the value slider is set to 0 is updated and the saturation is 0...

@dinoplane
Copy link
Contributor

dinoplane commented May 17, 2023

I wanna see if I could tackle this one cuz it looks kinda funky. 😎

@dinoplane
Copy link
Contributor

Upon further investigation, a way to reproduce this issue better is to:

  1. Click a random location on the slider (say saturation) that is not on the endpoints of the slider. Let go of the mouse.
  2. Click and drag the slider to 0.
  3. The bug will appear like so.

It seems there might be some weirder business going on too. When dragging from a non endpoint value to an endpoint, the _set_pick_color function is called twice.

Below: I placed print statements in _value_changed and _set_pick_color
image

image

Here is what happens when I set the saturation slider to 1:

image

Here is what happens when I set saturation to 0:

image

As one can see... _set_pick_color is called twice...

@dinoplane
Copy link
Contributor

I can also reproduce the "calling _set_pick_color twice" behavior sometimes by double clicking the slider...

@dinoplane
Copy link
Contributor

So just as @KoBeWi mentioned, hue is undefined when saturation is 0. In code, this is reflected when the colors are being converted.

When a color is converted, internally the following happens:

  1. ColorPicker::_value_changed() is called with the values in the sliders

  2. The change occurs in color = modes[current_mode]->get_color();

  3. Given the mode is ColorModeHSV and ColorModeHSV::get_color() is called, we set the color with color.set_hsv(). However, the color is stored as an rgba. So the hue value is lost if saturation or value = 0.

  4. On the next call to Color::get_h(), we convert from rgb to hsv, but because we don't have prior knowledge of the hue, we are left with hue = 0 (cuz saturation = 0 is just grays).

I'm struggling to think of an adequate solution for this... would it be best to cache the hue in color.cpp? or keep the changes in color_picker.cpp (caching hue there)?

@KoBeWi
Copy link
Member

KoBeWi commented May 31, 2023

If we are going to cache the hue, it's better to do in color picker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants