Add a `set_parent` method to `ChildBuilder` #5845

nicopap · 2022-09-01T08:44:28Z

Objective

Depending on the use-case, since #4197, it can be hard
to specify a hierarchy of entities in bevy, especially when
declaring parents.

Solution

Currently, it's possible to use parent.add_child(child),
however, there is a few cases where this is highly inconvenient,
and would be better expressed with a child.set_parent(parent) operation.

related: Add Transform-preserving hierarchy update method #5475

Changelog

Add a set_parent method to ChildBuilder
Add a with_world_mut method to EntityMut for improved safety.

This allows removing a few `unsafe` in child_builder.

djeedai

I'm not familiar enough with ECS to be sure the change is good, though I like the idea and it's certainly useful. Some remarks though on the documenting comments themselves that could be improved I think. Thanks!

djeedai · 2022-09-01T22:14:26Z

crates/bevy_ecs/src/world/entity_ref.rs

+    /// This is a safe alternative to [`EntityMut::world_mut`].
+    ///
+    /// If you _know_ that `use_world` doesn't change the current entity's location,
+    /// then `self.update_location` is extaneous and it might be more efficient to use


Suggested change

/// then `self.update_location` is extaneous and it might be more efficient to use

/// then `self.update_location` is extraneous and it might be more efficient to use

djeedai · 2022-09-01T22:16:01Z

crates/bevy_ecs/src/world/entity_ref.rs

@@ -508,6 +508,19 @@ impl<'w> EntityMut<'w> {
        self.world
    }

+    /// Access the inner `&mut World` within `use_world` and run [`Self::update_location`].


I've read the entire comment for this method but I have no idea what it means. This sounds like a fairly advanced usage. I don't know what use_world() does, nor update_location(). Can we try to give a little bit more detail, and maybe give an example of when this method is useful?

use_world is the function parameter. update_location is linked in the doc, because it is surrounded in [], if someone is reading the documentation and doesn't know what update_location is, they should be able to click on the link and read the update_location documentation.

Suggested change

/// Access the inner `&mut World` within `use_world` and run [`Self::update_location`].

/// Access the inner `&mut World` within the `use_world` function argument and run [`Self::update_location`].

djeedai · 2022-09-01T22:19:27Z

crates/bevy_hierarchy/src/child_builder.rs

@@ -166,7 +166,8 @@ impl Command for PushChildren {
    }
 }

-/// Command that removes children from an entity, and removes that child's parent and inserts it into the previous parent component
+/// Command that removes children from an entity,
+/// and removes that child's parent and inserts it into the previous parent component


I don't understand that second part. First, the use of singular on child is confusing since the first part of the sentence uses the plural children, though I assume we're talking here about the removed children? Then, we don't "remove the parent", we actually remove the Parent component from the Entity, don't we? And then I still don't understand the last bit about inserting into the previous parent component. Sorry I know the original comment was the same before your change, but if you could clean it up while you're touching it that'd be beneficial I think. Thanks!

It's the same comment than before, I only changed the formatting, I feel like this shouldn't be in scope of this PR, but I agree the phrasing can be improved.

Yep, I'm fine to leave this alone for this PR.

djeedai · 2022-09-01T22:22:53Z

crates/bevy_hierarchy/src/child_builder.rs

+    /// Spawns an [`Entity`] with no components
+    /// and inserts it into the children defined by the [`ChildBuilder`]
+    /// which adds the [`Parent`] component to it.


Same, that comment is quite confusing to me. How about:

/// Spawns an [`Entity`] without any component, and inserts it into the collection of children /// of the current [`ChildBuilder`]. A [`Parent`] component is automatically added to that new /// entity.

I think we can avoid bikeshedding on these doc changes here. I'm fine leaving them how they are or reverting them completely.

djeedai · 2022-09-01T22:23:46Z

crates/bevy_hierarchy/src/child_builder.rs

+
+    /// Set the parent.
+    ///
+    /// This overwrites the existing parent.


...I assume, for all children of the underlying collection at once?

The idea is that

child.set_parent(parent) // is equivalent to parent.add_child(child)

I don't understand what you mean by "for all children of the underlying collection". Could you precise?

If you mean the Children component of the parent, the answer is no. set_parent just sets the parent of an entity, so it does nothing else than:

Add a Parent(parent) component to the child

Push child to the Children component of the parent

djeedai · 2022-09-01T22:24:51Z

crates/bevy_hierarchy/src/child_builder.rs

+
+    fn set_parent(&mut self, parent: Entity) -> &mut Self {
+        let child = self.id();
+        self.commands().add(AddChild { child, parent });


Does that really work? What if there's already the opposite AddChild from the above add_child() call, do we correctly overwrite? I'm a bit surprised that we don't need to do any more book-keeping here.

I might add more test. But logically, if AddChild { parent, child } works, I don't see why AddChild { child, parent } wouldn't.

I'd like to see a simple test :)

djeedai · 2022-09-01T22:25:27Z

crates/bevy_hierarchy/src/child_builder.rs

@@ -345,7 +359,9 @@ impl<'w> WorldChildBuilder<'w> {
        self.world.entity_mut(entity)
    }

-    /// Spawns an [`Entity`] with no components and inserts it into the children defined by the [`WorldChildBuilder`] which adds the [`Parent`] component to it.
+    /// Spawns an [`Entity`] with no components and inserts it


Same remark as previously, comment is fairly confusing to me.

djeedai · 2022-09-01T22:27:18Z

crates/bevy_hierarchy/src/child_builder.rs

@@ -583,6 +584,7 @@ mod tests {
        assert_eq!(*world.get::<Parent>(child1).unwrap(), Parent(parent));
        assert_eq!(*world.get::<Parent>(child2).unwrap(), Parent(parent));

+        // Why are those assertions repeated?


Probably a mismerge. Can you please delete instead of adding a comment?

I wasn't willing to remove those asserts before someone reviewed the PR and told me they were not here for a reason I was overlooking (this is called "Chesterton's Fence")

So I added the comment to attract the attention of reviewers and allow them to comment on the asserts.

Yes absolutely, not blaming you here, that was the right call. But I think we both agree that this is useless and unlikely to have any real use? So I was just ACK'ing on the removal. Or do you want to wait for a second reviewer's opinion too? Im fine with that.

We should remove the duplicate assertions.

alice-i-cecile · 2022-09-02T12:01:01Z

crates/bevy_ecs/src/world/entity_ref.rs

+    /// then `self.update_location` is extaneous and it might be more efficient to use
+    /// the unsafe `world_mut` method instead.
+    #[inline]
+    pub fn with_world_mut(&mut self, use_world: impl FnOnce(&mut World)) {


This needs a doc example to be clear enough for the average user. Cool bit of machinery though!

alice-i-cecile · 2022-09-02T12:03:03Z

crates/bevy_hierarchy/src/child_builder.rs

-            // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype
-            self.update_location();
-        }
+        self.with_world_mut(|world| update_old_parents(world, parent, children));


Really like the elimination of unsafe here.

nicopap · 2022-11-04T11:15:53Z

Superseded by #6189

nicopap added 4 commits September 1, 2022 09:57

Add a safe world accessor method to EntityMut

4269966

This allows removing a few `unsafe` in child_builder.

Wrap child_builder doc comments

c582f3c

Add the set_parent method to ChildBuilder

624cca6

Add a question to the comments

359ac8c

nicopap added C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use A-Hierarchy Parent-child entity hierarchies labels Sep 1, 2022

djeedai reviewed Sep 1, 2022

View reviewed changes

alice-i-cecile reviewed Sep 2, 2022

View reviewed changes

nicopap closed this Nov 4, 2022

nicopap mentioned this pull request Nov 4, 2022

[Merged by Bors] - Fix unsound EntityMut::remove_children. Add EntityMut::world_scope #6464

Closed

nicopap deleted the set-parent branch August 30, 2023 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `set_parent` method to `ChildBuilder` #5845

Add a `set_parent` method to `ChildBuilder` #5845

nicopap commented Sep 1, 2022

djeedai left a comment

djeedai Sep 1, 2022

djeedai Sep 1, 2022

nicopap Sep 2, 2022

alice-i-cecile Sep 2, 2022

djeedai Sep 1, 2022

nicopap Sep 2, 2022

alice-i-cecile Sep 2, 2022

djeedai Sep 1, 2022

alice-i-cecile Sep 2, 2022

djeedai Sep 1, 2022

nicopap Sep 2, 2022

djeedai Sep 1, 2022

nicopap Sep 2, 2022

alice-i-cecile Sep 2, 2022

djeedai Sep 1, 2022

djeedai Sep 1, 2022

nicopap Sep 2, 2022 •

edited

Loading

djeedai Sep 2, 2022

alice-i-cecile Sep 2, 2022

alice-i-cecile Sep 2, 2022

alice-i-cecile Sep 2, 2022

nicopap commented Nov 4, 2022

	/// then `self.update_location` is extaneous and it might be more efficient to use
	/// then `self.update_location` is extraneous and it might be more efficient to use

	/// Access the inner `&mut World` within `use_world` and run [`Self::update_location`].
	/// Access the inner `&mut World` within the `use_world` function argument and run [`Self::update_location`].

Add a set_parent method to ChildBuilder #5845

Add a set_parent method to ChildBuilder #5845

Conversation

nicopap commented Sep 1, 2022

Objective

Solution

Changelog

djeedai left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicopap Sep 2, 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

nicopap commented Nov 4, 2022

Add a `set_parent` method to `ChildBuilder` #5845

Add a `set_parent` method to `ChildBuilder` #5845

nicopap Sep 2, 2022 •

edited

Loading