diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index d9580bd4eb..a28e6df23e 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -1,13 +1,51 @@ use crate::storage::SparseSetIndex; use bevy_utils::HashSet; +use core::fmt; use fixedbitset::FixedBitSet; use std::marker::PhantomData; +/// A wrapper struct to make Debug representations of [`FixedBitSet`] easier +/// to read, when used to store [`SparseSetIndex`]. +/// +/// Instead of the raw integer representation of the `FixedBitSet`, the list of +/// `T` valid for [`SparseSetIndex`] is shown. +/// +/// Normal `FixedBitSet` `Debug` output: +/// ```text +/// read_and_writes: FixedBitSet { data: [ 160 ], length: 8 } +/// ``` +/// +/// Which, unless you are a computer, doesn't help much understand what's in +/// the set. With `FormattedBitSet`, we convert the present set entries into +/// what they stand for, it is much clearer what is going on: +/// ```text +/// read_and_writes: [ ComponentId(5), ComponentId(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 { + bit_set, + _marker: PhantomData, + } + } +} +impl<'a, T: SparseSetIndex + fmt::Debug> fmt::Debug for FormattedBitSet<'a, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_list() + .entries(self.bit_set.ones().map(T::get_sparse_set_index)) + .finish() + } +} + /// Tracks read and write access to specific elements in a collection. /// /// 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)] +#[derive(Clone, Eq, PartialEq)] pub struct Access { /// All accessed elements. reads_and_writes: FixedBitSet, @@ -19,6 +57,18 @@ pub struct Access { marker: PhantomData, } +impl fmt::Debug for Access { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Access") + .field( + "read_and_writes", + &FormattedBitSet::::new(&self.reads_and_writes), + ) + .field("writes", &FormattedBitSet::::new(&self.writes)) + .field("reads_all", &self.reads_all) + .finish() + } +} impl Default for Access { fn default() -> Self { Self { @@ -55,11 +105,7 @@ impl Access { /// Returns `true` if this can access the element given by `index`. pub fn has_read(&self, index: T) -> bool { - if self.reads_all { - true - } else { - self.reads_and_writes.contains(index.sparse_set_index()) - } + self.reads_all || self.reads_and_writes.contains(index.sparse_set_index()) } /// Returns `true` if this can exclusively access the element given by `index`. @@ -106,7 +152,7 @@ impl Access { } self.writes.is_disjoint(&other.reads_and_writes) - && self.reads_and_writes.is_disjoint(&other.writes) + && other.writes.is_disjoint(&self.reads_and_writes) } /// Returns a vector of elements that the access and `other` cannot access at the same time. @@ -153,7 +199,7 @@ impl Access { /// `with` access. /// /// For example consider `Query>` this only has a `read` of `T` as doing -/// otherwise would allow for queries to be considered disjoint that actually aren't: +/// otherwise would allow for queries to be considered disjoint when they shouldn't: /// - `Query<(&mut T, Option<&U>)>` read/write `T`, read `U`, with `U` /// - `Query<&mut T, Without>` read/write `T`, without `U` /// from this we could reasonably conclude that the queries are disjoint but they aren't. @@ -165,12 +211,21 @@ impl Access { /// - `Query` accesses nothing /// /// See comments the `WorldQuery` impls of `AnyOf`/`Option`/`Or` for more information. -#[derive(Debug, Clone, Eq, PartialEq)] +#[derive(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() + } +} impl Default for FilteredAccess { fn default() -> Self { @@ -238,12 +293,9 @@ impl FilteredAccess { /// 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 - } else { - self.with.intersection(&other.without).next().is_some() - || self.without.intersection(&other.with).next().is_some() - } + self.access.is_compatible(&other.access) + || !self.with.is_disjoint(&other.without) + || !other.with.is_disjoint(&self.without) } /// Returns a vector of elements that this and `other` cannot access at the same time. @@ -271,6 +323,10 @@ impl FilteredAccess { /// A collection of [`FilteredAccess`] instances. /// /// Used internally to statically check if systems have conflicting access. +/// +/// It stores multiple sets of accesses. +/// - A "combined" set, which is the access of all filters in this set combined. +/// - The set of access of each individual filters in this set. #[derive(Debug, Clone)] pub struct FilteredAccessSet { combined_access: Access, @@ -284,13 +340,18 @@ impl FilteredAccessSet { &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 - } - /// Returns `true` if this and `other` can be active at the same time. + /// + /// Access conflict resolution happen in two steps: + /// 1. A "coarse" check, if there is no mutual unfiltered conflict between + /// `self` and `other`, we already know that the two access sets are + /// compatible. + /// 2. A "fine grained" check, it kicks in when the "coarse" check fails. + /// the two access sets might still be compatible if some of the accesses + /// are restricted with the `With` or `Without` filters so that access is + /// mutually exclusive. The fine grained phase iterates over all filters in + /// the `self` set and compares it to all the filters in the `other` set, + /// making sure they are all mutually compatible. pub fn is_compatible(&self, other: &FilteredAccessSet) -> bool { if self.combined_access.is_compatible(other.combined_access()) { return true; @@ -302,7 +363,6 @@ impl FilteredAccessSet { } } } - true } @@ -338,6 +398,20 @@ impl FilteredAccessSet { self.filtered_accesses.push(filtered_access); } + /// Adds a read access without filters to the set. + pub(crate) fn add_unfiltered_read(&mut self, index: T) { + let mut filter = FilteredAccess::default(); + filter.add_read(index); + self.add(filter); + } + + /// Adds a write access without filters to the set. + pub(crate) fn add_unfiltered_write(&mut self, index: T) { + let mut filter = FilteredAccess::default(); + filter.add_write(index); + self.add(filter); + } + pub fn extend(&mut self, filtered_access_set: FilteredAccessSet) { self.combined_access .extend(&filtered_access_set.combined_access); @@ -362,7 +436,30 @@ impl Default for FilteredAccessSet { #[cfg(test)] mod tests { - use crate::query::{Access, FilteredAccess}; + use crate::query::{Access, FilteredAccess, FilteredAccessSet}; + + #[test] + fn read_all_access_conflicts() { + // read_all / single write + let mut access_a = Access::::default(); + access_a.grow(10); + access_a.add_write(0); + + let mut access_b = Access::::default(); + access_b.read_all(); + + assert!(!access_b.is_compatible(&access_a)); + + // read_all / read_all + let mut access_a = Access::::default(); + access_a.grow(10); + access_a.read_all(); + + let mut access_b = Access::::default(); + access_b.read_all(); + + assert!(access_b.is_compatible(&access_a)); + } #[test] fn access_get_conflicts() { @@ -391,6 +488,22 @@ mod tests { assert_eq!(access_d.get_conflicts(&access_c), vec![0]); } + #[test] + fn filtered_combined_access() { + let mut access_a = FilteredAccessSet::::default(); + access_a.add_unfiltered_read(1); + + let mut filter_b = FilteredAccess::::default(); + filter_b.add_write(1); + + let conflicts = access_a.get_conflicts_single(&filter_b); + assert_eq!( + &conflicts, + &[1_usize], + "access_a: {access_a:?}, filter_b: {filter_b:?}" + ); + } + #[test] fn filtered_access_extend() { let mut access_a = FilteredAccess::::default(); diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index fdb9a21176..f746708fa6 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -370,6 +370,11 @@ impl SparseSet { } } +/// Represents something that can be stored in a [`SparseSet`] as an integer. +/// +/// Ideally, the `usize` values should be very small (ie: incremented starting from +/// zero), as the number of bits needed to represent a `SparseSetIndex` in a `FixedBitSet` +/// is proportional to the **value** of those `usize`. 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 fa5f5bf517..acfdd8f722 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -372,14 +372,16 @@ impl<'a, T: Resource> SystemParam for Res<'a, T> { unsafe impl SystemParamState for ResState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { let component_id = world.initialize_resource::(); - let combined_access = system_meta.component_access_set.combined_access_mut(); + let combined_access = system_meta.component_access_set.combined_access(); assert!( !combined_access.has_write(component_id), "error[B0002]: Res<{}> in system {} conflicts with a previous ResMut<{0}> access. Consider removing the duplicate access.", std::any::type_name::(), system_meta.name, ); - combined_access.add_read(component_id); + system_meta + .component_access_set + .add_unfiltered_read(component_id); let archetype_component_id = world .get_resource_archetype_component_id(component_id) @@ -479,7 +481,7 @@ impl<'a, T: Resource> SystemParam for ResMut<'a, T> { unsafe impl SystemParamState for ResMutState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { let component_id = world.initialize_resource::(); - let combined_access = system_meta.component_access_set.combined_access_mut(); + let combined_access = system_meta.component_access_set.combined_access(); if combined_access.has_write(component_id) { panic!( "error[B0002]: ResMut<{}> in system {} conflicts with a previous ResMut<{0}> access. Consider removing the duplicate access.", @@ -489,7 +491,9 @@ unsafe impl SystemParamState for ResMutState { "error[B0002]: ResMut<{}> in system {} conflicts with a previous Res<{0}> access. Consider removing the duplicate access.", std::any::type_name::(), system_meta.name); } - combined_access.add_write(component_id); + system_meta + .component_access_set + .add_unfiltered_write(component_id); let archetype_component_id = world .get_resource_archetype_component_id(component_id) @@ -963,14 +967,16 @@ unsafe impl SystemParamState for NonSendState { system_meta.set_non_send(); let component_id = world.initialize_non_send_resource::(); - let combined_access = system_meta.component_access_set.combined_access_mut(); + let combined_access = system_meta.component_access_set.combined_access(); assert!( !combined_access.has_write(component_id), "error[B0002]: NonSend<{}> in system {} conflicts with a previous mutable resource access ({0}). Consider removing the duplicate access.", std::any::type_name::(), system_meta.name, ); - combined_access.add_read(component_id); + system_meta + .component_access_set + .add_unfiltered_read(component_id); let archetype_component_id = world .get_resource_archetype_component_id(component_id) @@ -1075,7 +1081,7 @@ unsafe impl SystemParamState for NonSendMutState { system_meta.set_non_send(); let component_id = world.initialize_non_send_resource::(); - let combined_access = system_meta.component_access_set.combined_access_mut(); + let combined_access = system_meta.component_access_set.combined_access(); if combined_access.has_write(component_id) { panic!( "error[B0002]: NonSendMut<{}> in system {} conflicts with a previous mutable resource access ({0}). Consider removing the duplicate access.", @@ -1085,7 +1091,9 @@ unsafe impl SystemParamState for NonSendMutState { "error[B0002]: NonSendMut<{}> in system {} conflicts with a previous immutable resource access ({0}). Consider removing the duplicate access.", std::any::type_name::(), system_meta.name); } - combined_access.add_write(component_id); + system_meta + .component_access_set + .add_unfiltered_write(component_id); let archetype_component_id = world .get_resource_archetype_component_id(component_id)