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.

Sign up for GitHub

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

Jump to bottom

Add a set_parent method to ChildBuilder #5845

Closed
wants to merge 4 commits into from
Closed

Add a set_parent method to ChildBuilder #5845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4269966
Add a safe world accessor method to EntityMut
nicopap Sep 1, 2022
c582f3c
Wrap child_builder doc comments
nicopap Sep 1, 2022
624cca6
Add the set_parent method to ChildBuilder
nicopap Sep 1, 2022
359ac8c
Add a question to the comments
nicopap Sep 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions crates/bevy_ecs/src/world/entity_ref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,19 @@ impl<'w> EntityMut<'w> {
self.world
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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`].

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

Choose a reason for hiding this comment

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

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

/// the unsafe `world_mut` method instead.
#[inline]
pub fn with_world_mut(&mut self, use_world: impl FnOnce(&mut World)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

use_world(self.world);
self.update_location();
}

/// Return this `EntityMut`'s [`World`], consuming itself.
#[inline]
pub fn into_world_mut(self) -> &'w mut World {
Expand Down
56 changes: 29 additions & 27 deletions crates/bevy_hierarchy/src/child_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

pub struct RemoveChildren {
parent: Entity,
children: SmallVec<[Entity; 8]>,
Expand All @@ -192,7 +193,9 @@ impl<'w, 's, 'a> ChildBuilder<'w, 's, 'a> {
e
}

/// Spawns an [`Entity`] with no components and inserts it into the children defined by the [`ChildBuilder`] which adds the [`Parent`] component to it.
/// Spawns an [`Entity`] with no components
/// and inserts it into the children defined by the [`ChildBuilder`]
/// which adds the [`Parent`] component to it.
Comment on lines +196 to +198
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

pub fn spawn(&mut self) -> EntityCommands<'w, 's, '_> {
let e = self.commands.spawn();
self.push_children.children.push(e.id());
Expand Down Expand Up @@ -256,6 +259,11 @@ pub trait BuildChildren {
fn remove_children(&mut self, children: &[Entity]) -> &mut Self;
/// Adds a single child
fn add_child(&mut self, child: Entity) -> &mut Self;

/// Set the parent.
///
/// This overwrites the existing parent.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. Add a Parent(parent) component to the child
  2. Push child to the Children component of the parent

fn set_parent(&mut self, parent: Entity) -> &mut Self;
}

impl<'w, 's, 'a> BuildChildren for EntityCommands<'w, 's, 'a> {
Expand Down Expand Up @@ -314,6 +322,12 @@ impl<'w, 's, 'a> BuildChildren for EntityCommands<'w, 's, 'a> {
self.commands().add(AddChild { child, parent });
self
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see a simple test :)

self
}
}

/// Struct for adding children to an entity directly through the [`World`] for use in exclusive systems
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

/// into the children defined by the [`WorldChildBuilder`]
/// which adds the [`Parent`] component to it.
pub fn spawn(&mut self) -> EntityMut<'_> {
let parent_entity = self.parent_entity();
let entity = self.world.spawn().insert(Parent(parent_entity)).id();
Expand Down Expand Up @@ -383,31 +399,22 @@ pub trait BuildWorldChildren {

impl<'w> BuildWorldChildren for EntityMut<'w> {
fn with_children(&mut self, spawn_children: impl FnOnce(&mut WorldChildBuilder)) -> &mut Self {
{
let entity = self.id();
let entity = self.id();
self.with_world_mut(|world| {
let mut builder = WorldChildBuilder {
current_entity: None,
parent_entities: vec![entity],
// SAFETY: self.update_location() is called below. It is impossible to make EntityMut
// function calls on `self` within the scope defined here
world: unsafe { self.world_mut() },
world,
};

spawn_children(&mut builder);
}
self.update_location();
});
self
}

fn push_children(&mut self, children: &[Entity]) -> &mut Self {
let parent = self.id();
{
// SAFETY: parent entity is not modified and its location is updated manually
let world = unsafe { self.world_mut() };
update_old_parents(world, parent, children);
// 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));
Copy link
Member

Choose a reason for hiding this comment

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

Really like the elimination of unsafe here.

if let Some(mut children_component) = self.get_mut::<Children>() {
children_component
.0
Expand All @@ -421,13 +428,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> {

fn insert_children(&mut self, index: usize, children: &[Entity]) -> &mut Self {
let parent = self.id();
{
// SAFETY: parent entity is not modified and its location is updated manually
let world = unsafe { self.world_mut() };
update_old_parents(world, parent, children);
// 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));

if let Some(mut children_component) = self.get_mut::<Children>() {
children_component
Expand All @@ -442,9 +443,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> {

fn remove_children(&mut self, children: &[Entity]) -> &mut Self {
let parent = self.id();
// SAFETY: This doesn't change the parent's location
let world = unsafe { self.world_mut() };
remove_children(parent, children, world);
self.with_world_mut(|world| remove_children(parent, children, world));
self
}
}
Expand Down Expand Up @@ -542,6 +541,8 @@ mod tests {
parent.spawn().insert(C(4)).id(),
]
});
let mut children = children.to_vec();
children.push(commands.spawn().insert(C(5)).set_parent(parent).id());

queue.apply(&mut world);
assert_eq!(
Expand Down Expand Up @@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nicopap nicopap Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

We should remove the duplicate assertions.

assert_eq!(*world.get::<Parent>(child1).unwrap(), Parent(parent));
assert_eq!(*world.get::<Parent>(child2).unwrap(), Parent(parent));

Expand Down