Refactor of ColorPicker #62075

VitikaSoni · 2022-06-15T16:54:18Z

This PR covers the refactoring part of Refactor and UX Updates of ColorPicker GSoC'22 project with @akien-mga and @KoBeWi as mentors. The original proposal for refactoring can be found here and the wireframes for UX updates can be found here.

scene/gui/color_picker.cpp

AaronRecord

Left some suggestions 😃

(the comment above was meant to be part of these but I accidentally hit add single comment :/ )

scene/gui/color_picker.cpp

AaronRecord · 2022-06-15T18:00:40Z

scene/gui/color_picker.cpp

+}
+
+HSlider **ColorPicker::get_sliders() {
+	HSlider **hs = new HSlider *[5];


I wonder if there's a way to avoid allocating memory on the heap, theoretically if it's just returning 5 pointers, then it shouldn't need to. Probably doesn't matter that much though.

AaronRecord · 2022-06-15T18:14:36Z

Docs are failing because you need to update the class reference: https://docs.godotengine.org/en/stable/community/contributing/updating_the_class_reference.html#updating-the-documentation-template

VitikaSoni · 2022-06-16T11:49:19Z

Left some suggestions 😃

Thanks, will apply them :)

Rindbee · 2022-06-16T14:13:31Z

For slider_draw, the various modes are largely the same, and I think it's better to use a callback method to handle the differences. Easy to maintain.

scene/gui/color_picker.cpp

scene/gui/color_picker.h

scene/gui/color_picker.cpp

Rindbee · 2022-06-16T16:01:05Z

scene/gui/color_picker.cpp

