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

Check for type variations in inherited themes #82218

Merged

Conversation

YuriSizov
Copy link
Contributor

Fixes #82199.

As I added theme contexts in #81130, I also reworked some code that looked up theme type variation chains. I noticed a gap in logic and decided to fix it. The issue that I'd noticed caused us to accept type variations even if we couldn't resolve them to a native type. I found it problematic, as that could mean that we would never inherit proper theme properties that match the control's type because the type chain would just end abruptly (or go into an unexpected direction).

So I fixed it, but as it turns out there was a benefit from the old approach. We only ever checked variations in global themes, and that was still true after my fix. But because previously we would allow variations which we could not resolve to native types, variations added by themes owned by controls would accidentally function correctly.

So this PR adds proper support for these type variations. Now all inherited themes are checked before the global themes are, just like with the theme items themselves. This may make things a bit slower than before, but unless themes are abused it shouldn't cause any noticeable issues.


Note, that a type variation must completely resolve to a native type within one single theme. You cannot extend type variations from another theme at this time. In most cases this should be fine though. Supporting something more complex would be way more expensive, so I'd rather stop here and live with this limitation.

@Koyper
Copy link
Contributor

Koyper commented Sep 24, 2023

@YuriSizov Thanks for this rapid fix! Please forgive the somewhat tangential question here: what is your opinion on best-practice with the improvements in theming for doing a run-time theme change for the entire project, such as switching from a light to a dark theme? I'm encountering a very significant Godot 4 performance bottleneck when I script theme styleboxes to change background color when connected to a global light/dark signal (which worked well in GD 3.x). It seems like overrides are much more performant than modifying an internal theme resource property, or is there an explanation for that that I can work around? Thanks in advance.

@YuriSizov
Copy link
Contributor Author

@Koyper We can discuss this on RocketChat or on Discord, but this is not the best place to offer project help.

@akien-mga akien-mga merged commit 7287df4 into godotengine:master Sep 24, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the themes-vary-vary-duck-duck-goose branch September 25, 2023 10:25
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.

Theme type variations don't work for Panel, Label, Button, Tree, etc.
3 participants