Refactor ColorPicker

[KoBeWi]

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

ColorPicker's source code is full of fun stuff like this:

if (hsv_mode_enabled) {
	set_raw_mode(false);
	btn_raw->set_disabled(true);
} else if (raw_mode_enabled) {
	set_hsv_mode(false);
	btn_hsv->set_disabled(true);
} else {
	btn_raw->set_disabled(false);
	btn_hsv->set_disabled(false);
}

It has RGB, HSV and Raw move, but they never were properly separated, resulting in lots of spaghetti code, with OKHSL mode going to be added soon that adds even more messy code.

What's more, we have this:

Color modes are just two exclusive CheckButtons. So if we wanted to add more... well, there's just no space. The OKHSL workarounds this by coupling the color mode with a special picker shape (lol).

Speaking of shapes, there's no easy way to change them at runtime. Picker shape is a property; in editor there is a setting for default shape, but you can change it only via the setting. And in case of OKHSL mode, you are tied to a specific shape to be able to use it.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

ColorPicker code should be refactored and cleaned up, to make multiple color modes more maintainable and easier to add.

First, get rid of the CheckButtons. It was ok when Raw was the only option, but now it needs to be changed. Since all modes are exclusive, they could just be put under an OptionButton:

This way we can add infinite color modes and don't need to worry about space.

With the space saved by removing a button, we could probably add another button, for changing shapes. This would however require icons for each shape, as their names are too long. Also switching to OKHSL color mode should automatically change shape if it's different.

Bonus things to consider:

• being able to select what color modes are supported per picker (e.g. array of string etc.)
• being able to add custom color modes via scripting (see implementation)

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The easiest way to properly decouple color modes, that I can think of, is adding a struct called ColorMode. Each color mode differs in color rules and slider setup.

For each slider we have:

• labels (RGB, HSV etc)
• gradients
• value range (0-255, 0-100 etc)

And each color mode has a set of rules how to construct color from slider values and vice versa.

So the struct ColorMode would this set of methods (non-exhaustive):

• get_slider_count() - returns number of sliders (useful if we ever add CMYK or something); default impl returns 3
• get_slider_label(idx) - returns the label for slider idx
• get_slider_min(idx) - returns min value for the slider idx; default is 0
• get_slider_max(idx)
• get_slider_gradient(idx) - returns the gradient for slider idx
• set_color(color) - used to set slider values from color
• get_color() - the opposite
• get_shape_override() - returns shape name associated with this mode (for OKHSL); returns "" by default, which doesn't change mode

Alpha would be available independently from other sliders, but you'd be able to specify its range by using SLIDER_ALPHA_IDX constant as slider idx in the relevant methods.

Color modes RGB, HSV, WHATEVER would be defined using structs inheriting ColorMode that implement these methods. ColorPicker would have a method to add a color mode struct under given name. ColorPicker would then operate on the selected mode struct instead of navigating code with milion ifs.

As for the scripting option I mentioned, ColorMode struct could possibly be exposed as abstract class which you can implement and add to ColorPicker.

If this enhancement will not be used often, can it be worked around with a few lines of script?

🤔

Is there a reason why this should be core and not an add-on in the asset library?

It's about core class code health.

[Calinou]

There's also the question of whether RAW mode should be removed in favor of an "energy" or "intensity" slider: #1031
This way, you can use both HSV and define an HDR/overbright color (or even do this with OKHSL).

If this is done, we can also add a property hint that hides the energy slider. This is already possible for hiding the alpha channel in a ColorPicker (PROPERTY_HINT_COLOR_NO_ALPHA). This can be needed to improve usability, as some properties don't support HDR colors and will clamp them. (For reference, this is an issue with RAW mode currently.)

[KoBeWi]

So the intensity would be just another slider? Given that this and alpha would be global, we could separate them from other sliders:

[Calinou]

So the intensity would be just another slider? Given that this and alpha would be global, we could separate them from other sliders:

Yes, although intensity could be merged with the "value" slider in HSV mode. I would personally keep it always separate to avoid confusion though. The mockup looks good to me.

[groud]

I definitely agree on the UI part. Those check buttons are bad and should be replaced by an OptionButton.

I am less convinced by the ColorMode idea (or its implementation I suppose). On the one hand, it looks like it is a little bit too generic for something that only has three options, and on the other hand, it looks like it is quite specific and won't be able to support modes that would need any other kind of option (like a boolean to activate or something like that).

If we had to make this generic and keep the ColorMode idea, I would most likely go the other way around and make the ColorMode class itself register the controls it needs. Like, calling something like ColorPicker.add_color_control(Control *p_control), where the added control could have a slider, its label and the SpinBox. All could likely be generated using either an helper method or by having a dedicated class for that.

[KoBeWi]

If we had to make this generic and keep the ColorMode idea, I would most likely go the other way around and make the ColorMode class itself register the controls it needs. Like, calling something like ColorPicker.add_color_control(Control *p_control), where the added control could have a slider, its label and the SpinBox. All could likely be generated using either an helper method or by having a dedicated class for that.

The problem with this is that every color mode would have to add their own controls, which means that e.g. for 3 color modes, the picker would need to hold 9 slider/label/spinbox nodes instead of 3. With my approach we could have e.g. SLIDER_MAX that would define max value to be returned by get_slider_count(), allocate only that much sliders and reuse them between modes. If there ever is a need for non-slider color mode, adding it would be easy, because the class will already be abstracted out.

[groud]

The problem with this is that every color mode would have to add their own controls, which means that e.g. for 3 color modes, the picker would need to hold 9 slider/label/spinbox nodes instead of 3.

This is not a problem, there's not really anything to optimize here as hidden controls don't have a significant cost anyway. So having a few more hidden sliders is alright. Those consideration would be premature optimization.

I mean, in any case I don't mind that much anyway. If you feel that is the simplest way to solve the issue, I trust you on that, as it won't be a lot of work by the day we need to add a new mode anyway. (If all of this stays internal btw, otherwise the API will require more discussions)

[KoBeWi]

Most of the methods I listed can exist only when we assume sliders. There can also be helper methods that set slider values. With any controls, you have to make a variable for each control if you want to access it later and the overall complexity of the code would increase much. I'd go with what I suggested for now and only modify it when there is need.

Also everything would be internal. Exposing this functionality to allow custom color modes sounds cool, but it wasn't requested so far, so there is no point for now.

[gongpha]

I have some suggestions for these. The below LineEdit is too narrow for code value mode and each RGB value has double values that are too precise. (While in the float version in 3.X it shows 6 places, I still think it's too much.) I think there should be at most 2-4 places so it's easily readable in the code.

Color(0.58999997377396, 0.60000002384186, 0)

Also, when I select an overbright color, the HEX mode LineEdit disappears, including the code value mode. which I think should show the code values ​​when it's overbright, but not for HEX mode.

[RoniPerson]

Since it was mentioned above I would like to suggest a change to the edit_alpha behaviour. Currently it hides the alpha slider but it is still possible for the user to enter a transparent color via the presets.
When assigning a transparent preset the alpha should be cut of in compliance with the settings of the color picker.