+
+			Ref<StyleBoxEmpty> style_box_empty(memnew(StyleBoxEmpty));
+
+			for (int i = 0; i < 4; i++) {


Hard coded, suggest using current_sliders_count instead of 4

This needs to apply to all sliders. It could use a constant though.

In this case, it can be 4, and there is no need to write the following alpha_scroll separately.

alpha_scroll is actually separate from the 4 sliders.
The 4th slider is unused for now. It's potentially useful for CMYK or other color mode with more sliders (if there is any...). Although maybe it could be removed for now 🤔 This would be trivial if 4 was a constant not hard-coded.

So it's better to use current_sliders_count, which is reset according to the mode in ColorPicker::_update_controls.

Should I define a constant with 3 as its value? we will change it to 4 in future if needed.

alpha_scroll is actually separate from the 4 sliders. The 4th slider is unused for now. It's potentially useful for CMYK or other color mode with more sliders (if there is any...). Although maybe it could be removed for now 🤔 This would be trivial if 4 was a constant not hard-coded.

What if element 0 was always used for the alpha slider, and then all the code duplication with the other sliders and the alpha slider wouldn't be necessary? Would that work? 🤔

KoBeWi · 2022-06-16T22:20:56Z

scene/gui/color_picker.h

+	const int HSV_MODE_INDEX = 1;
+	const int OKHSL_MODE_INDEX = 3;


This shouldn't exist. Any code related to specific color modes should be handled by these modes.

There are checks for color mode being HSV or OKHSL in _w_input.

Yeah but this is more related to shapes than color modes. Maybe it's possible to handle it more nicely.

KoBeWi · 2022-06-16T22:22:35Z

scene/gui/color_picker.cpp


+	if (mode_option_button->get_selected() == HSV_MODE_INDEX || mode_option_button->get_selected() == OKHSL_MODE_INDEX) {


Is this condition necessary?
You can set h/s/v even for modes that don't need it.

Did this as it was checking for HSV mode previously but the call to _set_pick_color will do this anyway. Will remove this condition.

Nope, removing the condition is causing this bug in RGB mode with OKHSL picker

KoBeWi · 2022-06-16T22:24:11Z

Ok I looked through the code. Not all of it, but:

there should be enum with default color modes. This is relevant for the color_mode property
remove_theme method is not necessary. Instead I propose the following:
- apply_theme() could return bool. false by default. When it applies theme it would return true
- when apply_theme() returned true, remember it in some theme_modified variable
- add reset_theme() method to ColorPicker, which would do the same as remove_theme() now
- when changing color mode and theme_modified is true, call reset_theme()
all get() (get_name() etc) could be const
number of sliders is right now hard-coded to 4. It should be a constant
get_slider_count() could maybe return 3 by default, as it's the most common value
probably related to the original OKHSL PR, but the OKHSL mode is missing in the editor settings default color mode option
when you have shape override, you can still force-change a different shape. This is especially apparent when default color mode for the editor is OKHSL, but shape is different. It should be fixed
I see inconsistency in get_slider_label() and get_slider_max(). Some of them are using switch cases, some have array defined in ColorMode base class. Here's what I'd propose:
- get_slider_label() and get_slider_max() should not use switch case
- both should use an array
- the array shouldn't be defined in ColorMode
- instead, each ColorMode should just declare its own array. E.g. in ColorModeHSV, add Vector<float> slider_max = {359, 100, 100, 255} etc.

scene/gui/color_picker.h

scene/gui/color_picker.cpp

scene/gui/color_picker.h

scene/gui/color_picker.cpp

fire-forge · 2022-06-18T21:40:32Z

This is great! I found a few bugs:

The A slider in RAW mode doesn't have the arrow like in the other modes:
The sliders have slightly different widths because the Labels have slightly different widths. My perfectionist brain hates this lol.

Putting the Labels, sliders, and SpinBoxes in a GridContainer instead of HBoxContainers would fix this, because it would use the width of the widest Label instead of each Label individually.
When switching from one mode to OKHSV and then back to another color mode, the shape doesn't go back to what it was before:

Start at HSV	Switch to OKHSL	Back to HSV

There is something strange going on with the alpha slider in HSV and OKHSL modes. It seems to be rounding to either 0 or 1, so in the first half of the slider the color is fully transparent and in the second half it is fully opaque. It instantly switches between 0 and 1 at the center of the slider.
The top left corner of the HSV Rectangle Wheel shape is not #FFFFFF, instead it's just slightly off (#FCFCFC)

A few features that would be nice to have, but can be done separately from this PR:

A button to select the color picker shape. Currently, the only way to change the shape is to change it in editor settings. The button would be disabled in OKHSL mode because OKHSL only works with the circle shape.
Change RAW from a color mode to a toggle that works with all modes. RAW mode is just RGB but with a 100x limit on color values, but this capability would also be helpful with HSV mode:
- RGB: increase the range of the R, G, and B sliders like in the current RAW mode.
- HSV: increase the range of the V slider, but leave the H and S sliders because it doesn't make sense to allow higher values for those. This would be useful for getting e.g. an overbright white color because you could just increase the V slider instead of having to increase the R, G, and B sliders to the same value as in the current RAW mode.

KoBeWi · 2022-06-18T21:50:43Z

A button to select the color picker shape. Currently, the only way to change the shape is to change it in editor settings. The button would be disabled in OKHSL mode because OKHSL only works with the circle shape.

This will be done in a follow-up PR.

Change RAW from a color mode to a toggle that works with all modes. RAW mode is just RGB but with a 100x limit on color values, but this capability would also be helpful with HSV mode:

It was briefly discussed in #31679, but didn't gain much interest.

VitikaSoni · 2022-06-19T08:50:40Z

This is great! I found a few bugs:

Thanks for the review, will fix these bugs :)

VitikaSoni · 2022-06-19T09:18:05Z

When switching from one mode to OKHSV and then back to another color mode, the shape doesn't go back to what it was before:

Is this still needs to done even if the OptionButton to change shape will be added?

There is something strange going on with the alpha slider in HSV and OKHSL modes. It seems to be rounding to either 0 or 1, so in the first half of the slider the color is fully transparent and in the second half it is fully opaque. It instantly switches between 0 and 1 at the center of the slider.

Not able to reproduce this, am I doing something wrong?

The top left corner of the HSV Rectangle Wheel shape is not #FFFFFF, instead it's just slightly off (#FCFCFC)

This was happening before too, but I will see if I can fix this.

fire-forge · 2022-06-19T14:13:27Z

Is this still needs to done even if the OptionButton to change shape will be added?

Yes I think so, because OKHSV only works with the circle shape. I'd expect that the shape would be temporarily overridden while in OKHSV mode and then go back whenever switching to another mode. If an OptionButton for changing shape is added, then the button should probably be disabled in OKHSV mode as well.

Not able to reproduce this, am I doing something wrong?

Ok, I'll try to figure out how to reproduce it. Maybe it only happens under certain conditions?

scene/gui/color_picker.cpp

KoBeWi · 2022-06-25T17:41:30Z

I noticed that when you set picker shape to OKHSL circle and color mode to OKHSL in a ColorPicker node and then start the project, the picker will change to square when you change color mode at runtime.

IIRC this is the problem that you were trying to fix by tinkering with editor settings, but it can happen outside editor too, so a better fix is needed.

The problem is property order. Color mode comes before picker shape, so when ColorPicker is created, it assumes the square mode to be "last_shape". So setting picker shape has no effect.
Or worse, it has, because it can override the forced shape.

I see two solutions to this:

easier - change set_picker_shape to modify last_shape instead if it the shape is overridden. This might cause problems with property saving though.
more involved - add a method get_current_shape() that returns either picker_type or the forced type if color mode forces it. Then you wouldn't need last_shape, but it requires more changes.

scene/gui/color_picker.cpp

KoBeWi · 2022-06-25T20:31:23Z

I looked through the whole code and aside from some comments I left (especially the one about get_sliders()) the implementation and functionality looks good enough. I don't like how some color modes are hard-coded in ColorPicker (i.e. ifs checking for a specific mode), but the refactor already improves things a lot, so this can be changed later I guess.

VitikaSoni · 2022-06-27T09:48:46Z

I noticed that when you set picker shape to OKHSL circle and color mode to OKHSL in a ColorPicker node and then start the project, the picker will change to square when you change color mode at runtime.

In constructor of ColorPicker we were doing picker_type = SHAPE_HSV_RECTANGLE. We set last_shape to be equal to picker_type whenever there is a shape override. So in constructor, I changed picker_type = SHAPE_HSV_RECTANGLE to picker_type = SHAPE_MAX and the problem is gone. But let me know if you don't agree with this and I will change the code as you suggested.

KoBeWi · 2022-06-27T11:09:32Z

Unfortunately using SHAPE_MAX as default shape won't work as it breaks in the editor. The picker is glitched when added to scene and the shape can't be reset to default after changing (because MAX is not available in the list).

scene/gui/color_picker.h

scene/gui/color_picker.cpp

KoBeWi

Looks like there are no major problems left. Aside from the comments I just left, some details can still be improved later.

AaronRecord · 2022-07-05T15:39:17Z

Last thing you'll probably want to do is squash the commits :)

akien-mga

Looks great!

akien-mga · 2022-07-08T07:06:53Z

Thanks!

VitikaSoni requested review from a team as code owners June 15, 2022 16:54

VitikaSoni changed the title ~~Refactor and UX Updates of ColorPicker~~ [WIP]Refactor and UX Updates of ColorPicker Jun 15, 2022

KoBeWi added enhancement topic:gui labels Jun 15, 2022

KoBeWi added this to the 4.0 milestone Jun 15, 2022

VitikaSoni force-pushed the gsoc-colorpicker branch 2 times, most recently from ba983e2 to a84ac3b Compare June 15, 2022 17:22

AaronRecord reviewed Jun 15, 2022

View reviewed changes

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved

AaronRecord reviewed Jun 15, 2022

View reviewed changes

VitikaSoni force-pushed the gsoc-colorpicker branch from a84ac3b to 47a4c0c Compare June 16, 2022 12:50

VitikaSoni requested a review from a team as a code owner June 16, 2022 12:50

Rindbee reviewed Jun 16, 2022

View reviewed changes

KoBeWi reviewed Jun 16, 2022

View reviewed changes

scene/gui/color_picker.h Outdated Show resolved Hide resolved

KoBeWi reviewed Jun 16, 2022

View reviewed changes

scene/gui/color_picker.cpp Show resolved Hide resolved

KoBeWi reviewed Jun 16, 2022

View reviewed changes

scene/gui/color_picker.h Outdated Show resolved Hide resolved

Rindbee reviewed Jun 16, 2022

View reviewed changes

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved

VitikaSoni force-pushed the gsoc-colorpicker branch 2 times, most recently from eeba57f to 67fbd7c Compare June 19, 2022 13:47

KoBeWi reviewed Jun 25, 2022

View reviewed changes

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Jun 25, 2022

View reviewed changes

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Jun 25, 2022

View reviewed changes

scene/gui/color_picker.cpp Show resolved Hide resolved

KoBeWi reviewed Jun 25, 2022

View reviewed changes

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Jun 25, 2022

View reviewed changes

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved

VitikaSoni force-pushed the gsoc-colorpicker branch 2 times, most recently from ec38f9d to bc372f9 Compare June 27, 2022 11:49

KoBeWi reviewed Jul 4, 2022

View reviewed changes

scene/gui/color_picker.h Outdated Show resolved Hide resolved

KoBeWi reviewed Jul 4, 2022

View reviewed changes

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved

KoBeWi approved these changes Jul 4, 2022

View reviewed changes

VitikaSoni changed the title ~~[WIP]Refactor and UX Updates of ColorPicker~~ Refactor of ColorPicker Jul 5, 2022

VitikaSoni force-pushed the gsoc-colorpicker branch from 38cb759 to bf1faf3 Compare July 5, 2022 15:26

ColorPicker Refactor

0011d93

VitikaSoni force-pushed the gsoc-colorpicker branch from bf1faf3 to 0011d93 Compare July 6, 2022 16:45

Calinou added the usability label Jul 6, 2022

akien-mga approved these changes Jul 8, 2022

View reviewed changes

akien-mga merged commit 7d3ff92 into godotengine:master Jul 8, 2022

akien-mga mentioned this pull request Jul 8, 2022

Color Pickers Respect Default Editor Settings #62581

Merged

VitikaSoni deleted the gsoc-colorpicker branch July 8, 2022 14:26

VitikaSoni restored the gsoc-colorpicker branch July 8, 2022 14:26

VitikaSoni mentioned this pull request Jul 11, 2022

ColorPicker UX #62910

Merged

KoBeWi mentioned this pull request Dec 8, 2022

Refactor ColorPicker godotengine/godot-proposals#4353

Open

KoBeWi mentioned this pull request Nov 21, 2024

Refactor ColorPicker shapes #99515

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of ColorPicker #62075

Refactor of ColorPicker #62075

VitikaSoni commented Jun 15, 2022 •

edited

Loading

AaronRecord left a comment •

edited

Loading

AaronRecord Jun 15, 2022

AaronRecord commented Jun 15, 2022

VitikaSoni commented Jun 16, 2022 •

edited

Loading

Rindbee commented Jun 16, 2022

Rindbee Jun 16, 2022

KoBeWi Jun 16, 2022

Rindbee Jun 16, 2022

KoBeWi Jun 16, 2022

Rindbee Jun 16, 2022

VitikaSoni Jun 17, 2022

AaronRecord Jun 17, 2022 •

edited

Loading

KoBeWi Jun 16, 2022 •

edited

Loading

VitikaSoni Jun 17, 2022

KoBeWi Jun 17, 2022

KoBeWi Jun 16, 2022

VitikaSoni Jun 17, 2022

VitikaSoni Jun 18, 2022

KoBeWi commented Jun 16, 2022 •

edited

Loading

fire-forge commented Jun 18, 2022

KoBeWi commented Jun 18, 2022

VitikaSoni commented Jun 19, 2022

VitikaSoni commented Jun 19, 2022 •

edited

Loading

fire-forge commented Jun 19, 2022

KoBeWi commented Jun 25, 2022

KoBeWi commented Jun 25, 2022

VitikaSoni commented Jun 27, 2022

KoBeWi commented Jun 27, 2022

KoBeWi left a comment

AaronRecord commented Jul 5, 2022 •

edited

Loading

akien-mga left a comment

akien-mga commented Jul 8, 2022


		Ref<StyleBoxEmpty> style_box_empty(memnew(StyleBoxEmpty));

		for (int i = 0; i < 4; i++) {

		const int HSV_MODE_INDEX = 1;
		const int OKHSL_MODE_INDEX = 3;


		if (mode_option_button->get_selected() == HSV_MODE_INDEX \|\| mode_option_button->get_selected() == OKHSL_MODE_INDEX) {

Refactor of ColorPicker #62075

Refactor of ColorPicker #62075

Conversation

VitikaSoni commented Jun 15, 2022 • edited Loading

AaronRecord left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronRecord commented Jun 15, 2022

VitikaSoni commented Jun 16, 2022 • edited Loading

Rindbee commented Jun 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronRecord Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

KoBeWi Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoBeWi commented Jun 16, 2022 • edited Loading

fire-forge commented Jun 18, 2022

KoBeWi commented Jun 18, 2022

VitikaSoni commented Jun 19, 2022

VitikaSoni commented Jun 19, 2022 • edited Loading

fire-forge commented Jun 19, 2022

KoBeWi commented Jun 25, 2022

KoBeWi commented Jun 25, 2022

VitikaSoni commented Jun 27, 2022

KoBeWi commented Jun 27, 2022

KoBeWi left a comment

Choose a reason for hiding this comment

AaronRecord commented Jul 5, 2022 • edited Loading

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga commented Jul 8, 2022

VitikaSoni commented Jun 15, 2022 •

edited

Loading

AaronRecord left a comment •

edited

Loading

VitikaSoni commented Jun 16, 2022 •

edited

Loading

AaronRecord Jun 17, 2022 •

edited

Loading

KoBeWi Jun 16, 2022 •

edited

Loading

KoBeWi commented Jun 16, 2022 •

edited

Loading

VitikaSoni commented Jun 19, 2022 •

edited

Loading

AaronRecord commented Jul 5, 2022 •

edited

Loading