-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Replace the f32 font_size value of TextStyle with an enum value
#9524
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
Conversation
* `Px`: size in logical pixels, * `Vh`: size in percentage of viewport height, * `Default`: use default font size contained in the new `DefaultFontSize` resource. Only updated `text.rs` example so far, want feedback on the API etc before rewriting every example. Many problems left to solve: * Expect a lot of bugs with scale factor and Text2d. * The `hello bevy` text in the `text` example scales correctly when you resize the window but almost immediately runs out of font atlases. * Should there be `Vw`, `VMin` and `VMax` variants, or is only `Vh` really useful? * How should percentage values be implemented? * Is using `Default` for the name of an enum variant bad style?
|
Is there a reason why you aren't using the |
|
font_size value of TextStyle with a FontSize enumfont_size value of TextStyle with an enum value
bushrat011899
left a comment
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! I like the idea of trying to convert parameters with implicit units into types with explicit ones.
| /// The default font size. | ||
| /// Used when the size field of TextStyle is set to Default. | ||
| #[derive(Resource, Clone, Copy, Debug)] | ||
| pub enum DefaultFontSize { |
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.
Could this instead be a new-type over FontSize, with error handling to warn about a recursive definition? It would remove a few lines worth of repetition.
| #[derive(Resource, Clone, Copy, Debug)] | ||
| pub enum DefaultFontSize { | ||
| /// Default font size in logical pixels | ||
| Px(f32), |
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.
Should there be any notes here about reasonable values? e.g., negative values?
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.
I think it's implicit that negative values would be nonsense but we could add some sort of error message. In main atm if font_size is set to a negative we just fail silently and draw nothing. Perhaps there should be a warning there too.
|
I would be very happy to see |
|
I'm feeling similarly iffy about |
|
Yep this PR is very experimental. I didn't think about it too much and hacked together this implementation to identify the potential problems. It's an important feature. Agree that The main blocker is how we create a new texture atlas for every different
None of which seems that difficult to implement but I don't have unlimited time or that much experience to draw on. (1) is conceptually easy but the implementation details might be very tricky (like how to determine whether the fonts are close enough, what if we have multiple windows each with a different scale factor?). (2) Looks like it could be hacked together quite easily, just delete the oldest font atlas that isn't in use if we go past the font atlas limit. Perhaps this should be part of the changes to assets though, I'm not sure? (3) I thought this would be challenging but not so sure now. There is plenty of prior art on how to render sdf fonts. It might be the simplest approach. |
|
This PR would fix #5471 if merged. |
|
As a response to the third point I could see practical use for Vh, Vw, and Vmin. Those being the following:
I don't see any practical use for Vmax, but there is no reason to be limiting with no reason. If it would work and have no issues... why not let it be there? |
Well no features is also a feature. It can seem harmless to add extra variants but everything has to be maintained and documented and kept up to date etc. A huge range of similar options can be overwhelming for users too. Your argument makes sense about |
|
I'd also like to throw in a potential |
|
I agree with @JustAnotherCodemonkey a Our use case is to have a UI node have a shrink animation, but it's really tedious to have the characters on it shrink at the same time with the way font size is implemented. |
|
With respect to the issue of using
The difficulty, of course, is that Bevy doesn't have style inheritance. (Note that CSS doesn't have style inheritance either, except for a handful of properties - all of which are related to text styling. You can't inherit margins or padding for example.) Bevy Feathers dips its toe into these waters a little bit with its theming solution, but is not a complete solution for this problem. |
|
Suggestion: Replace the This has the same use case as Thus, I could define a body text which is I thought about naming this |
|
Some bikeshedding on naming: The global resource could be called Note that all of the variants (both here and in Val) represent units of measurement, so we don't need to include any suffixes that convey that this is a unit; all we need is the name of the unit (Percent, Px, etc.) |
|
Closed in favour of #22614 |
Objective
Replace the
font_sizefield ofTextStylewith an enum similar to Bevy UI'sValtype enabling users to set responsive font sizes that depend on the size of the viewport or the text's parent container.Fixes #9525
Solution
Replaces f32 font size with a
FontSizeenum. Currently implemented variants:Px: size in logical pixels,Vh: size in percentage of viewport height,Default: use default font size contained in the newDefaultFontSizeresource.Only updated
text.rsexample so far, want feedback on the API etc before rewriting every example.Many problems left to solve:
Text2d.hello bevytext in thetextexample scales correctly when you resize the window but almost immediately hits the font atlas limit.Vw,VMinandVMaxvariants, or is onlyVhreally useful?Defaultfor the name of an enum variant bad style? Maybe should just go with an explicitUseDefaultFontSize.Implementation is working though, small window with

FontSize::Vh(50.):Larger window with

FontSize::Vh(50.):Changelog
FontSizeenum withPx,VhandDefaultvariants.font_sizefield ofTextStyleis now aFontSize.DefaultFontSizeresource. Also implemented as an enum withPxandVhvariants but noDefaultvariant.Migration Guide
The
font_sizefield ofTextStyleis now aFontSize. TheFontSize::Pxvariant is the equivalent to setting an f32 value previously.