Panic at parent != previous When Adding a Child to a Parent When It's Already a Child

[zicklag]

Bevy version

Bevy 0.8.1, and Bevy 9f51651

What you did

use bevy::prelude::*;

fn main() -> anyhow::Result<()> {
    let mut world = World::new();

    let parent = world.spawn().id();
    let child = world.spawn().id();
    world.entity_mut(parent).push_children(&[child]);
    world.entity_mut(parent).push_children(&[child]);

    Ok(())
}

What went wrong

thread 'main' panicked at 'assertion failed: parent != previous', crates/bevy_hierarchy/src/child_builder.rs:54:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

IMHO this shouldn't panic.

Additional information

I opened this as a bug because I think something needs to change, even if that is simply the panic message. I got this message occasionally in my game, but it wasn't clear what the actual root cause was without looking deeper into what the Bevy code was trying to do. It turns out it's quite simple: if you try to add something as a child, when it's already a child, it panics here.

If we really think this should panic, then the message should be something like "attempted to add a child to a parent, when it was already a child".

I can see the following reasons to make this panic:

• If you are trying to add something as a child, when it's already a child, then it might be a logic error in the game, and if it is, you'll want to know about it. It could help you find bugs in your game you might not otherwise notice.
  • For instance, the fact that this happens in my game might mean I've done something wrong in my snapshot-restore system.
• There is the question of how pushing a child that is already in the list effects the child order. For instance, usually a push-child would put the child at the end, but if it's already in the list, does it get removed from where it is, and then pushed to the end?

But I think the reasons for not panicking are probably stronger:

• It's subjectively unintuitive that this panics. I haven't done anything that is fundamentally wrong, I've just told it to put it in the state it was already in.
• If you know that something must be the child of another thing, it may be easier just to say that and push the child, without checking whether or not it is already a child.
• Since updates to the hierarchy are handled by command buffer flushes, two separate systems could independently realize that an entity should be the child of the same parent, both push the same child to the same parent, and then cause a panic because both commands got flushed in the same stage.
  • While you should maybe be avoiding this scenario in your game anyway, fundamentally, the operation isn't inconsistent or wrong. It's just redundant.