Fix FilteredAccessSet get_conflicts inconsistency (#5105)
# Objective * Enable `Res` and `Query` parameter mutual exclusion * Required for https://github.com/bevyengine/bevy/pull/5080 The `FilteredAccessSet::get_conflicts` methods didn't work properly with `Res` and `ResMut` parameters. Because those added their access by using the `combined_access_mut` method and directly modifying the global access state of the FilteredAccessSet. This caused an inconsistency, because get_conflicts assumes that ALL added access have a corresponding `FilteredAccess` added to the `filtered_accesses` field. In practice, that means that SystemParam that adds their access through the `Access` returned by `combined_access_mut` and the ones that add their access using the `add` method lived in two different universes. As a result, they could never be mutually exclusive. ## Solution This commit fixes it by removing the `combined_access_mut` method. This ensures that the `combined_access` field of FilteredAccessSet is always updated consistently with the addition of a filter. When checking for filtered access, it is now possible to account for `Res` and `ResMut` invalid access. This is currently not needed, but might be in the future. We add the `add_unfiltered_{read,write}` methods to replace previous usages of `combined_access_mut`. We also add improved Debug implementations on FixedBitSet so that their meaning is much clearer in debug output. --- ## Changelog * Fix `Res` and `Query` parameter never being mutually exclusive. ## Migration Guide Note: this mostly changes ECS internals, but since the API is public, it is technically breaking: * Removed `FilteredAccessSet::combined_access_mut` * Replace _immutable_ usage of those by `combined_access` * For _mutable_ usages, use the new `add_unfiltered_{read,write}` methods instead of `combined_access_mut` followed by `add_{read,write}`
This commit is contained in:
Nicola Papale
parent
6763b31479
commit
00684d95f7
@ -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<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,
|
||||
@ -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 {
|
||||
@ -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`.
|
||||
@ -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.
|
||||
@ -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.
|
||||
@ -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 {
|
||||
@ -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)
|
||||
|| !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<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>,
|
||||
@ -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;
|
||||
@ -302,7 +363,6 @@ impl<T: SparseSetIndex> FilteredAccessSet<T> {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
true
|
||||
}
|
||||
|
||||
@ -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);
|
||||
@ -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() {
|
||||
@ -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();
|
||||
|
||||
@ -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;
|
||||
|
||||
@ -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)
|
||||
@ -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.",
|
||||
@ -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)
|
||||
@ -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)
|
||||
@ -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.",
|
||||
@ -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)
|
||||
|
||||
Loading…
Reference in New Issue
Block a user