Despawning a child UI node causes a panic

[ncallaway]

Attempting to despawn an entity that is a UI node child of another UI node causes a panic.

Example Reproduction

use bevy::prelude::*;

fn main() {
    App::build()
        .add_default_plugins()
        .add_startup_system(setup.system())
        .add_system(text_despawn_system.system())
        .run();
}

fn text_despawn_system(mut commands: Commands, mut query: Query<(Entity, &Text)>) {
    for (entity, _text) in &mut query.iter() {
        commands.despawn(entity);
    }
}

fn setup(
    mut commands: Commands,
    asset_server: Res<AssetServer>,
    mut materials: ResMut<Assets<ColorMaterial>>,
) {
    let font_handle = asset_server
        .load("assets/fonts/FiraMono-Medium.ttf")
        .unwrap();
    commands
        // 2d camera
        .spawn(UiCameraComponents::default())
        .spawn(NodeComponents {
            style: Style {
                size: Size::new(Val::Percent(100.0), Val::Percent(100.0)),
                justify_content: JustifyContent::SpaceBetween,
                ..Default::default()
            },
            material: materials.add(Color::NONE.into()),
            ..Default::default()
        })
        .with_children(|parent| {
            parent.spawn(TextComponents {
                style: Style {
                    align_self: AlignSelf::FlexEnd,
                    ..Default::default()
                },
                text: Text {
                    value: "Hello World, I won't exist for very long".to_string(),
                    font: font_handle,
                    style: TextStyle {
                        font_size: 60.0,
                        color: Color::WHITE,
                    },
                },
                ..Default::default()
            });
        });
}

Running this with RUST_BACKTRACE=1 cargo run produces:

thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', /Users/.../crates/bevy_ui/src/flex/mod.rs:86:32
stack backtrace:
   0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
   // omitted
   8: core::panicking::panic
   9: core::option::Option<T>::unwrap
  10: bevy_ui::flex::FlexSurface::update_children
  11: bevy_ui::flex::flex_node_system
  12: core::ops::function::Fn::call
  13: core::ops::function::impls::<impl core::ops::function::FnMut<A> for &F>::call_mut
  14: <Func as bevy_ecs::system::into_system::IntoQuerySystem<(),(Ra,Rb),(A,B,C,D,E)>>::system::{{closure}}
  15: <bevy_ecs::system::into_system::SystemFn<State,F,ThreadLocalF,Init,SetArchetypeAccess> as bevy_ecs::system::system::System>::run
  // omitted

The crashing line is

bevy/crates/bevy_ui/src/flex/mod.rs

Line 86 in db1bf64

let stretch_node = self.entity_to_stretch.get(child).unwrap();

We appear to be attempting to fetch a stretch node that doesn't exist in the entities_to_stretch map. The calling code is

pub fn flex_node_system(
    ...
    mut flex_surface: ResMut<FlexSurface>,
    ...
    mut children_query: Query<With<Node, (Entity, Changed<Children>)>>,
) {
    // ... omitted
    for (entity, children) in &mut children_query.iter() {
        flex_surface.update_children(entity, &children);
    }
    // ...omitted

[ncallaway]

I don't appear to have permission to label issues, so for whoever is triaging:

Suggested Labels: bug | crash | ui

Potential Label: focus-area

[Cupnfish]

before:

fn text_despawn_system(mut commands: Commands, mut query: Query<(Entity, &Text)>) {
    for (entity, _text) in &mut query.iter() {
        commands.despawn(entity);
    }
}

I think using despawn_recursive() can solve this problem, and this function is prepared for this situation.
after:

fn text_despawn_system(mut commands: Commands, mut query: Query<(Entity, &Text)>) {
    for (entity, _text) in &mut query.iter() {
        // despawn:Commands -> write word -> Despawn {Entity}
       // despawn_recursive: first, make the entity's own parent forget about it
        commands.despawn_recursive(entity);
    }
}

I also looked at the source code for a long time before the existence of this function. I think this function is sufficient to solve our problem, the only problem is that there are no good mistakes to remind us that we should call despawn_recursive() instead of directly calling despawn().
I think we need some better error handling
before:

        for child in children.iter() {
            let stretch_node = self.entity_to_stretch.get(child).unwarp();
         }

we clould like this:

        for child in children.iter() {
            if let Some(stretch_node) = self.entity_to_stretch.get(child){
                stretch_children.push(*stretch_node);
            } else {
                // TODO:Some error handling, or logging
            }
        }

··············································
Uh, sorry, I didn’t think about some places before.
The despwan_recursive() function does not solve our problem.
Since it will work with its own child nodes despwan, if we do not need this result, we should provide a similar function, which will let the parent node forget itself when despawn.
like despawn_recursive() :

    // first, make the entity's own parent forget about it
    if let Ok(parent) = world.get::<Parent>(entity).map(|parent| parent.0) {
        if let Ok(mut children) = world.get_mut::<Children>(parent) {
            children.retain(|c| *c != entity);
        }
    }

[MinerSebas]

Testing this in 0.5 the Example no longer panics, but still produces a Warning:
WARN bevy_ui::flex: Unstyled child in a UI entity hierarchy. You are using an entity without UI components as a child of an entity with UI components, results may be unexpected.

Updated minimal example:

use bevy::prelude::*;

fn main() {
    App::build()
        .add_plugins(DefaultPlugins)
        .add_startup_system(setup.system())
        .add_system(text_despawn_system.system())
        .run();
}

fn text_despawn_system(mut commands: Commands, query: Query<(Entity, &Text)>) {
    for (entity, _text) in &mut query.iter() {
        commands.entity(entity).despawn();
    }
}

fn setup(
    mut commands: Commands,
    asset_server: Res<AssetServer>,
    mut materials: ResMut<Assets<ColorMaterial>>,
) {
    let font_handle = asset_server
        .load("fonts/FiraMono-Medium.ttf");
        // 2d camera
    commands.spawn_bundle(UiCameraBundle::default());
    commands.spawn_bundle(NodeBundle {
            style: Style {
                size: Size::new(Val::Percent(100.0), Val::Percent(100.0)),
                justify_content: JustifyContent::SpaceBetween,
                ..Default::default()
            },
            material: materials.add(Color::NONE.into()),
            ..Default::default()
        })
        .with_children(|parent| {
            parent.spawn_bundle(TextBundle {
                style: Style {
                    align_self: AlignSelf::FlexEnd,
                    ..Default::default()
                },
                text: Text::with_section(
                    "Hello World, I won't exist for very long".to_string(),
                    TextStyle {
                        font: font_handle,
                        font_size: 60.0,
                        color: Color::WHITE,
                    },
                    TextAlignment {
                        horizontal: HorizontalAlign::Center,
                        ..Default::default()
                    },
                ),
                ..Default::default()
            });
        });
}

[FelixSelter]

I have the same issue. But @Cupnfish commands.entity(id).despawn_recursive(); worked

[ghost]

This can probably be closed, pretty ancient and despawn_recursive would be the proper solution.