Save color palette as resources to reuse later

[nongvantinh]

Close https://github.com/godotengine/godot-proposals/issues/7946

The lack of a palette library slows down development time because swatches contain many colors that may not match the mood an artist is trying to achieve. This can hinder their workflow as they search for the right color within a large set of mostly irrelevant options.

https://github.com/godotengine/godot/assets/53887662/c335f3ba-77f0-47dd-8eae-3bc53530bcd7

[AThousandShips]

This should use icons available at runtime, not editor icons, there are some appropriate ones available, and otherwise they should be added, this shouldn't depend on the editor

[fire]

I approve of allowing saving the color pallete from a usability team perspective.

[nongvantinh]

Update:

• Use FileDialog instead of EditorFileDialog.
• Save the palette in the user:// path instead of placing it in the project folder.
• Remove the Palette resource class because it doesn't need anymore.

@AThousandShips What do you mean when you say use icons available at runtime? I couldn't find the correct function to call.

[AThousandShips]

Icons available at runtime are those which aren't editor specific, meant in running projects not running the program itself sorry for confusion

[nongvantinh]

Sorry for the annoying force push. I wasn't familiar with the TTR part. I need one more favor. I couldn't find the correct icon to use in the function get_theme_icon.

[AThousandShips]

If it isn't in the default theme scene/theme/icons/ then you can add them from the editor or create new ones, and then you need to add them to the default theme, see in scene/theme/default_theme.cpp for details

But I think the normal folder icon is sufficient

Gonna test this one probably tomorrow but code looks good, got some final remarks I'll add when I've tested it out

[Calinou]

I wonder if it should be possible to give a name to each color, and have the name displayed in a tooltip when hovering the color in the ColorPicker's swatch list.

It's common for designers to assign names to colors to make slightly different colors easier to distinguish from each other, or even to refer to them in natural language. If I refer to "lime 800" from Tailwind's color palette, you can immediately know it's #3f6212.

This could be left to a future PR, but we should make sure this feature can be implemented in the future without breaking compatibility (i.e. without changing the data structure in the resource).

[AThousandShips]

Tested and it works as expected, however I don't think the list of resources allowed makes sense, it should probably just limit to res and tres because it currently allows you to save as things like animtex and mesh, which don't make sense

Edit: correction, it shouldn't even be that, it should be some binary format dedicated to this, it doesn't actually store a res or tres file either, so that needs to be fixed

[Calinou]

I suggest using ConfigFile to save palettes as opposed to FileAccess, so you have a text format that is friendly to version control. I'd also give them a custom file extension so they can be targeted more specifically, e.g. cpl.

A similar approach is used for script editor syntax themes, which use a tet file extension.

[nongvantinh]

Thank you all for your suggestions. I have revised accordingly, and now everything is set and ready for review.

[nongvantinh]

Revised, built, and tested. I hope this is the final change for my sloppiness.

[nongvantinh]

@Calinou All the concerns you mentioned have been addressed, but I'd appreciate your review and feedback.

[AThousandShips]

Please remove the [feat] part from the commit message

[nongvantinh]

Users might want to keep their previous location when saving and loading. So, only call set_current_dir once during initialization.

[nongvantinh]

Fixed all the issues @Calinou mentioned, and hopefully, there will be no regression. The bug that sometimes prevents color erasure from the palette is a pre-existing issue, the fix for it is included in this PR.

[KoBeWi]

When there are no colors to save, the menu has wrong size: Using Quick Load at runtime causes crash.

Palettes are saved as .res by default. I think .tres is more suited; it's better to save small resources as text. Also I'd rename Palette to ColorPalette.

If you expose a new class, you need to run doctool and fill the generated XML file.

[nongvantinh]

The quickloading option should not appear when running the app; it is only available in the editor.

Quick loading is only supported in the editor. Therefore, if I add this option to the ColorPicker, users will be aware of its availability. However, when they use the ColorPicker in their games, they will notice that the quickloading feature is missing, causing a discrepancy in workflow.

I am unsure whether to remove it entirely or implement a QuickOpenDialog that supports both the editor and in-game instances. Do you have any opinions on this?

[KoBeWi]

The quickloading option should not appear when running the app; it is only available in the editor.

But it does appear and causes a crash. It's fine if it would be only in the editor. If you want to get rid of the editor dependency, you could add an unexposed method set_quick_open_callback(), which would be a Callable set by the editor (there is a convenient setup_color_picker() method in EditorNode). When this callable is set, the ColorPicker would display the Quick Open option and selecting it would run the callback.

[nongvantinh]

Ops, I messed up git history again.

[KoBeWi]

This looks fine now, but there are some details to iron out. Aside from my comments, you could look into adding New option to ColorPicker menu, because there is no way to unload a palette currently. Also the menu button icons are not registered to ThemeDB, so they can't be easily changed.

The palette needs an icon (doesn't need to be added now).

[KoBeWi]

• [x] Manually deleting colors from palette will leave you with empty palette that you can't remove:

https://github.com/godotengine/godot/assets/2223172/b406544f-918d-4d42-be73-6f39b566c35b

• [x] When you load a palette for the first time at runtime, the header won't appear immediately:

https://github.com/godotengine/godot/assets/2223172/dcf8a822-2879-4f99-bc18-b47b667b4364

Might be related to https://github.com/godotengine/godot/pull/91604#discussion_r1614786484

---

Some of my previous comments weren't addressed.

[KoBeWi]

Crash when you quick open palette directly without using ColorPicker:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.3.beta.custom_build (be56cab58c056c074d1e02cd0b38641204e39f41)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] FileDialog::get_file_mode (C:\godot_source\scene\gui\file_dialog.cpp:982)
[1] FileDialog::get_file_mode (C:\godot_source\scene\gui\file_dialog.cpp:982)
[2] ColorPicker::_palette_file_selected (C:\godot_source\scene\gui\color_picker.cpp:837)
[3] call_with_variant_args_helper<ColorPicker,String const &,0> (C:\godot_source\core\variant\binder_common.h:304)
[4] call_with_variant_args<ColorPicker,String const &> (C:\godot_source\core\variant\binder_common.h:419)
[5] CallableCustomMethodPointer<ColorPicker,String const &>::call (C:\godot_source\core\object\callable_method_pointer.h:104)
[6] Callable::callp
[7] CallQueue::_call_function (C:\godot_source\core\object\message_queue.cpp:221)
[8] CallQueue::flush (C:\godot_source\core\object\message_queue.cpp:270)
[9] SceneTree::physics_process (C:\godot_source\scene\main\scene_tree.cpp:494)
[10] Main::iteration (C:\godot_source\main\main.cpp:4014)
[11] OS_Windows::run
[12] widechar_main
[13] _main
[14] main
[15] WinMain
[16] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[17] <couldn't map PC to fn name>
-- END OF BACKTRACE --

[KoBeWi]

Seems like there is one remaining bug 🤔

https://github.com/godotengine/godot/assets/2223172/c64bb1fe-0c3b-4213-9512-c85552a7b66e

* will appear with empty palette name when you toggle swatches.

[nongvantinh]

When the palette has too many colors, it will overflow the screen. I need to wrap it inside a scroll container. However, should I wrap the entire color picker in a scroll container or only the palette? Any suggestions?

[KoBeWi]

I think only the palette, because it's foldable. Although if you simply put a ScrollContainer it will likely be too small. Not sure how to solve that.

[nongvantinh]

I could introduce a foldable left side panel to the color picker, which will have a height equal to the ColorPicker's height. With the space acquired, I could implement the ability to give a name to each color. Perhaps, I could also allow users to dynamically resize this panel.

With that, I could also allow users to load multiple palettes simultaneously.

Edit:

Please note, I have a personal workload that will end in August, which requires me to put all my effort into it. Therefore, I will take a break from this and come back when it's done.

[nongvantinh]

I have come back. Here is the implementation I was talking about. What do you think about it?

https://github.com/user-attachments/assets/669ebe9b-c0d1-4f91-adc6-1886353421f3

[nongvantinh]

@Sauermann I tweaked the unit test for ColorPicker a little bit. Please help me review it.

[KoBeWi]

Here is the implementation I was talking about. What do you think about it?

That huge empty panel does not look good. I'd leave the size problem for later, it's pre-existing and the feature works fine as is. Maybe #94171 would help resolve this, e.g. by making the palette expand to 3 rows as necessary and then scrollable.

[AThousandShips]

Something in the changes have broken the tests, needs to be analyzed, the behaviour should be checked and identify what changed to break that test