Spawning duplicate components is inconsistent

[nicopap]

Bevy version

Bevy 0.9.0 / 7231e00

What I did

Since the addition of Bundle/Component agnostic spawns, the way bevy handles duplicate components is inconsistent.

Depending on how components are spawned, it either leads to a panic or just the first value of the component being overwritten.

Here, both SpatialBundle and TransformBundle have a Transform and GlobalTransform. When spawning them together with a spawn(<TUPLE>), it panics, but if I spawn them separately, it doesn't panic:

// THIS PANICKS!!!
commands.spawn((
  TransformBundle::default(),
  SpatialBundle::default(),
))
// THIS just overwrites the GlobalTransform and Transform components
commands.spawn(TransformBundle::default())
  .insert(SpatialBundle::default())

What I expect

I expect neither version to panic.

[DJMcNab]

For your first example, how would do you propose we handle:

.spawn((TransformBundle {transform: Transform::from_xyz(10, 20, 30), 
SpatialBundle {transform: Transform::from_xyz(30, 20, 10)}));

There's no meaningful sense by which not erroring can be the correct behaviour when completing the inserting in a single call, because there is no ordering within a Bundle, and adding that would also be a significant footgun (e.g. (CarefullyConstructedTransformIWant, BundleWithTransform) would delete your carefully constructed Transform). It would also make the field ordering public API, which is a terrible idea.
And if that's the case, then we may as well just panic for all these cases, rather than needing to think about the extremely awkward consequences of allowing overwriting; the bundle insertion code is already nasty enough without having to make sure this case would be sound too.

(Allowing it when both are exactly the Default would be permissible in theory, but it would also be an expensive footgun to check, and still has the issue of needing to make sure the implementation is sound).

If the issue is with it being inconsistent, perhaps we should make both versions panic?

[nicopap]

A few things:

• I found myself hitting that panic a few times already in different contexts, and had to revert to using the old "two inserts" way. So removing that escape hatch would be pretty annoying
• I'd love to see a compile time error for this. Would it be possible? For a flat struct that derives Bundle this is trivially possible. I don't think duplicate in tuples is much an issue (as it wasn't an issue with the old way of doing it)
• Currently it's very difficult to fix the code, because the panic message is very vague. It goes:

Bundle (bevy_pbr::bundle::MaterialMeshBundle<bevy_pbr::pbr_material::StandardMaterial>,
bevy_transform::components::transform::Transform) has duplicate components

For this particular example, it is fine, but when you have two distinct bundles with a matching component, especially from 3rd party crates, it becomes quickly intractable.

I'd be strongly against panicking in all case as long as the error message is not more helpful.

I'll give more concrete example where I hit this issue, but it's already late for me so I'll have to leave that for tomorrow.

[nicopap]

#6660 contains a compelling use case for non-panicking duplicate components. Reproducing here.

Note: B0004 recommends adding a SpatialBundle to the parent entity. However, following the advice for this particular situation since 0.9 will result in a panic because both Camera3dBundle and SpatialBundle add the GlobalTransform and Transform components (see #6622).

Following this, user AllenDang decided to replace SpatialBundle with the ComputedVisibility component. But for visibility propagation to work, both ComputedVisibility and Visibility components must be present. Hence, the child light was still invisible.

The fix was to add VisibilityBundle to the parent, rather than SpatialBundle or ComputedVisibility alone.

• Spawning duplicate components is inconsistent #6622

[nicopap]

I think the current behavior should be the one adopted. I think the only thing necessary would be to document the behavior