Stop inserting RelationshipTarget on SpawnRelatedBundle

[Pascualex]

Fixes #19715.

How the issue was introduced

On v0.16 improved spawn APIs were introduced. They are presented as having vastly improved ergonomics and the following comparison is offered as an example:

commands
    .spawn(Player)
    .with_children(|p| {
        p.spawn(RightHand).with_children(|p| {
            p.spawn(Glove);
            p.spawn(Sword);
        });
        p.spawn(LeftHand).with_children(|p| {
            p.spawn(Glove);
            p.spawn(Shield);
        });
    });

commands.spawn((
    Player,
    children![
        (RightHand, children![Glove, Sword]),
        (LeftHand, children![Glove, Shield]),
    ],
));

So both APIs do more or less the same thing but one is way more ergonomic than the other, right? Well, not exactly. As written on the release notes:

This API also allows us to optimize hierarchy construction time by cutting down on re-allocations, as we can generally (with the exception of cases like SpawnWith) statically determine how many related entities an entity will have and preallocate space for them in the RelationshipTarget component (ex: Children).

Unfortunately, this reasonable optimization requires a significant change in behavior: the children macro doesn't just spawn children (like with_children does) , it also inserts a new Children component beforehand.

This introduces a footgun, as users may expect to be able to bring these ergonomic improvements to a slightly different context:

commands
    .entity(entity)
    .insert(Player)
    .with_children(|p| {
        p.spawn(RightHand).with_children(|p| {
            p.spawn(Glove);
            p.spawn(Sword);
        });
        p.spawn(LeftHand).with_children(|p| {
            p.spawn(Glove);
            p.spawn(Shield);
        });
    });

commands
    .entity(entity)
    .insert((
        Player,
        children![
            (RightHand, children![Glove, Sword]),
            (LeftHand, children![Glove, Shield]),
        ],
    ));

But here the behavior is widely different. Since the children macro inserts a Children component, this means that existing children will be overriden.

Note that you can avoid the footgun by using insert_if_new, but it's very easy to forget and could cause issues for other components in the bundle:

commands
    .entity(entity)
    .insert_if_new((
        Player,
        children![
            (RightHand, children![Glove, Sword]),
            (LeftHand, children![Glove, Shield]),
        ],
    ));

Ergonomics should be the focus

Although optimizations are always welcome, I believe that in this case the main goal of the new APIs was to improve ergonomics. I think we should drop the optimization in favor of removing a footgun that worsens ergonomics. To do that, we just need to stop inserting the RelationshipTarget component on SpawnRelatedBundle.

Alternatives

An alternative proposed by @SkiFire13 is to make use of required components to insert the Children component only when it doesn't exist. This wouldn't preallocate space like the current optimization, but that is probably less impactful than avoiding the table move. Although I find this a great optimization without footgun issues, establishing a requirement relationship between SpawnRelatedBundle and RelationshipTarget would likely require complex changes since SpawnRelatedBundle is not a component.

[SkiFire13]

establishing a requirement relationship between SpawnRelatedBundle and RelationshipTarget would likely require complex changes since SpawnRelatedBundle is not a component.

As a more hacky but less complex alternative, we could have SpawnRelatedBundle contain a sparse component that requires the RelationshipTarget and that removes itself in its OnAdd hook. This would then require only an archetype move, which is much less costly than a table move.

[Pascualex]

As a more hacky but less complex alternative, we could have SpawnRelatedBundle contain a sparse component that requires the RelationshipTarget and that removes itself in its OnAdd hook. This would then require only an archetype move, which is much less costly than a table move.

I had arrived to a similar idea without the auto-removal, making that a sparse component would then make a lot of sense. Feels incredibly hacky but could make sense if the performance gains are good enough.

[Pascualex]

I have had to change the can_spawn_bundle_without_extract test since it was the only test that started failing with the change. I'm not sure what the goal of the test was or if it still makes sense to have it around after this change. There are other bevy_ecs tests failing on main, which could be hiding other test issues introduced here.

[janhohenheim]

Put this in 0.16.2 because even though it changes expected behavior to something else, I've seen quite a few people get tripped up by this, including me.
Add to that that issues coming from accidentally yeeting the old children are really hard to debug, as the result of your hierarchies suddenly shifting can do a lot of different things depending on the game / app

[viridia]

What about something like:

Children::spawn_append(...)

And an associated append_children! macro.

This makes it explicit as to whether the children will be overwritten or appended.

[alice-i-cecile]

I am definitely not comfortable shipping this level of subtle behavior change in a point release. @Pascualex, I am going to request a migration guide here.

