Fix error in AnyOf #14027
Fix error in AnyOf #14027
Conversation
cBournhonesque
added
A-ECS
alice-i-cecile
added
C-Bug
|
@SkiFire13 can I get your review here too? |
cBournhonesque
removed
the
P-Unsound
# Objective - Fixes a correctness error introduced in #14013 ... ## Solution I've been playing around a lot of with the access code and I realized that I introduced a soundness error when trying to simplify the code. When we have a `Or<(With<A>, With<B>)>` filter, we cannot call ``` let mut intermediate = FilteredAccess::default(); $name::update_component_access($name, &mut intermediate); _new_access.append_or(&intermediate); ``` because that's just equivalent to adding the new components as `Or` clauses. For example if the existing `filter_sets` was `vec![With<C>]`, we would then get `vec![With<C>, With<A>, With<B>]` which translates to `A or B or C`. Instead what we want is `(A and B) or (A and C)`, so we need to have each new OR clause compose with the existing access like so: ``` let mut intermediate = _access.clone(); // if we previously had a With<C> in the filter_set, this will become `With<C> AND With<A>` $name::update_component_access($name, &mut intermediate); _new_access.append_or(&intermediate); ``` ## Testing - Added a unit test that is broken in main, but passes in this PR
|
This PR caused a regression in 
Since the regression is only by 3%, and this was an important soundness bug, no further action needs to be taken. |
|
I think the PR might indeed have a perf impact for |
There's a chance! The automatic alerts are run in Github Actions, which are pretty noisy. The screenshot, though, is from my local computer, which is a lot quieter. Did you change any code called by |
This is code that is called by |
Ah, so it's just noise then. Sorry! (I got too excited that my fun side project was actually doing something lol) |
# Objective - Fixes a correctness error introduced in bevyengine#14013 ... ## Solution I've been playing around a lot of with the access code and I realized that I introduced a soundness error when trying to simplify the code. When we have a `Or<(With<A>, With<B>)>` filter, we cannot call ``` let mut intermediate = FilteredAccess::default(); $name::update_component_access($name, &mut intermediate); _new_access.append_or(&intermediate); ``` because that's just equivalent to adding the new components as `Or` clauses. For example if the existing `filter_sets` was `vec![With<C>]`, we would then get `vec![With<C>, With<A>, With<B>]` which translates to `A or B or C`. Instead what we want is `(A and B) or (A and C)`, so we need to have each new OR clause compose with the existing access like so: ``` let mut intermediate = _access.clone(); // if we previously had a With<C> in the filter_set, this will become `With<C> AND With<A>` $name::update_component_access($name, &mut intermediate); _new_access.append_or(&intermediate); ``` ## Testing - Added a unit test that is broken in main, but passes in this PR
Objective
Solution
I've been playing around a lot of with the access code and I realized that I introduced a soundness error when trying to simplify the code. When we have a
Or<(With<A>, With<B>)>filter, we cannot callbecause that's just equivalent to adding the new components as
Orclauses.For example if the existing
filter_setswasvec![With<C>], we would then getvec![With<C>, With<A>, With<B>]which translates toC or A or B.Instead what we want is
(C and A) or (C and B), so we need to have each new OR clause compose with the existing access like so:Testing