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: use try_insert instead of insert in bevy_ui to prevent panics when despawning ui nodes #13000

Merged

Conversation

MarcoMeijer
Copy link
Contributor

@MarcoMeijer MarcoMeijer commented Apr 16, 2024

Objective

Sometimes when despawning a ui node in the PostUpdate schedule it panics. This is because both a despawn command and insert command are being run on the same entity.

See this example code:

use bevy::{prelude::*, ui::UiSystem};

#[derive(Resource)]
struct SliceSquare(Handle<Image>);

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_systems(Update, create_ui)
        .add_systems(PostUpdate, despawn_nine_slice.after(UiSystem::Layout))
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn(Camera2dBundle::default());
    commands.insert_resource(SliceSquare(asset_server.load("textures/slice_square.png")));
}

fn create_ui(mut commands: Commands, slice_square: Res<SliceSquare>) {
    commands.spawn((
        NodeBundle {
            style: Style {
                width: Val::Px(200.),
                height: Val::Px(200.),
                ..default()
            },
            background_color: Color::WHITE.into(),
            ..default()
        },
        UiImage::new(slice_square.0.clone()),
        ImageScaleMode::Sliced(TextureSlicer {
            border: BorderRect::square(220.),
            center_scale_mode: SliceScaleMode::Stretch,
            sides_scale_mode: SliceScaleMode::Stretch,
            max_corner_scale: 1.,
        }),
    ));
}

fn despawn_nine_slice(mut commands: Commands, mut slices: Query<Entity, With<ImageScaleMode>>) {
    for entity in slices.iter_mut() {
        commands.entity(entity).despawn_recursive();
    }
}

This code spawns a UiNode with a sliced image scale mode, and despawns it in the same frame. The bevy_ui::texture_slice::compute_slices_on_image_change system tries to insert the ComputedTextureSlices component on that node, but that entity is already despawned causing this error:

error[B0003]: Could not insert a bundle (of type `bevy_ui::texture_slice::ComputedTextureSlices`) for entity Entity { index: 2, generation: 3 } because it doesn't 
exist in this World. See: https://bevyengine.org/learn/errors/#b0003
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic when applying buffers for system `bevy_ui::texture_slice::compute_slices_on_image_change`!
Encountered a panic in system `bevy_ecs::schedule::executor::apply_deferred`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!

Note that you might have to run the code a few times before this error appears.

Solution

Use try_insert instead of insert for non critical inserts in the bevy_ui crate.

Some notes

In a lot of cases it does not makes much sense to despawn ui nodes after the layout system has finished. Except maybe if you delete the root ui node of a tree. I personally encountered this issue in bevy 0.13.2 with a system that was running before the layout system. And in 0.13.2 the compute_slices_on_image_change system was also running before the layout system. But now it runs after the layout system. So the only way that this bug appears now is if you despawn ui nodes after the layout system. So I am not 100% sure if using try_insert in this system is the best option. But personally I still think it is better then the program panicking.

However the update_children_target_camera system does still run before the layout system. So I do think it might still be able to panic when ui nodes are despawned before the layout system. Though I haven't been able to verify that.

@NthTensor NthTensor added A-ECS Entities, components, systems, and events P-Crash A sudden unexpected crash labels Apr 17, 2024
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 18, 2024
@alice-i-cecile
Copy link
Member

Part of the broader efforts in #12660.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 18, 2024
@MarcoMeijer
Copy link
Contributor Author

@alice-i-cecile Can you maybe put it in the merge queue again? It seems like it got errors while installing some dependencies, which has nothing to do with the changes I made.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 19, 2024
Merged via the queue into bevyengine:main with commit 2ad27ee Apr 19, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events P-Crash A sudden unexpected crash S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants