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

Left some suggestions 😃

Thanks, will apply them :)

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.

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.

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.

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.

This is great! I found a few bugs:

Thanks for the review, will fix these bugs :)

• 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.

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?

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:

1. easier - change set_picker_shape to modify last_shape instead if it the shape is overridden. This might cause problems with property saving though.
2. 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.

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.

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.

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).

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

Thanks!