Despawned Node Entities are not removed from their parent Node's Children

[ncallaway]

An entity that is despawned doesn't appear to be removed from the Children component of their parent Node.

I'm not sure this is a bug, but I could see it eventually effective performance for a long running game that both spawns and despawns a lot of entities within a parent and iterates over the Children object a lot.

I think this is related to #351.

Example Reproduction

use bevy::prelude::*;

/// This example illustrates how to create a button that changes color and text based on its interaction state.
fn main() {
    App::build()
        .add_default_plugins()
        .init_resource::<ButtonMaterials>()
        .init_resource::<ButtonCount>()
        .add_startup_system(setup.system())
        .add_system(button_system.system())
        .run();
}

fn button_system(
    mut commands: Commands,
    button_materials: Res<ButtonMaterials>,
    asset_server: Res<AssetServer>,
    mut button_count: ResMut<ButtonCount>,
    mut interaction_query: Query<(
        Entity,
        &Button,
        Mutated<Interaction>,
        &mut Handle<ColorMaterial>,
        &Children,
    )>,
    mut root_query: Query<(Entity, &Root, &Children)>,
) {
    for (e, _, interaction, mut material, _) in &mut interaction_query.iter() {
        match *interaction {
            Interaction::Clicked => {
                let font = asset_server
                    .get_handle("assets/fonts/FiraMono-Medium.ttf")
                    .unwrap();

                // we're going to remvoe the button
                commands.despawn_recursive(e);

                // spawn a new button in the same spot
                for (parent, _, children) in &mut root_query.iter() {
                    println!("Now have: {} children", children.0.len());
                    let e = spawn_button_node(
                        &mut commands,
                        &button_materials,
                        font,
                        &(format!("Button {}", button_count.0))[..],
                    );
                    button_count.0 = button_count.0 + 1;
                    commands.push_children(parent, &[e]);
                    break;
                }
            }
            Interaction::Hovered => {
                *material = button_materials.hovered;
            }
            Interaction::None => {
                *material = button_materials.normal;
            }
        }
    }
}

fn setup(
    mut commands: Commands,
    asset_server: Res<AssetServer>,
    button_materials: Res<ButtonMaterials>,
) {
    let font = asset_server
        .load("assets/fonts/FiraMono-Medium.ttf")
        .unwrap();

    let root = Entity::new();
    commands
        .spawn(UiCameraComponents::default())
        .spawn_as_entity(
            root,
            NodeComponents {
                style: Style {
                    size: Size::new(Val::Percent(100.0), Val::Percent(100.0)),
                    flex_direction: FlexDirection::ColumnReverse,
                    justify_content: JustifyContent::FlexStart,
                    align_items: AlignItems::Center,
                    ..Default::default()
                },
                ..Default::default()
            },
        )
        .with(Root);
    let button = spawn_button_node(&mut commands, &button_materials, font, "First button");
    commands.push_children(root, &[button]);
}

struct ButtonMaterials {
    normal: Handle<ColorMaterial>,
    hovered: Handle<ColorMaterial>,
    pressed: Handle<ColorMaterial>,
}

impl FromResources for ButtonMaterials {
    fn from_resources(resources: &Resources) -> Self {
        let mut materials = resources.get_mut::<Assets<ColorMaterial>>().unwrap();
        ButtonMaterials {
            normal: materials.add(Color::rgb(0.02, 0.02, 0.02).into()),
            hovered: materials.add(Color::rgb(0.05, 0.05, 0.05).into()),
            pressed: materials.add(Color::rgb(0.1, 0.5, 0.1).into()),
        }
    }
}

#[derive(Default)]
struct ButtonCount(u32);

struct Root;

