Extend cloning functionality and add convenience methods to `EntityWorldMut` and `EntityCommands` #16826

JaySpruce · 2024-12-15T02:50:34Z

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 to or removed from the component filter.

Added the following methods to EntityCloneBuilder:

move_components
- Syntax: move_components(&mut self, enable: bool)
- Enables/disables removing the components from the original entity at the end of the cloning process, effectively moving them to the target entity.
without_required_components
- Syntax: without_required_components(&mut self, builder: impl FnOnce(&mut EntityCloneBuilder))
- Opens a scope where any components added to or removed from the filter will not automatically involve those components' required components.

Added the following methods to EntityWorldMut and EntityCommands:

clone_with
- Syntax: clone_with(&mut self, target: Entity, config: impl FnOnce(&mut EntityCloneBuilder))
- Easy access to EntityCloneBuilder.
clone_components
- Syntax: clone_components::<B: Bundle>(&mut self, target: Entity)
- Shortcut if you just want to clone components with default settings.
move_components
- Syntax: move_components::<B: Bundle>(&mut self, target: Entity)
- Shortcut if you want to move components with otherwise default settings.

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

Showcase

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

assert_eq!(world.entity(entity_a).get::<ComponentB>(), Some(&ComponentB(2)));
assert_eq!(world.entity(entity_b).get::<ComponentB>(), None);
world.entity_mut(entity_a).move_components::<ComponentB>(entity_b);
assert_eq!(world.entity(entity_a).get::<ComponentB>(), None);
assert_eq!(world.entity(entity_b).get::<ComponentB>(), Some(&ComponentB(2)));

eugineerd

I would suggest moving most of the functionality to EntityCloneBuilder which should allow to use it in clone_and_spawn methods as well. Also I'm a bit worried about the combinatorial explosion of methods - that is why I made EntityCloneBuilder in the first place. ( Now It's my time to nitpick :P )

eugineerd · 2024-12-15T06:10:28Z

crates/bevy_ecs/src/world/entity_ref.rs

+    pub fn clone_components_with_requires<B: Bundle>(&mut self, target: Entity) -> &mut Self {
+        self.assert_not_despawned();
+
+        let storages = &mut self.world.storages;
+        let components = &mut self.world.components;
+        let bundles = &mut self.world.bundles;
+
+        let bundle_id = bundles.register_contributed_bundle_info::<B>(components, storages);
+        // SAFETY: the `BundleInfo` for this `BundleId` is initialized above
+        let bundle_info = unsafe { bundles.get_unchecked(bundle_id) };
+        let component_ids = bundle_info.contributed_components().to_owned();
+
+        let mut builder = EntityCloneBuilder::new(self.world);
+        builder.deny_all().allow_by_ids(component_ids);
+        builder.clone_entity(self.entity, target);
+
+        self.world.flush();
+        self.update_location();
+        self
+    }


I'm pretty sure allowing and denying components with their required components can be done as a mode for EntityCloneBuilder, which should be more flexible:

