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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Limit FontAtlasSets #5708

Closed
wants to merge 15 commits into from
Closed

[Merged by Bors] - Limit FontAtlasSets #5708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
17b3c27
Limit FontAtlasSets
xtr3m3nerd Aug 16, 2022
6da90e1
format fixes
xtr3m3nerd Aug 16, 2022
fa65bb5
Resolve clippy errors
xtr3m3nerd Aug 16, 2022
ec9ca46
format fixes again
xtr3m3nerd Aug 16, 2022
c87df9e
Update crates/bevy_text/src/error.rs
xtr3m3nerd Aug 16, 2022
744f747
import from resolved module
xtr3m3nerd Aug 16, 2022
8b438f1
Merge branch 'font_fix' of https://github.com/xtr3m3nerd/bevy into fo…
xtr3m3nerd Aug 16, 2022
d6f7476
Add LRU Cache for managing font_atlas_set
xtr3m3nerd Aug 23, 2022
1b0fbea
Add documentation to allow_dynamic_font_size
xtr3m3nerd Aug 23, 2022
303a285
improve error message and fix documentation
xtr3m3nerd Aug 26, 2022
16373e6
Merge branch 'main' into font_fix
xtr3m3nerd Aug 30, 2022
b2fc494
Merge branch 'main' of https://github.com/bevyengine/bevy into font_fix
xtr3m3nerd Sep 6, 2022
81c1ac5
Change max_font_atlases to NonZeroUsize
xtr3m3nerd Sep 6, 2022
29b3571
Merge branch 'font_fix' of https://github.com/xtr3m3nerd/bevy into fo…
xtr3m3nerd Sep 6, 2022
8aae01a
Merge branch 'main' of https://github.com/bevyengine/bevy into font_fix
xtr3m3nerd Sep 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/bevy_text/src/error.rs
View file
Open in desktop
Expand Up @@ -7,4 +7,6 @@ pub enum TextError {
NoSuchFont,
#[error("failed to add glyph to newly-created atlas {0:?}")]
FailedToAddGlyph(GlyphId),
#[error("exceeded {0:?} available TextAltases for font. This can be caused by using an excessive number of font sizes. If you are changing font sizes dynamically consider using Transform::scale to modify the size. If you need more font sizes modify TextSettings.max_font_atlases." )]
ExceedMaxTextAtlases(usize),
}
28 changes: 26 additions & 2 deletions crates/bevy_text/src/font_atlas_set.rs
View file
Open in desktop
@@ -1,4 +1,4 @@
use crate::{error::TextError, Font, FontAtlas};
use crate::{error::TextError, Font, FontAtlas, TextSettings};
use ab_glyph::{GlyphId, OutlinedGlyph, Point};
use bevy_asset::{Assets, Handle};
use bevy_math::Vec2;
Expand All @@ -14,6 +14,7 @@ type FontSizeKey = FloatOrd;
#[uuid = "73ba778b-b6b5-4f45-982d-d21b6b86ace2"]
pub struct FontAtlasSet {
font_atlases: HashMap<FontSizeKey, Vec<FontAtlas>>,
queue: Vec<FontSizeKey>,
}

