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

Refactor Font configuration and import UI, and Font resources [with font size]. #61473

Closed
wants to merge 1 commit into from

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented May 27, 2022

Alternative version of #61406, the same changes but with font_size left as a separate theme property.

Screenshot 2022-05-27 at 22 19 18

@bruvzg bruvzg added this to the 4.0 milestone May 27, 2022
@bruvzg bruvzg requested review from a team as code owners May 27, 2022 16:15
@bruvzg bruvzg force-pushed the font_config_no_size branch 3 times, most recently from 5d512f8 to 171bb1d Compare May 27, 2022 21:15
@bruvzg bruvzg requested a review from a team as a code owner May 27, 2022 21:15
@bruvzg bruvzg force-pushed the font_config_no_size branch from 171bb1d to 29833ac Compare May 27, 2022 21:20
@bruvzg bruvzg force-pushed the font_config_no_size branch 2 times, most recently from 81adb2d to 5ae989d Compare June 7, 2022 08:39
@bruvzg bruvzg force-pushed the font_config_no_size branch from 5ae989d to a0079be Compare June 8, 2022 12:06
@reduz
Copy link
Member

reduz commented Jun 15, 2022

Regarding implementation wise, there are some things I don't quite get, which are a bit broad to put as code review so here are my doubts:

  • I don't understand why Font is being replaced by FontConfig in CanvasItem and most controls. Did we not decide to go against this direction in 4.0? Font on one side, and size and other settings on the other, separate.
  • My understanding for FontConfig was more that we need to just have something like FontConfig or FontSettings as something entirely optional and only for very few specific controls like Label, Label3D or Button because those are controls that users may want to use outside the theme. Having it in places like Tree as an example does not make a lot of sense to me.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 15, 2022

I have to say I prefer the version in #61406.

I have opened a new PR with font size moved to FontConfig, will keep it as two commits (one is the same as this, and second to move font size), since it's easier to rebase both versions this way - #62065

@bruvzg
Copy link
Member Author

bruvzg commented Jun 15, 2022

I don't understand why Font is being replaced by FontConfig in CanvasItem and most controls. Did we not decide to go against this direction in 4.0? Font on one side, and size and other settings on the other, separate.

Having it in places like Tree as an example does not make a lot of sense to me.

Everything in the FontConfig is useful for any control or function that's drawing text, there's nothing that's specific to Label or other controls. My idea was to move all font related settings from the controls, Font and FontData to the single resource to be accessible everywhere. So Font is what imported for a font file (which might have multiple font faces, for example), and FontConfig is a specific variation of the font that's used to actually draw text.

For some controls, it makes sense for font settings more accessible, there's another PR for this - #61991, but I do not see any reason to use different ways to set the same things.

@reduz
Copy link
Member

reduz commented Jun 15, 2022

@bruvzg I understand this, but the way I see this, its still very annoying because you are forcing the allocation of a FontConfig every time you want to draw something. I would suggest the following reorganization of the font resources:

Font

  • Contains what was in FontData
  • Contain all the functions for drawing from Font. There really is no justification in my head for having these two things in separate places.

