Skip to content

Finish #17558, re-adding insert_children #18409

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

Merged
merged 16 commits into from
Apr 1, 2025

Conversation

ElliottjPierce
Copy link
Contributor

@ElliottjPierce ElliottjPierce commented Mar 19, 2025

fixes #17478

Objective

Solution

  • Add a OrderedRelationshipSourceCollection, which allows sorting, ordering, rearranging, etc of a RelationshipSourceCollection.
  • Implement insert_related
  • Implement insert_children
  • Tidy up some docs while I'm here.

Testing

@bjoernp116 set up a unit test, and I added a doc test to OrderedRelationshipSourceCollection.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Mar 19, 2025
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use P-Regression Functionality that used to work but no longer does. Add a test for this! S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 19, 2025
Co-authored-by: Dmytro Banin <[email protected]>
<R::RelationshipTarget as RelationshipTarget>::Collection:
OrderedRelationshipSourceCollection,
{
self.add_related::<R>(related);
Copy link
Member

@cart cart Mar 19, 2025

Choose a reason for hiding this comment

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

In theory we can improve the performance here by taking advantage of the fact that we can know if the entities in related are in the RelationshipSourceCollection based on whether or not they have R (if they do not have R, they aren't in it, if they do have R and it points to the target they are in it, and if they do have R and it does not point to the target they are not in it).

We could skip the insert hook (using RelationshipHookMode) and manually do the insert ourselves.

That would allow us to skip the O(nm) runtime that this currently has (for the remove op), in cases where the scan is not necessary (which should be most cases).

Edit: it would also allow us to skip the cost of the "initial" insert into the wrong location

In cases where there are no reorders, it would also in theory allow us to do an optimized "resize collection, shift everything after index over, and insert everything in related", which would be much faster than inserting one-by one (which requires a "list shift" on every insert).

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'm still relatively new to relationships, but I think I've made the requested changes. Let me know if I missed something here though.

It now skips the re-inserting if it already exists, preventing hooks. If we do need to add it, it no longer has to search for it.

In cases where there are no reorders, it would also in theory allow us to do an optimized "resize collection, shift everything after index over, and insert everything in related", which would be much faster than inserting one-by one (which requires a "list shift" on every insert).

100% agree here, but I'm unsure how to practically do this. My best guess would be to remove all the duplicates from the new slice (which would allocate, maybe with bumpalo), then insert the relation component without hooks, then remove the entities in the passed slice from the target collection, and finally insert the slice into the vector directly. But between the allocation and removals IDK if that would be worth the performance. Maybe we could make it unsafe and presume that the entities have no duplicates and are not currently related?

@@ -59,6 +59,58 @@ pub trait RelationshipSourceCollection {
}
}

/// This trait signals that a [`RelationshipSourceCollection`] is ordered.
pub trait OrderedRelationshipSourceCollection: RelationshipSourceCollection {
Copy link
Member

Choose a reason for hiding this comment

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

Oh good I was hoping that something like this would be used.

@alice-i-cecile alice-i-cecile requested a review from viridia March 31, 2025 23:38
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.

Approved pending the documentation corrections already pointed out, nice work!

@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 Apr 1, 2025
Co-Authored-By: Talin <[email protected]>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 1, 2025
Merged via the queue into bevyengine:main with commit f5250db Apr 1, 2025
32 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Apr 2, 2025
Extension of #18409.

I was updating a migration guide for hierarchy commands and realized
`insert_children` wasn't added to `EntityCommands`, only
`EntityWorldMut`.

This adds that and `insert_related` (basically just some
copy-and-pasting).
mockersf pushed a commit that referenced this pull request Apr 3, 2025
fixes #17478

# Objective

- Complete #17558.
- the `insert_children` method was previously removed, and as #17478
points out, needs to be added back.

## Solution

- Add a `OrderedRelationshipSourceCollection`, which allows sorting,
ordering, rearranging, etc of a `RelationshipSourceCollection`.
- Implement `insert_related`
- Implement `insert_children`
- Tidy up some docs while I'm here.

## Testing

@bjoernp116 set up a unit test, and I added a doc test to
`OrderedRelationshipSourceCollection`.

---------

Co-authored-by: bjoernp116 <[email protected]>
Co-authored-by: Dmytro Banin <[email protected]>
Co-authored-by: Talin <[email protected]>
mockersf pushed a commit that referenced this pull request Apr 3, 2025
Extension of #18409.

I was updating a migration guide for hierarchy commands and realized
`insert_children` wasn't added to `EntityCommands`, only
`EntityWorldMut`.

This adds that and `insert_related` (basically just some
copy-and-pasting).
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 P-Regression Functionality that used to work but no longer does. Add a test for this! 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.

Missing a method to insert related entities at a specific index (insert_children)
7 participants