Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save color palette as resources to reuse later #91604

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nongvantinh
Copy link
Contributor

@nongvantinh nongvantinh commented May 5, 2024

Close godotengine/godot-proposals#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.

Untitled.video.-.Made.with.Clipchamp.mp4


save_swatches = memnew(Button);
save_swatches->set_flat(true);
save_swatches->set_tooltip_text("Save the current color palette to reuse latter");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
save_swatches->set_tooltip_text("Save the current color palette to reuse latter");
save_swatches->set_tooltip_text("Save the current color palette to reuse later");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for translatable text you should use translation macros like TTR or RTR.

@@ -35,6 +35,7 @@
#include "core/math/color.h"
#include "core/os/keyboard.h"
#include "core/os/os.h"
#include "editor/editor_node.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must #ifdef for TOOLS_ENABLED. Though ideally, the saving and loading palette feature should also work when using the ColorPicker in a game/application project. Possibly with a boolean flag to enable or disable this feature.

@AThousandShips AThousandShips changed the title [feat] Save color palette as resources to reuse later Save color palette as resources to reuse later May 6, 2024
@AThousandShips
Copy link
Member

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

@nongvantinh nongvantinh force-pushed the implement-7946 branch 2 times, most recently from 7c37110 to 0839503 Compare May 6, 2024 14:49
@fire
Copy link
Member

fire commented May 6, 2024

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

@fire fire requested a review from a team May 6, 2024 14:54
@nongvantinh
Copy link
Contributor Author

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
Copy link
Member

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

AThousandShips
AThousandShips previously approved these changes May 6, 2024
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@nongvantinh nongvantinh force-pushed the implement-7946 branch 2 times, most recently from 04ebe85 to acdd5f8 Compare May 6, 2024 15:11
@AThousandShips AThousandShips dismissed their stale review May 6, 2024 15:12

Accidental approval

@nongvantinh
Copy link
Contributor Author

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
Copy link
Member

AThousandShips commented May 6, 2024

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
Copy link
Member

Calinou commented May 6, 2024

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

@nongvantinh nongvantinh force-pushed the implement-7946 branch 2 times, most recently from 768da7a to 10f2235 Compare May 6, 2024 23:22
@AThousandShips
Copy link
Member

AThousandShips commented May 7, 2024

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
Copy link
Member

Calinou commented May 7, 2024

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 nongvantinh force-pushed the implement-7946 branch 2 times, most recently from feca7e7 to 1e0de24 Compare May 8, 2024 12:42
@KoBeWi
Copy link
Member

KoBeWi commented May 16, 2024

When there are no colors to save, the menu has wrong size:
image
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.

@@ -35,9 +35,14 @@
#include "core/math/color.h"
#include "core/os/keyboard.h"
#include "core/os/os.h"
#ifdef TOOLS_ENABLED
#include "editor/editor_quick_open.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't include editor code in scene/ files. You'll need to find a way to use quick open without the dependency (maybe it can be handled by parent node and a callback).

@nongvantinh
Copy link
Contributor Author

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
Copy link
Member

KoBeWi commented May 16, 2024

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.

scene/resources/color_palette.cpp Outdated Show resolved Hide resolved
scene/resources/color_palette.cpp Outdated Show resolved Hide resolved
scene/resources/color_palette.cpp Outdated Show resolved Hide resolved
scene/resources/color_palette.h Outdated Show resolved Hide resolved
@nongvantinh nongvantinh force-pushed the implement-7946 branch 2 times, most recently from 1b024dd to 22775e5 Compare May 18, 2024 03:39
@nongvantinh nongvantinh requested a review from a team as a code owner May 18, 2024 03:39
@nongvantinh
Copy link
Contributor Author

Ops, I messed up git history again.

@nongvantinh nongvantinh force-pushed the implement-7946 branch 2 times, most recently from 1e28141 to 168b4f7 Compare May 18, 2024 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Save and Load ability to swatches
7 participants