From eabbb3e903ba0cc683e38ccc7a985ec164163f29 Mon Sep 17 00:00:00 2001 From: Joy <51241057+maniwani@users.noreply.github.com> Date: Mon, 9 May 2022 14:39:22 +0000 Subject: [PATCH] Add comparison methods to FilteredAccessSet (#4211) # Objective - (Eventually) reduce noise in reporting access conflicts between unordered systems. - `SystemStage` only looks at unfiltered `ComponentId` access, any conflicts reported are potentially `false`. - the systems could still be accessing disjoint archetypes - Comparing systems' filtered access sets can maybe avoid that (for statically known component types). - #4204 ## Solution - Modify `SparseSetIndex` trait to require `PartialEq`, `Eq`, and `Hash` (all internal types except `BundleId` already did). - Add `is_compatible` and `get_conflicts` methods to `FilteredAccessSet` - (existing method renamed to `get_conflicts_single`) - Add docs for those and all the other methods while I'm at it. --- crates/bevy_ecs/src/bundle.rs | 2 +- crates/bevy_ecs/src/query/access.rs | 168 +++++++++++++++------ crates/bevy_ecs/src/storage/sparse_set.rs | 4 +- crates/bevy_ecs/src/system/system_param.rs | 4 +- 4 files changed, 123 insertions(+), 55 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 1ff332ec62ca6c..6b81435fc8b4bd 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -129,7 +129,7 @@ macro_rules! tuple_impl { all_tuples!(tuple_impl, 0, 15, C); -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] pub struct BundleId(usize); impl BundleId { diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index b324792460ad82..54bb81567ae196 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -3,15 +3,19 @@ use bevy_utils::HashSet; use fixedbitset::FixedBitSet; use std::marker::PhantomData; -/// `Access` keeps track of read and write accesses to values within a collection. +/// Tracks read and write access to specific elements in a collection. /// -/// This is used for ensuring systems are executed soundly. -#[derive(Debug, Eq, PartialEq, Clone)] +/// Used internally to ensure soundness during system initialization and execution. +/// See the [`is_compatible`](Access::is_compatible) and [`get_conflicts`](Access::get_conflicts) functions. +#[derive(Debug, Clone, Eq, PartialEq)] pub struct Access { - reads_all: bool, - /// A combined set of T read and write accesses. + /// All accessed elements. reads_and_writes: FixedBitSet, + /// The exclusively-accessed elements. writes: FixedBitSet, + /// Is `true` if this has access to all elements in the collection? + /// This field is a performance optimization for `&World` (also harder to mess up for soundness). + reads_all: bool, marker: PhantomData, } @@ -27,26 +31,29 @@ impl Default for Access { } impl Access { - pub fn grow(&mut self, bits: usize) { - self.reads_and_writes.grow(bits); - self.writes.grow(bits); + /// Increases the set capacity to the specified amount. + /// + /// Does nothing if `capacity` is less than or equal to the current value. + pub fn grow(&mut self, capacity: usize) { + self.reads_and_writes.grow(capacity); + self.writes.grow(capacity); } - /// Adds a read access for the given index. + /// Adds access to the element given by `index`. pub fn add_read(&mut self, index: T) { self.reads_and_writes.grow(index.sparse_set_index() + 1); self.reads_and_writes.insert(index.sparse_set_index()); } - /// Adds a write access for the given index. + /// Adds exclusive access to the element given by `index`. pub fn add_write(&mut self, index: T) { self.reads_and_writes.grow(index.sparse_set_index() + 1); - self.writes.grow(index.sparse_set_index() + 1); self.reads_and_writes.insert(index.sparse_set_index()); + self.writes.grow(index.sparse_set_index() + 1); self.writes.insert(index.sparse_set_index()); } - /// Returns true if this `Access` contains a read access for the given index. + /// Returns `true` if this can access the element given by `index`. pub fn has_read(&self, index: T) -> bool { if self.reads_all { true @@ -55,51 +62,54 @@ impl Access { } } - /// Returns true if this `Access` contains a write access for the given index. + /// Returns `true` if this can exclusively access the element given by `index`. pub fn has_write(&self, index: T) -> bool { self.writes.contains(index.sparse_set_index()) } - /// Sets this `Access` to having read access for all indices. + /// Sets this as having access to all indexed elements (i.e. `&World`). pub fn read_all(&mut self) { self.reads_all = true; } - /// Returns true if this `Access` has read access to all indices. - pub fn reads_all(&self) -> bool { + /// Returns `true` if this has access to all indexed elements (i.e. `&World`). + pub fn has_read_all(&self) -> bool { self.reads_all } - /// Clears all recorded accesses. + /// Removes all accesses. pub fn clear(&mut self) { self.reads_all = false; self.reads_and_writes.clear(); self.writes.clear(); } - /// Extends this `Access` with another, copying all accesses of `other` into this. + /// Adds all access from `other`. pub fn extend(&mut self, other: &Access) { self.reads_all = self.reads_all || other.reads_all; self.reads_and_writes.union_with(&other.reads_and_writes); self.writes.union_with(&other.writes); } - /// Returns true if this `Access` is compatible with `other`. + /// Returns `true` if the access and `other` can be active at the same time. /// - /// Two `Access` instances are incompatible with each other if one `Access` has a write for - /// which the other also has a write or a read. + /// `Access` instances are incompatible if one can write + /// an element that the other can read or write. pub fn is_compatible(&self, other: &Access) -> bool { + // Only systems that do not write data are compatible with systems that operate on `&World`. if self.reads_all { - 0 == other.writes.count_ones(..) - } else if other.reads_all { - 0 == self.writes.count_ones(..) - } else { - self.writes.is_disjoint(&other.reads_and_writes) - && self.reads_and_writes.is_disjoint(&other.writes) + return other.writes.count_ones(..) == 0; + } + + if other.reads_all { + return self.writes.count_ones(..) == 0; } + + self.writes.is_disjoint(&other.reads_and_writes) + && self.reads_and_writes.is_disjoint(&other.writes) } - /// Calculates conflicting accesses between this `Access` and `other`. + /// Returns a vector of elements that the access and `other` cannot access at the same time. pub fn get_conflicts(&self, other: &Access) -> Vec { let mut conflicts = FixedBitSet::default(); if self.reads_all { @@ -117,20 +127,28 @@ impl Access { .collect() } - /// Returns all read accesses. + /// Returns the indices of the elements this has access to. + pub fn reads_and_writes(&self) -> impl Iterator + '_ { + self.reads_and_writes.ones().map(T::get_sparse_set_index) + } + + /// Returns the indices of the elements this has non-exclusive access to. pub fn reads(&self) -> impl Iterator + '_ { self.reads_and_writes .difference(&self.writes) .map(T::get_sparse_set_index) } - /// Returns all write accesses. + /// Returns the indices of the elements this has exclusive access to. pub fn writes(&self) -> impl Iterator + '_ { self.writes.ones().map(T::get_sparse_set_index) } } -#[derive(Clone, Eq, PartialEq, Debug)] +/// An [`Access`] that has been filtered to include and exclude certain combinations of elements. +/// +/// Used internally to statically check if queries are disjoint. +#[derive(Debug, Clone, Eq, PartialEq)] pub struct FilteredAccess { access: Access, with: FixedBitSet, @@ -156,31 +174,43 @@ impl From> for FilteredAccessSet { } impl FilteredAccess { + /// Returns a reference to the underlying unfiltered access. #[inline] pub fn access(&self) -> &Access { &self.access } + /// Returns a mutable reference to the underlying unfiltered access. + #[inline] + pub fn access_mut(&mut self) -> &mut Access { + &mut self.access + } + + /// 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); } + /// 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); } + /// 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()); } + /// 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()); } + /// Returns `true` if this and `other` can be active at the same time. pub fn is_compatible(&self, other: &FilteredAccess) -> bool { if self.access.is_compatible(&other.access) { true @@ -190,56 +220,94 @@ impl FilteredAccess { } } + /// Returns a vector of elements that this and `other` cannot access at the same time. + pub fn get_conflicts(&self, other: &FilteredAccess) -> Vec { + if !self.is_compatible(other) { + // filters are disjoint, so we can just look at the unfiltered intersection + return self.access.get_conflicts(&other.access); + } + Vec::new() + } + + /// 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); } + /// Sets the underlying unfiltered access as having access to all indexed elements. pub fn read_all(&mut self) { self.access.read_all(); } } -#[derive(Clone, Debug)] + +/// A collection of [`FilteredAccess`] instances. +/// +/// Used internally to statically check if systems have conflicting access. +#[derive(Debug, Clone)] pub struct FilteredAccessSet { combined_access: Access, filtered_accesses: Vec>, } impl FilteredAccessSet { + /// Returns a reference to the unfiltered access of the entire set. #[inline] pub fn combined_access(&self) -> &Access { &self.combined_access } + /// Returns a mutable reference to the unfiltered access of the entire set. #[inline] pub fn combined_access_mut(&mut self) -> &mut Access { &mut self.combined_access } - pub fn get_conflicts(&self, filtered_access: &FilteredAccess) -> Vec { - // if combined unfiltered access is incompatible, check each filtered access for - // compatibility - let mut conflicts = HashSet::::default(); - if !filtered_access.access.is_compatible(&self.combined_access) { - for current_filtered_access in &self.filtered_accesses { - if !current_filtered_access.is_compatible(filtered_access) { - conflicts.extend( - current_filtered_access - .access - .get_conflicts(&filtered_access.access) - .iter() - .map(|ind| ind.sparse_set_index()), - ); + /// Returns `true` if this and `other` can be active at the same time. + pub fn is_compatible(&self, other: &FilteredAccessSet) -> bool { + if self.combined_access.is_compatible(other.combined_access()) { + return true; + } else { + for filtered in self.filtered_accesses.iter() { + for other_filtered in other.filtered_accesses.iter() { + if !filtered.is_compatible(other_filtered) { + return false; + } } } } - conflicts - .iter() - .map(|ind| T::get_sparse_set_index(*ind)) - .collect() + + true + } + + /// Returns a vector of elements that this set and `other` cannot access at the same time. + pub fn get_conflicts(&self, other: &FilteredAccessSet) -> Vec { + // if the unfiltered access is incompatible, must check each pair + let mut conflicts = HashSet::new(); + if !self.combined_access.is_compatible(other.combined_access()) { + for filtered in self.filtered_accesses.iter() { + for other_filtered in other.filtered_accesses.iter() { + conflicts.extend(filtered.get_conflicts(other_filtered).into_iter()); + } + } + } + conflicts.into_iter().collect() + } + + /// Returns a vector of elements that this set and `other` cannot access at the same time. + pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess) -> Vec { + // if the unfiltered access is incompatible, must check each pair + let mut conflicts = HashSet::new(); + if !self.combined_access.is_compatible(filtered_access.access()) { + for filtered in self.filtered_accesses.iter() { + conflicts.extend(filtered.get_conflicts(filtered_access).into_iter()); + } + } + conflicts.into_iter().collect() } + /// Adds the filtered access to the set. pub fn add(&mut self, filtered_access: FilteredAccess) { self.combined_access.extend(&filtered_access.access); self.filtered_accesses.push(filtered_access); diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 192cfd2aae9bcc..e21a4f4723c149 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -4,7 +4,7 @@ use crate::{ storage::BlobVec, }; use bevy_ptr::{OwningPtr, Ptr}; -use std::{cell::UnsafeCell, marker::PhantomData}; +use std::{cell::UnsafeCell, hash::Hash, marker::PhantomData}; #[derive(Debug)] pub struct SparseArray { @@ -372,7 +372,7 @@ impl SparseSet { } } -pub trait SparseSetIndex: Clone { +pub trait SparseSetIndex: Clone + PartialEq + Eq + Hash { fn sparse_set_index(&self) -> usize; fn get_sparse_set_index(value: usize) -> Self; } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index c6cc0556f7cc49..b56a2e3461d70f 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -154,7 +154,7 @@ fn assert_component_access_compatibility( current: &FilteredAccess, world: &World, ) { - let mut conflicts = system_access.get_conflicts(current); + let mut conflicts = system_access.get_conflicts_single(current); if conflicts.is_empty() { return; } @@ -531,7 +531,7 @@ unsafe impl<'w, 's> SystemParamState for WorldState { filtered_access.read_all(); if !system_meta .component_access_set - .get_conflicts(&filtered_access) + .get_conflicts_single(&filtered_access) .is_empty() { panic!("&World conflicts with a previous mutable system parameter. Allowing this would break Rust's mutability rules");