Use cached hue for color picker when saturation is 0

[dinoplane]

Hue is cached when the saturation in hsv mode is nonzero. When the saturation is 0, the cached hue is set as the slider in _update_color. This is to prevent #76968 from happening.

Fixes #76968

[akien-mga]

It seems you're a first time contributor, so welcome!

To ensure you can pass style checks from CI, we recommend running checks locally, using the tools described in https://docs.godotengine.org/en/latest/contributing/development/code_style_guidelines.html
There are notably pre-commit hooks in misc/hooks/ which you can set up locally if you want, to run checks on each commit.

Once the PR is ready, we'd typically ask you to squash the commits into one, you can see PR workflow for details.

Contributors familiar with the ColorPicker will review this as time permits.

[KoBeWi]

Seems like this doesn't work correctly 🤔

fNb3gnZONT.mp4

The saturation bar is always red when it's 0 and changing to different color mode and back resets the hue slider. tbh I don't see any difference from the previous behavior.

Also I think the change could be done in ColorModeHSV class. We should avoid hard-coding mode-specific code in ColorPicker if possible.

[dinoplane]

Ok I shall make these changes immediately.

[dinoplane]

On second thought, I don't think its possible to completely isolate the fix in ColorModeHSV. This is because in color_picker, the slider values are updated using sliders[i]->set_value(modes[current_mode]->get_slider_value(i)); .

ColorModeHSV::get_slider_value() is a const function, which means we cannot update a cached value (that would be a member of ColorModeHSV) from there. There would need to be some intermediate function called by the color picker for HSV mode that updates the cached value of the hue.

But I'm not sure if that is a good idea if we wanted to isolate the fix in ColorModeHSV...

Update: It may be a horrible idea to add new functionality only to ColorModeHSV... so I think a hacky solution may be to store the cached hue in the ColorPicker and use that value in ColorModeHSV::get_slider_value().

[dinoplane]

I have fixed the issue with the slider color and the changing of hues when the color mode is changed. I've isolated most of the behavior in color_mode.cpp, though as I've mentioned before, it seems to be impossible to completely contain the solution there.

If someone may review my code and make sure everything is okay, that would be great ;)

[KoBeWi]

ColorModeHSV::get_slider_value() is a const function, which means we cannot update a cached value (that would be a member of ColorModeHSV) from there.

You can change a member inside const function if it has mutable modifier. We do allow that for caching something.

[KoBeWi]

Works correctly now.
The special case you added is next to another special case, so it's probably fine.

Left 2 style comments that need to be addressed.

[KoBeWi]

You need to squash commits into 1. Also accepting changes from GitHub added me as co-author, so you need to remove that.

[dinoplane]

just squashed it all into one! Thank you very much!

[akien-mga]

I pushed a small update amending the commit message to make it more explicit about the bug being fixed.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