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

Expose loading system fonts #14150

Closed
wants to merge 10 commits into from
Closed

Conversation

Dimchikkk
Copy link
Contributor

Objective

Expose cosmic-text functionality to load system fonts.

Solution

Add load_system_fonts to TextPlugin struct.

Testing

cargo r --example text --release by setting load_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 with load_system_fonts field.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Text Rendering and layout for characters S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 7, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Jul 7, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Jul 7, 2024
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();
Copy link
Member

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.

Copy link
Member

@alice-i-cecile alice-i-cecile left a 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?

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 14, 2024
@Dimchikkk
Copy link
Contributor Author

Dimchikkk commented Jul 15, 2024

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:

  • load single font from path, from bytes, etc
  • load fonts from dir
  • load all system fonts (basically the same as load fonts from dir but from specific dir where fonts are expected on platform)

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.

Copy link
Member

@alice-i-cecile alice-i-cecile left a 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.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 15, 2024
///
/// 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,
Copy link
Member

@alice-i-cecile alice-i-cecile Jul 15, 2024

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" :)

Copy link
Contributor Author

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].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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();
Copy link
Member

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") {
Copy link
Member

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 {
Copy link
Member

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.

@janhohenheim
Copy link
Member

Looks mostly good, thanks. Just want to get clarification on some things and rename the fn new :)

@bas-ie bas-ie added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 30, 2024
@alice-i-cecile alice-i-cecile modified the milestones: 0.15, 0.16 Oct 8, 2024
@alice-i-cecile
Copy link
Member

#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Text Rendering and layout for characters C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants