diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 8b3dd275d412de..787daeb15f97ec 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`. @@ -98,15 +144,15 @@ impl Access { 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 { - return other.writes.count_ones(..) == 0; + return other.writes.is_empty(); } if other.reads_all { - return self.writes.count_ones(..) == 0; + return self.writes.is_empty(); } 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. @@ -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. @@ -284,12 +336,6 @@ 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. pub fn is_compatible(&self, other: &FilteredAccessSet) -> bool { if self.combined_access.is_compatible(other.combined_access()) { @@ -338,6 +384,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 +422,7 @@ impl Default for FilteredAccessSet { #[cfg(test)] mod tests { - use crate::query::{Access, FilteredAccess}; + use crate::query::{Access, FilteredAccess, FilteredAccessSet}; #[test] fn access_get_conflicts() { @@ -391,6 +451,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 1d5172b64a6245..229e1780c6326c 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -370,6 +370,13 @@ impl SparseSet { } } +/// Represents something that can be stored in a [`FixedBitSet`] as an interger. +/// +/// 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`. +/// +/// [`FixedBitSet`]: fixedbitset::FixedBitSet 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 96a574dd562df3..4ab8f92f7c8725 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -310,14 +310,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 resource_archetype = world.archetypes.resource(); let archetype_component_id = resource_archetype @@ -416,7 +418,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.", @@ -426,7 +428,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 resource_archetype = world.archetypes.resource(); let archetype_component_id = resource_archetype @@ -864,14 +868,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 resource_archetype = world.archetypes.resource(); let archetype_component_id = resource_archetype @@ -975,7 +981,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.", @@ -985,7 +991,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 resource_archetype = world.archetypes.resource(); let archetype_component_id = resource_archetype