-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Expose loading system fonts #14150
Expose loading system fonts #14150
Conversation
Self(cosmic_text::FontSystem::new_with_locale_and_db(locale, db)) | ||
let mut db = cosmic_text::fontdb::Database::new(); | ||
if load_system_fonts { | ||
db.load_system_fonts(); |
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 would really prefer if the loading strategy was more configurable: this should be easier to integrate into custom asset loading states for example.
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 this is important to include, but the design and documentation needs more work. How do system fonts tie into Bevy's asset loading strategies? How can you look for a specific system font? How does font fallback function?
Thanks for the nice code review! cosmic-text uses fontdb crate. fontdb crate has several methods to load fonts:
fontdb has query function that allows to look for font, it returns None if font wasn't loaded. cosmic-text has logic for handling font fallback - the algo seems pretty simple - if some glyph is missing it iterates over fallback fonts (that should be loaded prior) and tries to find the desired glyph. cosmic-text has custom font fallback implementation re-using some static fallback list from browsers: https://github.com/pop-os/cosmic-text/tree/main/src/font/fallback So, my current PR just added a tiny bit for loading system fonts by calling load system fonts function from fontdb crate. The goal was to cover scenario when some glyph is missing cosmic-text will try to load from fallback list... just an example use case - emoji gonna work even without loading emoji font explicitly. Querying specific fonts, setting font families, load from custom dirs need more design work but this small change is a small but quick win. |
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.
Alright, I think I'm okay with this as an incremental next step. Some more suggestions on docs before we move forward though, to make it easier for others to modify and learn.
/// | ||
/// For details on which directories are considered on each platform, | ||
/// refer to the source code of [`fontdb::Database::load_system_fonts`](https://docs.rs/fontdb/latest/fontdb/struct.Database.html#method.load_system_fonts). | ||
pub load_system_fonts: bool, |
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.
As a certain "Dimchikk" on Discord suggested, this would be more accurately described as "fallback to system fonts" :)
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.
After giving it more thought and reading @TotalKrill's contemplations, I decided to extend the documentation for this member variable to mention its primary use case rather than renaming it.
@@ -76,7 +76,18 @@ use bevy_sprite::SpriteSystem; | |||
/// When the `bevy_text` feature is enabled with the `bevy` crate, this | |||
/// plugin is included by default in the `DefaultPlugins`. | |||
#[derive(Default)] | |||
pub struct TextPlugin; | |||
pub struct TextPlugin { | |||
/// Attempts to load system fonts if [true]. |
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.
/// Attempts to load system fonts if [true]. | |
/// Attempts to load system fonts if `true`. |
Self(cosmic_text::FontSystem::new_with_locale_and_db(locale, db)) | ||
let mut db = cosmic_text::fontdb::Database::new(); | ||
if load_system_fonts { | ||
db.load_system_fonts(); |
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 assume this just noops on Wasm and Android? Or what happens there? Do we get a warning?
@@ -26,14 +32,21 @@ struct FpsText; | |||
struct ColorText; | |||
|
|||
fn setup(mut commands: Commands, asset_server: Res<AssetServer>) { | |||
// Emojis won't be rendered properly on wasm32 because the default loaded font doesn't support them, and loading system fonts isn't supported on this platform. | |||
let text_content = if cfg!(target_arch = "wasm32") { |
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.
File system stuff usually doesn't work on Android either, FYI
@@ -53,6 +56,23 @@ pub struct TextPipeline { | |||
swash_cache: SwashCache, | |||
} | |||
|
|||
impl TextPipeline { | |||
/// Create text pipeline providing a flag for loading system fonts | |||
pub fn new(load_system_fonts: bool) -> Self { |
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.
TextPipeline::new(true)
is very unreadable imo. Rename this to something like with_load_system_fonts
.
Looks mostly good, thanks. Just want to get clarification on some things and rename the |
#16365 looks a bit more active; I'm going to close this out and try and consolidate work there. Your review on that would be appreciated |
Objective
Expose
cosmic-text
functionality to load system fonts.Solution
Add
load_system_fonts
toTextPlugin
struct.Testing
cargo r --example text --release
by settingload_system_fonts
to true emoji is rendered correctly:20240705113308386.mp4
Migration Guide
TextPlugin started to have a field, so in places where TextPlugin was used have to use
bevy_text::TextPlugin::default()
or create a struct withload_system_fonts
field.