[alice-i-cecile]

This needs a dedicated test, a migration guide and a fresh set of benchmarks. This is very performance sensitive code, and I'm skeptical that this is the right fix.

To me, the correct fix is "if a matching relationship target already exists, we should be appending to it", but that doesn't appear to be what these code changes are doing.

[cart]

The allocation optimization was definitely a primary motivator. But additionally, from a "bundle principles" perspective, one thing I'm trying to uphold is "something in component position is that component". Part of that was making RelationshipTargets like Children actually insert that component on insert. If you were inserting any other component in that way, it would be "overwrite" behavior. I think preserving the "things in component position are components" principle is important for making Bevy code readable and predictable.

I've been thinking about a related idea recently: allowing components (when implementing the Component trait) to define the "insert" behavior (replace vs merge ... defaulting to replace of course). In that world, Children could define an "append" merge behavior, allowing us to align with_children with insert(Children::spawn()). Additionally, if we also lifted the "no duplicate components in bundles" constraint for "merge" components, it would also allow people to do this without stepping one themselves:

world.spawn(
  Foo,
  children![A, B],
  Bar,
  // this would get merged with [A, B], producing [A, B, C]
  children![C],
)

That being said, I'm not fully sold on this design. It makes Bevy less predictable, as you then need to know if a component has merge inserts enabled (and you need to know what merge algorithm would be used). I'm also not convinced that allowing multiple children![] in this context is a step in the right direction. Statically and functionally, they could have been expressed as a single list (ideally at the end of the bundle), and that would improve legibility.

[viridia]

EIBTI ("Explicit is Better than Implicit", from the Zen of Python)

[janhohenheim]

Would append_children![] also not be acceptable then? Since it also wouldn't behave like a regular component, but a merging one

[Pascualex]

I am definitely not comfortable shipping this level of subtle behavior change in a point release. @Pascualex, I am going to request a migration guide here.

@alice-i-cecile that's fair! Seems like we need some alignment before committing to an implementation anyway so it will take some time.

To me, the correct fix is "if a matching relationship target already exists, we should be appending to it", but that doesn't appear to be what these code changes are doing.

It kind of does that in the sense that it delegates the management of the relationship target to the relationship component hooks, which behave that way. But yeah, it doesn't do that in advance to optimize performance.

The allocation optimization was definitely a primary motivator.

@cart good to know, that changes things for me.

I think preserving the "things in component position are components" principle is important for making Bevy code readable and predictable.

Hmm, I understand the concern and agree with the idea behind it. I think the main sauce of SpawnRelatedBundle is the spawning of the related entities, regardless of wether we pre-insert the relationship target in advance or not. So the difference is subtle, but I understand the desire to keep that as a design principle.

A related thought: the fact that currently if you use insert_if_new(children![...]) you spawn the children even if you are ignoring the actual component insertion seems to go against that goal.

I've been thinking about a related idea recently: allowing components (when implementing the Component trait) to define the "insert" behavior (replace vs merge ... defaulting to replace of course).

Oh I see, that could even preserve the space-allocation optimization, I like it.

That being said, I'm not fully sold on this design. It makes Bevy less predictable, as you then need to know if a component has merge inserts enabled (and you need to know what merge algorithm would be used).

What if InsertMode was neither component-defined nor a separate argument but a bundle wrapper instead?

So we would replace the following:

// old
commands.entity(entity).insert_if_new(ComponentA);
// new
commands.entity(entity).insert(Keep(ComponentA));

And similarly we could do:

commands.entity(entity).insert(Merge(SpawnRelatedBundle { ... }));

That way RelationshipTarget::spawn could return Merge<SpawnRelatedBundle<...>>, which would be fairly explicit. It could also be an improvement to the current API to be able to do this:

// old
commands
    .entity(entity)
    .insert(ComponentA)
    .insert_if_new(ComponentB);
// new
commands
    .entity(entity)
    .insert((
        ComponentA,
        Keep(ComponentB),
    ));

Note that since it's a bundle wrapper it wouldn't introduce much complexity when applying to multiple components:

// old
commands
    .entity(entity)
    .insert_if_new((
        ComponentA,
        ComponentB,
    ));
// new
commands
    .entity(entity)
    .insert(Keep((
        ComponentA,
        ComponentB,
    )));

Alternative names for Keep and Merge could be:

• IgnoreIfExists / MergeIfExists
• IgnoreOnCollision / MergeOnCollision

[Pascualex]

I've created #19768 prototyping the alternative proposal.