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

Add a set_parent method to ChildBuilder #5845

Closed
wants to merge 4 commits into from

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Sep 1, 2022

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.


Changelog

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

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

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

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!

/// 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

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

@@ -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.

Comment on lines +196 to +198
/// Spawns an [`Entity`] with no components
/// and inserts it into the children defined by the [`ChildBuilder`]
/// which adds the [`Parent`] component to 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, 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.


/// 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 {
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 :)

@@ -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.

@@ -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

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.

/// 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)) {
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!

// 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.

@nicopap
Copy link
Contributor Author

nicopap commented Nov 4, 2022

Superseded by #6189

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Hierarchy Parent-child entity hierarchies C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants