Inserting `children!` overrides existing children

[Pascualex]

Bevy version

v0.16.1

What you did

Inserted the children! macro on an entity that had existing children.

What went wrong

Instead of appending the new children, the existing ones got overridden.

Minimal repro

I wanted to cover a variety of scenarios but the override seems to be consistent.

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(
            Startup,
            (
                spawn_entities,
                print_children,
                patch_entity_f,
                print_children,
            )
                .chain(),
        )
        .run();
}

#[derive(Resource)]
struct EntityF(Entity);

fn spawn_entities(mut commands: Commands) {
    commands.spawn((Name::new("Entity A"), children![(), ()]));
    commands
        .spawn((Name::new("Entity B"), children![()]))
        .insert(children![()]);
    commands
        .spawn(Name::new("Entity C"))
        .insert(children![()])
        .insert(children![()]);
    commands
        .spawn(Name::new("Entity D"))
        .with_child(())
        .with_child(());
    commands
        .spawn(Name::new("Entity E"))
        .with_child(())
        .with_child(())
        .insert(children![]);
    let entity_f = commands
        .spawn(Name::new("Entity F"))
        .with_child(())
        .with_child(())
        .id();
    commands.insert_resource(EntityF(entity_f));
}

fn patch_entity_f(entity_f: Res<EntityF>, mut commands: Commands) {
    commands.entity(entity_f.0).insert(children![]);
}

fn print_children(entities: Query<(&Name, Option<&Children>)>) {
    for (name, children) in &entities {
        let children_count = children.map(Children::len).unwrap_or(0);
        println!("{} has {} children", name, children_count);
    }
}

The output:

Entity A has 2 children
Entity B has 1 children
Entity C has 1 children
Entity D has 2 children
Entity E has 0 children
Entity F has 2 children

Entity A has 2 children
Entity B has 1 children
Entity C has 1 children
Entity D has 2 children
Entity E has 0 children
Entity F has 0 children

[SkiFire13]

Summary of the small observation I made on Discord:

The SpawnRelatedBundle and SpawnOneRelated bundles include the R::RelationshipTarget (i.e. Children in this case) component, hence using children![ ... ] is effectively the same as inserting an instance of Children, and this will override the existing one.

Ideally we would insert it only if it's not already present, kinda like a required component, but these are Bundles, which don't support required components (well, they have a register_required_components method, but it doesn't appear to be called anywhere other than in Bundle implementations).

The alternative of inserting the component only while executing the BundleEffect and if not already present would also fix the issue, however it would require an additional table move, which is not ideal for performance.

[janhohenheim]

Likely cause of Noxmore/bevy_trenchbroom#95 and half a dozen similar footguns I ran into while composing entity hierarchies

[janhohenheim]

Relevant discussion on Discord:

Rob:

This is expected behavior when you insert any component multiple times.
Use with_children, add_children, insert_children, remove_children etc entity commands.

Jan:

Agreed, it's just that the semantics of .insert(children![]), to me at least, suggest that this will add new children.
An example of a footgun I ran into is when I receive an entity from some trigger, like OnAdd, and I want it to have some children. Since I don't know whether the entity has a hierarchy or not, using children! is a footgun in that case.
Exactly this scenario (using children! in an observer) has confused some other users I talked with in the same way, so I'm not alone in this interpretation.
If we decide that this is a feature and not a bug, I would at least change the documentation to heavily discourage using children! outside of spawning something

Giacomo:

Anyway, in the implementation details the issue is that SpawnRelatedBundle behaves as if it wrapped a R::RelationshipTarget (i.e. Children), and inserting a Children component will overwrite the existing one. I don't think the last part will change, however there is likely room to change SpawnRelatedBundle to behave different. Technically speaking this behaviour is not needed, since inserting a child will automatically insert the Children component if not present (exactly what you want!), but this is inefficient because it requires a table move if it was not present. The way I see it there are 3 possibilities:

• ignore this footgun and pretend children![ ... ] acts like an instance of Children
• ignore the performance regression and make SpawnRelatedBundle not insert the R::RelationshipTarget, instead let the child do that
• implement some sort of required components for bundles (this would be the ideal but more complex solution IMO)