Entity cloning

[eugineerd]

Objective

Fixes #1515

This PR implements a flexible entity cloning system. The primary use case for it is to clone dynamically-generated entities.

Example:

#[derive(Component, Clone)]
pub struct Projectile;

#[derive(Component, Clone)]
pub struct Damage {
    value: f32,
}

fn player_input(
    mut commands: Commands,
    projectiles: Query<Entity, With<Projectile>>,
    input: Res<ButtonInput<KeyCode>>,
) {
    // Fire a projectile
    if input.just_pressed(KeyCode::KeyF) {
        commands.spawn((Projectile, Damage { value: 10.0 }));
    }

    // Triplicate all active projectiles
    if input.just_pressed(KeyCode::KeyT) {
        for projectile in projectiles.iter() {
            // To triplicate a projectile we need to create 2 more clones
            for _ in 0..2{
                commands.clone_entity(projectile)
            }
        }
    }
}

Solution

Commands

Add a clone_entity command to create a clone of an entity with all components that can be cloned. Components that can't be cloned will be ignored.

commands.clone_entity(entity)

If there is a need to configure the cloning process (like set to clone recursively), there is a second command:

commands.clone_entity_with(entity, |builder| {
    builder.recursive(true)
});

Both of these commands return EntityCommands of the cloned entity, so the copy can be modified afterwards.

Builder

All these commands use EntityCloneBuilder internally. If there is a need to clone an entity using World instead, it is also possible:

let entity = world.spawn(Component).id();
let entity_clone = world.spawn_empty().id();
EntityCloneBuilder::new(&mut world).clone_entity(entity, entity_clone);

Builder has methods to allow or deny certain components during cloning if required and can be extended by implementing traits on it. This PR includes two: CloneEntityWithObserversExt to configure adding cloned entity to observers of the original entity, and CloneEntityRecursiveExt to configure cloning an entity recursively.

Clone implementations

By default, all components that implement either Clone or Reflect will be cloned (with Clone-based implementation preferred in case component implements both).

This can be overriden on a per-component basis:

impl Component for SomeComponent {
    const STORAGE_TYPE: StorageType = StorageType::Table;

    fn get_component_clone_handler() -> ComponentCloneHandler {
        // Don't clone this component
        ComponentCloneHandler::Ignore
    }
}

ComponentCloneHandlers (component clone handler registry)

Clone implementation specified in get_component_clone_handler will get registered in ComponentCloneHandlers (stored in bevy_ecs::component::Components) at component registration time.

The clone handler implementation provided by a component can be overriden after registration like so:

let component_id = world.components().component_id::<Component>().unwrap()
world.get_component_clone_handlers_mut()
     .set_component_handler(component_id, ComponentCloneHandler::Custom(component_clone_custom))

The default clone handler for all components that do not explicitly define one (or don't derive Component) is component_clone_via_reflect if bevy_reflect feature is enabled, and component_clone_ignore (noop) otherwise.
Default handler can be overriden using ComponentCloneHandlers::set_default_handler

Handlers

Component clone handlers can be used to modify component cloning behavior. The general signature for a handler that can be used in ComponentCloneHandler::Custom is as follows:

pub fn component_clone_custom(
    world: &mut World,
    component_id: ComponentId,
    entity_cloner: &EntityCloner,
) {
    // implementation
}

The EntityCloner implementation (used internally by EntityCloneBuilder) assumes that after calling this custom handler, the target entity has the desired version of the component from the source entity.

Builder handler overrides

Besides component-defined and world-overriden handlers, EntityCloneBuilder also has a way to override handlers locally. It is mainly used to allow configuration methods like recursive and add_observers.

// From observer clone handler implementation
impl CloneEntityWithObserversExt for EntityCloneBuilder {
    fn add_observers(&mut self, add_observers: bool) -> &mut EntityCloneBuilder {
        if add_observers {
            self.override_component_clone_handler::<ObservedBy>(ComponentCloneHandler::Custom(
                component_clone_observed_by,
            ))
        } else {
            self.override_component_clone_handler::<ObservedBy>(ComponentCloneHandler::Ignore)
        }
    }
}

Testing

Includes some basic functionality tests, but haven't tested it on any real-world projects yet.
Also it would make sense to do some performance testing, I haven't done any yet.

Reflection-based component code is not as optimized as Clone-based approach. It could probably be optimized by adding some unsafe, but I'm not comfortable enough with that yet and in general Clone-based approach would theoretically still be faster.

[alice-i-cecile]

I have wanted this feature for years. Thank you so much for taking a crack at it! I don't have time to thoroughly review this right now, but please pester me during 0.16 to make sure this gets merged.

[chescock]

This seems really useful! I have some nits and suggestions, but nothing that should block it.

[chescock]

Oh, this is really clever!

This won't work for conditional impls, right? Like, #[derive(Component, Clone)] struct Foo<T>{ ... } won't use component_clone_via_clone even when T: Clone. It might be worth noting that in a doc comment somewhere.

[eugineerd]

That's true! Didn't think about that because generic components in general are a bit of a pain to work with. Not really sure where to add a note about that though.

[chescock]

Maybe there should be a section in the doc comments for EntityCloneBuilder that explain the strategy for choosing the clone handler. There are actually quite a few ways it can be set: EntityCloneBuilder::override_component_clone_handler(), Component::get_component_clone_handler(), ComponentCloneHandlers::set_component_handler(), ComponentCloneHandlers::set_default_handler(), and the bevy_reflect feature flag. And the docs for those methods could link back to that section so you can see their context.

[chescock]

The allowed field could use a doc comment, since the meaning isn't obvious from its name. It determines the meaning of filter, right?

[eugineerd]

Yes, will probably change it to filter_allows_components, I've gotten confused about what allowed means multiple times already.

[chescock]

Could you do this with a filter() call before collect() so that you don't need to allocate space in the Vec for components that will be skipped?

[eugineerd]

Agree, will change.

[chescock]

The double lookup here seems suboptimal. Could there be another version of get_handler that returns an Option so this can be a single lookup followed by an unwrap_or_else()?

For that matter, why is clone_handlers_overrides a ComponentCloneHandlers instead of a plain HashMap<ComponentId, ComponentCloneFn>? That adds some complexity, and you don't use the default_handler field. I think it also means that override_component_clone_handler(id, ComponentCloneHandler::Default) removes any overrides instead of overriding a custom handler with the default one, which seems surprising.

[eugineerd]

I wanted to reuse the interface of ComponentCloneHandlers and had some plans to use default_handler to replace filter entirely, but this is not true anymore. Agreed, should probably replace it with something simpler.

[chescock]

I think it would be more idiomatic to name these fn source() and fn target() without the get_ prefix.

[eugineerd]

Yeah, it seems that way, will change.

[chescock]

Would it make sense to have a helper method that calls set_component_handler(id, component_clone_via_clone::<C>) with the correct id? You couldn't put it on ComponentCloneHandlers since you can't get the component_id from that, but you could put it on World or Components.

[eugineerd]

I'm not sure how useful that will be. I though that most users would not need to add component_clone_via_clone handlers at runtime, instead they would manually impl Clone for a component or set a handler in get_component_clone_handler. Is it for components with conditional Clone implementation with generics?

[chescock]

Yeah, I was thinking about the conditional Clone implementations. One of the reasons it seems perfectly fine to use default cloning in that case is that anyone who really needs component_clone_via_clone can just register it for the concrete component types! But I agree that's likely to be rare, so it's probably not worth adding extra methods for it unless we see folks doing that in the wild.

[chescock]

It would be good to have a way to use ComponentId instead of TypeId to support dynamic components that don't have TypeIds. Methods like that are usually named _id, so that's what I thought these were!

[chescock]

For that matter... why store TypeId instead of ComponentId? If you store the &mut World in the EntityCloneBuilder instead of taking it as a parameter to clone_entity, then you can convert from TypeId to ComponentId immediately. That's what QueryBuilder does, for example. That would also let you handle bundles without needing to store a fn pointer.

[eugineerd]

Yeah, I think this is a leftover from the previous design. Storing ComponentIds makes more sense.

[chescock]

Should these methods explicitly clear the opposite list? I think cloner.deny::<A>().allow::<B>().deny::<C>() will deny "A", even though the allow method in the middle claims to have disallowed it.

Alternately, it might be more clear to require deny_all() to be called, and then add or remove from the sets as needed, so cloner.deny_all().allow::<A>().allow::<B>().deny::<A>() would re-deny A while still allowing B.

Not that anyone is likely to write either of those things...

[eugineerd]

Reworked filtering using the alternative approach proposed.

[chescock]

There is a blanket impl <C: Component> Bundle for C, so you could combine this method with allow<T: Component>() and just have a single allow() and deny() that works for any bundle, including plain components.

[eugineerd]

Agreed, I just though that storing a closure instead of an id just to allow a single component might not be optimal, but if builder is changed to store ComponentIds anyway using World that should be fine.

[eugineerd]

I did some basic profiling to see how cloning compares to just spawning an entity and performance is not that great (all time is relative to total runtime, as reported by perf):

• Spawning 100 000 entities with 10 Clone-able components took ~15% of runtime, while cloning the entities took ~80% of runtime.
• ComponentCloneHandlers::get_handler is surprisingly slow ~4.2% of runtime is spent there. This can be optimized by replacing HashMap by Vec and using ComponentId as index. This would increase the memory footprint a bit, but since Components uses the same trick to store ComponentInfo I think it should be fine.
• ~53% of time is spent inside EntityWorldMut::insert. This can be reduced by avoiding archetype moves with some unsafe code inserting all components using EntityWorldMut::insert_by_ids and changing ComponentCloneHandlers to do cloning by providing them with a pointer where to write the cloned value.
• ~8.2% of time is spent allocating a Vec for ComponentIds. This seems to go down to ~2.2% if I preallocate Vec using Archetype::component_count. Maybe Archetype::components doesn't give correct size hint to the iterator? I'm not sure.

I think optimizations for ComponentCloneHandlers::get_handler and ComponentId allocations can be added to this PR, but what about optimizing component insertion? Should it also be done in this PR or maybe this can be added later (along with batch cloning)?

[chescock]

• ~8.2% of time is spent allocating a Vec for ComponentIds. This seems to go down to ~2.2% if I preallocate Vec using Archetype::component_count. Maybe Archetype::components doesn't give correct size hint to the iterator? I'm not sure.

Oh, sorry, I bet this was due to my suggestion to use filter there!

Should it also be done in this PR or maybe this can be added later (along with batch cloning)?

I'm not a maintainer, but I vote for merging this first and doing performance improvements as a follow-up, especially if you don't expect the perf improvements to change the public API. That lets it get used sooner in places where the current perf is already good enough!

[eugineerd]

especially if you don't expect the perf improvements to change the public API.

I think from the current public API only custom clone handlers (ComponentCloneFn) and EntityCloner might not have enough information/are too flexible (giving &mut World to handlers) to implement cloning more efficiently. I have a prototype that 2x the cloning performance (and I expect there are ways to improve it even more), however it is very unsafe and I don't yet know how to implement it safely in user-friendly way.

Thinking about it a bit more though, maybe the current flexible API can co-exist with the new more optimized but limited API that will be implemented later.