diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index ea18adf841073..b75f658b4bdf8 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -25,6 +25,7 @@ struct FormattedBitSet<'a, T: SparseSetIndex> { bit_set: &'a FixedBitSet, _marker: PhantomData, } + 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 fmt::Debug for Access { .finish() } } + impl Default for Access { fn default() -> Self { Self::new() @@ -213,31 +216,22 @@ impl Access { /// 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` accesses nothing +/// - `Query>` 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 { access: Access, - with: FixedBitSet, - without: FixedBitSet, -} -impl fmt::Debug for FilteredAccess { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("FilteredAccess") - .field("access", &self.access) - .field("with", &FormattedBitSet::::new(&self.with)) - .field("without", &FormattedBitSet::::new(&self.without)) - .finish() - } + // An array of filter sets to express `With` or `Without` clauses in disjunctive normal form, for example: `Or<(With, With)>`. + // Filters like `(With, Or<(With, Without)>` are expanded into `Or<((With, With), (With, Without))>`. + filter_sets: Vec>, } impl Default for FilteredAccess { fn default() -> Self { Self { access: Access::default(), - with: Default::default(), - without: Default::default(), + filter_sets: vec![AccessFilters::default()], } } } @@ -266,30 +260,46 @@ impl FilteredAccess { /// 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, With)>`, which is represented by an array of two `AccessFilter` instances. + /// Adding `AND With` via this method transforms it into the equivalent of `Or<((With, With), (With, With))>`. + 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, With)>`, which is represented by an array of two `AccessFilter` instances. + /// Adding `AND Without` via this method transforms it into the equivalent of `Or<((With, Without), (With, Without))>`. + 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) { - 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) { + self.filter_sets.append(&mut other.filter_sets.clone()); } pub fn extend_access(&mut self, other: &FilteredAccess) { @@ -298,9 +308,23 @@ impl FilteredAccess { /// Returns `true` if this and `other` can be active at the same time. pub fn is_compatible(&self, other: &FilteredAccess) -> 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, Without)>>` is compatible `Query<&mut C, (With, Without)>`, + // but `Query<&mut C, Or<(Without, Without)>>` isn't compatible with `Query<&mut C, Or<(With, With)>>`. + 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 FilteredAccess { } /// Adds all access and filters from `other`. - pub fn extend(&mut self, access: &FilteredAccess) { - 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, Without)>` with `Or<(With, Without)>` will result in + /// `Or<((With, With), (With, Without), (Without, With), (Without, Without))>`. + pub fn extend(&mut self, other: &FilteredAccess) { + 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 FilteredAccess { } } +#[derive(Clone, Eq, PartialEq)] +struct AccessFilters { + with: FixedBitSet, + without: FixedBitSet, + _index_type: PhantomData, +} + +impl fmt::Debug for AccessFilters { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("AccessFilters") + .field("with", &FormattedBitSet::::new(&self.with)) + .field("without", &FormattedBitSet::::new(&self.without)) + .finish() + } +} + +impl Default for AccessFilters { + fn default() -> Self { + Self { + with: FixedBitSet::default(), + without: FixedBitSet::default(), + _index_type: PhantomData, + } + } +} + +impl AccessFilters { + 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, Without)`). + // 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 Default for FilteredAccessSet { #[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::::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::::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::::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::::default(); + // Exclusive access to `(&mut A, &mut B)`. + access_a.add_write(0); + access_a.add_write(1); + + // Filter by `With`. + let mut access_b = FilteredAccess::::default(); + access_b.and_with(2); + + // Filter by `(With, Without)`. + let mut access_c = FilteredAccess::::default(); + access_c.and_with(3); + access_c.and_without(4); + + // Turns `access_b` into `Or<(With, (With, Without))>`. + 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, (With, Without))>>`. + 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::::default(); + expected.add_write(0); + expected.add_write(1); + // The resulted access is expected to represent `Or<((With, With, With), (With, With, With, Without))>`. + 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); + } } diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 680fc0c319413..e271483e2e9ca 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/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) { 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> - // - // If we were to unconditionally add `$name`'s `with`/`without` accesses then `AnyOf<(&A, ())>` - // would have a `With` 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) { diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index a47350e88bfa5..d8c94f45f1e0c 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -86,7 +86,7 @@ unsafe impl WorldQuery for With { #[inline] fn update_component_access(&id: &ComponentId, access: &mut FilteredAccess) { - access.add_with(id); + access.and_with(id); } #[inline] @@ -183,7 +183,7 @@ unsafe impl WorldQuery for Without { #[inline] fn update_component_access(&id: &ComponentId, access: &mut FilteredAccess) { - access.add_without(id); + access.and_without(id); } #[inline] @@ -339,33 +339,21 @@ macro_rules! impl_query_filter_tuple { fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { let ($($filter,)*) = state; - // We do not unconditionally add `$filter`'s `with`/`without` accesses to `access` - // as this would be unsound. For example the following two queries should conflict: - // - Query<&mut B, Or<(With, ())>> - // - Query<&mut B, Without> - // - // If we were to unconditionally add `$name`'s `with`/`without` accesses then `Or<(With, ())>` - // would have a `With` access which is incorrect as this `WorldQuery` will match entities that - // do not have the `A` component. This is the same logic as the `AnyOf<...>: WorldQuery` impl. - // - // The correct thing to do here is to only add a `with`/`without` access to `_access` if all - // `$filter` params have that `with`/`without` access. More jargony put- we add the intersection - // of all `with`/`without` accesses of the `$filter` 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(); $filter::update_component_access($filter, &mut intermediate); - _intersected_access.extend_intersect_filter(&intermediate); - _intersected_access.extend_access(&intermediate); + _new_access.append_or(&intermediate); + _new_access.extend_access(&intermediate); } else { - $filter::update_component_access($filter, &mut _intersected_access); + $filter::update_component_access($filter, &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) { diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 1ce4bfc17f915..4bcb9378c29d5 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -483,6 +483,13 @@ mod tests { run_system(&mut world, sys); } + #[test] + fn any_of_and_without() { + fn sys(_: Query<(AnyOf<(&A, &B)>, &mut C)>, _: Query<&mut C, (Without, Without)>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + #[test] #[should_panic = "error[B0001]"] fn or_has_no_filter_with() { @@ -498,6 +505,113 @@ mod tests { run_system(&mut world, sys); } + #[test] + fn or_has_filter_with() { + fn sys( + _: Query<&mut C, Or<(With, With)>>, + _: Query<&mut C, (Without, Without)>, + ) { + } + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + fn or_expanded_with_and_without_common() { + fn sys(_: Query<&mut D, (With, Or<(With, With)>)>, _: Query<&mut D, Without>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + fn or_expanded_nested_with_and_without_common() { + fn sys( + _: Query<&mut E, (Or<((With, With), (With, With))>, With)>, + _: Query<&mut E, (Without, Without)>, + ) { + } + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic = "error[B0001]"] + fn or_expanded_nested_with_and_disjoint_without() { + fn sys( + _: Query<&mut E, (Or<((With, With), (With, With))>, With)>, + _: Query<&mut E, Without>, + ) { + } + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic = "error[B0001]"] + fn or_expanded_nested_or_with_and_disjoint_without() { + fn sys( + _: Query<&mut D, Or<(Or<(With, With)>, Or<(With, With)>)>>, + _: Query<&mut D, Without>, + ) { + } + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + fn or_expanded_nested_with_and_common_nested_without() { + fn sys( + _: Query<&mut D, Or<((With, With), (With, With))>>, + _: Query<&mut D, Or<(Without, Without)>>, + ) { + } + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + fn or_with_without_and_compatible_with_without() { + fn sys( + _: Query<&mut C, Or<(With, Without)>>, + _: Query<&mut C, (With, Without)>, + ) { + } + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic = "error[B0001]"] + fn with_and_disjoint_or_empty_without() { + fn sys(_: Query<&mut B, With>, _: Query<&mut B, Or<((), Without)>>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic = "error[B0001]"] + fn or_expanded_with_and_disjoint_nested_without() { + fn sys( + _: Query<&mut D, Or<(With, With)>>, + _: Query<&mut D, Or<(Without, Without)>>, + ) { + } + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic = "error[B0001]"] + fn or_expanded_nested_with_and_disjoint_nested_without() { + fn sys( + _: Query<&mut D, Or<((With, With), (With, With))>>, + _: Query<&mut D, Or<(Without, Without)>>, + ) { + } + let mut world = World::default(); + run_system(&mut world, sys); + } + #[test] fn or_doesnt_remove_unrelated_filter_with() { fn sys(_: Query<&mut B, (Or<(With, With)>, With)>, _: Query<&mut B, Without>) {}