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 any_component_removed condition #8326

Merged
merged 2 commits into from
Apr 18, 2023

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Apr 7, 2023

Added helper extracted from #7711. that PR contains some controversy conditions, but this one should be good to go.


Changelog

Added

  • any_component_removed condition.

// Simply checking `is_empty` would not be enough.
// PERF: note that `count` is efficient (not actually looping/iterating),
// due to Bevy having a specialized implementation for events.
move |mut removals: RemovedComponents<T>| !removals.iter().count() != 0
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
move |mut removals: RemovedComponents<T>| !removals.iter().count() != 0
move |mut removals: RemovedComponents<T>| removals.iter().next().is_none()

This otherwise potentially does a full traversal.

Copy link
Member

Choose a reason for hiding this comment

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

This contradicts the perf note above.

I would still prefer to remove the perf note and just use is_empty though.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I completely forgot we overrode the default implementations for those. Agreed on the use of is_empty instead.

Copy link
Contributor Author

@Shatur Shatur Apr 7, 2023

Choose a reason for hiding this comment

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

is_empty can't be used. The reason is mentioned in the comment: it won't advance the reader and the condition will be triggered twice. We use the same strategy for events:

move |mut reader: EventReader<T>| reader.iter().count() > 0

@james7132 james7132 added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Apr 7, 2023
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 18, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 18, 2023
Merged via the queue into bevyengine:main with commit 315f3ca Apr 18, 2023
@Shatur Shatur deleted the any-component-removed branch April 18, 2023 16:17
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-Feature A new feature, making something new possible 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.

4 participants