-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Conversation
25bce0c
to
1f3c6da
Compare
1f3c6da
to
3b3c804
Compare
808f2d5
to
eea548a
Compare
eea548a
to
2924bfd
Compare
I've split 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 |
There was a problem hiding this 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.
Thanks! |
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 forThemeDB
to be to themes and themable nodes whatClassDB
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()
, allowingControl
s andWindow
s 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.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.
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.