Improve or-with disjoint checks (#7085)

# 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

---------

Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

crates/bevy_ecs/src/query/access.rs

@@ -25,6 +25,7 @@ struct FormattedBitSet<'a, T: SparseSetIndex> {
     bit_set: &'a FixedBitSet,
     _marker: PhantomData<T>,
 }
+
 impl<'a, T: SparseSetIndex> FormattedBitSet<'a, T> {
     fn new(bit_set: &'a FixedBitSet) -> Self {
         Self {
@@ -33,6 +34,7 @@ impl<'a, T: SparseSetIndex> FormattedBitSet<'a, T> {
         }
     }
 }
+
 impl<'a, T: SparseSetIndex + fmt::Debug> fmt::Debug for FormattedBitSet<'a, T> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.debug_list()
@@ -69,6 +71,7 @@ impl<T: SparseSetIndex + fmt::Debug> fmt::Debug for Access<T> {
             .finish()
     }
 }
+
 impl<T: SparseSetIndex> Default for Access<T> {
     fn default() -> Self {
         Self::new()
@@ -213,31 +216,22 @@ impl<T: SparseSetIndex> Access<T> {
 /// is read/write `T`, read `U`. It must still have a read `U` access otherwise the following
 /// queries would be incorrectly considered disjoint:
 /// - `Query<&mut T>`  read/write `T`
-/// - `Query<Option<&T>` accesses nothing
+/// - `Query<Option<&T>>` accesses nothing
 ///
 /// See comments the `WorldQuery` impls of `AnyOf`/`Option`/`Or` for more information.
-#[derive(Clone, Eq, PartialEq)]
+#[derive(Debug, Clone, Eq, PartialEq)]
 pub struct FilteredAccess<T: SparseSetIndex> {
     access: Access<T>,
-    with: FixedBitSet,
-    without: FixedBitSet,
-}
-impl<T: SparseSetIndex + fmt::Debug> fmt::Debug for FilteredAccess<T> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        f.debug_struct("FilteredAccess")
-            .field("access", &self.access)
-            .field("with", &FormattedBitSet::<T>::new(&self.with))
-            .field("without", &FormattedBitSet::<T>::new(&self.without))
-            .finish()
-    }
+    // An array of filter sets to express `With` or `Without` clauses in disjunctive normal form, for example: `Or<(With<A>, With<B>)>`.
+    // Filters like `(With<A>, Or<(With<B>, Without<C>)>` are expanded into `Or<((With<A>, With<B>), (With<A>, Without<C>))>`.
+    filter_sets: Vec<AccessFilters<T>>,
 }
 
 impl<T: SparseSetIndex> Default for FilteredAccess<T> {
     fn default() -> Self {
         Self {
             access: Access::default(),
-            with: Default::default(),
-            without: Default::default(),
+            filter_sets: vec![AccessFilters::default()],
         }
     }
 }
@@ -266,30 +260,46 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
     /// Adds access to the element given by `index`.
     pub fn add_read(&mut self, index: T) {
         self.access.add_read(index.clone());
-        self.add_with(index);
+        self.and_with(index);
     }
 
     /// Adds exclusive access to the element given by `index`.
     pub fn add_write(&mut self, index: T) {
         self.access.add_write(index.clone());
-        self.add_with(index);
+        self.and_with(index);
     }
 
-    /// Retains only combinations where the element given by `index` is also present.
-    pub fn add_with(&mut self, index: T) {
-        self.with.grow(index.sparse_set_index() + 1);
-        self.with.insert(index.sparse_set_index());
+    /// Adds a `With` filter: corresponds to a conjunction (AND) operation.
+    ///
+    /// Suppose we begin with `Or<(With<A>, With<B>)>`, which is represented by an array of two `AccessFilter` instances.
+    /// Adding `AND With<C>` via this method transforms it into the equivalent of  `Or<((With<A>, With<C>), (With<B>, With<C>))>`.
+    pub fn and_with(&mut self, index: T) {
+        let index = index.sparse_set_index();
+        for filter in &mut self.filter_sets {
+            filter.with.grow(index + 1);
+            filter.with.insert(index);
+        }
     }
 
-    /// Retains only combinations where the element given by `index` is not present.
-    pub fn add_without(&mut self, index: T) {
-        self.without.grow(index.sparse_set_index() + 1);
-        self.without.insert(index.sparse_set_index());
+    /// Adds a `Without` filter: corresponds to a conjunction (AND) operation.
+    ///
+    /// Suppose we begin with `Or<(With<A>, With<B>)>`, which is represented by an array of two `AccessFilter` instances.
+    /// Adding `AND Without<C>` via this method transforms it into the equivalent of  `Or<((With<A>, Without<C>), (With<B>, Without<C>))>`.
+    pub fn and_without(&mut self, index: T) {
+        let index = index.sparse_set_index();
+        for filter in &mut self.filter_sets {
+            filter.without.grow(index + 1);
+            filter.without.insert(index);
+        }
     }
 
-    pub fn extend_intersect_filter(&mut self, other: &FilteredAccess<T>) {
-        self.without.intersect_with(&other.without);
-        self.with.intersect_with(&other.with);
+    /// Appends an array of filters: corresponds to a disjunction (OR) operation.
+    ///
+    /// As the underlying array of filters represents a disjunction,
+    /// where each element (`AccessFilters`) represents a conjunction,
+    /// we can simply append to the array.
+    pub fn append_or(&mut self, other: &FilteredAccess<T>) {
+        self.filter_sets.append(&mut other.filter_sets.clone());
     }
 
     pub fn extend_access(&mut self, other: &FilteredAccess<T>) {
@@ -298,9 +308,23 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
 
     /// Returns `true` if this and `other` can be active at the same time.
     pub fn is_compatible(&self, other: &FilteredAccess<T>) -> bool {
-        self.access.is_compatible(&other.access)
-            || !self.with.is_disjoint(&other.without)
-            || !other.with.is_disjoint(&self.without)
+        if self.access.is_compatible(&other.access) {
+            return true;
+        }
+
+        // If the access instances are incompatible, we want to check that whether filters can
+        // guarantee that queries are disjoint.
+        // Since the `filter_sets` array represents a Disjunctive Normal Form formula ("ORs of ANDs"),
+        // we need to make sure that each filter set (ANDs) rule out every filter set from the `other` instance.
+        //
+        // For example, `Query<&mut C, Or<(With<A>, Without<B>)>>` is compatible `Query<&mut C, (With<B>, Without<A>)>`,
+        // but `Query<&mut C, Or<(Without<A>, Without<B>)>>` isn't compatible with `Query<&mut C, Or<(With<A>, With<B>)>>`.
+        self.filter_sets.iter().all(|filter| {
+            other
+                .filter_sets
+                .iter()
+                .all(|other_filter| filter.is_ruled_out_by(other_filter))
+        })
     }
 
     /// Returns a vector of elements that this and `other` cannot access at the same time.
@@ -313,10 +337,34 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
     }
 
     /// Adds all access and filters from `other`.
-    pub fn extend(&mut self, access: &FilteredAccess<T>) {
-        self.access.extend(&access.access);
-        self.with.union_with(&access.with);
-        self.without.union_with(&access.without);
+    ///
+    /// Corresponds to a conjunction operation (AND) for filters.
+    ///
+    /// Extending `Or<(With<A>, Without<B>)>` with `Or<(With<C>, Without<D>)>` will result in
+    /// `Or<((With<A>, With<C>), (With<A>, Without<D>), (Without<B>, With<C>), (Without<B>, Without<D>))>`.
+    pub fn extend(&mut self, other: &FilteredAccess<T>) {
+        self.access.extend(&other.access);
+
+        // We can avoid allocating a new array of bitsets if `other` contains just a single set of filters:
+        // in this case we can short-circuit by performing an in-place union for each bitset.
+        if other.filter_sets.len() == 1 {
+            for filter in &mut self.filter_sets {
+                filter.with.union_with(&other.filter_sets[0].with);
+                filter.without.union_with(&other.filter_sets[0].without);
+            }
+            return;
+        }
+
+        let mut new_filters = Vec::with_capacity(self.filter_sets.len() * other.filter_sets.len());
+        for filter in &self.filter_sets {
+            for other_filter in &other.filter_sets {
+                let mut new_filter = filter.clone();
+                new_filter.with.union_with(&other_filter.with);
+                new_filter.without.union_with(&other_filter.without);
+                new_filters.push(new_filter);
+            }
+        }
+        self.filter_sets = new_filters;
     }
 
     /// Sets the underlying unfiltered access as having access to all indexed elements.
@@ -325,6 +373,43 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
     }
 }
 
+#[derive(Clone, Eq, PartialEq)]
+struct AccessFilters<T> {
+    with: FixedBitSet,
+    without: FixedBitSet,
+    _index_type: PhantomData<T>,
+}
+
+impl<T: SparseSetIndex + fmt::Debug> fmt::Debug for AccessFilters<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("AccessFilters")
+            .field("with", &FormattedBitSet::<T>::new(&self.with))
+            .field("without", &FormattedBitSet::<T>::new(&self.without))
+            .finish()
+    }
+}
+
+impl<T: SparseSetIndex> Default for AccessFilters<T> {
+    fn default() -> Self {
+        Self {
+            with: FixedBitSet::default(),
+            without: FixedBitSet::default(),
+            _index_type: PhantomData,
+        }
+    }
+}
+
+impl<T: SparseSetIndex> AccessFilters<T> {
+    fn is_ruled_out_by(&self, other: &Self) -> bool {
+        // Although not technically complete, we don't consider the case when `AccessFilters`'s
+        // `without` bitset contradicts its own `with` bitset (e.g. `(With<A>, Without<A>)`).
+        // Such query would be considered compatible with any other query, but as it's almost
+        // always an error, we ignore this case instead of treating such query as compatible
+        // with others.
+        !self.with.is_disjoint(&other.without) || !self.without.is_disjoint(&other.with)
+    }
+}
+
 /// A collection of [`FilteredAccess`] instances.
 ///
 /// Used internally to statically check if systems have conflicting access.
@@ -441,7 +526,10 @@ impl<T: SparseSetIndex> Default for FilteredAccessSet<T> {
 
 #[cfg(test)]
 mod tests {
+    use crate::query::access::AccessFilters;
     use crate::query::{Access, FilteredAccess, FilteredAccessSet};
+    use fixedbitset::FixedBitSet;
+    use std::marker::PhantomData;
 
     #[test]
     fn read_all_access_conflicts() {
@@ -514,22 +602,67 @@ mod tests {
         let mut access_a = FilteredAccess::<usize>::default();
         access_a.add_read(0);
         access_a.add_read(1);
-        access_a.add_with(2);
+        access_a.and_with(2);
 
         let mut access_b = FilteredAccess::<usize>::default();
         access_b.add_read(0);
         access_b.add_write(3);
-        access_b.add_without(4);
+        access_b.and_without(4);
 
         access_a.extend(&access_b);
 
         let mut expected = FilteredAccess::<usize>::default();
         expected.add_read(0);
         expected.add_read(1);
-        expected.add_with(2);
+        expected.and_with(2);
         expected.add_write(3);
-        expected.add_without(4);
+        expected.and_without(4);
 
         assert!(access_a.eq(&expected));
     }
+
+    #[test]
+    fn filtered_access_extend_or() {
+        let mut access_a = FilteredAccess::<usize>::default();
+        // Exclusive access to `(&mut A, &mut B)`.
+        access_a.add_write(0);
+        access_a.add_write(1);
+
+        // Filter by `With<C>`.
+        let mut access_b = FilteredAccess::<usize>::default();
+        access_b.and_with(2);
+
+        // Filter by `(With<D>, Without<E>)`.
+        let mut access_c = FilteredAccess::<usize>::default();
+        access_c.and_with(3);
+        access_c.and_without(4);
+
+        // Turns `access_b` into `Or<(With<C>, (With<D>, Without<D>))>`.
+        access_b.append_or(&access_c);
+        // Applies the filters to the initial query, which corresponds to the FilteredAccess'
+        // representation of `Query<(&mut A, &mut B), Or<(With<C>, (With<D>, Without<E>))>>`.
+        access_a.extend(&access_b);
+
+        // Construct the expected `FilteredAccess` struct.
+        // The intention here is to test that exclusive access implied by `add_write`
+        // forms correct normalized access structs when extended with `Or` filters.
+        let mut expected = FilteredAccess::<usize>::default();
+        expected.add_write(0);
+        expected.add_write(1);
+        // The resulted access is expected to represent `Or<((With<A>, With<B>, With<C>), (With<A>, With<B>, With<D>, Without<E>))>`.
+        expected.filter_sets = vec![
+            AccessFilters {
+                with: FixedBitSet::with_capacity_and_blocks(3, [0b111]),
+                without: FixedBitSet::default(),
+                _index_type: PhantomData,
+            },
+            AccessFilters {
+                with: FixedBitSet::with_capacity_and_blocks(4, [0b1011]),
+                without: FixedBitSet::with_capacity_and_blocks(5, [0b10000]),
+                _index_type: PhantomData,
+            },
+        ];
+
+        assert_eq!(access_a, expected);
+    }
 }

crates/bevy_ecs/src/query/fetch.rs

@@ -1281,34 +1281,21 @@ macro_rules! impl_anytuple_fetch {
             fn update_component_access(state: &Self::State, _access: &mut FilteredAccess<ComponentId>) {
                 let ($($name,)*) = state;
 
-                // We do not unconditionally add `$name`'s `with`/`without` accesses to `_access`
-                // as this would be unsound. For example the following two queries should conflict:
-                // - Query<(AnyOf<(&A, ())>, &mut B)>
-                // - Query<&mut B, Without<A>>
-                //
-                // If we were to unconditionally add `$name`'s `with`/`without` accesses then `AnyOf<(&A, ())>`
-                // would have a `With<A>` access which is incorrect as this `WorldQuery` will match entities that
-                // do not have the `A` component. This is the same logic as the `Or<...>: WorldQuery` impl.
-                //
-                // The correct thing to do here is to only add a `with`/`without` access to `_access` if all
-                // `$name` params have that `with`/`without` access. More jargony put- we add the intersection
-                // of all `with`/`without` accesses of the `$name` params to `_access`.
-                let mut _intersected_access = _access.clone();
+                let mut _new_access = _access.clone();
                 let mut _not_first = false;
                 $(
                     if _not_first {
                         let mut intermediate = _access.clone();
                         $name::update_component_access($name, &mut intermediate);
-                        _intersected_access.extend_intersect_filter(&intermediate);
-                        _intersected_access.extend_access(&intermediate);
+                        _new_access.append_or(&intermediate);
+                        _new_access.extend_access(&intermediate);
                     } else {
-
-                        $name::update_component_access($name, &mut _intersected_access);
+                        $name::update_component_access($name, &mut _new_access);
                         _not_first = true;
                     }
                 )*
 
-                *_access = _intersected_access;
+                *_access = _new_access;
             }
 
             fn update_archetype_component_access(state: &Self::State, _archetype: &Archetype, _access: &mut Access<ArchetypeComponentId>) {