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

add a default font #8445

Merged
merged 4 commits into from
Apr 21, 2023
Merged

add a default font #8445

merged 4 commits into from
Apr 21, 2023

Conversation

mockersf
Copy link
Member

Objective

  • Have a default font

Solution

  • Add a font based on FiraMono containing only ASCII characters and use it as the default font
  • It is behind a feature default_font enabled by default
  • I also updated examples to use it, but not UI examples to still show how to use a custom font

Changelog

  • If you display text without using the default handle provided by TextStyle, the text will be displayed

@mockersf mockersf added A-Assets Load files from disk to use for things like images, models, and sounds A-UI Graphical user interfaces, styles, layouts, and widgets labels Apr 19, 2023
@IceSentry
Copy link
Contributor

Is there any kind of licensing concerns when distributing a font like that?

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 an important enough UX win that I'm fine to eat the cost of having this on by default.

Feature flag means that in real projects this is actually zero cost. This is by far the simplest solution to this problem, and will make prototyping and examples much quicker.

@alice-i-cecile
Copy link
Member

The font appears to already be listed in https://github.com/bevyengine/bevy/blob/main/CREDITS.md. The license for them does not appear to impose any relevant restrictions, either to us or to downstream users.

@UkoeHB
Copy link
Contributor

UkoeHB commented Apr 20, 2023

Can you configure the default font for your entire app?

@alice-i-cecile
Copy link
Member

No: this is baked into the source code of Bevy (when using the provided feature flag). Better theming support is also important, but this feature is strictly intended for easy debugging and teaching.

@nicopap
Copy link
Contributor

nicopap commented Apr 20, 2023

@mockersf
Copy link
Member Author

mockersf commented Apr 20, 2023

Can you configure the default font for your entire app?

Yes, if you set the default font handle to another font, it will be used by default in your app. But then that doesn't depend on this PR, this already works...

Something like:

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_systems(
            Update,
            set_default_font.run_if(resource_exists::<FontHandle>()),
        )
        .run();
}

#[derive(Resource)]
struct FontHandle(Handle<Font>);

fn set_default_font(
    mut commands: Commands,
    mut fonts: ResMut<Assets<Font>>,
    font_handle: Res<FontHandle>,
) {
    if let Some(font) = fonts.remove(&font_handle.0) {
        fonts.set_untracked(TextStyle::default().font, font);
        commands.remove_resource::<FontHandle>();
    }
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    let font = asset_server.load("fonts/FiraSans-Bold.ttf");
    commands.insert_resource(FontHandle(font));
}

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 20, 2023
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 20, 2023
crates/bevy_text/src/text.rs Outdated Show resolved Hide resolved
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Generally in favor of this. Glad you could slim the fira font down so much!

crates/bevy_text/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_text/src/lib.rs Show resolved Hide resolved
@@ -67,6 +73,10 @@ pub enum YAxisOrientation {
BottomToTop,
}

#[cfg(feature = "default_font")]
pub const DEFAULT_FONT_HANDLE: HandleUntyped =
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @UkoeHB: I think this should not be behind a feature flag so other people can set the default font (without needing to include the fira default font). I think "default fonts" should be supported without flags and the current default_font feature should be default_font_fira_sans or something similar.

@nicoburns
Copy link
Contributor

What was the motivation behind choosing a monospace font as the default? Don't most people typically want to use proportional fonts?

@cart
Copy link
Member

cart commented Apr 20, 2023

Debug UIs: more likely to prefer monospace (but could probably get away with proportional)
Short text labels in game: probably proportional
Long form text: almost certainly proportional

I suspect "real game uis" will most often want proportional, but "real game uis" will also probably want their own fonts.
I think we might actually want proportional here. If the rationale is "provide a good font for debug uis" we should do a side-by-side comparison.

@alice-i-cecile
Copy link
Member

Oh right: proportional fonts will have additional bugs, so at least internally we probably want to use a proportional one for testing.

@mockersf
Copy link
Member Author

What was the motivation behind choosing a monospace font as the default? Don't most people typically want to use proportional fonts?

For a placeholder / debug font, it seemed more natural to me

@cart
Copy link
Member

cart commented Apr 21, 2023

I think proportional will probably have my vote, but I also dont think we need to make that call now. Once comments are resolved I see no reason not to merge this. We can always swap out later.

@nicoburns
Copy link
Contributor

I think proportional will probably have my vote, but I also dont think we need to make that call now. Once comments are resolved I see no reason not to merge this. We can always swap out later.

Same here on both counts. Proportional seems like a better default to me (but I don't feel super-strongly about this), and in any case this is easy to change later and need not block this PR.

@IceSentry
Copy link
Contributor

Personally, I saw this as more of a debug tool and I prefer mono for that.

Maybe have a default_font_mono and default_font_proportional feature? I don't think this is a great solution, but it could work.

@mockersf
Copy link
Member Author

Another reason for mono: I may have messed up, but my "small" FiraSans font is 39kB while FiraMono ends up at 19kB

crates/bevy_text/src/text.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Apr 21, 2023

Another reason for mono: I may have messed up, but my "small" FiraSans font is 39kB while FiraMono ends up at 19kB

Hmm yeah thats a big difference. Lets stick with mono for now and we can make a call later

@cart cart added this pull request to the merge queue Apr 21, 2023
Merged via the queue into bevyengine:main with commit e0e5f3a Apr 21, 2023
@superdump
Copy link
Contributor

I saw this and immediately thought ‘great! Now we can implement debug plugins with UI overlays!’

mockersf pushed a commit that referenced this pull request Apr 25, 2023
# Objective

- Fixes #8484

## Solution

Since #8445 fonts need to register a debug asset, otherwise the
`debug_asset_server` feature doesn't work. This adds the debug asset
registration
@aevyrie
Copy link
Member

aevyrie commented May 10, 2023

Also in favor of mono. Whenever I need debug overlays, I need to render numbers, and mono is necessary to make those overlays useful.

  • Mono: easy text alignment for comparing numeric values, numbers don't jump around, longer text isn't as nice to look at.
  • Proportional: text alignment is impossible for numeric values, text looks nicer.

I think this makes mono much better fit for purpose.

@@ -97,6 +97,8 @@ bevy_render = ["dep:bevy_render", "bevy_scene?/bevy_render"]
# Enable assertions to check the validity of parameters passed to glam
glam_assert = ["bevy_math/glam_assert"]

default_font = ["bevy_text?/default_font"]
Copy link
Contributor

Choose a reason for hiding this comment

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

@mockersf is that ? a typo or a special syntax I don't know? 😊

Copy link
Member Author

@mockersf mockersf Jun 21, 2023

Choose a reason for hiding this comment

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

Not a typo! https://doc.rust-lang.org/cargo/reference/features.html#dependency-features

it means "enable default_font feature of optional crate bevy_text only if the crate is enabled from another feature, otherwise just ignore it"

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet, I didn't know about that feature, quite useful. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-UI Graphical user interfaces, styles, layouts, and widgets S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants