Add MoveBundle command #3717

hlb8122 · 2022-01-18T11:40:23Z

Objective

Moving one component from a entity to another entity is a common operation. It would make sense if this was supported out the box.

Solution

Create a move_bundle API on EntityCommands which results in a MoveBundle command. The MoveBundle command peforms EntityMut::remove_bundle::<T> on the source entity and then EntityMut::insert_bundle::<T> on the target entity.

Possible Additions

It might be reasonable to also add a CloneBundle which does the same except replaces remove::<T> with get::<T> then a clone.

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

DJMcNab

We also want a version to move a single component. The normal scheme would be to call that move:

fn move<C: Component>(&mut self,target: Entity) {
    self.move_bundle::<(C,)>(target)
}

And obviously these need some docs

But otherwise this looks good to me. Ideally we'd have an example of using this somewhere, but I don't know that any of our examples would want this.

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

hlb8122 · 2022-01-31T00:28:08Z

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

+    ///     }
+    /// }
+    /// ```
+    pub fn r#move<C: Component>(&mut self, target: Entity) -> &mut Self {


What should we do about r#move?

One possible side step here is to name these move_to and move_bundle_to.

#3820 could then be clone_to and clone_bundle_to.

This would also allow for the addition of move_from, which would take the source Entity as an argument.

IMO move_to is probably better. And then do the same with clone_to.

@alice-i-cecile Will do, I'm also happy with move_onto, if you'd prefer that?

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

alice-i-cecile

Fix up the names then this LGTM.

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

alice-i-cecile · 2022-02-17T19:42:55Z

IMO we should add the move_from variants in the same PR. I've been writing the clonable-entities RFC and having both directions is very useful for ergonomics.

hlb8122 · 2022-02-17T19:55:51Z

@alice-i-cecile Should I link to the *_to methods in the *_from documentation and then omit the doc tests?

hlb8122 · 2022-02-17T21:18:57Z

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

@@ -512,6 +512,95 @@ impl<'w, 's, 'a> EntityCommands<'w, 's, 'a> {
        self
    }

+    /// Moves a [`Bundle`] to a target entity.


/// {Moves/Clones} a [`{Component/Bundle}`] {to/from} a {target/source} entity.

I've picked this for all the documentation across the MoveBundle and CloneBundle PR. If anyone has some better suggestions I can change it because, as is, this is pretty aburpt.

B-Reif · 2022-02-26T22:35:11Z

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

+        let bundle_opt = if let Some(mut source) = world.get_entity_mut(self.source) {
+            source.remove_bundle::<T>()
+        } else {
+            panic!("Could not move a bundle (of type `{}`) from entity {:?} because it doesn't exist in this World.\n\


Nit: can we extract a function to do this panic instead of copying the message?

There's a slight difference between the two:

from entity

and

to entity

Which means it might be a bit gnarly. e.g.

fn panic_fmt<Ty>(verb: &'static str, preposition: &'static str, entity: Entity) { panic!("Could not {} a bundle (of type `{}`) {} entity {:?} because it doesn't exist in this World.\n\ If this command was added to a newly spawned entity, ensure that you have not despawned that entity within the same stage.\n\ This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", verb, std::any::type_name::<Ty>(), preposition, entity); }

Ye/nay?

Yeah I like this.

@alice-i-cecile @B-Reif 8bc7023 hows that look?

I can do it for https://github.com/bevyengine/bevy/pull/3717/files#diff-ebaeb267b15dfe866ccce4f60af1f90f8fd44f31fa1b6ac7529f4b41f38b743bR576 too if you'd like?

Yes please; let's keep this consistent.

mockersf · 2022-06-08T22:05:10Z

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

+        let bundle = if let Some(some) = bundle_opt {
+            some
+        } else {
+            return;


should it panic in this case?

If we're being consistent, yes.

I dislike this behavior, but that's for a different PR.

alice-i-cecile · 2023-01-08T21:43:20Z

@hlb8122 do you want to clean this up so we can merge it?

NthTensor · 2024-09-17T18:17:01Z

This looks like it has gone stale, and required components change the landscape here significantly. Closing as not planned.

alice-i-cecile · 2024-09-17T18:19:13Z

I still think this is a compelling idea, but I agree, this needs to be completely remade.

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 18, 2022

hlb8122 commented Jan 18, 2022

View reviewed changes

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved

DJMcNab reviewed Jan 18, 2022

View reviewed changes

mockersf added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jan 18, 2022

alice-i-cecile reviewed Jan 18, 2022

View reviewed changes

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved

hlb8122 commented Jan 31, 2022

View reviewed changes

hlb8122 marked this pull request as ready for review January 31, 2022 00:28

hlb8122 mentioned this pull request Jan 31, 2022

Add CloneBundle command #3820

Closed

hlb8122 force-pushed the add-move-component branch from d104951 to da17cc9 Compare January 31, 2022 11:15

hlb8122 commented Jan 31, 2022

View reviewed changes

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved

hlb8122 force-pushed the add-move-component branch 2 times, most recently from def26d4 to 2c769cb Compare February 6, 2022 13:33

alice-i-cecile requested changes Feb 17, 2022

View reviewed changes

mockersf reviewed Feb 17, 2022

View reviewed changes

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved

hlb8122 force-pushed the add-move-component branch from f3f29d9 to cb07eff Compare February 17, 2022 20:53

hlb8122 commented Feb 17, 2022

View reviewed changes

Add MoveBundle command

f807e2f

hlb8122 force-pushed the add-move-component branch from 9381fed to f807e2f Compare February 19, 2022 11:56

Fix assert

51bfad2

B-Reif reviewed Feb 26, 2022

View reviewed changes

alice-i-cecile approved these changes Feb 26, 2022

View reviewed changes

Add panic_fmt method

8bc7023

hlb8122 force-pushed the add-move-component branch from 72a9056 to 8bc7023 Compare March 8, 2022 11:30

mockersf reviewed Jun 8, 2022

View reviewed changes

NthTensor closed this Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MoveBundle command #3717

Add MoveBundle command #3717

hlb8122 commented Jan 18, 2022 •

edited

Loading

DJMcNab left a comment

hlb8122 Jan 31, 2022

hlb8122 Jan 31, 2022 •

edited

Loading

hlb8122 Jan 31, 2022

alice-i-cecile Feb 17, 2022

hlb8122 Feb 17, 2022

alice-i-cecile left a comment

alice-i-cecile commented Feb 17, 2022

hlb8122 commented Feb 17, 2022

hlb8122 Feb 17, 2022 •

edited

Loading

B-Reif Feb 26, 2022

hlb8122 Mar 8, 2022 •

edited

Loading

alice-i-cecile Mar 8, 2022

hlb8122 Mar 8, 2022 •

edited

Loading

hlb8122 Mar 8, 2022

alice-i-cecile Mar 8, 2022

mockersf Jun 8, 2022

alice-i-cecile Jun 8, 2022

alice-i-cecile commented Jan 8, 2023

NthTensor commented Sep 17, 2024

alice-i-cecile commented Sep 17, 2024

Add MoveBundle command #3717

Add MoveBundle command #3717

Conversation

hlb8122 commented Jan 18, 2022 • edited Loading

Objective

Solution

Possible Additions

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlb8122 Jan 31, 2022 • 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

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Feb 17, 2022

hlb8122 commented Feb 17, 2022

hlb8122 Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlb8122 Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlb8122 Mar 8, 2022 • 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

alice-i-cecile commented Jan 8, 2023

NthTensor commented Sep 17, 2024

alice-i-cecile commented Sep 17, 2024

hlb8122 commented Jan 18, 2022 •

edited

Loading

hlb8122 Jan 31, 2022 •

edited

Loading

hlb8122 Feb 17, 2022 •

edited

Loading

hlb8122 Mar 8, 2022 •

edited

Loading

hlb8122 Mar 8, 2022 •

edited

Loading