Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve or-with disjoint checks #7085

Merged
merged 12 commits into from Apr 17, 2023
Merged

Improve or-with disjoint checks #7085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ad1c14b
Improve or-with disjoint checks
mvlabat Jan 3, 2023
470c6d5
Add more tests
mvlabat Jan 4, 2023
c3225fa
Fix comments in crates/bevy_ecs/src/query/access.rs
mvlabat Jan 4, 2023
2f4de50
Add support or-without filters as well
mvlabat Jan 15, 2023
3a861d9
Rename ExpandedOrAccess to NormalizedAccess, expand docs
mvlabat Feb 16, 2023
891be6b
Expand the impl NormalizedAccess documentation
mvlabat Feb 16, 2023
a7d57c7
Add comments to the filtered_access_extend_or test
mvlabat Feb 16, 2023
133578a
Add failing tests for scenarios discovered by @SkiFire13
mvlabat Feb 17, 2023
0d50b8c
Fix and rework the implementation to consider with and without filter…
mvlabat Feb 17, 2023
831e1c7
Update crates/bevy_ecs/src/query/access.rs docs
mvlabat Feb 17, 2023
cfc634d
Update comments
mvlabat Feb 18, 2023
ec9e2b7
Replace smallvec with vec
mvlabat Apr 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
211 changes: 172 additions & 39 deletions crates/bevy_ecs/src/query/access.rs
View file
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically speaking to be complete this also needs || !self.with.is_disjoing(&self.without) || !other.with.is_disjoint(&other.without) in order to handle extreme cases like (With<T>, Without<T>) being empty and thus disjoint with everything else. Not sure if we want to properly support those cases though, in practice if they happen they're almost always a mistake. The condition being at the end means it would be checked only if we're about to find a conflict, so the performance hit should not happen in the happy path.

However if it's kept like this I would add a comment explaining the missing conditions and why they're not implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was also thinking about this case. I'm inclined not to support it, I agree that it almost always means there's an error. Added a comment.

}
}

/// 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() {
mvlabat marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
Expand Up @@ -1275,34 +1275,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 {
mvlabat marked this conversation as resolved.
Show resolved Hide resolved
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