#[derive(Debug, Clone)]
Expand All @@ -26,6 +27,7 @@ impl Default for FontAtlasSet {
fn default() -> Self {
FontAtlasSet {
font_atlases: HashMap::with_capacity_and_hasher(1, Default::default()),
queue: Vec::new(),
}
}
}
Expand All @@ -50,7 +52,22 @@ impl FontAtlasSet {
texture_atlases: &mut Assets<TextureAtlas>,
textures: &mut Assets<Image>,
outlined_glyph: OutlinedGlyph,
text_settings: &TextSettings,
) -> Result<GlyphAtlasInfo, TextError> {
if !text_settings.allow_dynamic_font_size {
if self.font_atlases.len() >= text_settings.max_font_atlases.get() {
return Err(TextError::ExceedMaxTextAtlases(
text_settings.max_font_atlases.get(),
));
}
} else {
// Clear last space in queue to make room for new font size
while self.queue.len() >= text_settings.max_font_atlases.get() - 1 {
if let Some(font_size_key) = self.queue.pop() {
self.font_atlases.remove(&font_size_key);
}
}
}
let glyph = outlined_glyph.glyph();
let glyph_id = glyph.id;
let glyph_position = glyph.position;
Expand All @@ -65,6 +82,7 @@ impl FontAtlasSet {
Vec2::splat(512.0),
)]
});
self.queue.insert(0, FloatOrd(font_size));
let glyph_texture = Font::get_outlined_glyph_texture(outlined_glyph);
let add_char_to_font_atlas = |atlas: &mut FontAtlas| -> bool {
atlas.add_glyph(
Expand Down Expand Up @@ -106,11 +124,17 @@ impl FontAtlasSet {
}

pub fn get_glyph_atlas_info(
&self,
&mut self,
font_size: f32,
glyph_id: GlyphId,
position: Point,
) -> Option<GlyphAtlasInfo> {
// Move to front of used queue.
let some_index = self.queue.iter().position(|x| *x == FloatOrd(font_size));
if let Some(index) = some_index {
let key = self.queue.remove(index);
self.queue.insert(0, key);
}
self.font_atlases
.get(&FloatOrd(font_size))
.and_then(|font_atlases| {
Expand Down
11 changes: 9 additions & 2 deletions crates/bevy_text/src/glyph_brush.rs
View file
Open in desktop
Expand Up @@ -7,7 +7,7 @@ use glyph_brush_layout::{
FontId, GlyphPositioner, Layout, SectionGeometry, SectionGlyph, SectionText, ToSectionText,
};

use crate::{error::TextError, Font, FontAtlasSet, GlyphAtlasInfo, TextAlignment};
use crate::{error::TextError, Font, FontAtlasSet, GlyphAtlasInfo, TextAlignment, TextSettings};

pub struct GlyphBrush {
fonts: Vec<FontArc>,
Expand Down Expand Up @@ -43,6 +43,7 @@ impl GlyphBrush {
Ok(section_glyphs)
}

#[allow(clippy::too_many_arguments)]
pub fn process_glyphs(
&self,
glyphs: Vec<SectionGlyph>,
Expand All @@ -51,6 +52,7 @@ impl GlyphBrush {
fonts: &Assets<Font>,
texture_atlases: &mut Assets<TextureAtlas>,
textures: &mut Assets<Image>,
text_settings: &TextSettings,
) -> Result<Vec<PositionedGlyph>, TextError> {
if glyphs.is_empty() {
return Ok(Vec::new());
Expand Down Expand Up @@ -104,7 +106,12 @@ impl GlyphBrush {
.get_glyph_atlas_info(section_data.2, glyph_id, glyph_position)
.map(Ok)
.unwrap_or_else(|| {
font_atlas_set.add_glyph_to_atlas(texture_atlases, textures, outlined_glyph)
font_atlas_set.add_glyph_to_atlas(
texture_atlases,
textures,
outlined_glyph,
text_settings,
)
})?;

let texture_atlas = texture_atlases.get(&atlas_info.texture_atlas).unwrap();
Expand Down
23 changes: 22 additions & 1 deletion crates/bevy_text/src/lib.rs
View file
Open in desktop
Expand Up @@ -28,14 +28,34 @@ pub mod prelude {

use bevy_app::prelude::*;
use bevy_asset::AddAsset;
use bevy_ecs::schedule::ParallelSystemDescriptorCoercion;
use bevy_ecs::{schedule::ParallelSystemDescriptorCoercion, system::Resource};
use bevy_render::{RenderApp, RenderStage};
use bevy_sprite::SpriteSystem;
use bevy_window::ModifiesWindows;
use std::num::NonZeroUsize;

#[derive(Default)]
pub struct TextPlugin;

/// [`TextPlugin`] settings
#[derive(Resource)]
pub struct TextSettings {
xtr3m3nerd marked this conversation as resolved.
Show resolved Hide resolved
/// Maximum number of font atlases supported in a ['FontAtlasSet']
pub max_font_atlases: NonZeroUsize,
/// Allows font size to be set dynamically exceeding the amount set in max_font_atlases.
/// Note each font size has to be generated which can have a strong performance impact.
pub allow_dynamic_font_size: bool,
Copy link
Member

Choose a reason for hiding this comment

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

IMO this setting plus the one above should be refactored to a Option<NonZeroUsize>.

This reduces the number of fields to reason about, and ensures that users handle the "there is no limit" case correctly. Make invalid states unrepresentable and all that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So on this point I disagree that we should merge this into a single field, the reason for that is that in this implementation we do not and should not support a "there is no limit case".
The two use cases being supported here are:

  1. Panicking if the user surpasses the max_font_atlases limit (allow_dynamic_font_size == false | default behavior)
  2. Use a rotating buffer of the size max_font_atlases if the limit is exceeded (allow_dynamic_font_size == true | this behavior must be set by the user since it has a high performance cost associated with it)

In each case both fields are necessary to determine the behavior and the size of the buffer. I do agree that max_font_atlases should be changed to NonZeroUsize since there is no use case where we would want it to be 0. I have made a change to reflect that.

We should not support a "there is no limit" because ultimately there would leave no protection for the user from the memory leak and with these two settings they should be able to reach any desired behavior while still providing safeguards.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see! Okay, please feel free to resolve this comment then.

}

impl Default for TextSettings {
fn default() -> Self {
Self {
max_font_atlases: NonZeroUsize::new(16).unwrap(),
allow_dynamic_font_size: false,
}
}
}

impl Plugin for TextPlugin {
fn build(&self, app: &mut App) {
app.add_asset::<Font>()
Expand All @@ -46,6 +66,7 @@ impl Plugin for TextPlugin {
.register_type::<VerticalAlign>()
.register_type::<HorizontalAlign>()
.init_asset_loader::<FontLoader>()
.init_resource::<TextSettings>()
.insert_resource(TextPipeline::default())
.add_system_to_stage(
CoreStage::PostUpdate,
Expand Down
4 changes: 3 additions & 1 deletion crates/bevy_text/src/pipeline.rs
View file
Open in desktop
Expand Up @@ -11,7 +11,7 @@ use glyph_brush_layout::{FontId, SectionText};

use crate::{
error::TextError, glyph_brush::GlyphBrush, scale_value, Font, FontAtlasSet, PositionedGlyph,
TextAlignment, TextSection,
TextAlignment, TextSection, TextSettings,
};

#[derive(Default, Resource)]
Expand Down Expand Up @@ -49,6 +49,7 @@ impl TextPipeline {
font_atlas_set_storage: &mut Assets<FontAtlasSet>,
texture_atlases: &mut Assets<TextureAtlas>,
textures: &mut Assets<Image>,
text_settings: &TextSettings,
) -> Result<TextLayoutInfo, TextError> {
let mut scaled_fonts = Vec::new();
let sections = sections
Expand Down Expand Up @@ -103,6 +104,7 @@ impl TextPipeline {
fonts,
texture_atlases,
textures,
text_settings,
)?;

Ok(TextLayoutInfo { glyphs, size })
Expand Down
7 changes: 5 additions & 2 deletions crates/bevy_text/src/text2d.rs
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ use bevy_window::{WindowId, WindowScaleFactorChanged, Windows};

use crate::{
Font, FontAtlasSet, HorizontalAlign, Text, TextError, TextLayoutInfo, TextPipeline,
VerticalAlign,
TextSettings, VerticalAlign,
};

/// The calculated size of text drawn in 2D scene.
Expand Down Expand Up @@ -153,6 +153,7 @@ pub fn update_text2d_layout(
mut textures: ResMut<Assets<Image>>,
fonts: Res<Assets<Font>>,
windows: Res<Windows>,
text_settings: Res<TextSettings>,
mut scale_factor_changed: EventReader<WindowScaleFactorChanged>,
mut texture_atlases: ResMut<Assets<TextureAtlas>>,
mut font_atlas_set_storage: ResMut<Assets<FontAtlasSet>>,
Expand Down Expand Up @@ -190,13 +191,15 @@ pub fn update_text2d_layout(
&mut *font_atlas_set_storage,
&mut *texture_atlases,
&mut *textures,
text_settings.as_ref(),
) {
Err(TextError::NoSuchFont) => {
// There was an error processing the text layout, let's add this entity to the
// queue for further processing
queue.insert(entity);
}
Err(e @ TextError::FailedToAddGlyph(_)) => {
Err(e @ TextError::FailedToAddGlyph(_))
| Err(e @ TextError::ExceedMaxTextAtlases(_)) => {
panic!("Fatal error when processing text: {}.", e);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel like it needs to be a panic; aren't we just retiring the latest atlas when the max number is exceeded? We can recover from handling any number of text atlases at the cost of recomputation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So as mentioned in the comment above this change is supporting two different use cases:

  1. Panicking if the user surpasses the max_font_atlases limit (allow_dynamic_font_size == false | default behavior)
  2. Use a rotating buffer of the size max_font_atlases if the limit is exceeded (allow_dynamic_font_size == true | this behavior must be set by the user since it has a high performance cost associated with it)

The panic in the default behavior is important because it is notifying the user that the font system is being used in a way that it was not designed to. The point of setting allow_dynamic_font_size=true is to allow the user to bypass this error in which case it will start using the rotating buffer. If the user does this in a use case like the one in which I discovered this memory leak they would be experiencing a large performance drop since every frame a new text font atlas would be generated and then thrown out a couple frames later. In this situation the cache is effectively pointless since the font needed is generated each frame, but again this comes at a noticeable performance cost.

With the panic we are letting the user know they are doing something dangerous and perhaps should rethink how they are approaching the problem. If they absolutely need to generate an endless amount of fonts and are aware of the cost associated with doing that then they can set allow_dynamic_font_size=true, but I do not think in any way this should be the default behavior because that would just set up a user who has no understanding of how the font system works under the hood confused as to why their code is running so slow just to change font sizes.

To me the performance hit was visually noticeable with an immediate drop in frame rate. If it would help I can do some performance tests to get some hard numbers on this cost. This is the primary reason why I do not suggest having the rotating buffer be the default behavior. With the panic we give the user an opportunity to educate themselves on the implications of using the system in this manner before they choose to make the switch.

}
Ok(info) => {
Expand Down
7 changes: 5 additions & 2 deletions crates/bevy_ui/src/widget/text.rs
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ use bevy_ecs::{
use bevy_math::Vec2;
use bevy_render::texture::Image;
use bevy_sprite::TextureAtlas;
use bevy_text::{Font, FontAtlasSet, Text, TextError, TextLayoutInfo, TextPipeline};
use bevy_text::{Font, FontAtlasSet, Text, TextError, TextLayoutInfo, TextPipeline, TextSettings};
use bevy_window::Windows;

#[derive(Debug, Default)]
Expand Down Expand Up @@ -44,6 +44,7 @@ pub fn text_system(
mut textures: ResMut<Assets<Image>>,
fonts: Res<Assets<Font>>,
windows: Res<Windows>,
text_settings: Res<TextSettings>,
ui_scale: Res<UiScale>,
mut texture_atlases: ResMut<Assets<TextureAtlas>>,
mut font_atlas_set_storage: ResMut<Assets<FontAtlasSet>>,
Expand Down Expand Up @@ -116,13 +117,15 @@ pub fn text_system(
&mut *font_atlas_set_storage,
&mut *texture_atlases,
&mut *textures,
text_settings.as_ref(),
) {
Err(TextError::NoSuchFont) => {
// There was an error processing the text layout, let's add this entity to the
// queue for further processing
new_queue.push(entity);
}
Err(e @ TextError::FailedToAddGlyph(_)) => {
Err(e @ TextError::FailedToAddGlyph(_))
| Err(e @ TextError::ExceedMaxTextAtlases(_)) => {
panic!("Fatal error when processing text: {}.", e);
Copy link
Member

Choose a reason for hiding this comment

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

Ditto on this panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See response on other panic

}
Ok(info) => {
Expand Down