Fix `ColorPicker`'s RAW mode colors and values #88072

WhalesState · 2024-02-07T18:37:41Z

Fixes: Switching to RAW mode and then to RGB will change slider max value until refreshed #62894

Makes RAW sliders colored same as RGB mode.
Fix RAW mode maximum values, from 100 to 1.0.
Allow RGB and RAW modes to have greater values for overbright.
Changes RAW step value to 1.0 / 255.0, to match the RGB mode values when changed.
Prevent OKHSL mode from overriding the picker shape.
Fix conversion between RAW and RGB modes when color is overbright.

WhalesState · 2024-02-07T19:02:26Z

Please add the bug label too, since it fixes three annoying issues.

AThousandShips · 2024-02-07T19:02:49Z

Please open a bug report first :)

Remember you need to add "fixes" before each issue number

WhalesState · 2024-02-07T19:11:04Z

Remember you need to add "fixes" before each issue number

Thanks ^^

AThousandShips · 2024-02-07T19:18:01Z

See also:

fix switching RAW mode to RGB mode issue #62913

WhalesState · 2024-02-07T19:25:53Z

See also:

fix switching RAW mode to RGB mode issue #62913

We share the same fix
from float slider_max[4] = { 100, 100, 100, 1 }; to float slider_max[4] = { 1, 1, 1, 1 };

Cammymoop · 2024-02-09T23:09:11Z

Being able to set values greater than 1.0 in raw mode is a feature, not a bug. #62894 is about not restoring the maximum value of the slider when going back to RGB mode, not changing the max value in raw mode.

WhalesState · 2024-02-10T13:12:06Z

Being able to set values greater than 1.0 in raw mode is a feature, not a bug. #62894 is about not restoring the maximum value of the slider when going back to RGB mode, not changing the max value in raw mode.

we can still allow this feature by making the SpinBox able to set values higher than 1.0 by enabling allow_greater, but I still don't understand the use of it, since they will not be visualized correctly, is it for glow/emission strength to make some colors more brighter that others? maybe because the glow the strength is a global variable for each environment.

however, allowing both RGB and RAW mode to set greater values will fix this issue and keep the feature.

Cammymoop · 2024-02-10T14:13:55Z

Yes, it is for colors that are overbright in cases such as emissive colors with glow effects, or using modulate to brighten something.

I wouldn't suggest using allow_greater on the RAW mode sliders and limiting them to 1, because afaik the main reason raw mode is used at all is for overbright colors. RGB mode uses code specifically (see get_slider_max *edit) to set different slider max values in the case where you switched from raw and still want to edit the overbright color I guess but I'm not sure that that part is usefull, and you could just as easily switch to HSV instead (which would be more helpful to edit an overbright color) now that raw is a separate tab from RGB, but HSV does clamp the overbright color so that's pointless currently.

A proper way to deal with overbright colors on all color modes (probably a separate intensity slider) is the only elegant fix I think, but I think making RGB mode use allow_greater or just making it clamp is the way to go for this bug. HSV could use allow_greater theoretically, but I don't think the internal conversion supports that properly, okhsl will definitely not work with unclamped lightness so don't bother with that one.

WhalesState · 2024-02-10T14:45:30Z

ok, after allowing both RGB and RAW modes to have greater values, we are back again to this issue #62894, for some reason the slider maximum value changes after converting from RAW to RGB.

!!!

The issue is now clear, maximum slider values are not defined for RGB, I think it do calculates it manually which causes this issue.

here it is.

MajorMcDoom · 2024-05-10T19:39:36Z

Hi there, I was going to ask, while we're at it, is it difficult to make it possible to accept negative values as well? I know the most common use case is overbright colors, but negative colors are also used in many graphics tricks, and being able to export one would be great. The thing is that each component in Color can, in code, represent any float. But when exported in the inspector, the values are artificially being constrained by the UX of the ColorPicker. I think that's quite a shame, and it would be great if we could get over that hurdle with this PR. But if it's too difficult, I think getting over the limit of 100 is already a pretty big win.

Calinou · 2024-05-12T23:15:58Z

Hi there, I was going to ask, while we're at it, is it difficult to make it possible to accept negative values as well? I know the most common use case is overbright colors, but negative colors are also used in many graphics tricks, and being able to export one would be great. The thing is that each component in Color can, in code, represent any float. But when exported in the inspector, the values are artificially being constrained by the UX of the ColorPicker. I think that's quite a shame, and it would be great if we could get over that hurdle with this PR. But if it's too difficult, I think getting over the limit of 100 is already a pretty big win.

The issue is that many properties can't handle negative colors by design (or it'll result in unintended effects). We'd need additional property hints to denote that specific properties can't use negative colors, similar to the existing PROPERTY_HINT_COLOR_NO_ALPHA. Once this hint is added, we'd then have to comb through all the existing properties of type Color and add hints as required. It's a lot of work for a feature that won't be needed often, as some nodes like Light3D already have a Negative property.

KoBeWi · 2025-01-23T14:15:04Z

scene/gui/color_picker.h

@@ -276,7 +276,6 @@ class ColorPicker : public VBoxContainer {
 	void _copy_color_to_hsv();
 	void _copy_hsv_to_color();

-	PickerShapeType _get_actual_shape() const;


Looks like unrelated change.
EDIT:
It's covered by #99662
It's better to keep PRs more focused.

This change is here for nearly a year now, and #99662 has merge conflict and was opened 9 months after this PR.

KoBeWi · 2025-01-23T14:26:01Z

Still not convinced about slider max value change. While it's possible to input bigger number in the SpinBox, you lose ability to adjust overbright via dragging, and I know some people like doing that. Not to mention that pressing Enter to accept SpinBox value will close the popup.

WhalesState · 2025-01-25T19:11:20Z

Still not convinced about slider max value change. While it's possible to input bigger number in the SpinBox, you lose ability to adjust overbright via dragging, and I know some people like doing that. Not to mention that pressing Enter to accept SpinBox value will close the popup.

For RAW and RGB modes, We can allow dragging using the SpinBox, they can click and hold the up/down button and move the mouse up or down to change the value, we just need to change the maximum value for the SpinBox only and not the sliders to not lose the colorized sliders for RAW mode, also a maximum value of 10 can be more usable than 100. We can add ColorMode::get_spinbox_max to solve this.

KoBeWi · 2025-01-25T20:37:16Z

With all these get() methods in ColorMode, I wonder if we shouldn't just introduce a single ColorModeConfig struct, which would hold all these parameters (not in this PR).
get_spinbox_max() would solve the problem. I think it should have some default return value, like -1, which would make it match the slider max.

WhalesState · 2025-01-25T20:57:10Z

Maybe i can use this to return the slider max value by default virtual float get_spinbox_max(int idx) const override { return get_slider_max(idx); }.

KoBeWi · 2025-01-25T21:00:47Z

Makes sense.

WhalesState · 2025-01-25T21:30:10Z

Failed because Spinbox and Slider are shared, will have to update them manually for this to work.

WhalesState · 2025-01-25T23:04:58Z

Added a new class OverbrightSpinbox and used it for RGB and RAW modes.

First i have tried to disconnect the value_changed and connects it again after updating the slider but I was afraid if it will be slow, so i just connected the value_changed of the slider to OverbrightSpinbox::_update which will return early to avoid calling set_value_no_signal twice.

Didn't test it yet since it's still building after the rebase, Just need to know if this approach looks fine.

Also I can reuse ColorMode::can_allow_greater instead of ColorMode::has_overbright_spinbox since both are for overbright.

WhalesState · 2025-01-26T00:41:39Z

Works as expected.

Only the alpha slider shares it's value with the alpha spinbox, I ended up using the new OverBrightSpinBox for all the other SpinBoxes to avoid deleting/recreating spinboxes.

Screencast.from.01-26-2025.02.34.15.AM.webm

WhalesState · 2025-01-26T00:52:31Z

scene/gui/color_mode.h

@@ -106,20 +112,21 @@ class ColorModeRGB : public ColorMode {
 class ColorModeRAW : public ColorMode {
 public:
 	String labels[3] = { "R", "G", "B" };
-	float slider_max[4] = { 100, 100, 100, 1 };
+	float slider_max[4] = { 1, 1, 1, 1 };
+	float spinbox_max[4] = { 10, 10, 10, 255 };


Note: The 4th value is never used because we don't apply this to the alpha SpinBox. Maybe i can reduce the array size to 3.

WhalesState requested a review from a team as a code owner February 7, 2024 18:37

AThousandShips added enhancement topic:gui labels Feb 7, 2024

AThousandShips added this to the 4.x milestone Feb 7, 2024

WhalesState force-pushed the color-picker branch 2 times, most recently from 9b7e6fe to a9acca2 Compare February 7, 2024 18:41

AThousandShips added bug usability and removed enhancement labels Feb 7, 2024

AThousandShips modified the milestones: 4.x, 4.3 Feb 7, 2024

AThousandShips mentioned this pull request Feb 7, 2024

fix switching RAW mode to RGB mode issue #62913

Open

WhalesState force-pushed the color-picker branch 2 times, most recently from dc45daf to 4a55155 Compare February 10, 2024 15:40

KoBeWi mentioned this pull request May 10, 2024

Color picker in "Raw" mode uses wrong ranges for sliders #91810

Closed

WhalesState marked this pull request as draft June 9, 2024 07:50

WhalesState force-pushed the color-picker branch from 4a55155 to d776645 Compare June 9, 2024 08:22

WhalesState marked this pull request as ready for review June 9, 2024 08:25

KoBeWi removed this from the 4.3 milestone Aug 1, 2024

WhalesState marked this pull request as ready for review October 26, 2024 22:21

KoBeWi removed the archived label Oct 26, 2024

KoBeWi added this to the 4.4 milestone Oct 26, 2024

WhalesState marked this pull request as draft January 3, 2025 22:06

WhalesState force-pushed the color-picker branch 2 times, most recently from 359646e to ce4e330 Compare January 3, 2025 22:52

WhalesState marked this pull request as ready for review January 3, 2025 22:52

WhalesState mentioned this pull request Jan 10, 2025

ColorPicker: allow other color wheels in okhsl mode #99662

Open

WhalesState force-pushed the color-picker branch from ce4e330 to 611734c Compare January 11, 2025 22:21

WhalesState mentioned this pull request Jan 13, 2025

Fix legacy color picker crash. #101513

Merged

KoBeWi reviewed Jan 23, 2025

View reviewed changes

WhalesState force-pushed the color-picker branch from 611734c to d389463 Compare January 25, 2025 21:16

WhalesState force-pushed the color-picker branch from d389463 to 26ac62e Compare January 25, 2025 22:57

WhalesState force-pushed the color-picker branch from 26ac62e to cd23f68 Compare January 26, 2025 00:26

Fix ColorPicker's RAW mode colors and values

d7af676

WhalesState force-pushed the color-picker branch from cd23f68 to d7af676 Compare January 26, 2025 00:35

WhalesState commented Jan 26, 2025

View reviewed changes

WhalesState closed this Jan 28, 2025

WhalesState deleted the color-picker branch January 28, 2025 15:15

AThousandShips added the archived label Jan 28, 2025

AThousandShips removed this from the 4.4 milestone Jan 28, 2025

KoBeWi mentioned this pull request Jan 31, 2025

Fix ColorPicker sliders in overbright RGB #102240

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `ColorPicker`'s RAW mode colors and values #88072

Fix `ColorPicker`'s RAW mode colors and values #88072

WhalesState commented Feb 7, 2024 •

edited

Loading

WhalesState commented Feb 7, 2024

AThousandShips commented Feb 7, 2024 •

edited

Loading

WhalesState commented Feb 7, 2024

AThousandShips commented Feb 7, 2024

WhalesState commented Feb 7, 2024

Cammymoop commented Feb 9, 2024

WhalesState commented Feb 10, 2024 •

edited

Loading

Cammymoop commented Feb 10, 2024 •

edited

Loading

WhalesState commented Feb 10, 2024 •

edited

Loading

MajorMcDoom commented May 10, 2024

Calinou commented May 12, 2024 •

edited

Loading

KoBeWi Jan 23, 2025 •

edited

Loading

WhalesState Jan 25, 2025

KoBeWi commented Jan 23, 2025

WhalesState commented Jan 25, 2025 •

edited

Loading

KoBeWi commented Jan 25, 2025 •

edited

Loading

WhalesState commented Jan 25, 2025 •

edited

Loading

KoBeWi commented Jan 25, 2025

WhalesState commented Jan 25, 2025

WhalesState commented Jan 25, 2025 •

edited

Loading

WhalesState commented Jan 26, 2025

WhalesState Jan 26, 2025

Fix ColorPicker's RAW mode colors and values #88072

Fix ColorPicker's RAW mode colors and values #88072

Conversation

WhalesState commented Feb 7, 2024 • edited Loading

WhalesState commented Feb 7, 2024

AThousandShips commented Feb 7, 2024 • edited Loading

WhalesState commented Feb 7, 2024

AThousandShips commented Feb 7, 2024

WhalesState commented Feb 7, 2024

Cammymoop commented Feb 9, 2024

WhalesState commented Feb 10, 2024 • edited Loading

Cammymoop commented Feb 10, 2024 • edited Loading

WhalesState commented Feb 10, 2024 • edited Loading

MajorMcDoom commented May 10, 2024

Calinou commented May 12, 2024 • edited Loading

KoBeWi Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

WhalesState Jan 25, 2025

Choose a reason for hiding this comment

KoBeWi commented Jan 23, 2025

WhalesState commented Jan 25, 2025 • edited Loading

KoBeWi commented Jan 25, 2025 • edited Loading

WhalesState commented Jan 25, 2025 • edited Loading

KoBeWi commented Jan 25, 2025

WhalesState commented Jan 25, 2025

WhalesState commented Jan 25, 2025 • edited Loading

WhalesState commented Jan 26, 2025

WhalesState Jan 26, 2025

Choose a reason for hiding this comment

Fix `ColorPicker`'s RAW mode colors and values #88072

Fix `ColorPicker`'s RAW mode colors and values #88072

WhalesState commented Feb 7, 2024 •

edited

Loading

AThousandShips commented Feb 7, 2024 •

edited

Loading

WhalesState commented Feb 10, 2024 •

edited

Loading

Cammymoop commented Feb 10, 2024 •

edited

Loading

WhalesState commented Feb 10, 2024 •

edited

Loading

Calinou commented May 12, 2024 •

edited

Loading

KoBeWi Jan 23, 2025 •

edited

Loading

WhalesState commented Jan 25, 2025 •

edited

Loading

KoBeWi commented Jan 25, 2025 •

edited

Loading

WhalesState commented Jan 25, 2025 •

edited

Loading

WhalesState commented Jan 25, 2025 •

edited

Loading