commands despawn does not update childrens parent

[PhaestusFox]

Bevy version

0.8

What you did

I making a video about the new changes to bevy's hierarchy and I found that if you despawn an entity that has children, said children will keep the now non-existent entity as their parent.

#[derive(Component)]
struct Test([Entity; 3]);

fn startup(mut commands: Commands) {
 let c0 = commands.spawn().id();
 let c1 = commands.spawn().add_child(c0).id();
 let c2 = commands.spawn().add_child(c0).id();
 commands.insert_resource(Test([c0, c1, c2]));
 commands.entity(c2).despawn();
}

fn run(test: Res<Test>,
    child: Query<&Children>,
    parent: Query<&Parent>,
    input: Res<Input<KeyCode>>,
) {
    if input.just_pressed(KeyCode::Space) {
        println!("C0:{:?}, C1:{:?}, C2:{:?}", test.0[0],test.0[1],test.0[2]);
        println!("C0 Parent is {}", match parent.get(test.0[0]) {
            Ok(e) => {
            if e.get() == test.0[1] {
                "C1".to_string()
            } else if e.get() == test.0[2] {
                "C2".to_string()
            } else {
                format!("{:?}", e)
            }
            },
            Err(e)=> {format!("{:?}", e)},
        } );
        println!("C1 Children are {:?}", child.get(test.0[1]));
        println!("C2 Children are {:?}", child.get(test.0[2]));
    }
}

What went wrong

• what were you expecting?
  console to read

C0:1v0, C1:2v0, C2:3v0
C0 Parent is QueryDoesNotMatch(1v0)
C1 Children are Err(QueryDoesNotMatch(2v0))
C2 Children are Err(NoSuchEntity(3v0))

• what actually happened?
  console to reads

C0:1v0, C1:2v0, C2:3v0
C0 Parent is C2
C1 Children are Err(QueryDoesNotMatch(2v0))
C2 Children are Err(NoSuchEntity(3v0))

I believe this should be classified as a bug because it would be unexpected for children to reference a parent that has been despawned, this also breads the ease of finding root nodes as these nodes are now free floating.

How to fix

I don't know whether
they should just be cut free to become roots themself -- easier
or if
they should be grafted up a parent if there is one to attach to if not then made free -- harder/slower

[BorisBoutillier]

I looked a bit at this issue and here is a passing test case with grand_parent -> parent -> child hierarchy, that doubly breaks the Children/Parent relation, namely the two last asserts.

#[test]
fn despawn_parent() {
    let mut world = World::default();
    let mut queue = CommandQueue::default();
    let mut commands = Commands::new(&mut queue, &world);

    let grand_parent = commands.spawn().insert(C(0)).id();
    let parent = commands.spawn().insert(C(1)).id();
    let child = commands.spawn().insert(C(2)).id();

    commands.entity(grand_parent).add_child(parent);
    commands.entity(parent).add_child(child);

    queue.apply(&mut world);

    assert_eq!(*world.get::<Parent>(child).unwrap(), Parent(parent));
    assert_eq!(
        (*world.get::<Children>(parent).unwrap()).0,
        Children::from_entities(&[child]).0
    );
    assert_eq!(*world.get::<Parent>(parent).unwrap(), Parent(grand_parent));
    assert_eq!(
        (*world.get::<Children>(grand_parent).unwrap()).0,
         Children::from_entities(&[parent]).0
    );

    let mut queue = CommandQueue::default();
    let mut commands = Commands::new(&mut queue, &world);

    commands.entity(parent).despawn();

    queue.apply(&mut world);

    assert_eq!(*world.get::<Parent>(child).unwrap(), Parent(parent));
    assert_eq!(
        (*world.get::<Children>(grand_parent).unwrap()).0,
         Children::from_entities(&[parent]).0
    );
}

We can see that after call of despawn on parent, we have the following two issues:

• parent is still in theChildren of grand_parent
• child stiill has a Parent set to parent

The despawn commands does not handle at all the hiearchy relation.

[Shatur]

I tried to write a workaround in my game:

    fn despawn_invalid_system(
        mut commands: Commands,
        world: &World,
        parents: Query<(Entity, &Children)>,
    ) {
        for (parent_entity, children) in &parents {
            for &child_entity in children {
                if world.get_entity(child_entity).is_none() {
                    commands
                        .entity(parent_entity)
                        .remove_children(&[child_entity]);
                }
            }
        }
    }

But it crashes because remove_children checks if an entity is valid.
Yes, it would be great to update hierarchy on despawn.

[focustense]

I'm not sure if this issue is getting much attention, but I wanted to bump it because there's a very old PR, #386, whose title implies that the issue is fixed, and a few other issues reference that PR and also imply that the issue is fixed. However, the fix from 2020 only applies to despawn_recursive (formerly despawn_with_children_recursive). The plain old despawn() command is still not hierarchy-aware.

So for anyone still getting hit by this, you can do one of two things: either (a) don't use despawn(), instead always use despawn_recursive(), or (b) remove the parent first by calling .remove_parent().despawn().

I honestly wonder if despawn() really still has a purpose. At this point it's probably safe to assume that any non-trivial Bevy app relies on hierarchy somehow. Maybe there are cases when a user really wants to despawn an entity in the middle of the graph without despawning its children, but even so, the behavior of despawn() is almost never going to be what they expect, it just leaves the hierarchy in a broken and inconsistent state. If there's a reason to keep child entities alive, then they can treat it like a database (which ECS basically is, after all) and reparent the children before despawning the parent.

Just my two bits, but maybe it's time to make despawn an alias for despawn_recursive, and if it's really essential then rename the original despawn to something with an implied warning, like despawn_ignoring_hierarchy. In terms of footguns, legacy despawn has become a pretty big one; I'm willing to bet that approximately 100% of users actually expect either a recursive despawn, or an "orphaning" despawn that removes the despawned entity from its parent and removes any of its own children from the hierarchy, leaving them with no parent.

[ItsDoot]

This will be resolved when #12235 is closed.