From 2ad27ee4ee5c0be3b25dd57e7f21d88f3a439f4a Mon Sep 17 00:00:00 2001 From: Marco Meijer <46689298+MarcoMeijer@users.noreply.github.com> Date: Fri, 19 Apr 2024 20:12:08 +0200 Subject: [PATCH] fix: use try_insert instead of insert in bevy_ui to prevent panics when despawning ui nodes (#13000) # 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: ```rs use bevy::{prelude::*, ui::UiSystem}; #[derive(Resource)] struct SliceSquare(Handle); 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) { commands.spawn(Camera2dBundle::default()); commands.insert_resource(SliceSquare(asset_server.load("textures/slice_square.png"))); } fn create_ui(mut commands: Commands, slice_square: Res) { 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>) { 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: ```md 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. --- crates/bevy_ui/src/accessibility.rs | 6 +++--- crates/bevy_ui/src/texture_slice.rs | 4 ++-- crates/bevy_ui/src/update.rs | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ui/src/accessibility.rs b/crates/bevy_ui/src/accessibility.rs index 7620c18519208..ab18b77d5a161 100644 --- a/crates/bevy_ui/src/accessibility.rs +++ b/crates/bevy_ui/src/accessibility.rs @@ -78,7 +78,7 @@ fn button_changed( } commands .entity(entity) - .insert(AccessibilityNode::from(node)); + .try_insert(AccessibilityNode::from(node)); } } } @@ -107,7 +107,7 @@ fn image_changed( } commands .entity(entity) - .insert(AccessibilityNode::from(node)); + .try_insert(AccessibilityNode::from(node)); } } } @@ -137,7 +137,7 @@ fn label_changed( } commands .entity(entity) - .insert(AccessibilityNode::from(node)); + .try_insert(AccessibilityNode::from(node)); } } } diff --git a/crates/bevy_ui/src/texture_slice.rs b/crates/bevy_ui/src/texture_slice.rs index 16cfacff76670..6712d53ad6ece 100644 --- a/crates/bevy_ui/src/texture_slice.rs +++ b/crates/bevy_ui/src/texture_slice.rs @@ -177,7 +177,7 @@ pub(crate) fn compute_slices_on_asset_event( atlas, &atlas_layouts, ) { - commands.entity(entity).insert(slices); + commands.entity(entity).try_insert(slices); } } } @@ -213,7 +213,7 @@ pub(crate) fn compute_slices_on_image_change( atlas, &atlas_layouts, ) { - commands.entity(entity).insert(slices); + commands.entity(entity).try_insert(slices); } } } diff --git a/crates/bevy_ui/src/update.rs b/crates/bevy_ui/src/update.rs index d3750996d9398..e93e8214b97c8 100644 --- a/crates/bevy_ui/src/update.rs +++ b/crates/bevy_ui/src/update.rs @@ -63,7 +63,7 @@ fn update_clipping( } } else if let Some(inherited_clip) = maybe_inherited_clip { // No previous calculated clip, add a new CalculatedClip component with the inherited clipping rect - commands.entity(entity).insert(CalculatedClip { + commands.entity(entity).try_insert(CalculatedClip { clip: inherited_clip, }); } @@ -163,7 +163,7 @@ fn update_children_target_camera( match camera_to_set { Some(camera) => { - commands.entity(child).insert(camera.clone()); + commands.entity(child).try_insert(camera.clone()); } None => { commands.entity(child).remove::();