Skip to content

Commit

Permalink
Improve or-with disjoint checks (bevyengine#7085)
Browse files Browse the repository at this point in the history 
# 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 <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
  • Loading branch information
@vladbat00 @MinerSebas @alice-i-cecile
3 people authored Apr 17, 2023
1 parent c488b70 commit 71fccb2
Show file tree
Hide file tree
Showing 4 changed files with 298 additions and 76 deletions.
211 changes: 172 additions & 39 deletions crates/bevy_ecs/src/query/access.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()],
}
}
}
Expand Down Expand Up @@ -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>) {
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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);
}
}
23 changes: 5 additions & 18 deletions crates/bevy_ecs/src/query/fetch.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>) {
Expand Down
Loading

0 comments on commit 71fccb2

Please sign in to comment.