Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Entity cloning #16132

Merged
merged 18 commits into from
Dec 3, 2024
Merged

Entity cloning #16132

merged 18 commits into from
Dec 3, 2024

Conversation

eugineerd
Copy link
Contributor

@eugineerd eugineerd commented Oct 28, 2024

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 EntityCloneBuilder extensions: 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

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 DeferredWorld,
    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 Self {
        if add_observers {
            self.override_component_clone_handler::<ObservedBy>(ComponentCloneHandler::Custom(
                component_clone_observed_by,
            ))
        } else {
            self.remove_component_clone_handler_override::<ObservedBy>()
        }
    }
}

Testing

Includes some basic functionality tests and doctests.

Performance-wise this feature is the same as calling clone followed by insert for every entity component. There is also some inherent overhead due to every component clone handler having to access component data through World, but this can be reduced without breaking current public API in a later PR.

@pablo-lua pablo-lua added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 28, 2024
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Oct 28, 2024
@alice-i-cecile
Copy link
Member

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.

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Oct 28, 2024
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/clone_entities.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/clone_entities.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/clone_entities.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/clone_entities.rs Outdated Show resolved Hide resolved
@@ -1834,3 +1926,106 @@ impl RequiredComponents {
}
}
}

/// Component [clone handler function](ComponentCloneFn) implemented using the [`Clone`] trait.
/// Can be [set](ComponentCloneHandlers::set_component_handler) as clone handler for the specific component it is implemented for.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@eugineerd eugineerd Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

crates/bevy_ecs/src/entity/clone_entities.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/clone_entities.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/clone_entities.rs Outdated Show resolved Hide resolved
@eugineerd
Copy link
Contributor Author

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
Copy link
Contributor

chescock commented Nov 2, 2024

  • ~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
Copy link
Contributor Author

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.

@hymm hymm self-requested a review November 7, 2024 07:03
@eugineerd
Copy link
Contributor Author

Made all clone handlers use DeferredWorld instead of normal World in hopes of optimizing later without making breaking changes. Disallowing structural ECS changes would allow clone handlers to read component data from entity row in the table/sparse set without having to go through the World first. This requires a bit of unsafe and this PR already feels big enough, so I think it would be best to do that as a follow-up.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 2, 2024
@alice-i-cecile
Copy link
Member

Really impressive work: this is well-documented and engineered, and strikes a great balance between sensible defaults and the ability to configure more complex use cases. Merging! We can optimize this in follow-up.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 3, 2024
Merged via the queue into bevyengine:main with commit 2e267bb Dec 3, 2024
29 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Dec 6, 2024
## Objective

I was resolving a conflict between #16132 and my PR #15929 and thought
the `clone_entity` commands made more sense in `EntityCommands`.

## Solution

Moved `Commands::clone_entity` to `EntityCommands::clone`, moved
`Commands::clone_entity_with` to `EntityCommands::clone_with`.

## Testing

Ran the two tests that used the old methods.

## Showcase

```
// Create a new entity and keep its EntityCommands.
let mut entity = commands.spawn((ComponentA(10), ComponentB(20)));

// Create a clone of the first entity
let mut entity_clone = entity.clone();
```

The only potential downside is that the method name is now the same as
the one from the `Clone` trait. `EntityCommands` doesn't implement
`Clone` though, so there's no actual conflict.

Maybe I'm biased because this'll work better with my PR, but I think the
UX is nicer regardless.
@eugineerd eugineerd mentioned this pull request Dec 8, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 16, 2024
…rldMut` and `EntityCommands` (#16826)

## Objective

Thanks to @eugineerd's work on entity cloning (#16132), we now have a
robust way to copy components between entities. We can extend this to
implement some useful functionality that would have been more
complicated before.

Closes #15350.

## Solution

`EntityCloneBuilder` now automatically includes required components
alongside any component added/removed from the component filter.

Added the following methods to `EntityCloneBuilder`:
- `move_components`
- `without_required_components`

Added the following methods to `EntityWorldMut` and `EntityCommands`:
- `clone_with`
- `clone_components`
- `move_components`

Also added `clone_and_spawn` and `clone_and_spawn_with` to
`EntityWorldMut` (`EntityCommands` already had them).

## Showcase

```
assert_eq!(world.entity(entity_a).get::<B>(), Some(&B));
assert_eq!(world.entity(entity_b).get::<B>(), None);
world.entity_mut(entity_a).clone_components::<B>(entity_b);
assert_eq!(world.entity(entity_a).get::<B>(), Some(&B));
assert_eq!(world.entity(entity_b).get::<B>(), Some(&B));