builder.with_required_components(|builder| { builder .allow::<B>() // Calling with_required_components can set an internal flag which would .allow_by_ids(ids); // make all allow* and deny* methods also include contributed_components });

Something like this?

Code

pub fn with_required_components( &mut self, f: impl FnOnce(&mut EntityCloneBuilder) + Send + Sync + 'static, ) -> &mut Self { self.attach_required_components = true; f(self); self.attach_required_components = false; self } pub fn allow<T: Bundle>(&mut self) -> &mut Self { if self.filter_allows_components { T::get_component_ids(self.world.components(), &mut |id| { if let Some(id) = id { self.filter.insert(id); if self.attach_required_components { if let Some(info) = self.world.components().get_info(id) { for required in info.required_components().iter_ids() { self.filter.insert(required); } } } } }); } else { T::get_component_ids(self.world.components(), &mut |id| { if let Some(id) = id { self.filter.remove(&id); if self.attach_required_components { if let Some(info) = self.world.components().get_info(id) { for required in info.required_components().iter_ids() { self.filter.remove(&required); } } } } }); } self }

I think this code can actually be deduplicated a bit if we replace if self.filter_allows_components with something like this:

fn apply_filter( &mut self, id: ComponentId, ) { if self.filter_allows_components { self.filter.insert(id); } else { self.filter.remove(&id); } }

I can't get around the borrow checker to get that to work, but moving the ifs around is still an improvement

eugineerd · 2024-12-15T06:21:24Z

crates/bevy_ecs/src/world/entity_ref.rs

+    pub fn move_components<B: Bundle>(&mut self, target: Entity) -> &mut Self {
+        self.assert_not_despawned();
+
+        let mut builder = EntityCloneBuilder::new(self.world);
+        builder.deny_all().allow::<B>();
+        builder.clone_entity(self.entity, target);
+
+        let storages = &mut self.world.storages;
+        let components = &mut self.world.components;
+        let bundles = &mut self.world.bundles;
+        let bundle_id = bundles.register_info::<B>(components, storages);
+
+        // SAFETY: the `BundleInfo` for this `BundleId` is initialized above
+        unsafe { self.remove_bundle(bundle_id) };
+
+        self.world.flush();
+        self.update_location();
+        self
+    }


Moving components can also be a configuration option for EntityCloneBuilder that would call remove_bundle as a final step in clone_entity. Theoretically, it would also allow us to add an optimization in the future that would allow EntityCloner to move components that don't implement Clone or Reflect.

eugineerd · 2024-12-15T06:37:39Z

crates/bevy_ecs/src/world/entity_ref.rs

+    ///
+    /// - If this entity has been despawned while this `EntityWorldMut` is still alive.
+    /// - If the target entity does not exist.
+    pub fn move_components_with_requires<B: Bundle>(&mut self, target: Entity) -> &mut Self {


Maybe we can avoid combinatorial explosion of methods by just having move_components and move_components_with similar to spawn_and_clone? Although I'm not sure if configuring through EntityCloneBuilder is a good idea - it would allow to set cloning hierarchy which would not make much sense in the context of cloning components from one entity to the next. Maybe there should be a ComponentCloneBuilder or something similar that would re-expose only the methods that should be allowed?

JaySpruce · 2024-12-15T17:40:01Z

All true, I think I'll cut it down to these:

clone_with (gives plain access to the builder)
clone_and_spawn
clone_and_spawn_with
clone_components
move_components

I'll add the required components stuff to the builder, and tinker with the moving functionality too

alice-i-cecile · 2024-12-15T19:16:33Z

I really like this direction, and @eugineerd's feedback is excellent. Ping me when you need a review please :)

JaySpruce · 2024-12-16T00:37:55Z

I added the helper functions for the filter, so the allow/deny methods are slimmer now.

Required components work as discussed, the with_required_components method opening a sub-builder thing that includes required components in any allow/deny.

Moving components is just a bool in EntityCloneBuilder, which enables or disables removing all components that were cloned at the end of clone_entity.

This is the full syntax:

world.entity_mut(entity_a).clone_with(entity_b, |builder| {
    builder.deny_all();
    builder.enable_move_on_clone();
    builder.with_required_components(|builder| {
        builder.allow::<B>();
    });
});

Also added EntityWorldMut::remove_by_ids because I needed it (there was remove_by_id, so remove matches insert now, which has insert_by_id and insert_by_ids).

I think we're good to go for round 2, @alice-i-cecile & @eugineerd.

alice-i-cecile · 2024-12-16T05:29:50Z

crates/bevy_ecs/src/entity/clone_entities.rs

        }
        .clone_entity(world);

        world.flush_commands();
    }

+    /// Allows for a scoped mode where any changes to the filter that allow/deny components
+    /// will also allow/deny those components' required components, recursively.
+    pub fn with_required_components(


Very cool. @cart is almost certainly going to want to want this on by default, possibly without the ability to disable it. The vision is to treat these as "one indivisible object" wherever possible, at least for "required" components :)

I agree with that philosophy, and I think the toggle should basically be "replace required components with their default values". The default value should be "copy all required components that you can".

A bit off-topic for this PR, but I wonder if DynamicSceneBuilder should also extract required components in that case?

That's essentially what the toggle is, since components are ultimately inserted with normal insert methods. I can switch the default though.

Here's a couple colors for the bikeshed:

default_required_components

with_default_required_components

replace_required_components

replace_required_components_with_default

without_required_components

Yeah, we should probably be unifying the mechanisms between this and the scenes stuff. Another PR / issue though! As for the bikeshed, go with the long and clear one please: this is quite niche :)

alice-i-cecile · 2024-12-16T05:30:57Z

crates/bevy_ecs/src/entity/clone_entities.rs

+        self
+    }
+
+    /// The default setting for the cloner. The source entity will keep


The bit about this being the default should be in the second line, not the first.

alice-i-cecile · 2024-12-16T05:31:59Z

crates/bevy_ecs/src/system/commands/mod.rs

+    ///
+    /// # Panics
+    ///
+    /// The command will panic when applied if either of the entities do not exist.


