Adds basic support for working with system fonts#16365
Adds basic support for working with system fonts#16365bushrat011899 wants to merge 10 commits intobevyengine:mainfrom
Conversation
bushrat011899
added
C-Feature
|
Awesome! I swapped to this branch locally with my playground example and every locale now displays without the need to load any external font files 🎊 I suppose this does increase the need for users to have the ability to control |
|
If it's enabled by default, add an example that shows how to disable this feature. Reasoning is simple: if an app is shipped with a font, why waste time loading all system fonts? IMO add something like: So user will have simple way, and control when to load. |
I've left it is a feature that's disabled by default. To enable system fonts, you enable the |
|
I've just noticed #14150, was that effort abandoned? |
|
I never saw that! Looks like it wasn't linked to the original issue. I'm happy to incorporate changes from that PR, or close this out in favour of that one, I'll leave that up to the reviewers. |
|
Most likely not a Bevy issue, but the example hangs on my macbook when it eventually tries to load edit: I think this reproduces in bare cosmic, but was fixed in an unreleased commit. I'll make an issue to beg for a release over there. |
Also added a mapping between `AssetId<Font>` and `fontdb::ID` to avoid creating new assets and handles when it's not required.
|
nit: IMO, the current example could be made more practical for users seeking to use this feature (and also for automated testing). It would be nice to show a more common scenario than cycling through every font one at a time, and to demonstrate how to react to the system fonts becoming available. |
# Objective Upgrade to `cosmic-text` 0.13 https://github.com/pop-os/cosmic-text/releases This should include some performance improvements for layout and system font loading. ## Solution Bump version, fix the one changed API. ## Testing Tested some examples locally, will invoke the example runner. ## Layout Perf ||main fps|cosmic-13 fps| |-|-|-| |many_buttons --recompute-text --no-borders|6.79|9.42 🟩 +38.7%| |many_text2d --no-frustum-culling --recompute|3.19|4.28 🟩 +34.0%| |many_glyphs --recompute-text|7.09|11.17 🟩 +57.6%| |text_pipeline |140.15|139.90 ⬜ -0.2%| ## System Font Loading Perf I tested on macOS somewhat lazily by adding the following system to the `system_fonts` example from #16365. <details> <summary>Expand code</summary> ```rust fn exit_on_load( mut reader: EventReader<bevy::text::SystemFontsAvailable>, mut writer: EventWriter<AppExit>, ) { for _evt in reader.read() { writer.write(AppExit::Success); } } ``` </details> And running `hyperfine 'cargo run --release --example system_fonts --features=system_font'`. The results were nearly identical with and without this PR cherry-picked there. Co-authored-by: Alice Cecile <[email protected]>
| if keyboard_input.just_pressed(KeyCode::Space) { | ||
| let mut name = default(); | ||
|
|
||
| // The primary way to find a font is through FontLibrary::find, | ||
| // which iterates over all loaded fonts, and returns the first that | ||
| // satisfies the provided predicate. | ||
| // In this example, we're just looking for any font we haven't already | ||
| // shown. | ||
|
|
||
| let Some(font) = fonts.find(|font| { | ||
| let family_name = font.families[0].0.as_str(); | ||
|
|
||
| if used.contains(family_name) { | ||
| return false; | ||
| } | ||
|
|
||
| used.insert(Box::from(family_name)); | ||
| name = String::from(family_name); | ||
| true | ||
| }) else { | ||
| *query.0 = Text2d::new("No more fonts to show!"); | ||
| return; | ||
| }; | ||
|
|
||
| *query.0 = Text2d::new(name); | ||
| query.1.font = font; | ||
| } |
There was a problem hiding this comment.
Nitpick - I think there's an idiomatic way to avoid some returns here.
| if keyboard_input.just_pressed(KeyCode::Space) { | |
| let mut name = default(); | |
| // The primary way to find a font is through FontLibrary::find, | |
| // which iterates over all loaded fonts, and returns the first that | |
| // satisfies the provided predicate. | |
| // In this example, we're just looking for any font we haven't already | |
| // shown. | |
| let Some(font) = fonts.find(|font| { | |
| let family_name = font.families[0].0.as_str(); | |
| if used.contains(family_name) { | |
| return false; | |
| } | |
| used.insert(Box::from(family_name)); | |
| name = String::from(family_name); | |
| true | |
| }) else { | |
| *query.0 = Text2d::new("No more fonts to show!"); | |
| return; | |
| }; | |
| *query.0 = Text2d::new(name); | |
| query.1.font = font; | |
| } | |
| if keyboard_input.just_pressed(KeyCode::Space) { | |
| let mut name = default(); | |
| // The primary way to find a font is through FontLibrary::find, | |
| // which iterates over all loaded fonts, and returns the first that | |
| // satisfies the provided predicate. | |
| // In this example, we're just looking for any font we haven't already | |
| // shown. | |
| let next_font = fonts.find(|font|{ | |
| let family_name = font.families[0].0.as_str(); | |
| if used.contains(family_name) { | |
| false | |
| } else { | |
| used.insert(Box::from(family_name)); | |
| name = String::from(family_name); | |
| true | |
| } | |
| }); | |
| match next_font { | |
| Some(font) => { | |
| *query.0 = Text2d::new(name); | |
| query.1.font = font; | |
| }, | |
| None => { | |
| *query.0 = Text2d::new("No more fonts to show!"); | |
| }, | |
| } | |
| } |
There was a problem hiding this comment.
Actually I think for the end, clippy usually prefers if let ... else also but I did this in github comments not my editor XD
There was a problem hiding this comment.
An example alternate approach that I think ends up a lot more readable, if as in my other comment more info was returned from .find
if keyboard_input.just_pressed(KeyCode::Space) {
// The primary way to find a font is through FontLibrary::find,
// which iterates over all loaded fonts, and returns the first that
// satisfies the provided predicate.
// In this example, we're just looking for any font we haven't already
// shown.
let next_font = fonts.find(|font|{
let family_name = font.families[0].0.as_str();
used.contains(family_name)
});
if let Some((handle, face_info)) = next_font {
let family_name = face_info.families[0].0.as_str();
*query.0 = Text2d::new(family_name);
query.1.font = handle;
} else {
*query.0 = Text2d::new("No more fonts to show!");
}
}| impl FontLibrary<'_> { | ||
| /// Find a [`Font`] based on the provided criteria. | ||
| /// You are given access to the font's [`FaceInfo`] to aid with selection. | ||
| pub fn find(&mut self, mut f: impl FnMut(&FaceInfo) -> bool) -> Option<Handle<Font>> { |
There was a problem hiding this comment.
It might be useful to return the FaceInfo as part of the Option also- noticed in the example you had to use a mutable local variable to store data from the FaceInfo. Maybe something like Option<(Handle<Font>, &FaceInfo)>?
There was a problem hiding this comment.
Also worth noting that this function signature, through the predicate, "leaks" the abstraction of a cosmic_text::fontdb::FaceInfo. That might be intended and fine, but worth pointing out if it is preferable to keep the bevy interface more opaque (which could make it easier to replace cosmic_text in the future if necessary)
|
Any chance this can be added to bevy 0.17 or 0.18 milestones? Really looking forward to this functionality and hoping it's still on the roadmap. |
alice-i-cecile
added
M-Release-Note
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
10 participants
Objective
cosmic_textfont fallback causes inconsistent text rendering results #16354Solution
system_font, which will instructcosmic_textto begin loading system fonts on startup.FontLibrary, which allows for convenient searching of all loaded fonts, returning aHandle<Font>if you have found a suitable candidate.system_fonts, which demonstrates searching for and using system fonts.Testing
system_fontsexampleShowcase
Users now have access to the
FontLibrarysystem parameter, which allows searching for available fonts. This helps with using the newsystem_fontsfeature, which will load system fonts at runtime.In the below snippets,
fontsis of typeFontLibraryNotes
Loading system fonts increases startup time by a few seconds. I suspect this is platform dependent, and is largely out of our control.The startup cost is now negligible (I cannot measure the difference, much less than a second), since moving system font loading onto a background task. I do believe it's worth keeping this behind a feature gate as it's already organised this way, and there may be a performance cost on some platforms. More options for the user is better IMO.system_fontsfeature is enabled on a platformcosmic_textdoes not support, it is simply a no-op, making the feature safe to enable in all cases.FontLibrarysystem param could use some more methods, such as an iterator. I have deliberately only exposed afindmethod since we need to do some work with theAssets<Font>to ensure it's available for theTextPipeline.Fonttype assumes every font will have binary data associated with it (thedatafield). For system fonts this is not the case. I'm simply inserting no data (empty slice) for these fonts. It might be prudent to updateFontto be anenuminstead to make this more clear.