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 more tools for traversing hierarchies #15627

Merged
merged 20 commits into from
Oct 7, 2024

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Oct 3, 2024

Objective

Solution

Extend HierarchyQueryExt with the following methods:

  • parent
  • children
  • root_parent
  • iter_leaves
  • iter_siblings
  • iter_descendants_depth_first

I've opted to make both iter_leaves and iter_siblings collect the list of matching Entities for now, rather that operate by reference like the existing iter_descendants. This was simpler, and in the case of iter_siblings especially, the number of matching entities is likely to be much smaller.

I've kept the generics in the type signature however, so we can go back and optimize that freely without a breaking change whenever we want.

Testing

I've added some basic testing, but they're currently failing. If you'd like to help, I'd welcome suggestions or a PR to my PR over the weekend <3

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Hierarchy S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 3, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 3, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Oct 3, 2024
@alice-i-cecile alice-i-cecile added the S-Needs-Help The author needs help finishing this PR. label Oct 3, 2024
@villor
Copy link
Contributor

villor commented Oct 5, 2024

Did a PR with fixed siblings iter.

The problem was that the query has to be Query<(Option<&Parent>, Option<&Children>)> since we are using the same query for the parent and children.

Also got rid of the SiblingsIter since it wasn't really necessary. Instead we got a ugly mess of into_iter().flat_map(), but at least it won't allocate anymore 😅

I wonder if it would be a good idea to convert HierarchyQueryExt into system params instead 🤔 what is the reason it is an extension in the first place?

Fix iter_siblings and some readability improvements for tests
@alice-i-cecile
Copy link
Member Author

I wonder if it would be a good idea to convert HierarchyQueryExt into system params instead 🤔 what is the reason it is an extension in the first place?

I'm pretty sure it predates the SystemParam derive macro, and has grown organically from there. I really like the filtering capabilities, but I agree: I think that the SystemParam approach is a lot simpler as both a user and a maintainer. Adding a filter generic should be totally doable. Follow-up though!

@villor
Copy link
Contributor

villor commented Oct 6, 2024

@alice-i-cecile

Pushed some more commits, PR here

  • Cleaned up my previous flat_map->into_iter chaos. Much more readable now!
  • Made sure children() returns a slice like the doc comment says, and cleaned up the implementation.
  • Renamed root_parent -> root_ancestor
  • Removed LeafIter and instead return the traversal iterator directly (reducing allocations)
  • Added a doc comment about the iter_leaves traversal order. (Would it make sense to make this one traverse depth-first instead for the "correct" order?)

Edit: Here's another PR which also adds depth-first and changes iter_leaves to use it.

@alice-i-cecile
Copy link
Member Author

@villor you're the best. I really appreciate your help with this!

@alice-i-cecile alice-i-cecile marked this pull request as ready for review October 6, 2024 19:51
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged S-Needs-Help The author needs help finishing this PR. labels Oct 6, 2024
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Like the options this provides, and helps justify the existence of what is otherwise a pretty small crate! I do think this should be refactored into a SystemParam, but that's a larger change that can easily be built on top of this work in 0.16. I also see a lot of refactoring in the near future anyway with #15635, so no real need to do that here and now.

@villor
Copy link
Contributor

villor commented Oct 6, 2024

Like the options this provides, and helps justify the existence of what is otherwise a pretty small crate! I do think this should be refactored into a SystemParam, but that's a larger change that can easily be built on top of this work in 0.16. I also see a lot of refactoring in the near future anyway with #15635, so no real need to do that here and now.

SystemParam is going to be the bomb! I imagine we could add other APIs as well like change detection on different parts of the hierarchy. A lot more options open up once we can control the queries more 😁

And when we have #15635 we can make it generic over any one-to-many relation!

@bushrat011899 bushrat011899 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 6, 2024
Co-authored-by: poopy <gonesbird@gmail.com>
Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
@alice-i-cecile
Copy link
Member Author

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1718 if you'd like to help out.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events and removed A-Hierarchy labels Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants