-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 #1790
Add a default font #1790
Conversation
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 is a really clean implementation of a simple idea. Should speed up prototyping nicely and streamline the beginner experience.
The only thing I think should be discussed more is what we want to use for the default font. Something with nice variants, like Recursive would be handy for being able to quickly test various font weights / styles.
The way this is implemented it will require the user to have the font in their If we bundle the font we also need to make sure the size increase isn't to big and there are no licensing issues. |
Can we mitigate the file size issue by using the feature gate on default assets like this PR does? |
right now it's just be part of the crate source basically, it's using the include_bytes macro. i think that means if the feature gate is disabled it won't be in the compiled exe i assumed there were no licensing issues because it's already in the repo, but i didn't actually check 👀
ya, although if we don't change the font in the examples, could also have more than one included font? also, i didn't change the areas that used firasans-medium just to make sure there's an example of loading a new font asset edit: i think this is the license https://github.com/bBoxType/FiraSans/blob/master/OFL.txt re: which font to use, one option is just stick w/ this since it's already used throughout the examples, and open another issue / rfc to decide on a font / set of fonts? |
6b6bb33
to
0acc9c0
Compare
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.
Good idea - I think we should have this.
use bevy_app::AppBuilder; | ||
use bevy_asset::Assets; | ||
|
||
pub(crate) fn load_default_font(app: &mut AppBuilder) { |
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.
Why is this not a startup system?
fn infotext_system(mut commands: Commands, asset_server: Res<AssetServer>) { | ||
let font = asset_server.load("fonts/FiraSans-Bold.ttf"); | ||
fn infotext_system(mut commands: Commands) { | ||
let font = DEFAULT_FONT_HANDLE.typed(); |
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 variable is no longer needed.
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.
if you want to change something back to that font youd need it
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.
Can replace its TextStyle
with a style using ..TextStyle::Default
?
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 guess? i don't really see why it matters / quad_handle works the same way, why not just leave it exposed
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 guess it's just that the mechanism is weird, especially the .typed()
thing
I'm not saying not to leave it exposed, I'm more suggesting not to use it in the 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.
ah, fair point
let world = app.world_mut(); | ||
let world_cell = world.cell(); | ||
let mut fonts = world_cell.get_resource_mut::<Assets<Font>>().unwrap(); | ||
let font_bytes = include_bytes!("assets/fonts/FiraSans-Bold.ttf"); |
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 adds ~500KB to the size of the bevy_text
package (currently lib.rs thinks it is 1.5MB, so an additional 1/3).
In theory, we should have this in a different crate, however because the amount of metadata needed for creating a crate anyway, it might not be worthwhile, if 95% of people leave the default font in.
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.
maybe a default assets crate? if other fonts / binary assets get added, could go in the same one?
I'm not yet completely sold on this implementation. This creates a situation where all Bevy apps come packaged with a default font, unless they specifically opt out. I expect that "cargo features" generally aren't something most people will think about when publishing their game, so this will likely result in some high percentage of Bevy apps including extra font bytes (90 percent? 99 percent?). Imo we should be as lean as possible by default. Some alternatives to discuss:
|
|
|
Closing this out as I think the other options (1-4) are all preferable. |
Adds a default font under a new "bevy_default_assets" feature flag. Used FiraSans-Bold since it's used in most of the examples as the default font. Resolves #1017