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

Register theme properties with ThemeDB #81312

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Sep 4, 2023

Depends on #81305. Marking it as draft, but it's fully ready and can be reviewed.

This PR is a part of godotengine/godot-proposals#4486, implementing a mechanism to register/bind theme items with their owner class. This allows us to have a stable and reliable database of theme properties required by registered nodes.

Status quo

Currently we rely on the default theme. Whatever is defined in it is considered to be the source of truth, even if the control itself doesn't use it, or if it requires something that is currently missing from the default theme for one reason or another. This comes with a couple of problems, the main one being that controls and their own theme properties are defined in separate places, the relevant parts of code are not connected and discrepancies happen from time to time.

This PR solves this issue. A while back I've implemented ThemeDB with the goal to be the source of truth for everything related to themes. The idea was for ThemeDB to be to themes and themable nodes what ClassDB is to the Godot classes and API. Now we're a bit closer to that goal.

How it changes

This PR introduces a set of methods and macros that can be used from inside of static void _bind_methods(), allowing Controls and Windows to statically provide the information about theme properties that they use. This system tracks both own properties and external properties. It also hooks into the cache, removing the need to implement _update_theme_item_cache in every control. Instead, theme properties can be connected to theme definitions in a declarative manner.

	BIND_THEME_ITEM_CUSTOM(Theme::DATA_TYPE_STYLEBOX, Label, normal_style, "normal");
	BIND_THEME_ITEM(Theme::DATA_TYPE_CONSTANT, Label, line_spacing);

	BIND_THEME_ITEM(Theme::DATA_TYPE_FONT, Label, font);
	BIND_THEME_ITEM(Theme::DATA_TYPE_FONT_SIZE, Label, font_size);
	BIND_THEME_ITEM(Theme::DATA_TYPE_COLOR, Label, font_color);
	BIND_THEME_ITEM(Theme::DATA_TYPE_COLOR, Label, font_shadow_color);
	BIND_THEME_ITEM_CUSTOM(Theme::DATA_TYPE_CONSTANT, Label, font_shadow_offset.x, "shadow_offset_x");
	BIND_THEME_ITEM_CUSTOM(Theme::DATA_TYPE_CONSTANT, Label, font_shadow_offset.y, "shadow_offset_y");
	BIND_THEME_ITEM(Theme::DATA_TYPE_COLOR, Label, font_outline_color);
	BIND_THEME_ITEM_CUSTOM(Theme::DATA_TYPE_CONSTANT, Label, font_outline_size, "outline_size");
	BIND_THEME_ITEM_CUSTOM(Theme::DATA_TYPE_CONSTANT, Label, font_shadow_outline_size, "shadow_outline_size");

I went with macros and lambdas here to hide the complexity and make the setup process as simple as possible. You still define ThemeCache structs the same way as before. The only difference is that assignments from _update_theme_item_cache are replaced with these binds, which serve multiple purposes behind the scenes. Some nodes would still implement _update_theme_item_cache to add to the default logic (e.g. cache the base scale), but those occurrences would be rare.

The syntax for binding theme items is similar to how we setup class members, so it should be easy to understand.

	BIND_THEME_ITEM(Theme::DATA_TYPE_ICON, ColorPicker, bar_arrow);
	BIND_THEME_ITEM_CUSTOM(Theme::DATA_TYPE_ICON, ColorPicker, sample_background_icon, "sample_bg");
	BIND_THEME_ITEM(Theme::DATA_TYPE_ICON, ColorPicker, overbright_indicator);
	BIND_THEME_ITEM(Theme::DATA_TYPE_ICON, ColorPicker, picker_cursor);
	BIND_THEME_ITEM_CUSTOM(Theme::DATA_TYPE_ICON, ColorPicker, color_hue_icon, "color_hue");

	BIND_THEME_ITEM_EXT(Theme::DATA_TYPE_STYLEBOX, ColorPicker, mode_button_normal, "tab_unselected", "TabContainer");
	BIND_THEME_ITEM_EXT(Theme::DATA_TYPE_STYLEBOX, ColorPicker, mode_button_pressed, "tab_selected", "TabContainer");
	BIND_THEME_ITEM_EXT(Theme::DATA_TYPE_STYLEBOX, ColorPicker, mode_button_hover, "tab_selected", "TabContainer");

Future features

In follow-up PRs I will use this system to generate theme property overrides and documentation. The fact that we also register external properties would allow us to document which theme properties a node borrows from another node, if we choose to do so. Overall this should lead to a more controllable and reliable way to reason about theme items.

This system can also be used by extensions and, potentially, scripting. For extensions we can definitely add a similar set of macros, so they can bind theme items with custom GUI nodes that they add. This means that such nodes would be respected in the editor UI, their properties would appear in the docs and in the overrides list of the inspector. For scripting, I would need to think a bit more about how to expose the feature, which syntax to use for it (which is something that I've outlined in godotengine/godot-proposals#4486 as well, but the exact approach is still unclear to me).

Do keep in mind, that the default theme is still needed for the actual values. Without it components are unstyled. That does limit the potential usefulness for extensions and scripting for now, but — baby steps!


I've only updated scene/ files in this PR. Editor nodes can be done in a follow-up. Even if they already use _update_theme_item_cache, it shouldn't matter, because that's still a valid approach. Plus editor nodes don't need to expose theme items, so this isn't critical.

I also don't want to cause conflicts with #80573 🙃


PS. I also included semi-related change in TabContainer, swapping deferred calls from using strings to using callables. One of the affected methods was relevant to themes, so I updated both.

@YuriSizov YuriSizov added this to the 4.2 milestone Sep 4, 2023
@YuriSizov YuriSizov marked this pull request as ready for review September 5, 2023 13:22
@YuriSizov YuriSizov requested review from a team as code owners September 5, 2023 13:22
scene/gui/button.cpp Outdated Show resolved Hide resolved
scene/theme/theme_db.cpp Outdated Show resolved Hide resolved
@YuriSizov YuriSizov marked this pull request as draft September 8, 2023 18:17
@YuriSizov YuriSizov force-pushed the theme-static-binds branch 2 times, most recently from 808f2d5 to eea548a Compare September 11, 2023 11:43
@YuriSizov YuriSizov marked this pull request as ready for review September 11, 2023 11:43
@YuriSizov
Copy link
Contributor Author

YuriSizov commented Sep 11, 2023

I've split BIND_THEME_ITEM into two, with the basic version using the same name for the cache field and the theme property. I'd say most properties are like that, with a few classes having some exceptions and a couple of classes being built out of exceptions. Some of it is due to bad naming of theme properties themselves, and some can be addressed by making the cache field name match the properties at a small cost of code clarity.

But for now it should be fine.

Edit: In a follow-up I'll fix GraphEdit classes to use this system as well (there are also some other UI nodes that still use get_theme_* here and there, probably added after the theme cache changes; I'll fix them as well). I want to keep this PR clear.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@akien-mga akien-mga merged commit d084012 into godotengine:master Sep 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

3 participants