Improved Entity Mapping and Cloning

[cart]

Fixes #17535

Bevy's approach to handling "entity mapping" during spawning and cloning needs some work. The addition of Relations both introduced a new "duplicate entities" bug when spawning scenes in the scene system and made the weaknesses of the current mapping system exceedingly clear:

1. Entity mapping requires a ton of boilerplate (implement or derive VisitEntities and VisitEntitesMut, then register / reflect MapEntities). Knowing the incantation is challenging and if you forget to do it in part or in whole, spawning subtly breaks.
2. Entity mapping a spawned component in scenes incurs unnecessary overhead: look up ReflectMapEntities, create a brand new temporary instance of the component using FromReflect, map the entities in that instance, and then apply that on top of the actual component using reflection. We can do much better.

Additionally, while our new Entity cloning system is already pretty great, it has some areas we can make better:

• It doesn't expose semantic info about the clone (ex: ignore or "clone empty"), meaning we can't key off of that in places where it would be useful, such as scene spawning. Rather than duplicating this info across contexts, I think it makes more sense to add that info to the clone system, especially given that we'd like to use cloning code in some of our spawning scenarios.
• EntityCloner is currently built in a way that prioritizes a single entity clone
• EntityCloner's recursive cloning is built to be done "inside out" in a parallel context (queue commands that each have a clone of EntityCloner). By making EntityCloner the orchestrator of the clone we can remove internal arcs, improve the clarity of the code, make EntityCloner mutable again, and simplify the builder code.
• EntityCloner does not currently take into account entity mapping. This is necessary to do true "bullet proof" cloning, would allow us to unify the per-component scene spawning and cloning UX, and ultimately would allow us to use EntityCloner in place of raw reflection for scenes like Scene(World) (which would give us a nice performance boost: fewer archetype moves, less reflection overhead).

Solution

Improved Entity Mapping

First, components now have first-class "entity visiting and mapping" behavior:

#[derive(Component, Reflect)]
#[reflect(Component)]
struct Inventory {
    size: usize,
    #[entities]
    items: Vec<Entity>,
}

Any field with the #[entities] annotation will be viewable and mappable when cloning and spawning scenes.

Compare that to what was required before!

#[derive(Component, Reflect, VisitEntities, VisitEntitiesMut)]
#[reflect(Component, MapEntities)]
struct Inventory {
    #[visit_entities(ignore)]
    size: usize,
    items: Vec<Entity>,
}

Additionally, for relationships #[entities] is implied, meaning this "just works" in scenes and cloning:

#[derive(Component, Reflect)]
#[relationship(relationship_target = Children)]
#[reflect(Component)]
struct ChildOf(pub Entity);

Note that Component does not implement VisitEntities directly. Instead, it has Component::visit_entities and Component::visit_entities_mut methods. This is for a few reasons:

1. We cannot implement VisitEntities for C: Component because that would conflict with our impl of VisitEntities for anything that implements IntoIterator<Item=Entity>. Preserving that impl is more important from a UX perspective.
2. We should not implement Component: VisitEntities VisitEntities in the Component derive, as that would increase the burden of manual Component trait implementors.
3. Making VisitEntitiesMut directly callable for components would make it easy to invalidate invariants defined by a component author. By putting it in the Component impl, we can make it harder to call naturally / unavailable to autocomplete using fn visit_entities_mut(this: &mut Self, ...).

ReflectComponent::apply_or_insert is now ReflectComponent::apply_or_insert_mapped. By moving mapping inside this impl, we remove the need to go through the reflection system to do entity mapping, meaning we no longer need to create a clone of the target component, map the entities in that component, and patch those values on top. This will make spawning mapped entities much faster (The default Component::visit_entities_mut impl is an inlined empty function, so it will incur no overhead for unmapped entities).

The Bug Fix

To solve #17535, spawning code now skips entities with the new ComponentCloneBehavior::Ignore and ComponentCloneBehavior::RelationshipTarget variants (note RelationshipTarget is a temporary "workaround" variant that allows scenes to skip these components. This is a temporary workaround that can be removed as these cases should really be using EntityCloner logic, which should be done in a followup PR. When that is done, ComponentCloneBehavior::RelationshipTarget can be merged into the normal ComponentCloneBehavior::Custom).

Improved Cloning

