-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 FontRef
and DefaultFont
#5549
Conversation
If the |
That could work, but this new api encourages people to just use Even if it did, it would be a bit weird how order dependent it would become. Like you change the order of a ui widget and now suddenly all the text in your app looks different. |
I don't mind doing it as a fallback, but personally I'd rather get an error if it's not set. Eventually, once we ship an editor we'll be able to ship a font and initialize the resource with a default value, but right now it feels less error prone to not do that. Eventually we could also load the system font, but that should be done in a separate PR. |
Alright, I implemented the fallback to the first font on default, but as I expected if the fonts are added in the same system it doesn't work because the font isn't added to the assets store yet. I also fixed a bug where text wouldn't load on startup in desktop_app mode. |
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 really like this small but useful ux improvement, thanks!
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.
Changes look really good but we should add a note in the changelog explaining the change and addition of the DefaultFont resource
I added a short description to the migration guide. I'm not sure what would be needed in the changelog section. |
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.
This looks good to me now, for some reason I had the recollection that we usually add things to the CHANGELOG.md
but it seems we only do that when we do releases. In that case what is currently in the PR description should be enough.
.add_system_to_stage( | ||
CoreStage::PostUpdate, | ||
update_text2d_layout.after(ModifiesWindows), | ||
); | ||
) | ||
.add_system(load_font); |
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.
Are there any system-ordering issues we should be careful about here?
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.
Not really, I guess it could make sense to run it in PostUpdate so it can run in the same frame as when the Text was added but it will work either way
1f1c939
to
5e5efec
Compare
} | ||
bevy_asset::LoadState::Failed => panic!("Failed to load font {:?}", path), | ||
}; | ||
section.style.font = FontRef::Handle(handle); |
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.
Not a huge fan of this dataflow. A style type should be "declarative". The user sets a value and the underlying system makes it happen under the hood, without changing the users' declared style.
Setting style.font = FontRef::Default
should hold that default indefinitely, not serve as a placeholder/queued action that gets overwritten in place. Same goes for FontRef::Path
.
I understand why you're doing this (we need to hold a strong handle somewhere to keep the font alive). But this weird dataflow isn't worth it from my perspective.
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 agree, I'm just honestly not sure how else I could do that. I'll try to think of something better.
/// If the font has already been loaded then it simply converts the [`FontRef::Path`] to a [`FontRef::Handle`] | ||
/// Otherwise it tries to load the font with the [`AssetServer`] | ||
pub fn load_font( | ||
mut query: Query<&mut Text, Added<Text>>, |
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.
This will only run for added text. FontRef should would consistently, even if changed at runtime.
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.
Ah, right, I need to handle situations where fonts are updated, I didn't think about that one.
I closed this PR because the solution in #8445 solves this in a much nicer way. |
Objective
Creating a
TextStyle
requires anHandle<Font>
which means you either need to pass the text_style around or pass an asset_server everywhere. This makes declaring reusable ui components harder than necessary.It should be possible to not need to pass around anything and create it where we want it and let bevy take care of loading the font.
Solution
FontRef
inspired by theShaderRef
from the newMaterial
DefaultFont
resourceFontRef
and the newbevy_text::load_font()
system will take care of loading the font asset and converting the path to anHandle<Font>
.Handle
to aFontRef::Handle
Changelog
FontRef
DefaultFont
TextStyle
now usesFontRef
instead ofHandle<Font>
Migration Guide
Instead of requiring an asset_server to load a font you can now use a string path to a font asset and bevy will take care of loading it properly. This makes declaring
TextStyle
components much easier. If you want to keep the asset_server you can easily convert aHandle<Font>
to aFontRed
using.into()
.Notes
I updated all the examples using fonts, but I only used the new DefaultFont on the game_menu example.
If anyone is scared of the amount of files changed, the vast majority of the changes in most files are just converting existing asset_server.load() to the new
FontRef
.Closes #5515