Gah more panics :( Not your problem: this needs a holistic solution.

I do have another PR for that :P (not holistic, just commands). Not sure how it'll interact with work in the fallible systems area though

alice-i-cecile · 2024-12-16T05:33:19Z

crates/bevy_ecs/src/world/entity_ref.rs

+    /// [`Clone`] or [`Reflect`](bevy_reflect::Reflect).
+    ///
+    /// Configure through [`EntityCloneBuilder`] as follows:
+    /// ```ignore


Don't ignore this: make it work (possibly hiding lines for importing dependencies / boilerplate). Ignored doc tests are extremely brittle.

alice-i-cecile · 2024-12-16T05:35:45Z

crates/bevy_ecs/src/entity/clone_entities.rs

+    ///
+    /// This only applies to components that are allowed through the filter
+    /// at the time [`EntityCloneBuilder::clone_entity`] is called.
+    pub fn enable_move_on_clone(&mut self) -> &mut Self {


Bikeshed: I think these methods might be clearer if they were move_components and keep_components. The enable/disable terminology was confusing to me at first.

I think this should just be one method: move_components that takes a bool. That way it would be in line with the other EntityCloneBuilder options (like add_observers). It was designed to look somewhat similar to std::fs::OpenOptions.

eugineerd

Looks good to me! I like the addition of various convenience methods and move component functionality is pretty useful.

I have also prepared an optimization based on code from #16717 for move_components that would allow us to move components that don't implement Reflect or Clone after both PRs are merged.

alice-i-cecile

I'm happy with the state of this now; let's merge and refine in follow-up :)

add clone_components, move_components, and variants

1d6e8d1

alice-i-cecile modified the milestones: 0.15.1, 0.16 Dec 15, 2024

eugineerd reviewed Dec 15, 2024

View reviewed changes

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 15, 2024

move variant functionality to builder, add clone_with

4e6abaf

JaySpruce changed the title ~~Add clone_components and move_components (and variants) to EntityWorldMut and EntityCommands~~ Extend cloning functionality and add convenience methods to EntityWorldMut and EntityCommands Dec 16, 2024

alice-i-cecile reviewed Dec 16, 2024

View reviewed changes

implement suggestions

97642e8

eugineerd approved these changes Dec 16, 2024

View reviewed changes

alice-i-cecile approved these changes Dec 16, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Dec 16, 2024

Merged via the queue into bevyengine:main with commit 5a94beb Dec 16, 2024
28 checks passed

JaySpruce deleted the moving_and_cloning branch December 16, 2024 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend cloning functionality and add convenience methods to `EntityWorldMut` and `EntityCommands` #16826

Extend cloning functionality and add convenience methods to `EntityWorldMut` and `EntityCommands` #16826

JaySpruce commented Dec 15, 2024 •

edited

Loading

eugineerd left a comment •

edited

Loading

eugineerd Dec 15, 2024

JaySpruce Dec 15, 2024 •

edited

Loading

eugineerd Dec 15, 2024

JaySpruce Dec 15, 2024

eugineerd Dec 15, 2024

eugineerd Dec 15, 2024

JaySpruce commented Dec 15, 2024

alice-i-cecile commented Dec 15, 2024

JaySpruce commented Dec 16, 2024

alice-i-cecile Dec 16, 2024

eugineerd Dec 16, 2024

JaySpruce Dec 16, 2024 •

edited

Loading

alice-i-cecile Dec 16, 2024

alice-i-cecile Dec 16, 2024

alice-i-cecile Dec 16, 2024

JaySpruce Dec 16, 2024

alice-i-cecile Dec 16, 2024

alice-i-cecile Dec 16, 2024

eugineerd Dec 16, 2024

eugineerd left a comment

alice-i-cecile left a comment

Extend cloning functionality and add convenience methods to EntityWorldMut and EntityCommands #16826

Extend cloning functionality and add convenience methods to EntityWorldMut and EntityCommands #16826

Conversation

JaySpruce commented Dec 15, 2024 • edited Loading

Objective

Solution

Showcase

eugineerd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaySpruce Dec 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaySpruce commented Dec 15, 2024

alice-i-cecile commented Dec 15, 2024

JaySpruce commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaySpruce Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugineerd left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

Extend cloning functionality and add convenience methods to `EntityWorldMut` and `EntityCommands` #16826

Extend cloning functionality and add convenience methods to `EntityWorldMut` and `EntityCommands` #16826

JaySpruce commented Dec 15, 2024 •

edited

Loading

eugineerd left a comment •

edited

Loading

JaySpruce Dec 15, 2024 •

edited

Loading

JaySpruce Dec 16, 2024 •

edited

Loading