`FontAtlasSet` is broken

[guest-twenty-one]

trafficstars

Bevy version

0.9.0

What went wrong

Minimal repro:

const MAX_ATLASES: usize = 1;
const DYNAMIC_SIZE: bool = false;
const TEXT: &str = "";

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .insert_resource(TextSettings {
            max_font_atlases: NonZeroUsize::new(MAX_ATLASES).unwrap(),
            allow_dynamic_font_size: DYNAMIC_SIZE,
        })
        .add_startup_system(setup)
        .run();
}

fn setup(mut commands: Commands, asset_server: ResMut<AssetServer>) {
    commands.spawn(Camera2dBundle::default());

    commands.spawn(TextBundle::from_sections([TextSection::new(
        TEXT,
        TextStyle {
            font: asset_server.load("fonts/sharetech-mono-regular.ttf"),
            font_size: 24.0,
            color: Color::RED,
        },
    )]));
}

Consider the following configurations:

const MAX_ATLASES: usize = 1;
const DYNAMIC_SIZE: bool = false;
const TEXT: &str = "AB"; // any text with 2 or more distinct characters

the application panics with:

'Fatal error when processing text: exceeded 1 available TextAltases for font. This can be caused by using an excessive number of font sizes or ui scaling...

even though it's using only one font size.

const MAX_ATLASES: usize = 2;
const DYNAMIC_SIZE: bool = true;
const TEXT: &str = "AB"; // any text with `MAX_ATLASES or more` distinct characters

the application panics with:

thread 'main' panicked at 'called Option::unwrap() on a None value', crates/bevy_ui/src/render/mod.rs:362:22

const MAX_ATLASES: usize = 1;
const DYNAMIC_SIZE: bool = true;
const TEXT: &str = "A"; // any non-empty text

the application doesn't panic, but it enters an endless loop.

I gave it a quick glance, and it seems that these lines: https://github.com/bevyengine/bevy/blob/6763b31479246afddecbe4e717e138d4e9653dca/crates/bevy_text/src/font_atlas_set.rs#L57-L70 https://github.com/bevyengine/bevy/blob/6763b31479246afddecbe4e717e138d4e9653dca/crates/bevy_text/src/font_atlas_set.rs#L85

shouldn't be executed unconditionally, but only if the font with the given size isn't already present in FontAtlasSet (1st and 2nd example).

Also, the extra - 1 on this line: https://github.com/bevyengine/bevy/blob/6763b31479246afddecbe4e717e138d4e9653dca/crates/bevy_text/src/font_atlas_set.rs#L65 seems to be the cause of the endless loop in the third example.

[alice-i-cecile]

Great bug report, thank you.

[guest-twenty-one]

Also, it seems there's a fundamental flaw with the eviction queue implementation:

• if FontAtlasSet is full, the oldest FontAtlas from the queue is picked for removal
• whether that FontAtlas is still being used or not by another Text node is not taken into consideration at all
• if it happens that another Text node is still using it, the application will panic (the same panic as in the 2nd example above)

This makes usage of TextSettings with allow_dynamic_font_size: true with the current implementation completely unpredictable and should probably be avoided.

[rparrett]

Additionally, when closing a window that has a scale factor, there's a single frame where there is no longer a window / scale factor and the default is used so new font atlases are created at a different size. (could be platform specific, testing on MacOS)

This can currently cause a panic when closing the window.

If we fix this by changing to a warning, we should open a separate issue for this.