fn spawn_button_node(
    commands: &mut Commands,
    mats: &Res<ButtonMaterials>,
    font: Handle<Font>,
    label: &str,
) -> Entity {
    let e = Entity::new();
    commands
        .spawn_as_entity(
            e,
            ButtonComponents {
                style: Style {
                    size: Size::new(Val::Px(300.0), Val::Px(65.0)),
                    // center button
                    margin: Rect::all(Val::Auto),
                    // horizontally center child text
                    justify_content: JustifyContent::Center,
                    // vertically center child text
                    align_items: AlignItems::Center,
                    ..Default::default()
                },
                material: mats.normal,
                ..Default::default()
            },
        )
        .with_children(|parent| {
            parent.spawn(TextComponents {
                text: Text {
                    value: label.to_string(),
                    font: font,
                    style: TextStyle {
                        font_size: 28.0,
                        color: Color::rgb(0.8, 0.8, 0.8),
                    },
                },
                ..Default::default()
            });
        });

    e
}

Result

> cargo run
Now have: 1 children
Now have: 2 children
Now have: 3 children
Now have: 4 children
Now have: 5 children
Now have: 6 children
Now have: 7 children
Now have: 8 children
Now have: 9 children
Now have: 10 children
Now have: 11 children

My expectation is that despawned children are eventually removed from the Parent component. I don't expect it to be immediatey, so I wouldn't be surprised to occasionally see 3 children in my Parent even if there's only 1 child still alive, but I do expect them to be removed eventually.

[ncallaway]

@karroffel or @cart This behavior is also leading to a crash if you later call .despawn_recursive on the entity that has the no longer existing child:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: NoSuchEntity', /Users/noah/.cargo/git/checkouts/bevy-f7ffde730c324c74/89a1d36/crates/bevy_transform/src/hierarchy/hierarchy.rs:57:27
stack backtrace:
  ...
   7: core::panicking::panic_fmt
   8: core::option::expect_none_failed
   9: core::result::Result<T,E>::unwrap
  **10: bevy_transform::hierarchy::hierarchy::despawn_with_children_recursive**
  **11: bevy_transform::hierarchy::hierarchy::despawn_with_children_recursive**
  12: <bevy_transform::hierarchy::hierarchy::DespawnRecursive as bevy_ecs::system::commands::WorldWriter>::write
  13: bevy_ecs::system::commands::Commands::apply
  14: <Func as bevy_ecs::system::into_system::IntoQuerySystem<(bevy_ecs::system::commands::Commands,),(Ra,Rb,Rc,Rd,Re),(A,B,C)>>::system::{{closure}}
  15: <bevy_ecs::system::into_system::SystemFn<State,F,ThreadLocalF,Init,SetArchetypeAccess> as bevy_ecs::system::system::System>::run_thread_local
  16: bevy_ecs::schedule::parallel_executor::ExecutorStage::run
  17: bevy_ecs::schedule::parallel_executor::ParallelExecutor::run
  18: bevy_app::app::App::update
  ...

This happens due to this .unwrap() ignoring the NoSuchEntity error in the result:

bevy/crates/bevy_transform/src/hierarchy/hierarchy.rs

Line 57 in 63fd4ae

world.despawn(entity).unwrap();

Would you rather incorporate the crashing behavior for spurious children into this issue, or would you rather I create a separate issue for that?

[CleanCut]

I think the problem is that this function doesn't check for and remove the root entity from the children of its own parent. Does that sound right, @cart? I'd be willing to make a PR for this, but I'm not yet sure how to look up a parent efficiently.

bevy/crates/bevy_transform/src/hierarchy/hierarchy.rs

Lines 46 to 58 in db8ec7d

	fn despawn_with_children_recursive(world: &mut World, entity: Entity) {
	if let Some(children) = world
	.get::<Children>(entity)
	.ok()
	.map(|children| children.0.iter().cloned().collect::<Vec<Entity>>())
	{
	for e in children {
	despawn_with_children_recursive(world, e);
	}
	}
	world.despawn(entity).unwrap();
	}

[ncallaway]

@CleanCut The entity should have a Parent component on it that has a reference to its parent entity

[CleanCut]

@ncallaway I should have guessed that--otherwise it would be extremely inefficient to ever discover your own parent, which would make composing transformation matrices insanely expensive! I'll take a shot at making a pull request.

[CleanCut]

I opened #386 with an attempted fix.