• Option<ComponentCloneHandler> has been replaced by ComponentCloneBehavior, which encodes additional intent and context (ex: Default, Ignore, Custom, RelationshipTarget (this last one is temporary)).
• Global per-world entity cloning configuration has been removed. This felt overly complicated, increased our API surface, and felt too generic. Each clone context can have different requirements (ex: what a user wants in a specific system, what a scene spawner wants, etc). I'd prefer to see how far context-specific EntityCloners get us first.
• EntityCloner's internals have been reworked to remove Arcs and make it mutable.
• EntityCloner is now directly stored on EntityClonerBuilder, simplifying the code somewhat
• EntityCloner's "bundle scratch" pattern has been moved into the new BundleScratch type, improving its usability and making it usable in other contexts (such as future cross-world cloning code). Currently this is still private, but with some higher level safe APIs it could be used externally for making dynamic bundles
• EntityCloner's recursive cloning behavior has been "externalized". It is now responsible for orchestrating recursive clones, meaning it no longer needs to be sharable/clone-able across threads / read-only.
• EntityCloner now does entity mapping during clones, like scenes do. This gives behavior parity and also makes it more generically useful.
• RelatonshipTarget::RECURSIVE_SPAWN is now RelationshipTarget::LINKED_SPAWN, and this field is used when cloning relationship targets to determine if cloning should happen recursively. The new LINKED_SPAWN term was picked to make it more generically applicable across spawning and cloning scenarios.

Next Steps

• I think we should adapt EntityCloner to support cross world cloning. I think this PR helps set the stage for that by making the internals slightly more generalized. We could have a CrossWorldEntityCloner that reuses a lot of this infrastructure.
• Once we support cross world cloning, we should use EntityCloner to spawn Scene(World) scenes. This would yield significant performance benefits (no archetype moves, less reflection overhead).

[bushrat011899]

Wasm CI issues should be resolved once you upstream from main.

[eugineerd]

Mostly looked at the cloning changes here since this is the part I'm most familiar with.

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

[alice-i-cecile]

The entity mapping changes will have implications for our networking ecosystem too. Hopefully positive! I've pulled in a few reviewers from there to get their opinions.

[alice-i-cecile]

Alright, not as bad to review as I was worried about. I have a few cleanup nits, but the general strategy and motivation is sound, and I really like making the clone + despawn behavior a trait constant that we use for both behaviors.

Making sure that VisitEntities mentions and explains this new behavior is the main thing I want to see improved before this is merged.

Removing Arcs in EntityCloner makes me particularly happy; I'm glad you found a way to do that. Eliminating global cloning config also seems reasonable; I agree with your argument that this decision is extremly contextual.

This easy path for entity mapping will further widen the gap between components and resources, but as laid out in #17485, I don't think further duplicating features like this is sustainable.

I like both of your follow-up suggestions, but unsuprisingly I'd prefer they be done in later PRs :)

[alice-i-cecile]

This could use a more descriptive variable name; the previous name was much clearer.

[alice-i-cecile]

I'd like more docs on exactly what "recursive" means, probably on EntityCloner.

[alice-i-cecile]

I feel like the docs for MapEntities should call out the new behavior / possibilities on Component.

[alice-i-cecile]

Suggested change

	/// Maps the `target` entity to the given `source`. For some implementations this might not actually determine the result
	/// Maps the `target` entity to the given `source`. For some implementations this might not actually determine the result

[alice-i-cecile]

Suggested change

	/// If this is true, when despawning or cloning (when [recursion is enabled](crate::entity::EntityClonerBuilder::recursive)), the related entities targeting this entity will also be despawned or cloned.
	/// If this is true, when despawning or cloning (when [recursion is enabled](crate::entity::EntityClonerBuilder::recursive)), the related entities targeting this entity will also be despawned or cloned.
	///
	/// For example, this is set to `true` for Bevy's built-in parent-child relation, defined by [`ChildOf`](crate::prelude::ChildOf) and [`Children`](crate::prelude::Children).
	/// This means that when a parent is despawned, any children targeting that parent are also despawned (and the same applies to cloning).
	///
	/// To get around this behavior, you can first break the relationship between entities, and *then* despawn or clone.

[alice-i-cecile]

I don't understand why _commands exists here if this is unused.

[alice-i-cecile]

Nit: this feels like a bad place for these tests in general (3k lines?!), but given that all of the other component derive tests are here that's a change for a follow-up PR.