assert_eq!(world.entity(entity_a).get::<C>(), Some(&C(5)));
assert_eq!(world.entity(entity_b).get::<C>(), None);
world.entity_mut(entity_a).move_components::<C>(entity_b);
assert_eq!(world.entity(entity_a).get::<C>(), None);
assert_eq!(world.entity(entity_b).get::<C>(), Some(&C(5)));
```
github-merge-queue bot pushed a commit that referenced this pull request Dec 18, 2024
# Objective

#16132 introduced entity cloning functionality, and while it works and
is useful, it can be made faster. This is the promised follow-up to
improve performance.

## Solution

**PREFACE**: This is my first time writing `unsafe` in rust and I have
only vague idea about what I'm doing. I would encourage reviewers to
scrutinize `unsafe` parts in particular.

The solution is to clone component data to an intermediate buffer and
use `EntityWorldMut::insert_by_ids` to insert components without
additional archetype moves.

To facilitate this, `EntityCloner::clone_entity` now reads all
components of the source entity and provides clone handlers with the
ability to read component data straight from component storage using
`read_source_component` and write to an intermediate buffer using
`write_target_component`. `ComponentId` is used to check that requested
type corresponds to the type available on source entity.

Reflect-based handler is a little trickier to pull of: we only have
`&dyn Reflect` and no direct access to the underlying data.
`ReflectFromPtr` can be used to get `&dyn Reflect` from concrete
component data, but to write it we need to create a clone of the
underlying data using `Reflect`. For this reason only components that
have `ReflectDefault` or `ReflectFromReflect` or `ReflectFromWorld` can
be cloned, all other components will be skipped. The good news is that
this is actually only a temporary limitation: once #13432 lands we will
be able to clone component without requiring one of these `type data`s.

This PR also introduces `entity_cloning` benchmark to better compare
changes between the PR and main, you can see the results in the
**showcase** section.

## Testing

- All previous tests passing
- Added test for fast reflect clone path (temporary, will be removed
after reflection-based cloning lands)
- Ran miri

## Showcase
Here's a table demonstrating the improvement:

| **benchmark** | **main, avg** | **PR, avg** | **change, avg** |
| ----------------------- | ------------- | ----------- |
--------------- |
| many components reflect | 18.505 µs | 2.1351 µs | -89.095% |
| hierarchy wide reflect* | 22.778 ms | 4.1875 ms | -81.616% |
| hierarchy tall reflect* | 107.24 µs | 26.322 µs | -77.141% |
| hierarchy many reflect | 78.533 ms | 9.7415 ms | -87.596% |
| many components clone | 1.3633 µs | 758.17 ns | -45.937% |
| hierarchy wide clone* | 2.7716 ms | 3.3411 ms | +20.546% |
| hierarchy tall clone* | 17.646 µs | 20.190 µs | +17.379% |
| hierarchy many clone | 5.8779 ms | 4.2650 ms | -27.439% |

*: these benchmarks have entities with only 1 component

## Considerations
Once #10154 is resolved a large part of the functionality in this PR
will probably become obsolete. It might still be a little bit faster
than using command batching, but the complexity might not be worth it.

## Migration Guide
- `&EntityCloner` in component clone handlers is changed to `&mut
ComponentCloneCtx` to better separate data.
- Changed `EntityCloneHandler` from enum to struct and added convenience
functions to add default clone and reflect handler more easily.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Chris Russell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command to clone entities
5 participants