Skip to content

Implement RelationshipSourceCollection for IndexSet #18471

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
Merged
merged 9 commits into from
May 4, 2025
Merged

Implement RelationshipSourceCollection for IndexSet #18471

merged 9 commits into from
May 4, 2025
+152 −2

Conversation

Brezak
Copy link
Contributor

@Brezak Brezak commented Mar 21, 2025

Objective

IndexSet doesn't implement RelationshipSourceCollection

Solution

Implement MapEntities for IndexSet
Implement RelationshipSourceCollection for IndexSet

Testing

cargo clippy

@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 S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Mar 23, 2025
@JaySpruce JaySpruce 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 Mar 28, 2025
Victoronz
Victoronz reviewed Apr 4, 2025
View reviewed changes
crates/bevy_ecs/src/relationship/relationship_source_collection.rs Outdated
}

fn reserve(&mut self, additional: usize) {
self.0.reserve(additional);
Copy link
Contributor

Choose a reason for hiding this comment

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

This trait impl should not rely on crate-public fields, but use an explicit deref_mut() call instead!

@Brezak Brezak force-pushed the relationship-collection-indexset branch from ec0ce67 to 2f2f64e Compare April 5, 2025 12:02
@Brezak Brezak requested a review from Victoronz April 5, 2025 12:08
Victoronz
Victoronz reviewed Apr 5, 2025
View reviewed changes
crates/bevy_ecs/src/entity/map_entities.rs Outdated
@@ -48,7 +51,7 @@ use smallvec::SmallVec;
pub trait MapEntities {
/// Updates all [`Entity`] references stored inside using `entity_mapper`.
///
/// Implementors should look up any and all [`Entity`] values stored within `self` and
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder, what exactly lints this correction? It is not a typo, both are correct, and the "o" version is more common in this context I believe

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 using this extensions https://marketplace.visualstudio.com/items/?itemName=tekumara.typos-vscode. Which supposedly uses https://github.com/crate-ci/typos.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see, then it seems that the vscode extension hasn't updated its crate-ci version yet, the newer versions of which no longer lint "implementor".

@Victoronz
Copy link
Contributor

Victoronz commented Apr 5, 2025 •
edited
Loading

small nit, the various set types are re-exported in the entity module, so no need to specify the specific submodule in their imports!
super::index_set::IndexSet -> super::IndexSet

@Brezak Brezak force-pushed the relationship-collection-indexset branch from 796b80c to bb7fd75 Compare April 6, 2025 13:10
@mockersf mockersf enabled auto-merge May 4, 2025 09:08
@mockersf mockersf added this pull request to the merge queue May 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch May 4, 2025
@mockersf mockersf enabled auto-merge May 4, 2025 10:10
@mockersf mockersf added this pull request to the merge queue May 4, 2025
Merged via the queue into bevyengine:main with commit e05e74a May 4, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@Victoronz Victoronz Victoronz approved these changes

@chescock chescock chescock approved these changes

@JaySpruce JaySpruce JaySpruce approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Brezak @Victoronz @chescock @mockersf @JaySpruce @alice-i-cecile