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 FilteredAccess::empty and simplify the implementatin of update_component_access for AnyOf/Or #14352

Merged
merged 8 commits into from
Jul 29, 2024

Conversation

SkiFire13
Copy link
Contributor

@SkiFire13 SkiFire13 commented Jul 16, 2024

Objective

  • The implementation of update_component_access for AnyOf/Or is kinda weird due to special casing the first filter, let's simplify it;
  • Fundamentally we want to fold/reduce the various filters using an OR operation, however in order to do a proper fold we need a neutral element for the initial accumulator, which for OR is FALSE. However we didn't have a way to create a FilteredAccess value corresponding to FALSE and thus the only option was reducing, which special cases the first element as being the initial accumulator.

This is an alternative to #14026

Solution

  • Introduce FilteredAccess::empty as a way to create a FilteredAccess corresponding to the logical proposition FALSE;
  • Use it as the initial accumulator for the above operations, allowing to handle all the elements to fold in the same way.

Migration Guide

  • The behaviour of AnyOf<()> and Or<()> has been changed to match no archetypes rather than all archetypes to naturally match the corresponding logical operation. Consider replacing them with () instead.

@SkiFire13 SkiFire13 force-pushed the false-filteredaccess branch 2 times, most recently from 9d814d4 to a7b3a88 Compare July 16, 2024 19:17
@janhohenheim janhohenheim added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 17, 2024
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

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

This makes those implementations much clearer!

crates/bevy_ecs/src/query/access.rs Show resolved Hide resolved
@SkiFire13 SkiFire13 force-pushed the false-filteredaccess branch from 2291bf7 to 2a23d74 Compare July 19, 2024 19:33
Copy link
Contributor

@Guvante Guvante left a comment

Choose a reason for hiding this comment

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

Does this change the behavior of Or<()>? The old code seems to be a no-op while the new code seems to remove all filter_sets. Mostly from a breaking changes standpoint.

@SkiFire13
Copy link
Contributor Author

Does this change the behavior of Or<()>? The old code seems to be a no-op while the new code seems to remove all filter_sets. Mostly from a breaking changes standpoint.

Yes, this is already mentioned in the migration guide.

I could avoid this breaking change by adding a bit of complexity in the code, but the current behaviour seems wrong to me so this is also an occasion to fix it. Or should match those archetypes that satisfy one of its inner filters, but if there's no filter then no filter is satisfied and no archetype should be matched, while the current behaviour is to match all of them. That said, I don't think someone is actually using Or<()> since it makes no sense, but if they are then chances are they expect the new behaviour, not the old one.

@BD103 BD103 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 Jul 29, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jul 29, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 29, 2024
Merged via the queue into bevyengine:main with commit 7de271f Jul 29, 2024
30 checks passed
@SkiFire13 SkiFire13 deleted the false-filteredaccess branch July 30, 2024 06:19
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-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

7 participants