Remove extra allocation in AccessFilters

[cBournhonesque]

Objective

Some parts of the AccessFilter code is currently pretty hard to understand. For example take a filter like Or<(With<A>, With<B>)>.
Instead of having code like:

_access.append_or(component_a);
_access.append_or(component_b);

we have some strange-looking code that does something different for the first component in the Or filter:

A::update_component_access(state_a, &mut _access);
_access.append_or(component_b);

The reason is that if we just called append_or for all components, we would get a filter_sets in the form of vec![(), With<A>, With<B>], because the default value for filter_sets is vec![AccessFilters::default()].
This is incorrect because then systems such as q1: Query<Entity, Or<(With<A>)>, q2: Query<Entity, Without<A> would fail because the two queries would not be disjoint anymore (since the first query q1's filter would effectively be Or<((), With<A>>).
Instead what we want is a filter_set in the form of vec![With<A>, With<B>]. The special logic for the first tuple element ensures that the existing filter_set is first replaced by the filter_set value of the first component A (to remove the empty AccessFilters::default()) and then we call append_or as per usual.

I'm not entirely sure why the default FilteredAccess contains a filter_sets in the form of vec![AccessFilters::default()].
Maybe to guarantee that append_and is correct (filter_sets is in disjunctive normal form so adding 'and' terms requires at least one element in filter_sets).

Solution

• Make the default filter_sets vec![] instead of vec![AccessFilters::default()], which removes an allocation whenever we create a filter. (and whenever we clone a FilteredAccess, which is quite frequent)
• The AnyOf and Or WorldQuery logic becomes simplified as there is no need to have special logic for the first element of the tuple anymore
• Some of the other access-related code needs to be adapted to handle empty filter_sets correctly

Testing

• Via unit tests

[SkiFire13]

Make the default filter_sets vec![] instead of vec![AccessFilters::default()], which removes an allocation whenever we create a filter.

Note that this breaks the model where we see this vec as an OR of AND filters. You're changing from an OR with a single AND subformula that's empty, which corresponds to a TRUE, with an OR that's empty, which instead corresponds to FALSE.

IMO this makes the code even harder to understand (which seemed to be one of the motivations for this PR...). You can already see this in effect with the strange change you had to make to append_or and in other places.

Edit: the TRUE/FALSE swap made me realize what's the problem with the implementation of Or/AnyOf. They basically need to fold/reduce the accesses of the individual elements using an OR operation between them. However FilteredAccess does not provide a neutral element for the OR operation, namely a corresponding value for FALSE, so we're forced to reduce using the first element as accumulator, hence the weird code. And this is also why your PR manages to simplify those implementations, because you switched the default value to FALSE. So IMO the proper solution for simplifying that code would be providing a constructor for FilteredAccess that creates one with a FALSE value, i.e. with an totally empty filter_sets.

[cBournhonesque]

Note that this breaks the model where we see this vec as an OR of AND filters. You're changing from an OR with a single AND subformula that's empty, which corresponds to a TRUE, with an OR that's empty, which instead corresponds to FALSE.

The previous default is OR< () >, where () is "no filters", which is matched by any query.
The new default would be <empty> which also means "no filters" and is matched by any query.
They both have similar meanings.

It's true that we have to be careful in a couple places because we were using
self.filter_sets.iter().all().
In the previous case it would iterate through () (no filter). Note that it doesn't necessarily mean TRUE.
In the new case we get FALSE in this specific situation because all() returns FALSE on an empty vector.

Edit: the TRUE/FALSE swap made me realize what's the problem with the implementation of Or/AnyOf. They basically need to fold/reduce the accesses of the individual elements using an OR operation between them. However FilteredAccess does not provide a neutral element for the OR operation, namely a corresponding value for FALSE, so we're forced to reduce using the first element as accumulator, hence the weird code. And this is also why your PR manages to simplify those implementations, because you switched the default value to FALSE. So IMO the proper solution for simplifying that code would be providing a constructor for FilteredAccess that creates one with a FALSE value, i.e. with an totally empty filter_sets.

In the case of Or/AnyOf we would get something like Or< (), With<A>, With<B> > if we did a naive implementation.
Note that the initial value () does not mean TRUE/FALSE, the problem comes more from the fact that 2 queries like
With<A>/Without<A> would be conflicting because their access would look like
Or<(), With<A>> and Or<(), Without<A>> where they share an empty access ().

So yes the issues is that the default accumulator value of () causes issues, not really the fact that it's TRUE or FALSE

IMO this makes the code even harder to understand (which seemed to be one of the motivations for this PR...). You can already see this in effect with the strange change you had to make to append_or and in other places.

I agree that the weak point of this PR is the append_or change, to handle the side-affect of appending a OR<()> filter.

However I do think the new approach is easier to reason with:

• the default filter sets should be empty at the beginning, rather than Or<()>. It just makes more sense and is easier to reason about. I think the only reason why they weren't was to make the code for and_with or and_without easier

• even if the complexity is the same, I think the new approach is worthwhile because it removes an allocation when we create a new FilteredAccess. There are a number of situations where we might need to create new FilteredAccess frequently.
  For example uncached queries (cc @james-j-obrien ) or in this situation: EntityRef/Mut get_components #13375 (comment)

Maybe I should update the PR description to make more explicit the benefit of avoiding an allocation on every access

[SkiFire13]

The previous default is OR< () >, where () is "no filters", which is matched by any query.
The new default would be <empty> which also means "no filters" and is matched by any query.
They both have similar meanings.

Arguably, it previously was () and now is Or<()>, though now that I see the implementation of Or<()> it seems wrong to me (it is equivalent to (), i.e. TRUE, but it should be FALSE).

They are similar because they are both empty, but they have completly different meanings. One is the neutral element for AND (i.e. TRUE, that is all archetypes are matched), the other is the neutral element for OR (i.e. FALSE, that is no archetype is matched).

In the previous case it would iterate through () (no filter). Note that it doesn't necessarily mean TRUE.
In the new case we get FALSE in this specific situation because all() returns FALSE on an empty vector.

You don't necessarily have to force an equivalence between filters and logic formulas, but it does make everything much easier. The fact that all() returns false for empty iterators while any() returns true for empty iterators is strictly related to this, since all implements an and operation between all the condition starting from its neutral element, true, so true is naturally the value for when there's no element, likewise for any with an or operation that has false as neutral element.

In the case of Or/AnyOf we would get something like Or< (), With<A>, With<B> > if we did a naive implementation.

With the current implementation, yes. If we had a FALSE FilteredAccess however it would work, because FALSE OR A OR B is the same as A OR B.

Note that the initial value () does not mean TRUE/FALSE

If you interpret filters as logic formulas it would mean TRUE. (If you don't interpret filters as logic formulas then good luck reasoning about whether the way you implement some filters is correct or not)

So yes the issues is that the default accumulator value of () causes issues, not really the fact that it's TRUE or FALSE

When you view the filter as a logic formula however the problem is exactly that FALSE is used when it should have been TRUE or viceversa.

the default filter sets should be empty at the beginning, rather than Or<()>. It just makes more sense and is easier to reason about.

I firmly disagree with this, as it has the completly opposite meaning of what it naturally would have.

I think the new approach is worthwhile because it removes an allocation when we create a new FilteredAccess.

This is not that useful because in most situations you end up allocating that Vec anyway the moment and_with/and_without are called. The only case where this doesn't happen is in append_or, where it would also be avoided if we had a way to construct the FALSE FilteredAccess (which is just what you did here, an empty filter_sets.

[alice-i-cecile]

Closing in favor of #14352.