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

fix font atlas overflow #495

Merged
merged 4 commits into from Sep 16, 2020
Merged

fix font atlas overflow #495

merged 4 commits into from Sep 16, 2020

Conversation

verzuz
Copy link
Contributor

@verzuz verzuz commented Sep 15, 2020

This should fix the issue of overflowing (and panicing) font_atlasses.

@karroffel karroffel added A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior C-Crash A sudden unexpected crash A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets labels Sep 15, 2020
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.

In general this looks good to me. Just a few small nit-picks. Thanks!

@@ -55,7 +57,13 @@ impl FontAtlasSet {
let font_atlas = self
Copy link
Member

Choose a reason for hiding this comment

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

nit: lets rename this font_atlases now that its a vec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure - makes sense

if let Some(outlined_glyph) = scaled_font.outline_glyph(glyph.clone()) {
let glyph_texture = Font::get_outlined_glyph_texture(outlined_glyph);
font_atlas.add_char(textures, texture_atlases, character, &glyph_texture);
let add_char_to_fontatlas = |atlas: &mut FontAtlas| -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

nit: as long as it is, naming conventions dictate that this should be add_char_to_font_atlas :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed - although I hate putting it into a Fn ... but clippy complained :-(

Copy link
Member

Choose a reason for hiding this comment

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

bawhaha yeah sometimes clippy is overly opinionated imo.

character,
&glyph_texture,
) {
panic!("could not add character to newly created fontatlas");
Copy link
Member

Choose a reason for hiding this comment

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

nit: lets use type name casing here: FontAtlas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing

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

cart commented Sep 15, 2020

Awesome. This is good to go. Thanks again!

@cart cart merged commit d4ab2f4 into bevyengine:master Sep 16, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
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-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Crash A sudden unexpected crash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants