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

Improve or-with disjoint checks #7085

Merged
merged 12 commits into from
Apr 17, 2023

Conversation

vladbat00
Copy link
Contributor

@vladbat00 vladbat00 commented Jan 3, 2023

Objective

This PR attempts to improve query compatibility checks in scenarios involving Or filters.

Currently, for the following two disjoint queries, Bevy will throw a panic:

fn sys(_: Query<&mut C, Or<(With<A>, With<B>)>>, _: Query<&mut C, (Without<A>, Without<B>)>) {}

This PR addresses this particular scenario.

Solution

FilteredAccess::with now stores a vector of AccessFilters (representing a pair of with and without bitsets), where each member represents an Or "variant".
Filters like (With<A>, Or<(With<B>, Without<C>)> are expected to be expanded into A * B + A * !C.

When calculating whether queries are compatible, every AccessFilters of a query is tested for incompatibility with every AccessFilters of another query.


Changelog

  • Improved system and query data access compatibility checks in scenarios involving Or filters

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Jan 3, 2023
@alice-i-cecile
Copy link
Member

Wording improvement for you.

Improve compatibility checks in scenarios involving Or filters

Improved system and query data access compatibility checks in scenarios involving Or filters.

@alice-i-cecile alice-i-cecile self-requested a review January 3, 2023 22:11
@vladbat00 vladbat00 force-pushed the better-or-with-access branch from bb8359f to 9218793 Compare January 3, 2023 22:13
@BoxyUwU BoxyUwU added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Jan 4, 2023
@infmagic2047
Copy link
Contributor

The implementation looks correct to me. The methods definitely need better names, now that the filter is no longer represented as a set (maybe names based on "and/or").

We can use the same ExpandedOrWithAccess for Without filters with very little changes (I think only is_disjoint needs to be changed), so it may be a good idea.

@alice-i-cecile
Copy link
Member

The implementation looks correct to me. The methods definitely need better names, now that the filter is no longer represented as a set (maybe names based on "and/or").

We can use the same ExpandedOrWithAccess for Without filters with very little changes (I think only is_disjoint needs to be changed), so it may be a good idea.

I would like to see both of these things done in this PR.

Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

This mostly makes sense, I don't see where union_with is used though.

crates/bevy_ecs/src/query/access.rs Outdated Show resolved Hide resolved
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'd like to see the small comments resolved, and add another system compatibility test for nested Ors:

        fn sys(_: Query<&mut D, Or<(Or<(With<A>, With<B>)>), (Or<(With<A>, With<C>))>>>, _: Query<&mut D, Without<A>>) {}

I have zero confidence that I got those brackets right, but you get the idea.

@BoxyUwU BoxyUwU self-requested a review January 15, 2023 00:30
@vladbat00
Copy link
Contributor Author

Updated the implementation to support Or filters for Without, added more tests (including the one suggested by Alice).

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Being unusually harsh around clarity here: this code was already tricky, and I'm nervous about ongoing correctness and ability to change this code as needed.

@SkiFire13
Copy link
Contributor

The short discussion on Discord prompted me to try to explain what this PR does as operations on boolean SAT formulas. I feel like this might be a nice addition to the internal documentation:

The new ExpandedOrAccess expresses the with/without clauses as a boolean SAT formula in disjunctive normal form (i.e. a sequence of OR of formulas that contain a sequence of AND of either terms or NOT of terms).
In the query type, normal tuples represent AND formulas while Or<(...)> represent OR formulas. With<T> represent a normal term while Without<T> represents a NOT term.

The sequence of ORs is represented as two SmallVec<[FixedBitSet; 8]> (one in each ExpandedOrAccess) and the inner formulas with AND is split in two FixedBitSets, one in each ExpandedOrAccess. One contains the terms that are "positive" (in the with ExpandedOrAccess) and the other the terms that are negative, i.e. are behind a NOT (in the without ExpandedOrAccess).
(Observation: wouldn't it be more intuitive to place the with and without FixedBitSet in the same structure, and then place instances of that one in a single SmallVec<[NewStructure; 8]>, in order to be more similar to the AND formulas?)

Regarding the operations:
extend_intersect_filter is essentially an OR between two sequences of ORs, so they can just be chained together.

add_with is essentially computing A & ((B1 & ... & Bn) | (C1 & ... & Cn) | ...), which is equivalent to (B1 & ... & Bn & A) | (C1 & ... & Cn & A) | ..., that is we need to insert A into each existing AND formula (i.e. into each FixedBitSet in the with or without ExpandedOrAccess depends on whether A is positive (With<T>) or negative (Without<T>))

extend/union_with is essentially computing ((A1 & ... & An) | (B1 & ... & Bn) | ...) & ((C1 & ... & Cn) | (D1 & ... & Dn) | ...), which is equivalent to (A1 & ... & An & C1 & ... & Cn) | (A1 & ... & An & D1 & ... & Dn) | (B1 & ... & Bn & C1 & ... & Cn) | (B1 & ... & Bn & D1 & ... & Dn) | ..., that is it computes the cartesian product of the two lists of AND formulas and maps each "tuple" to an AND formula, which can be flattened.

is_disjoint is equivalent to computing extend/union_with (with the difference that it can now be computed lazily) and then checking if the resulting formula can ever be satisfied, that is if any of the resulting AND formulas can ever be satisfied. Thus this reduces to check for each tuple of AND formula if they are disjoint, which can be computed by checking that both with FixedBitSet are disjoint with each without FixedBitSet. The PR does this in Filteredaccess::is_compatible and then ExpandedOrAccess::is_disjoint.
(Observation: the PR only checks two combinations of with and without, thus it assumes that a single FilteredAccess doesn't contain conflicting filters)

@vladbat00
Copy link
Contributor Author

@SkiFire13 thanks for the excellent write-up! It's quite on point, and it's even been educational for me: for example, I was looking myself for terminology such as DNF to describe the idea behind this. You've done a much better job, as I still struggle with correct semantics, naming and documenting things :D

(Observation: wouldn't it be more intuitive to place the with and without FixedBitSet in the same structure, and then place instances of that one in a single SmallVec<[NewStructure; 8]>, in order to be more similar to the AND formulas?)

I was considering it, but I found it made the code much messier. Lengths of with and without arrays can be different, which requires additional workarounds to combine them into a single array, introducing more problems than it would actually solve.

(Observation: the PR only checks two combinations of with and without, thus it assumes that a single FilteredAccess doesn't contain conflicting filters)

I'm afraid I didn't understand this part, could you expand on that, please? I'm trying to understand if that means that I overlooked some edge cases.

@vladbat00
Copy link
Contributor Author

@alice-i-cecile thank you for the comments! That's absolutely fair, I totally agree that things can be named and documented better. I feel somewhat limited by my vocabulary and discrete maths knowledge, so I'll appreciate direct suggestions for naming things and documentation. I feel a bit awkward asking others to document my code, so I'll do my best to improve the bits that I can, but I suspect that this PR will need an additional round of polish from someone with better tech writing skills ☺️

@vladbat00 vladbat00 force-pushed the better-or-with-access branch from 238e97f to eb910ad Compare February 16, 2023 22:14
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Dramatically better now that we have more tests, more docs, and a clear theoretical framework. Some clarity suggestions for the docs.

@vladbat00 vladbat00 force-pushed the better-or-with-access branch from e6d6550 to 25498a4 Compare February 17, 2023 22:14

impl<T: SparseSetIndex> AccessFilters<T> {
fn is_ruled_out_by(&self, other: &Self) -> bool {
!self.with.is_disjoint(&other.without) || !self.without.is_disjoint(&other.with)
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically speaking to be complete this also needs || !self.with.is_disjoing(&self.without) || !other.with.is_disjoint(&other.without) in order to handle extreme cases like (With<T>, Without<T>) being empty and thus disjoint with everything else. Not sure if we want to properly support those cases though, in practice if they happen they're almost always a mistake. The condition being at the end means it would be checked only if we're about to find a conflict, so the performance hit should not happen in the happy path.

However if it's kept like this I would add a comment explaining the missing conditions and why they're not implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was also thinking about this case. I'm inclined not to support it, I agree that it almost always means there's an error. Added a comment.

crates/bevy_ecs/src/query/fetch.rs Show resolved Hide resolved
crates/bevy_ecs/src/query/filter.rs Show resolved Hide resolved
@vladbat00 vladbat00 force-pushed the better-or-with-access branch from fca1ef2 to 09c7810 Compare February 18, 2023 08:18
_intersected_access.extend_intersect_filter(&intermediate);
_intersected_access.extend_access(&intermediate);
_new_access.append_or(&intermediate);
_new_access.extend_access(&intermediate);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This made me wonder if this line is actually needed. I see why we call extend_access for AnyOf, but I don't think we need to do this for Or. If anyone can confirm, I'll drop it.

@james7132 james7132 added this to the 0.11 milestone Feb 27, 2023
@vladbat00 vladbat00 force-pushed the better-or-with-access branch from 09c7810 to cfc634d Compare April 8, 2023 21:50
@alice-i-cecile alice-i-cecile 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 9, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 17, 2023
Merged via the queue into bevyengine:main with commit 71fccb2 Apr 17, 2023
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 D-Complex Quite challenging from either a design or technical perspective. Ask for help! 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.

8 participants