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

Add LabelSettings resource for quick Label theme property override. #62139

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jun 17, 2022

Depends on #62108

Adds LabelSettings resource, to quickly override text properties of the Label.

Screenshot 2022-06-17 at 12 13 14

@AaronRecord
Copy link
Contributor

How does this work with theme overrides? I thought similar functionality was already possible with #50169?

@bruvzg bruvzg force-pushed the label_font_setttings branch 2 times, most recently from 4a68d88 to 285ad1d Compare June 23, 2022 20:23
@bruvzg bruvzg force-pushed the label_font_setttings branch from 285ad1d to 5240871 Compare July 1, 2022 08:00
@reduz
Copy link
Member

reduz commented Jul 6, 2022

@AaronRecord This entirely overrides the theme, its used in cases where you don't care about using themes (simple UIs or Label3D)

@bruvzg
Copy link
Member Author

bruvzg commented Jul 6, 2022

Rebased to match latest Font and FontFile change of #62108.

@bruvzg bruvzg force-pushed the label_font_setttings branch 2 times, most recently from e6bd505 to 6886648 Compare July 7, 2022 12:18
@nathanfranke
Copy link
Contributor

nathanfranke commented Jul 12, 2022

I think if the user doesn't care about themes, there's no harm in just making "quick access" properties that automatically override the theme.

image

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 12, 2022

@bruvzg Needs a rebase, and also we've decided that this shouldn't be added to Button. Otherwise, can merge!

Regarding better interoperability with themes, we've discussed it at length and decided that it can be solved at a later point.

@nathanfranke
Copy link
Contributor

Regarding better interoperability with themes, we've discussed it at length and decided that it can be solved at a later point.

When that is done, will LabelSettings be removed?

@bruvzg bruvzg force-pushed the label_font_setttings branch from 6886648 to f63d541 Compare July 12, 2022 13:05
@bruvzg bruvzg changed the title Add LabelSettings resource for quick Label and Button theme property override. Add LabelSettings resource for quick Label theme property override. Jul 12, 2022
@YuriSizov
Copy link
Contributor

Regarding better interoperability with themes, we've discussed it at length and decided that it can be solved at a later point.

When that is done, will LabelSettings be removed?

The two options mentioned in the meeting were to either extend FontVariation to include colors and what not, or to incorporate LabelSettings into the theme. In either case, I guess not removed, no.

In my own opinion, a proper solution would require some significant changes to theming, so it is unlikely we'll get there with the upcoming versions. I imagine we'd need to split theme properties into states so each state can have its own configuration of fonts and styles. Then those would probably need to be able to partially override each other (the need for this already exists for styleboxes). So for the foreseeable future we're "stuck" with LabelSettings and maybe some smaller changes to FontVariation.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Approving to indicate the consensus of the review meeting

@nathanfranke
Copy link
Contributor

The two options mentioned in the meeting were to either extend FontVariation to include colors and what not, or to incorporate LabelSettings into the theme. In either case, I guess not removed, no.

These are all included already in theme overrides. We would only need to add a shortcut.
image

@AaronRecord
Copy link
Contributor

AaronRecord commented Jul 12, 2022

I personally agree with nathanfranke, to me this just feels like adding a new system that does the exact same thing as theme overrides and theme types. The only use case I see that this adds that doesn't already exist is having a resource based source for theme overrides, but that only exists for labels. Maybe theme overrides could be made into a resource? Although couldn't you already do something similar by just creating a custom scene with a label as its only node where you set the theme override settings appropriately? The biggest issue I see with the current system is theme overrides are a little less convenient and probably less intuitive for beginners, but I think this could be solved with something like what aaronfranke said. The people that are vouching for this change are a lot more experienced than I am, so maybe this is a good idea, I'm just struggling to understand why this change is useful, and if it is I just want to understand why.

@aaronfranke
Copy link
Member

@AaronRecord I think you mean @nathanfranke

@YuriSizov
Copy link
Contributor

@AaronRecord We've discussed this in the meeting, with the same arguments being brought up. Ultimately, this PR a) gives better usability for people who want simple things and don't want to deal with themes, and b) it supports non-themeable nodes like Label3D.

I would really want this kind of customization to be available as a part of the theme as well, as it's more convenient and more organized, but we decided to postpone resolving that part.

@nathanfranke
Copy link
Contributor

FYI a) is invalid because I've already proposed an alternative.
As for b) I don't even think we should have Label3D. Rather, we should make it easier to display a UI hierarchy in 3D (e.g. viewport with a 3d billboard texture).

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.

We discussed this again in a maintainers meeting and agreed to go with this approach for now.

As others here I'm personally not fully happy with the redundancy this introduces with Theme and theme override properties, but we agreed that solving this problem is quite complex and would require rethinking Theme in a way that's not feasible for 4.0 in a timely manner. So we go with this for now and we'll see if/how to improve things further later on.

@akien-mga akien-mga merged commit 715f556 into godotengine:master Jul 19, 2022
@akien-mga
Copy link
Member

Thanks!

@bruvzg bruvzg deleted the label_font_setttings branch July 19, 2022 18:16
@nathanfranke
Copy link
Contributor

Opened discussion: godotengine/godot-proposals#4919

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.

7 participants