update_text2d_layout creates new font atlases when the primary window is closed

[ickshonpe]

Objective

Necessary conditions:

• Scale factor != 1
• Text is being displayed with Text2d
• The primary window is closed on a frame where the text or text's bounds are modified.

Then when update_text2d_layout runs, it finds no primary window and assumes a scale factor of 1.
The previous scale_factor was not equal to 1 and the text pipeline's old font atlases were created for a non-1 scale factor, so it creates new font atlases even though the app is closing.

The bug was first identified in #6666

Minimal Example

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                present_mode: bevy::window::PresentMode::Immediate,
                ..Default::default()
            }),
            ..default()
        }))
        .insert_resource(UiScale { scale: std::f64::consts::PI })
        .add_startup_system(setup)
        .add_system(update)
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn(Text2dBundle {
        text: Text {
            sections: (0..10).map(|i| TextSection {
                value: i.to_string(),
                style: TextStyle {
                    font: asset_server.load("fonts/FiraSans-Bold.ttf"),
                    font_size: (10 + i) as f32,
                    color: Color::WHITE,
                }
            }).collect(),
            ..Default::default()            
        },
        ..Default::default()
    });
}

fn update(mut text: Query<&mut Text>) {
    for mut text in text.iter_mut() {
        text.set_changed();
    }
}

Output

On closing the window you'll see the warning (if you don't, increase the number of text sections):

WARN bevy_text::glyph_brush: warning[B0005]: Number of font atlases has exceeded the maximum of 16. Performance and memory usage may suffer.

The app should only create font atlases on startup, but it doesn't display this warning until after you close the window

Solution

Skip update_text_layout when there is no primary window.

Changelog

• If no primary window is found, skip update_text2d_layout.
• Added a Local flag skipped to update_text2d_layout. This should ensure there are no edge cases where text might not get drawn at all.

[ickshonpe]

The UI has the same problem, but it might be more complicated and I need to test some things so there will be a separate PR.

[rparrett]

I forgot all about this bug.

This fix makes sense to me, but I'd like to follow up after #9676 is resolved.

[BenjaminBrienen]

@ickshonpe can you rebase this?

[ickshonpe]

Yep going to concentrate on all my old PRs the next couple of days and get them merged or closed.

[ickshonpe]

@ickshonpe can you rebase this?

Rebased. The warning has been removed but the bug is still there and this fix still works.

[rparrett]

Looking at this again, and maybe it's not correct to skip text when there's no window? This would break text for headless rendering scenarios, right?

Maybe instead the scale factor could first fall back to the previous scale factor before falling back to 1.0.

Something like this?

Expand Diff

diff --git a/crates/bevy_text/src/text2d.rs b/crates/bevy_text/src/text2d.rs
index 893f127b8..28688591e 100644
--- a/crates/bevy_text/src/text2d.rs
+++ b/crates/bevy_text/src/text2d.rs
@@ -223,7 +223,7 @@ pub fn extract_text2d_sprite(
 /// It does not modify or observe existing ones.
 #[allow(clippy::too_many_arguments)]
 pub fn update_text2d_layout(
-    mut last_scale_factor: Local<f32>,
+    mut last_scale_factor: Local<Option<f32>>,
     // Text items which should be reprocessed again, generally when the font hasn't loaded yet.
     mut queue: Local<EntityHashSet>,
     mut textures: ResMut<Assets<Image>>,
@@ -246,13 +246,15 @@ pub fn update_text2d_layout(
     // TODO: Support window-independent scaling: https://github.com/bevyengine/bevy/issues/5621
     let scale_factor = windows
         .get_single()
+        .ok()
         .map(|window| window.resolution.scale_factor())
+        .or_else(|| *last_scale_factor)
         .unwrap_or(1.0);
 
     let inverse_scale_factor = scale_factor.recip();
 
-    let factor_changed = *last_scale_factor != scale_factor;
-    *last_scale_factor = scale_factor;
+    let factor_changed = *last_scale_factor != Some(scale_factor);
+    *last_scale_factor = Some(scale_factor);

[ickshonpe]

Looking at this again, and maybe it's not correct to skip text when there's no window? This would break text for headless rendering scenarios, right?

Yep that's definitely an improvement.

I think Text2d is a bit broken though anyway for anything not the primary window. It looks like it will be really annoying to get it to work properly like storing multiple versions of each font and computing multiple different text layouts for each render target with a different scale factor.