ColorPicker UX

[VitikaSoni]

This PR covers the UX part of Refactor and UX Updates of ColorPicker GSoC'22 project with @akien-mga and @KoBeWi as mentors.
Wireframes for UX updates by @redlamp: here.
Refactoring PR: #62075

Before	Mode selection	Shape selection	Preset dragging	Recent presets	Uncolorized sliders

[VitikaSoni]

Should the number of recent color presets be configurable?

[VitikaSoni]

The size of ColorPicker doesn't go back when we hide presets, how to fix this?

[Rindbee]

The size of ColorPicker doesn't go back when we hide presets, how to fix this?

How about making it inherit MarginContainer? The effect is equivalent to adding a MarginContainer externally on the original basis. Anyway, its theme_override_constants/margin is currently invalid, and there is a problem about it.

[KoBeWi]

From testing:

• Swatches dropdowns could flip the arrow when pressed (otherwise you don't know if it's folder or empty)
  • an easy way to do that is adding update_drop_down_arrows() method, called each time dropdown is toggled or the theme is updated
• Why is this vertical? EDIT: ColorPicker hue slider (HSV and OKHSL modes) is rotated 90 degrees #63775, already has a fix
  
• when you select OKHSL, the picker width increases
  • seems to be happening when the shape changes
  • related to the value bar?
• also it would be nice if the OKHSL maybe appeared as a tab after it was selected (so the 3 tabs show 3 recent color modes). But that's not important and probably difficult to do
• Uncolored sliders don't reach the edge. That doesn't look good
  
• Also colored slider arrows don't reach edge either, so it doesn't precisely show the selected color
  • although it was like that before, so probably ok
• A color shouldn't be added to recent if it's the same as the most recent one
  • I checked in GIMP and if the selected color already exists on the list, it's moved to front instead of duplicated. This could be considered, but not important (and probably not worth it)

Other than that it looks really good, well done ;)

[KoBeWi]

I just noticed that recent colors are stored per-picker. I wonder if it's difficult to make them global for the editor.

[VitikaSoni]

I just noticed that recent colors are stored per-picker. I wonder if it's difficult to make them global for the editor.

I can take reference from preset_cache, it doesn't seem difficult. Do we need to save them too?

[KoBeWi]

Yeah, they can be saved too.

[fire]

Might be a good idea to pause, commit and then open a new pr. This pr's feature backlog is growing a lot.

[KoBeWi]

Found a crash:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.0.alpha.custom_build (f6714581cc29e14d2b0799a36d5ce91e172489c5)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] List<Color,DefaultAllocator>::move_to_back (C:\godot_source\core\templates\list.h:547)
[1] ColorPicker::_recent_preset_pressed (C:\godot_source\scene\gui\color_picker.cpp:1324)
[2] call_with_variant_args_helper<ColorPicker,bool,ColorPresetButton *,0,1> (C:\godot_source\core\variant\binder_common.h:267)
[3] call_with_variant_args<ColorPicker,bool,ColorPresetButton *> (C:\godot_source\core\variant\binder_common.h:377)
[4] CallableCustomMethodPointer<ColorPicker,bool,ColorPresetButton *>::call (C:\godot_source\core\object\callable_method_pointer.h:105)
[5] Callable::callp (C:\godot_source\core\variant\callable.cpp:51)
[6] CallableCustomBind::call (C:\godot_source\core\variant\callable_bind.cpp:100)
[7] Callable::callp (C:\godot_source\core\variant\callable.cpp:51)
[8] Object::emit_signalp (C:\godot_source\core\object\object.cpp:1046)
[9] Object::emit_signal<bool> (C:\godot_source\core\object\object.h:873)
[10] BaseButton::_toggled (C:\godot_source\scene\gui\base_button.cpp:144)
[11] BaseButton::on_action_event (C:\godot_source\scene\gui\base_button.cpp:170)
[12] BaseButton::gui_input (C:\godot_source\scene\gui\base_button.cpp:69)
[13] Control::_call_gui_input (C:\godot_source\scene\gui\control.cpp:1718)
[14] Viewport::_gui_call_input (C:\godot_source\scene\main\viewport.cpp:1292)
[15] Viewport::_gui_input_event (C:\godot_source\scene\main\viewport.cpp:1600)
[16] Viewport::push_input (C:\godot_source\scene\main\viewport.cpp:2744)
[17] Window::_window_input (C:\godot_source\scene\main\window.cpp:1077)
[18] call_with_variant_args_helper<Window,Ref<InputEvent> const &,0> (C:\godot_source\core\variant\binder_common.h:262)
[19] call_with_variant_args<Window,Ref<InputEvent> const &> (C:\godot_source\core\variant\binder_common.h:377)
[20] CallableCustomMethodPointer<Window,Ref<InputEvent> const &>::call (C:\godot_source\core\object\callable_method_pointer.h:105)
[21] Callable::callp (C:\godot_source\core\variant\callable.cpp:51)
[22] DisplayServerWindows::_dispatch_input_event (C:\godot_source\platform\windows\display_server_windows.cpp:2112)
[23] DisplayServerWindows::_dispatch_input_events (C:\godot_source\platform\windows\display_server_windows.cpp:2077)
[24] Input::_parse_input_event_impl (C:\godot_source\core\input\input.cpp:663)
[25] Input::flush_buffered_events (C:\godot_source\core\input\input.cpp:886)
[26] DisplayServerWindows::process_events (C:\godot_source\platform\windows\display_server_windows.cpp:1819)
[27] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:907)
[28] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:179)
[29] _main (C:\godot_source\platform\windows\godot_windows.cpp:201)
[30] main (C:\godot_source\platform\windows\godot_windows.cpp:215)
[31] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:229)
[32] __scrt_common_main_seh (D:\a01\_work\26\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[33] BaseThreadInitThunk
-- END OF BACKTRACE --

Not sure how to reproduce, it happens when clicking recent colors, mixed with selecting colors in other ways and in different pickers (in editor).

[KoBeWi]

The crash with recent presets is fixed in the editor, but it can still happen at runtime (you can test with 2 ColorPickers next to each other). Maybe you just need to add #ifdef TOOLS_ENABLED somewhere? It's not used outside the editor anyway.

Also you could squash the commits now. Be sure to use a descriptive commit message.

[VitikaSoni]

The crash with recent presets is fixed in the editor, but it can still happen at runtime (you can test with 2 ColorPickers next to each other). Maybe you just need to add #ifdef TOOLS_ENABLED somewhere? It's not used outside the editor anyway.

Okay, will fix it. Do we need to cache recent presets at runtime or each picker should have their own recent presets? If we want to cache them, then I don't know when and how to call _update_recent_presets on other pickers (in editor, it's called when ColorPickerButton is pressed).

[KoBeWi]

Do we need to cache recent presets at runtime or each picker should have their own recent presets?

They can have their own presets for now. It can be improved later.

[KoBeWi]

Doesn't seem to crash anymore and is probably good enough to go.

Some things to consider in the future:

• refactoring picker shapes similar to color modes (already planned AFAIK)
• removing the editor dependency from ColorPicker (I'm going to do it once this PR is merged, as I have an idea how)
• unifying preset lists. Right now each list has a static "cache" equivalent that is only used in the editor. Maybe we could change them all to statics, so all ColorPickes will share this automatically
• I might be forgetting one more thing 🤔

[KoBeWi]

I might be forgetting one more thing 🤔

I remember now. The folding state of presets could be remembered somewhere. Someone might want to have presets always unfolded when they open the picker.

[akien-mga]

Looks great :)

[akien-mga]

Thanks! Great work 🎉