FontVariation

  • Contains variation options, you can pass this to the font drawing code in Font optionally, but its not mandatory. You use only if you really need it (which in far most cases you don't). You can expose this as a property in controls that make sense and the controls can just pass it to the draw functions in Font.

FontSettings

  • Contains a Font, a size, outline settings, variation, etc. this is only for use in classes like Label, Button, Label3D and no more than that (Tree, TextEdit, LineEdit or ItemList as an example do not make sense because they are more complex controls, just use theme override). It is of very limited scope meant for classes where you want it to be easier to ignore the theme entirely and use those settings to draw.
  • In those few controls, you need to add some if() condition to check whether this is present and use this as the settings to draw rather than the theme. Entirely hardcoded because that's the point.

With this, we have just a single Font class that does everything. A FontVariation class that you can use in the rare use cases where you need this, without forcing it to the user, and a FontSettings class that is only used for some few controls where you want to override and ignore theme.

@YuriSizov
Copy link
Contributor

I understand @YuriSizov's concern of having to duplicate the resource to change the font size, but this is easily addressed by making the resource unique on duplicate, by adding the hint: PROPERTY_USAGE_DO_NOT_SHARE_ON_DUPLICATE to it. For users it will always be a unique thing that they can expand and edit if they want, or ignore and use the theme otherwise.

How do we communicate to the user that out of all resources this one cannot be shared for some reason? My main concern is that changing a font size should be just that - changing the size. It's the most frustrating part of working with fonts in 3.x to our users that they have to work with resources to do that. It's not intuitive, not how people treat fonts at all.

Remember, this is a replacement to a toolbar that already meets the expectations of an average user who has ever worked with fonts in any other application. In that toolbar changing font properties, however foreign to Godot's vision it was, was intuitive and familiar. This suggestion complicates things, even if it makes it neater for our internal design and logic. IMO user's expectations should come first and our internal concerns should come second.

So, to reiterate, changing a size of a font is a highly used operation that should be accessible. It should be as simple as changing a numeric value. If you want to have it stored in a resource internally, fine, but it should not require copying of resources on the user side. And as a sidebar, if we do have a flag to implicitly copy a resource instead of sharing, we need to communicate it very well to the user, because this is also not the expected behavior anywhere in Godot.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 15, 2022

You use only if you really need it (which in far most cases you don't). You can expose this as a property in controls that make sense and the controls can just pass it to the draw functions in Font.

I do not see any control that won't need, it's pretty much an essential feature. Absence of it will make any variable font or font collection unusable.

Otherwise, Font / FontVariation makes sense. But there's an issue with FontVaraiation, the set of the available variation properties is depended on the specific font, so it won't be usable as an independent resource. Obviously, it can get required property info when it's part of control and inspector plugin can access control's font, but it seems a bit hacky for me.

I'm not sure what's so special with Label and Button to deserve its own FontSettings, outline doesn't seem like something that need to be changed often, and without it's basically the same as FontVariation.

@reduz
Copy link
Member

reduz commented Jun 15, 2022

@YuriSizov IMO this touches on a different problem that I was about to open a proposal for discussion about to get feedback, but the bottom line is that most users are completely unaware that, when duplicating, internal subresources are shared. For this is is actually unexpected (because its very hard to track) and led us to having the usage flag above as a hack.

On the other side, users only understand well that its the resources saved to a file that you are sharing. So, in this specific case, if they wanted to actually share it they would save it to a file.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 15, 2022

I guess another option would be to do exactly the opposite, keep Font and font size theme properties, and add a new font_variation Theme property. Which do not need to be a new class and can be just a Dictionary (all logic is in the inspector plugin anyway).

Text draw functions stay in the Font, and take size and variations as optional arguments.

@reduz
Copy link
Member

reduz commented Jun 15, 2022

@bruvzg The problem to me is that, in practice, a control can use multiple fonts and sizes for different things and mix and match them, but changing the variation is very rare or very context specific so I am not entirely sure that having it as a theme property will be of much use. This is why I suggested only exposing it as a regular property in controls that may make sense to use, or even in Control if you want to override for the whole control.

@YuriSizov
Copy link
Contributor

Ok, the sharing part we can discuss in a separate proposal, but I still advocate for a better font editor tool that would work intuitively, even if internally it's turtles and duplicated resources all the way down.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 15, 2022

but changing the variation is very rare

Changing variations (and the same for font collections) is just a much more convenient way to have multiple fonts. So if I for example want to use normal and bold fonts, I can have a single variable font file instead of two files (which also will take less space). And I won't need to replace files (or event compile new fonts, if the preferred one is not available by default) if I want to change the thickness of the bold font, for example. Also, if we will ever have support for using system fonts (which I'm planning to add), there are not many options, many of the system fonts are shipped as collections.

@Zireael07
Copy link
Contributor

@YuriSizov IMO this touches on a different problem that I was about to open a proposal for discussion about to get feedback, but the bottom line is that most users are completely unaware that, when duplicating, internal subresources are shared.

For that reason, we really need an indicator of unique vs shared status, as I asked 2 years ago in godotengine/godot-proposals#904

@reduz
Copy link
Member

reduz commented Jun 15, 2022

@Zireael07 Put this up instead, which is hopefully a simpler solution - godotengine/godot-proposals#4672

@reduz
Copy link
Member

reduz commented Jun 15, 2022

@bruvzg The problem is that, in most cases, users just download font packs that come with the different intensities, so its rare to change the boldness via parameter in general.

How about this then? FontVariation can inherit Font too (or FontBase), and it contains both a Font property and the variation properties. So you can still use it as a variation of an existing font. IMO this should be the simplest way to solve the problem.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 15, 2022

The problem is that, in most cases, users just download font packs that come with the different intensities, so its rare to change the boldness via parameter in general.

Well, I understand it's a less common option, but I do not like an idea of arbitrary restricting a feature to the Label and Button.

How about this then? FontVariation can inherit Font too (or FontBase), and it contains both a Font property and the variation properties. So you can still use it as a variation of an existing font. IMO this should be the simplest way to solve the problem.

Hm, I like this option, it should work.

So, something like this:

FontBase (abstract class).
↳ Font (imported from a font file, no runtime options, includes fallbacks selected during import).
↳ FontVariation (created by user, basicaly the same as `FontConfig` in this PR).

This is probably the most logical way to do it indeed.

@reduz
Copy link
Member

reduz commented Jun 15, 2022

@bruvzg Yeah I think that requires the least code change and hacks and it should just simply work transparently with all the existing code.

@reduz
Copy link
Member

reduz commented Jun 15, 2022

Ok, the sharing part we can discuss in a separate proposal, but I still advocate for a better font editor tool that would work intuitively, even if internally it's turtles and duplicated resources all the way down.

I fully agree, but I think if we solve the shared duplication problem, then for users it will be very obvious that its not shared by default, so there is no need to have a separate font size setting. And if they do want to share, they can simply save the font setting to disk and reuse it wherever they want.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 15, 2022

So I guess there are 3 separate issues:

  • Simplify current FontData + Font + bunch of control properties ⇾ to the new FontBase + Font + FontVariation: I'll update this PR (or open a new one) tomorrow.
  • Resource sharing / duplication: No idea what should be done, but it's not something limited to fonts, so probably should be solved separately.
  • More convenient access to the font / outline / color for the common controls, like Label: so far I do not like anything implemented or proposed:
    • Current theme properties are hidden in the bottom of inspector and inconvenient to access.
    • Shortcuts for the theme properties (Make enum/constant binds 64-bit. #61991) is a bit faster to access, but might cause more confusion, and feels like a dirty hack.
    • Current toolbar seems to be most user-friendly, but not working nice with small screen sizes, and inconsistent with the rest of editor UI.
    • FontSetting property/class just for a Label seems like overkill, and probably won't be any more convenient and less confusing.

@reduz
Copy link
Member

reduz commented Jun 15, 2022

@bruvzg

  • Simplify current FontData + Font + bunch of control properties ⇾ to the new FontBase + Font + FontVariation: I'll update this PR (or open a new one) tomorrow.

Sounds good!

  • Resource sharing / duplication: No idea what should be done, but it's not something limited to fonts, so probably should be solved separately.

Definitely out of the scope of this.

  • More convenient access to the font / outline / color for the common controls, like Label: so far I do not like anything implemented or proposed:

I think the way it is now in theme is ok because it does what it has to do. The only problem is the cases where you want to have a custom outline. IMO of this we can also add the outline setting to FontVariation, since its kinda the same thing as with Font/FontData.

  • Current theme properties are hidden in the bottom of inspector and inconvenient to access.
  • Shortcuts for the theme properties (Add support for property shortcuts, to display the same property multiple times. #61991) is a bit faster to access, but might cause more confusion, and feels like a dirty hack.
  • Current toolbar seems to be most user-friendly, but not working nice with small screen sizes, and inconsistent with the rest of editor UI.
  • FontSetting property/class just for a Label seems like overkill, and probably won't be any more convenient and less confusing.

All the above is solved by FontSetting. I know it may seem counterintuitive, but to me that is the right solution. This is what we discussed originally, the actual use cases that you want to solve are:

  • Users who want to use themes will go to the theme editor and edit their theme, and touch some theme overrides. This works fine as-is and this feature is useless to them.
  • Users who don't want to use themes because they use TextureButton, TextureProgress, TextureRect, but still stumble on the theme stuff when using Label or Label3D. For them going to the theme overrides is annoying. FontSettings merely solves this. Its a minor problem with a simple solution. It does not happen with other controls, hence it does not need a more generic solution.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 15, 2022

All the above is solved by FontSetting. I know it may seem counterintuitive, but to me that is the right solution. This is what we discussed originally, the actual use cases that you want to solve are:

Well, I do not think it's the best solution, but it's non-intrusive and won't break anything, and if we'll have something better, it's replacing it will be easy, so I guess why not.

@reduz
Copy link
Member

reduz commented Jun 15, 2022

@bruvzg I agree its no the cleanest, but its very simple and solves the problem at hand. We can eventually do something better if we figure out what at some point.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 16, 2022

#62108 - a new PR with FontBase/Font/FontVariation. I'll open a separate PR for the Labels FontConfiguration a bit later (it should be completely independent for other Font changes).

@bruvzg
Copy link
Member Author

bruvzg commented Jun 17, 2022

#62139 - a second part, with the settings resource for Label and Button (done on top of #62108).

@bruvzg bruvzg deleted the font_config_no_size branch June 17, 2022 10:19
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