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

[Merged by Bors] - Fix FilteredAccessSet get_conflicts inconsistency #5105

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Fix FilteredAccessSet get_conflicts inconsistency #5105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d1d190
Fix FilteredAccessSet get_conflicts inconsistency
nicopap Jun 26, 2022
8df367a
Add read_all access conflict test
nicopap Jul 1, 2022
d9dd553
Precise use of SparseSetIndex
nicopap Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 136 additions & 23 deletions crates/bevy_ecs/src/query/access.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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> {
Copy link
Member

Choose a reason for hiding this comment

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

This debug formatting is so much nicer.

bit_set: &'a FixedBitSet,
_marker: PhantomData<T>,
}
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<T: SparseSetIndex> {
/// All accessed elements.
reads_and_writes: FixedBitSet,
Expand All @@ -19,6 +57,18 @@ pub struct Access<T: SparseSetIndex> {
marker: PhantomData<T>,
}

impl<T: SparseSetIndex + fmt::Debug> fmt::Debug for Access<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Access")
.field(
"read_and_writes",
&FormattedBitSet::<T>::new(&self.reads_and_writes),
)
.field("writes", &FormattedBitSet::<T>::new(&self.writes))
.field("reads_all", &self.reads_all)
.finish()
}
}
impl<T: SparseSetIndex> Default for Access<T> {
fn default() -> Self {
Self {
Expand Down Expand Up @@ -55,11 +105,7 @@ impl<T: SparseSetIndex> Access<T> {

/// 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`.
Expand Down Expand Up @@ -106,7 +152,7 @@ impl<T: SparseSetIndex> Access<T> {
}

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.
Expand Down Expand Up @@ -153,7 +199,7 @@ impl<T: SparseSetIndex> Access<T> {
/// `with` access.
///
/// For example consider `Query<Option<&T>>` 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<U>>` read/write `T`, without `U`
/// from this we could reasonably conclude that the queries are disjoint but they aren't.
Expand All @@ -165,12 +211,21 @@ impl<T: SparseSetIndex> Access<T> {
/// - `Query<Option<&T>` 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<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()
}
}

impl<T: SparseSetIndex> Default for FilteredAccess<T> {
fn default() -> Self {
Expand Down Expand Up @@ -238,12 +293,9 @@ 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 {
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)
nicopap marked this conversation as resolved.
Show resolved Hide resolved
|| !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.
Expand Down Expand Up @@ -271,6 +323,10 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
/// 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<T: SparseSetIndex> {
combined_access: Access<T>,
Expand All @@ -284,13 +340,18 @@ impl<T: SparseSetIndex> FilteredAccessSet<T> {
&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<T> {
&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<T>) -> bool {
if self.combined_access.is_compatible(other.combined_access()) {
return true;
Expand All @@ -302,7 +363,6 @@ impl<T: SparseSetIndex> FilteredAccessSet<T> {
}
}
}

true
}

Expand Down Expand Up @@ -338,6 +398,20 @@ impl<T: SparseSetIndex> FilteredAccessSet<T> {
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<T>) {
self.combined_access
.extend(&filtered_access_set.combined_access);
Expand All @@ -362,7 +436,30 @@ impl<T: SparseSetIndex> Default for FilteredAccessSet<T> {

#[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::<usize>::default();
access_a.grow(10);
access_a.add_write(0);

let mut access_b = Access::<usize>::default();
access_b.read_all();

assert!(!access_b.is_compatible(&access_a));

// read_all / read_all
let mut access_a = Access::<usize>::default();
access_a.grow(10);
access_a.read_all();

let mut access_b = Access::<usize>::default();
access_b.read_all();

assert!(access_b.is_compatible(&access_a));
}

#[test]
fn access_get_conflicts() {
Expand Down Expand Up @@ -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::<usize>::default();
access_a.add_unfiltered_read(1);

let mut filter_b = FilteredAccess::<usize>::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::<usize>::default();
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_ecs/src/storage/sparse_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,11 @@ impl<I: SparseSetIndex, V> SparseSet<I, V> {
}
}

/// 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;
Expand Down
24 changes: 16 additions & 8 deletions crates/bevy_ecs/src/system/system_param.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,14 +372,16 @@ impl<'a, T: Resource> SystemParam for Res<'a, T> {
unsafe impl<T: Resource> SystemParamState for ResState<T> {
fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self {
let component_id = world.initialize_resource::<T>();
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::<T>(),
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)
Expand Down Expand Up @@ -479,7 +481,7 @@ impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
unsafe impl<T: Resource> SystemParamState for ResMutState<T> {
fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self {
let component_id = world.initialize_resource::<T>();
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.",
Expand All @@ -489,7 +491,9 @@ unsafe impl<T: Resource> SystemParamState for ResMutState<T> {
"error[B0002]: ResMut<{}> in system {} conflicts with a previous Res<{0}> access. Consider removing the duplicate access.",
std::any::type_name::<T>(), 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)
Expand Down Expand Up @@ -963,14 +967,16 @@ unsafe impl<T: 'static> SystemParamState for NonSendState<T> {
system_meta.set_non_send();

let component_id = world.initialize_non_send_resource::<T>();
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::<T>(),
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)
Expand Down Expand Up @@ -1075,7 +1081,7 @@ unsafe impl<T: 'static> SystemParamState for NonSendMutState<T> {
system_meta.set_non_send();

let component_id = world.initialize_non_send_resource::<T>();
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.",
Expand All @@ -1085,7 +1091,9 @@ unsafe impl<T: 'static> SystemParamState for NonSendMutState<T> {
"error[B0002]: NonSendMut<{}> in system {} conflicts with a previous immutable resource access ({0}). Consider removing the duplicate access.",
std::any::type_name::<T>(), 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)
Expand Down