From 3445370621519b7857d1d1d3bc48bccab29cacd9 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Fri, 6 Oct 2023 10:34:06 -0700 Subject: [PATCH 01/27] generalize query state --- crates/bevy_ecs/macros/src/fetch.rs | 6 + crates/bevy_ecs/src/query/access.rs | 1234 ++++++++++++++------------- crates/bevy_ecs/src/query/fetch.rs | 43 +- crates/bevy_ecs/src/query/filter.rs | 18 +- crates/bevy_ecs/src/query/state.rs | 131 +++ 5 files changed, 827 insertions(+), 605 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index e28d33475b778..4a804bab9e10d 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -352,6 +352,12 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { } } + fn new_state(components: &#path::component::Components) -> #state_struct_name #user_ty_generics { + #state_struct_name { + #(#named_field_idents: <#field_types>::new_state(components),)* + } + } + fn matches_component_set(state: &Self::State, _set_contains_id: &impl Fn(#path::component::ComponentId) -> bool) -> bool { true #(&& <#field_types>::matches_component_set(&state.#named_field_idents, _set_contains_id))* } diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 808d31d2c48e7..b55e6efb48a1d 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -1,722 +1,750 @@ -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, - } - } -} + use crate::storage::SparseSetIndex; + use bevy_utils::HashSet; + use core::fmt; + use fixedbitset::FixedBitSet; + use std::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(Clone, Eq, PartialEq)] -pub struct Access { - /// 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, - /// Is `true` if this has mutable access to all elements in the collection. - /// If this is true, then `reads_all` must also be true. - writes_all: bool, - 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) - .field("writes_all", &self.writes_all) - .finish() + /// 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 Default for Access { - fn default() -> Self { - Self::new() - } -} - -impl Access { - /// Creates an empty [`Access`] collection. - pub const fn new() -> Self { - Self { - reads_all: false, - writes_all: false, - reads_and_writes: FixedBitSet::new(), - writes: FixedBitSet::new(), - 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() } } - /// Increases the set capacity to the specified amount. + /// Tracks read and write access to specific elements in a collection. /// - /// 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 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()); + /// 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(Clone, Eq, PartialEq)] + pub struct Access { + /// 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, + /// Is `true` if this has mutable access to all elements in the collection. + /// If this is true, then `reads_all` must also be true. + writes_all: bool, + 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) + .field("writes_all", &self.writes_all) + .finish() + } } - /// 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.reads_and_writes.insert(index.sparse_set_index()); - self.writes.grow(index.sparse_set_index() + 1); - self.writes.insert(index.sparse_set_index()); + impl Default for Access { + fn default() -> Self { + Self::new() + } } - /// Returns `true` if this can access the element given by `index`. - pub fn has_read(&self, index: T) -> bool { - self.reads_all || self.reads_and_writes.contains(index.sparse_set_index()) - } + impl Access { + /// Creates an empty [`Access`] collection. + pub const fn new() -> Self { + Self { + reads_all: false, + writes_all: false, + reads_and_writes: FixedBitSet::new(), + writes: FixedBitSet::new(), + marker: PhantomData, + } + } - /// Returns `true` if this can access anything. - pub fn has_any_read(&self) -> bool { - self.reads_all || !self.reads_and_writes.is_clear() - } + /// 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); + } - /// Returns `true` if this can exclusively access the element given by `index`. - pub fn has_write(&self, index: T) -> bool { - self.writes_all || self.writes.contains(index.sparse_set_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()); + } - /// Returns `true` if this accesses anything mutably. - pub fn has_any_write(&self) -> bool { - self.writes_all || !self.writes.is_clear() - } + /// 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.reads_and_writes.insert(index.sparse_set_index()); + self.writes.grow(index.sparse_set_index() + 1); + self.writes.insert(index.sparse_set_index()); + } - /// 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 can access the element given by `index`. + pub fn has_read(&self, index: T) -> bool { + self.reads_all || self.reads_and_writes.contains(index.sparse_set_index()) + } - /// Sets this as having mutable access to all indexed elements (i.e. `EntityMut`). - pub fn write_all(&mut self) { - self.reads_all = true; - self.writes_all = true; - } + /// Returns `true` if this can access anything. + pub fn has_any_read(&self) -> bool { + self.reads_all || !self.reads_and_writes.is_clear() + } - /// Returns `true` if this has access to all indexed elements (i.e. `&World`). - pub fn has_read_all(&self) -> bool { - self.reads_all - } + /// Returns `true` if this can exclusively access the element given by `index`. + pub fn has_write(&self, index: T) -> bool { + self.writes_all || self.writes.contains(index.sparse_set_index()) + } - /// Returns `true` if this has write access to all indexed elements (i.e. `EntityMut`). - pub fn has_write_all(&self) -> bool { - self.writes_all - } + /// Returns `true` if this accesses anything mutably. + pub fn has_any_write(&self) -> bool { + self.writes_all || !self.writes.is_clear() + } - /// Removes all accesses. - pub fn clear(&mut self) { - self.reads_all = false; - self.writes_all = false; - self.reads_and_writes.clear(); - self.writes.clear(); - } + /// Sets this as having access to all indexed elements (i.e. `&World`). + pub fn read_all(&mut self) { + self.reads_all = true; + } - /// Adds all access from `other`. - pub fn extend(&mut self, other: &Access) { - self.reads_all = self.reads_all || other.reads_all; - self.writes_all = self.writes_all || other.writes_all; - self.reads_and_writes.union_with(&other.reads_and_writes); - self.writes.union_with(&other.writes); - } + /// Sets this as having mutable access to all indexed elements (i.e. `EntityMut`). + pub fn write_all(&mut self) { + self.reads_all = true; + self.writes_all = true; + } - /// Returns `true` if the access and `other` can be active at the same time. - /// - /// [`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 { - if self.writes_all { - return !other.has_any_read(); + /// Returns `true` if this has access to all indexed elements (i.e. `&World`). + pub fn has_read_all(&self) -> bool { + self.reads_all } - if other.writes_all { - return !self.has_any_read(); + /// Returns `true` if this has write access to all indexed elements (i.e. `EntityMut`). + pub fn has_write_all(&self) -> bool { + self.writes_all } - if self.reads_all { - return !other.has_any_write(); + /// Removes all accesses. + pub fn clear(&mut self) { + self.reads_all = false; + self.writes_all = false; + self.reads_and_writes.clear(); + self.writes.clear(); } - if other.reads_all { - return !self.has_any_write(); + /// Adds all access from `other`. + pub fn extend(&mut self, other: &Access) { + self.reads_all = self.reads_all || other.reads_all; + self.writes_all = self.writes_all || other.writes_all; + self.reads_and_writes.union_with(&other.reads_and_writes); + self.writes.union_with(&other.writes); } - self.writes.is_disjoint(&other.reads_and_writes) - && other.writes.is_disjoint(&self.reads_and_writes) - } + /// Returns `true` if the access and `other` can be active at the same time. + /// + /// [`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 { + if self.writes_all { + return !other.has_any_read(); + } - /// 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 { - // QUESTION: How to handle `other.writes_all`? - conflicts.extend(other.writes.ones()); + if other.writes_all { + return !self.has_any_read(); + } + + if self.reads_all { + return !other.has_any_write(); + } + + if other.reads_all { + return !self.has_any_write(); + } + + self.writes.is_disjoint(&other.reads_and_writes) + && other.writes.is_disjoint(&self.reads_and_writes) } - if other.reads_all { - // QUESTION: How to handle `self.writes_all`. - conflicts.extend(self.writes.ones()); + /// [`Access`] is subset of `other` access. + pub fn is_subset(&self, other: &Access) -> bool { + if self.writes_all { + return other.writes_all; + } + + if other.writes_all { + return true; + } + + if self.reads_all { + return other.reads_all || other.writes_all; + } + + if other.reads_all { + return self.has_any_write(); + } + + let reads = self.reads_and_writes.difference(&self.writes).collect::(); + + reads.is_subset(&other.reads_and_writes) && self.writes.is_subset(&other.writes) } - if self.writes_all { - conflicts.extend(other.reads_and_writes.ones()); + /// 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 { + // QUESTION: How to handle `other.writes_all`? + conflicts.extend(other.writes.ones()); + } + + if other.reads_all { + // QUESTION: How to handle `self.writes_all`. + conflicts.extend(self.writes.ones()); + } + + if self.writes_all { + conflicts.extend(other.reads_and_writes.ones()); + } + + if other.writes_all { + conflicts.extend(self.reads_and_writes.ones()); + } + + conflicts.extend(self.writes.intersection(&other.reads_and_writes)); + conflicts.extend(self.reads_and_writes.intersection(&other.writes)); + conflicts + .ones() + .map(SparseSetIndex::get_sparse_set_index) + .collect() } - if other.writes_all { - conflicts.extend(self.reads_and_writes.ones()); + /// 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) } - conflicts.extend(self.writes.intersection(&other.reads_and_writes)); - conflicts.extend(self.reads_and_writes.intersection(&other.writes)); - conflicts - .ones() - .map(SparseSetIndex::get_sparse_set_index) - .collect() - } + /// 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 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 exclusive access to. + pub fn writes(&self) -> impl Iterator + '_ { + self.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) + /// An [`Access`] that has been filtered to include and exclude certain combinations of elements. + /// + /// Used internally to statically check if queries are disjoint. + /// + /// Subtle: a `read` or `write` in `access` should not be considered to imply a + /// `with` access. + /// + /// For example consider `Query>` this only has a `read` of `T` as doing + /// 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. + /// + /// In order to solve this the actual access that `Query<(&mut T, Option<&U>)>` has + /// is read/write `T`, read `U`. It must still have a read `U` access otherwise the following + /// queries would be incorrectly considered disjoint: + /// - `Query<&mut T>` read/write `T` + /// - `Query>` accesses nothing + /// + /// See comments the [`WorldQuery`](super::WorldQuery) impls of [`AnyOf`](super::AnyOf)/`Option`/[`Or`](super::Or) for more information. + #[derive(Debug, Clone, Eq, PartialEq)] + pub struct FilteredAccess { + access: Access, + // An array of filter sets to express `With` or `Without` clauses in disjunctive normal form, for example: `Or<(With, With)>`. + // Filters like `(With, Or<(With, Without)>` are expanded into `Or<((With, With), (With, Without))>`. + filter_sets: Vec>, + } + + impl Default for FilteredAccess { + fn default() -> Self { + Self { + access: Access::default(), + filter_sets: vec![AccessFilters::default()], + } + } } - /// 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) - } -} - -/// An [`Access`] that has been filtered to include and exclude certain combinations of elements. -/// -/// Used internally to statically check if queries are disjoint. -/// -/// Subtle: a `read` or `write` in `access` should not be considered to imply a -/// `with` access. -/// -/// For example consider `Query>` this only has a `read` of `T` as doing -/// 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. -/// -/// In order to solve this the actual access that `Query<(&mut T, Option<&U>)>` has -/// is read/write `T`, read `U`. It must still have a read `U` access otherwise the following -/// queries would be incorrectly considered disjoint: -/// - `Query<&mut T>` read/write `T` -/// - `Query>` accesses nothing -/// -/// See comments the [`WorldQuery`](super::WorldQuery) impls of [`AnyOf`](super::AnyOf)/`Option`/[`Or`](super::Or) for more information. -#[derive(Debug, Clone, Eq, PartialEq)] -pub struct FilteredAccess { - access: Access, - // An array of filter sets to express `With` or `Without` clauses in disjunctive normal form, for example: `Or<(With, With)>`. - // Filters like `(With, Or<(With, Without)>` are expanded into `Or<((With, With), (With, Without))>`. - filter_sets: Vec>, -} - -impl Default for FilteredAccess { - fn default() -> Self { - Self { - access: Access::default(), - filter_sets: vec![AccessFilters::default()], + impl From> for FilteredAccessSet { + fn from(filtered_access: FilteredAccess) -> Self { + let mut base = FilteredAccessSet::::default(); + base.add(filtered_access); + base } } -} -impl From> for FilteredAccessSet { - fn from(filtered_access: FilteredAccess) -> Self { - let mut base = FilteredAccessSet::::default(); - base.add(filtered_access); - base - } -} + impl FilteredAccess { + /// Returns a reference to the underlying unfiltered access. + #[inline] + pub fn access(&self) -> &Access { + &self.access + } -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 + } - /// 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.and_with(index); + } - /// Adds access to the element given by `index`. - pub fn add_read(&mut self, index: T) { - self.access.add_read(index.clone()); - self.and_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.and_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.and_with(index); - } + /// Adds a `With` filter: corresponds to a conjunction (AND) operation. + /// + /// Suppose we begin with `Or<(With, With)>`, which is represented by an array of two `AccessFilter` instances. + /// Adding `AND With` via this method transforms it into the equivalent of `Or<((With, With), (With, With))>`. + pub fn and_with(&mut self, index: T) { + let index = index.sparse_set_index(); + for filter in &mut self.filter_sets { + filter.with.grow(index + 1); + filter.with.insert(index); + } + } - /// Adds a `With` filter: corresponds to a conjunction (AND) operation. - /// - /// Suppose we begin with `Or<(With, With)>`, which is represented by an array of two `AccessFilter` instances. - /// Adding `AND With` via this method transforms it into the equivalent of `Or<((With, With), (With, With))>`. - pub fn and_with(&mut self, index: T) { - let index = index.sparse_set_index(); - for filter in &mut self.filter_sets { - filter.with.grow(index + 1); - filter.with.insert(index); + /// Adds a `Without` filter: corresponds to a conjunction (AND) operation. + /// + /// Suppose we begin with `Or<(With, With)>`, which is represented by an array of two `AccessFilter` instances. + /// Adding `AND Without` via this method transforms it into the equivalent of `Or<((With, Without), (With, Without))>`. + pub fn and_without(&mut self, index: T) { + let index = index.sparse_set_index(); + for filter in &mut self.filter_sets { + filter.without.grow(index + 1); + filter.without.insert(index); + } } - } - /// Adds a `Without` filter: corresponds to a conjunction (AND) operation. - /// - /// Suppose we begin with `Or<(With, With)>`, which is represented by an array of two `AccessFilter` instances. - /// Adding `AND Without` via this method transforms it into the equivalent of `Or<((With, Without), (With, Without))>`. - pub fn and_without(&mut self, index: T) { - let index = index.sparse_set_index(); - for filter in &mut self.filter_sets { - filter.without.grow(index + 1); - filter.without.insert(index); + /// Appends an array of filters: corresponds to a disjunction (OR) operation. + /// + /// As the underlying array of filters represents a disjunction, + /// where each element (`AccessFilters`) represents a conjunction, + /// we can simply append to the array. + pub fn append_or(&mut self, other: &FilteredAccess) { + self.filter_sets.append(&mut other.filter_sets.clone()); } - } - /// Appends an array of filters: corresponds to a disjunction (OR) operation. - /// - /// As the underlying array of filters represents a disjunction, - /// where each element (`AccessFilters`) represents a conjunction, - /// we can simply append to the array. - pub fn append_or(&mut self, other: &FilteredAccess) { - self.filter_sets.append(&mut other.filter_sets.clone()); - } + /// Adds all of the accesses from `other` to `self`. + pub fn extend_access(&mut self, other: &FilteredAccess) { + self.access.extend(&other.access); + } - /// Adds all of the accesses from `other` to `self`. - pub fn extend_access(&mut self, other: &FilteredAccess) { - self.access.extend(&other.access); - } + /// 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) { + return true; + } - /// 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) { - return true; - } - - // If the access instances are incompatible, we want to check that whether filters can - // guarantee that queries are disjoint. - // Since the `filter_sets` array represents a Disjunctive Normal Form formula ("ORs of ANDs"), - // we need to make sure that each filter set (ANDs) rule out every filter set from the `other` instance. - // - // For example, `Query<&mut C, Or<(With, Without)>>` is compatible `Query<&mut C, (With, Without)>`, - // but `Query<&mut C, Or<(Without, Without)>>` isn't compatible with `Query<&mut C, Or<(With, With)>>`. - self.filter_sets.iter().all(|filter| { - other - .filter_sets - .iter() - .all(|other_filter| filter.is_ruled_out_by(other_filter)) - }) - } + // If the access instances are incompatible, we want to check that whether filters can + // guarantee that queries are disjoint. + // Since the `filter_sets` array represents a Disjunctive Normal Form formula ("ORs of ANDs"), + // we need to make sure that each filter set (ANDs) rule out every filter set from the `other` instance. + // + // For example, `Query<&mut C, Or<(With, Without)>>` is compatible `Query<&mut C, (With, Without)>`, + // but `Query<&mut C, Or<(Without, Without)>>` isn't compatible with `Query<&mut C, Or<(With, With)>>`. + self.filter_sets.iter().all(|filter| { + other + .filter_sets + .iter() + .all(|other_filter| filter.is_ruled_out_by(other_filter)) + }) + } - /// 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); + /// access is compatible with other access. This does not take into account the filtered access. + pub fn is_subset(&self, other: &FilteredAccess) -> bool { + self.access.is_subset(&other.access) } - Vec::new() - } - /// Adds all access and filters from `other`. - /// - /// Corresponds to a conjunction operation (AND) for filters. - /// - /// Extending `Or<(With, Without)>` with `Or<(With, Without)>` will result in - /// `Or<((With, With), (With, Without), (Without, With), (Without, Without))>`. - pub fn extend(&mut self, other: &FilteredAccess) { - self.access.extend(&other.access); - - // We can avoid allocating a new array of bitsets if `other` contains just a single set of filters: - // in this case we can short-circuit by performing an in-place union for each bitset. - if other.filter_sets.len() == 1 { - for filter in &mut self.filter_sets { - filter.with.union_with(&other.filter_sets[0].with); - filter.without.union_with(&other.filter_sets[0].without); + /// 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); } - return; + Vec::new() } - let mut new_filters = Vec::with_capacity(self.filter_sets.len() * other.filter_sets.len()); - for filter in &self.filter_sets { - for other_filter in &other.filter_sets { - let mut new_filter = filter.clone(); - new_filter.with.union_with(&other_filter.with); - new_filter.without.union_with(&other_filter.without); - new_filters.push(new_filter); + /// Adds all access and filters from `other`. + /// + /// Corresponds to a conjunction operation (AND) for filters. + /// + /// Extending `Or<(With, Without)>` with `Or<(With, Without)>` will result in + /// `Or<((With, With), (With, Without), (Without, With), (Without, Without))>`. + pub fn extend(&mut self, other: &FilteredAccess) { + self.access.extend(&other.access); + + // We can avoid allocating a new array of bitsets if `other` contains just a single set of filters: + // in this case we can short-circuit by performing an in-place union for each bitset. + if other.filter_sets.len() == 1 { + for filter in &mut self.filter_sets { + filter.with.union_with(&other.filter_sets[0].with); + filter.without.union_with(&other.filter_sets[0].without); + } + return; + } + + let mut new_filters = Vec::with_capacity(self.filter_sets.len() * other.filter_sets.len()); + for filter in &self.filter_sets { + for other_filter in &other.filter_sets { + let mut new_filter = filter.clone(); + new_filter.with.union_with(&other_filter.with); + new_filter.without.union_with(&other_filter.without); + new_filters.push(new_filter); + } } + self.filter_sets = new_filters; } - self.filter_sets = new_filters; - } - /// Sets the underlying unfiltered access as having access to all indexed elements. - pub fn read_all(&mut self) { - self.access.read_all(); - } + /// Sets the underlying unfiltered access as having access to all indexed elements. + pub fn read_all(&mut self) { + self.access.read_all(); + } - /// Sets the underlying unfiltered access as having mutable access to all indexed elements. - pub fn write_all(&mut self) { - self.access.write_all(); + /// Sets the underlying unfiltered access as having mutable access to all indexed elements. + pub fn write_all(&mut self) { + self.access.write_all(); + } } -} - -#[derive(Clone, Eq, PartialEq)] -struct AccessFilters { - with: FixedBitSet, - without: FixedBitSet, - _index_type: PhantomData, -} - -impl fmt::Debug for AccessFilters { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("AccessFilters") - .field("with", &FormattedBitSet::::new(&self.with)) - .field("without", &FormattedBitSet::::new(&self.without)) - .finish() + + #[derive(Clone, Eq, PartialEq)] + struct AccessFilters { + with: FixedBitSet, + without: FixedBitSet, + _index_type: PhantomData, } -} -impl Default for AccessFilters { - fn default() -> Self { - Self { - with: FixedBitSet::default(), - without: FixedBitSet::default(), - _index_type: PhantomData, + impl fmt::Debug for AccessFilters { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("AccessFilters") + .field("with", &FormattedBitSet::::new(&self.with)) + .field("without", &FormattedBitSet::::new(&self.without)) + .finish() } } -} - -impl AccessFilters { - fn is_ruled_out_by(&self, other: &Self) -> bool { - // Although not technically complete, we don't consider the case when `AccessFilters`'s - // `without` bitset contradicts its own `with` bitset (e.g. `(With, Without)`). - // Such query would be considered compatible with any other query, but as it's almost - // always an error, we ignore this case instead of treating such query as compatible - // with others. - !self.with.is_disjoint(&other.without) || !self.without.is_disjoint(&other.with) + + impl Default for AccessFilters { + fn default() -> Self { + Self { + with: FixedBitSet::default(), + without: FixedBitSet::default(), + _index_type: PhantomData, + } + } } -} - -/// 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, - 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 + + impl AccessFilters { + fn is_ruled_out_by(&self, other: &Self) -> bool { + // Although not technically complete, we don't consider the case when `AccessFilters`'s + // `without` bitset contradicts its own `with` bitset (e.g. `(With, Without)`). + // Such query would be considered compatible with any other query, but as it's almost + // always an error, we ignore this case instead of treating such query as compatible + // with others. + !self.with.is_disjoint(&other.without) || !self.without.is_disjoint(&other.with) + } } - /// Returns `true` if this and `other` can be active at the same time. + /// A collection of [`FilteredAccess`] instances. /// - /// 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`](super::With) or [`Without`](super::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; - } - for filtered in &self.filtered_accesses { - for other_filtered in &other.filtered_accesses { - if !filtered.is_compatible(other_filtered) { - return false; - } - } + /// 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, + 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 } - 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()) { + /// 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`](super::With) or [`Without`](super::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; + } for filtered in &self.filtered_accesses { for other_filtered in &other.filtered_accesses { - conflicts.extend(filtered.get_conflicts(other_filtered).into_iter()); + if !filtered.is_compatible(other_filtered) { + return false; + } } } + true } - 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 { - conflicts.extend(filtered.get_conflicts(filtered_access).into_iter()); + /// 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 { + for other_filtered in &other.filtered_accesses { + conflicts.extend(filtered.get_conflicts(other_filtered).into_iter()); + } + } } + conflicts.into_iter().collect() } - 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); - } + /// 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 { + conflicts.extend(filtered.get_conflicts(filtered_access).into_iter()); + } + } + conflicts.into_iter().collect() + } - /// 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 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); + } - /// 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); - } + /// 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 all of the accesses from the passed set to `self`. - pub fn extend(&mut self, filtered_access_set: FilteredAccessSet) { - self.combined_access - .extend(&filtered_access_set.combined_access); - self.filtered_accesses - .extend(filtered_access_set.filtered_accesses); - } + /// 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); + } - /// Removes all accesses stored in this set. - pub fn clear(&mut self) { - self.combined_access.clear(); - self.filtered_accesses.clear(); - } -} + /// Adds all of the accesses from the passed set to `self`. + pub fn extend(&mut self, filtered_access_set: FilteredAccessSet) { + self.combined_access + .extend(&filtered_access_set.combined_access); + self.filtered_accesses + .extend(filtered_access_set.filtered_accesses); + } -impl Default for FilteredAccessSet { - fn default() -> Self { - Self { - combined_access: Default::default(), - filtered_accesses: Vec::new(), + /// Removes all accesses stored in this set. + pub fn clear(&mut self) { + self.combined_access.clear(); + self.filtered_accesses.clear(); } } -} -#[cfg(test)] -mod tests { - use crate::query::access::AccessFilters; - use crate::query::{Access, FilteredAccess, FilteredAccessSet}; - use fixedbitset::FixedBitSet; - use std::marker::PhantomData; + impl Default for FilteredAccessSet { + fn default() -> Self { + Self { + combined_access: Default::default(), + filtered_accesses: Vec::new(), + } + } + } - #[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); + #[cfg(test)] + mod tests { + use crate::query::access::AccessFilters; + use crate::query::{Access, FilteredAccess, FilteredAccessSet}; + use fixedbitset::FixedBitSet; + use std::marker::PhantomData; - let mut access_b = Access::::default(); - access_b.read_all(); + #[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); - assert!(!access_b.is_compatible(&access_a)); + let mut access_b = Access::::default(); + access_b.read_all(); - // read_all / read_all - let mut access_a = Access::::default(); - access_a.grow(10); - access_a.read_all(); + assert!(!access_b.is_compatible(&access_a)); - let mut access_b = Access::::default(); - access_b.read_all(); + // read_all / read_all + let mut access_a = Access::::default(); + access_a.grow(10); + access_a.read_all(); - assert!(access_b.is_compatible(&access_a)); - } + let mut access_b = Access::::default(); + access_b.read_all(); - #[test] - fn access_get_conflicts() { - let mut access_a = Access::::default(); - access_a.add_read(0); - access_a.add_read(1); + assert!(access_b.is_compatible(&access_a)); + } - let mut access_b = Access::::default(); - access_b.add_read(0); - access_b.add_write(1); + #[test] + fn access_get_conflicts() { + let mut access_a = Access::::default(); + access_a.add_read(0); + access_a.add_read(1); - assert_eq!(access_a.get_conflicts(&access_b), vec![1]); + let mut access_b = Access::::default(); + access_b.add_read(0); + access_b.add_write(1); - let mut access_c = Access::::default(); - access_c.add_write(0); - access_c.add_write(1); + assert_eq!(access_a.get_conflicts(&access_b), vec![1]); - assert_eq!(access_a.get_conflicts(&access_c), vec![0, 1]); - assert_eq!(access_b.get_conflicts(&access_c), vec![0, 1]); + let mut access_c = Access::::default(); + access_c.add_write(0); + access_c.add_write(1); - let mut access_d = Access::::default(); - access_d.add_read(0); + assert_eq!(access_a.get_conflicts(&access_c), vec![0, 1]); + assert_eq!(access_b.get_conflicts(&access_c), vec![0, 1]); - assert_eq!(access_d.get_conflicts(&access_a), vec![]); - assert_eq!(access_d.get_conflicts(&access_b), vec![]); - assert_eq!(access_d.get_conflicts(&access_c), vec![0]); - } + let mut access_d = Access::::default(); + access_d.add_read(0); - #[test] - fn filtered_combined_access() { - let mut access_a = FilteredAccessSet::::default(); - access_a.add_unfiltered_read(1); + assert_eq!(access_d.get_conflicts(&access_a), vec![]); + assert_eq!(access_d.get_conflicts(&access_b), vec![]); + assert_eq!(access_d.get_conflicts(&access_c), vec![0]); + } - let mut filter_b = FilteredAccess::::default(); - filter_b.add_write(1); + #[test] + fn filtered_combined_access() { + let mut access_a = FilteredAccessSet::::default(); + access_a.add_unfiltered_read(1); - let conflicts = access_a.get_conflicts_single(&filter_b); - assert_eq!( - &conflicts, - &[1_usize], - "access_a: {access_a:?}, filter_b: {filter_b:?}" - ); - } + let mut filter_b = FilteredAccess::::default(); + filter_b.add_write(1); - #[test] - fn filtered_access_extend() { - let mut access_a = FilteredAccess::::default(); - access_a.add_read(0); - access_a.add_read(1); - access_a.and_with(2); + let conflicts = access_a.get_conflicts_single(&filter_b); + assert_eq!( + &conflicts, + &[1_usize], + "access_a: {access_a:?}, filter_b: {filter_b:?}" + ); + } - let mut access_b = FilteredAccess::::default(); - access_b.add_read(0); - access_b.add_write(3); - access_b.and_without(4); + #[test] + fn filtered_access_extend() { + let mut access_a = FilteredAccess::::default(); + access_a.add_read(0); + access_a.add_read(1); + access_a.and_with(2); - access_a.extend(&access_b); + let mut access_b = FilteredAccess::::default(); + access_b.add_read(0); + access_b.add_write(3); + access_b.and_without(4); - let mut expected = FilteredAccess::::default(); - expected.add_read(0); - expected.add_read(1); - expected.and_with(2); - expected.add_write(3); - expected.and_without(4); + access_a.extend(&access_b); - assert!(access_a.eq(&expected)); - } + let mut expected = FilteredAccess::::default(); + expected.add_read(0); + expected.add_read(1); + expected.and_with(2); + expected.add_write(3); + expected.and_without(4); - #[test] - fn filtered_access_extend_or() { - let mut access_a = FilteredAccess::::default(); - // Exclusive access to `(&mut A, &mut B)`. - access_a.add_write(0); - access_a.add_write(1); - - // Filter by `With`. - let mut access_b = FilteredAccess::::default(); - access_b.and_with(2); - - // Filter by `(With, Without)`. - let mut access_c = FilteredAccess::::default(); - access_c.and_with(3); - access_c.and_without(4); - - // Turns `access_b` into `Or<(With, (With, Without))>`. - access_b.append_or(&access_c); - // Applies the filters to the initial query, which corresponds to the FilteredAccess' - // representation of `Query<(&mut A, &mut B), Or<(With, (With, Without))>>`. - access_a.extend(&access_b); - - // Construct the expected `FilteredAccess` struct. - // The intention here is to test that exclusive access implied by `add_write` - // forms correct normalized access structs when extended with `Or` filters. - let mut expected = FilteredAccess::::default(); - expected.add_write(0); - expected.add_write(1); - // The resulted access is expected to represent `Or<((With, With, With), (With, With, With, Without))>`. - expected.filter_sets = vec![ - AccessFilters { - with: FixedBitSet::with_capacity_and_blocks(3, [0b111]), - without: FixedBitSet::default(), - _index_type: PhantomData, - }, - AccessFilters { - with: FixedBitSet::with_capacity_and_blocks(4, [0b1011]), - without: FixedBitSet::with_capacity_and_blocks(5, [0b10000]), - _index_type: PhantomData, - }, - ]; + assert!(access_a.eq(&expected)); + } - assert_eq!(access_a, expected); + #[test] + fn filtered_access_extend_or() { + let mut access_a = FilteredAccess::::default(); + // Exclusive access to `(&mut A, &mut B)`. + access_a.add_write(0); + access_a.add_write(1); + + // Filter by `With`. + let mut access_b = FilteredAccess::::default(); + access_b.and_with(2); + + // Filter by `(With, Without)`. + let mut access_c = FilteredAccess::::default(); + access_c.and_with(3); + access_c.and_without(4); + + // Turns `access_b` into `Or<(With, (With, Without))>`. + access_b.append_or(&access_c); + // Applies the filters to the initial query, which corresponds to the FilteredAccess' + // representation of `Query<(&mut A, &mut B), Or<(With, (With, Without))>>`. + access_a.extend(&access_b); + + // Construct the expected `FilteredAccess` struct. + // The intention here is to test that exclusive access implied by `add_write` + // forms correct normalized access structs when extended with `Or` filters. + let mut expected = FilteredAccess::::default(); + expected.add_write(0); + expected.add_write(1); + // The resulted access is expected to represent `Or<((With, With, With), (With, With, With, Without))>`. + expected.filter_sets = vec![ + AccessFilters { + with: FixedBitSet::with_capacity_and_blocks(3, [0b111]), + without: FixedBitSet::default(), + _index_type: PhantomData, + }, + AccessFilters { + with: FixedBitSet::with_capacity_and_blocks(4, [0b1011]), + without: FixedBitSet::with_capacity_and_blocks(5, [0b10000]), + _index_type: PhantomData, + }, + ]; + + assert_eq!(access_a, expected); + } } -} diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 1f7fe29ec729f..ab5f6855a2be3 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1,7 +1,7 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId}, change_detection::{Ticks, TicksMut}, - component::{Component, ComponentId, ComponentStorage, StorageType, Tick}, + component::{Component, ComponentId, ComponentStorage, StorageType, Tick, Components}, entity::Entity, query::{Access, DebugCheckedUnwrap, FilteredAccess}, storage::{ComponentSparseSet, Table, TableRow}, @@ -440,6 +440,9 @@ pub unsafe trait WorldQuery { /// Creates and initializes a [`State`](WorldQuery::State) for this [`WorldQuery`] type. fn init_state(world: &mut World) -> Self::State; + /// Creates a [`State`](WorldQuery::State) for this [`WorldQuery`] type + fn new_state(components: &Components) -> Self::State; + /// Returns `true` if this query matches a set of components. Otherwise, returns `false`. fn matches_component_set( state: &Self::State, @@ -515,6 +518,8 @@ unsafe impl WorldQuery for Entity { fn init_state(_world: &mut World) {} + fn new_state(_components: &Components) {} + fn matches_component_set( _state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool, @@ -594,6 +599,7 @@ unsafe impl WorldQuery for EntityRef<'_> { } fn init_state(_world: &mut World) {} + fn new_state(_components: &Components) {} fn matches_component_set( _state: &Self::State, @@ -674,6 +680,7 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> { } fn init_state(_world: &mut World) {} + fn new_state(_components: &Components) {} fn matches_component_set( _state: &Self::State, @@ -815,6 +822,10 @@ unsafe impl WorldQuery for &T { world.init_component::() } + fn new_state(components: &Components) -> ComponentId { + components.component_id::().unwrap() + } + fn matches_component_set( &state: &ComponentId, set_contains_id: &impl Fn(ComponentId) -> bool, @@ -976,6 +987,10 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { world.init_component::() } + fn new_state(components: &Components) -> ComponentId { + components.component_id::().unwrap() + } + fn matches_component_set( &state: &ComponentId, set_contains_id: &impl Fn(ComponentId) -> bool, @@ -1137,6 +1152,10 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { world.init_component::() } + fn new_state(components: &Components) -> ComponentId { + components.component_id::().unwrap() + } + fn matches_component_set( &state: &ComponentId, set_contains_id: &impl Fn(ComponentId) -> bool, @@ -1249,6 +1268,10 @@ unsafe impl WorldQuery for Option { T::init_state(world) } + fn new_state(components: &Components) -> T::State { + T::new_state(components) + } + fn matches_component_set( _state: &T::State, _set_contains_id: &impl Fn(ComponentId) -> bool, @@ -1383,6 +1406,10 @@ unsafe impl WorldQuery for Has { fn init_state(world: &mut World) -> ComponentId { world.init_component::() } + + fn new_state(components: &Components) -> ComponentId { + components.component_id::().unwrap() + } fn matches_component_set( _state: &Self::State, @@ -1480,6 +1507,10 @@ macro_rules! impl_tuple_fetch { ($($name::init_state(_world),)*) } + fn new_state(_components: &Components) -> Self::State { + ($($name::new_state(_components),)*) + } + fn matches_component_set(state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { let ($($name,)*) = state; true $(&& $name::matches_component_set($name, _set_contains_id))* @@ -1603,6 +1634,10 @@ macro_rules! impl_anytuple_fetch { ($($name::init_state(_world),)*) } + fn new_state(_components: &Components) -> Self::State { + ($($name::new_state(_components),)*) + } + fn matches_component_set(_state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { let ($($name,)*) = _state; false $(|| $name::matches_component_set($name, _set_contains_id))* @@ -1677,6 +1712,10 @@ unsafe impl WorldQuery for NopWorldQuery { Q::init_state(world) } + fn new_state(components: &Components) -> Self::State { + Q::new_state(components) + } + fn matches_component_set( state: &Self::State, set_contains_id: &impl Fn(ComponentId) -> bool, @@ -1740,6 +1779,8 @@ unsafe impl WorldQuery for PhantomData { fn init_state(_world: &mut World) -> Self::State {} + fn new_state(_components: &Components) -> Self::State {} + fn matches_component_set( _state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool, diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 9db8fbf36c4fa..e283c36652b4d 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -1,6 +1,6 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId}, - component::{Component, ComponentId, ComponentStorage, StorageType, Tick}, + component::{Component, ComponentId, ComponentStorage, StorageType, Tick, Components}, entity::Entity, query::{Access, DebugCheckedUnwrap, FilteredAccess, WorldQuery}, storage::{Column, ComponentSparseSet, Table, TableRow}, @@ -105,6 +105,10 @@ unsafe impl WorldQuery for With { world.init_component::() } + fn new_state(components: &Components) -> Self::State { + components.component_id::().unwrap() + } + fn matches_component_set( &id: &ComponentId, set_contains_id: &impl Fn(ComponentId) -> bool, @@ -206,6 +210,10 @@ unsafe impl WorldQuery for Without { world.init_component::() } + fn new_state(components: &Components) -> Self::State { + components.component_id::().unwrap() + } + fn matches_component_set( &id: &ComponentId, set_contains_id: &impl Fn(ComponentId) -> bool, @@ -370,6 +378,10 @@ macro_rules! impl_query_filter_tuple { ($($filter::init_state(world),)*) } + fn new_state(components: &Components) -> Self::State { + ($($filter::new_state(components),)*) + } + fn matches_component_set(_state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { let ($($filter,)*) = _state; false $(|| $filter::matches_component_set($filter, _set_contains_id))* @@ -533,6 +545,10 @@ macro_rules! impl_tick_filter { world.init_component::() } + fn new_state(components: &Components) -> ComponentId { + components.component_id::().unwrap() + } + fn matches_component_set(&id: &ComponentId, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { set_contains_id(id) } diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index ee962498f1a1b..a862eebf951fb 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -138,6 +138,58 @@ impl QueryState { state } + /// Transform a query into a more generic query. If the original query state did + /// not have the same access this will panic. + /// Probably should not call update archetype generation on this query state as the results will + /// be very unpredictable as new archetypes could be added that don't match old query + pub fn generalize( + self, + world: UnsafeWorldCell, + ) -> QueryState { + if !Q::IS_ARCHETYPAL || !F::IS_ARCHETYPAL || !NewQ::IS_ARCHETYPAL { + panic!("generalizing is not allow with queries that use `Changed` or `Added`"); + } + // SAFETY: we are using unsafe world to access &Components which should always be safe + let fetch_state = unsafe { NewQ::new_state(world.world().components()) }; + #[allow(clippy::let_unit_value)] + // the archetypal filters have already been applied, so we don't need them. + // SAFETY: we are using unsafe world to access &Components which should always be safe + let filter_state = unsafe { <()>::new_state(world.world().components()) }; + + let mut component_access = FilteredAccess::default(); + NewQ::update_component_access(&fetch_state, &mut component_access); + + let mut filter_component_access = FilteredAccess::default(); + <()>::update_component_access(&filter_state, &mut filter_component_access); + + // Merge the temporary filter access with the main access. This ensures that filter access is + // properly considered in a global "cross-query" context (both within systems and across systems). + component_access.extend(&filter_component_access); + + if !component_access.is_subset(&self.component_access) { + panic!("access is not compatible with new query type"); + } + + QueryState { + world_id: self.world_id, + archetype_generation: self.archetype_generation, + matched_table_ids: self.matched_table_ids, + matched_archetype_ids: self.matched_archetype_ids, + fetch_state, + filter_state, + component_access, + matched_tables: Default::default(), + matched_archetypes: Default::default(), + archetype_component_access: Default::default(), + #[cfg(feature = "trace")] + par_iter_span: bevy_utils::tracing::info_span!( + "par_for_each", + query = std::any::type_name::(), + filter = std::any::type_name::(), + ), + } + } + /// Checks if the query is empty for the given [`World`], where the last change and current tick are given. /// /// # Panics @@ -1424,6 +1476,7 @@ impl QueryState { #[cfg(test)] mod tests { + use crate as bevy_ecs; use crate::{prelude::*, query::QueryEntityError}; #[test] @@ -1529,4 +1582,82 @@ mod tests { let mut query_state = world_1.query::(); let _panics = query_state.get_many_mut(&mut world_2, []); } + + #[test] + fn generalize_query_state() { + #[derive(Component)] + struct A(pub usize); + + #[derive(Component)] + struct B; + + let mut world = World::new(); + world.spawn((A(22), B)); + + let query_state = world.query::<(&A, &B)>(); + let mut new_query_state = query_state.generalize::<&A>(world.as_unsafe_world_cell()); + assert_eq!(new_query_state.iter(&world).len(), 1); + let a = new_query_state.single(&world); + + assert_eq!(a.0, 22); + } + + #[test] + fn generalize_query_cannot_get_unmatched_data() { + #[derive(Component)] + struct A(pub usize); + + #[derive(Component)] + struct B; + + #[derive(Component)] + struct C; + + let mut world = World::new(); + world.spawn((A(22), B)); + world.spawn((A(23), B, C)); + + let query_state = world.query_filtered::<(&A, &B), Without>(); + let mut new_query_state = query_state.generalize::<&A>(world.as_unsafe_world_cell()); + // even though we change the query to not have Without, we cannot get the component with C. + let a = new_query_state.single(&world); + + assert_eq!(a.0, 22); + } + + #[test] + #[should_panic] + fn generalize_to_no_included_components_not_allowed() { + #[derive(Component)] + struct A(pub usize); + + #[derive(Component)] + struct B; + + let mut world = World::new(); + world.init_component::(); + world.init_component::(); + world.spawn(A(22)); + + let query_state = world.query::<&A>(); + let mut _new_query_state = query_state.generalize::<(&A, &B)>(world.as_unsafe_world_cell()); + } + + #[test] + #[should_panic] + fn generalize_non_archtypal_not_allowed() { + #[derive(Component)] + struct A(pub usize); + + #[derive(Component)] + struct B; + + let mut world = World::new(); + world.init_component::(); + world.init_component::(); + world.spawn(A(22)); + + let query_state = world.query_filtered::<(&A, &B), Added>(); + let mut _new_query_state = query_state.generalize::<&A>(world.as_unsafe_world_cell()); + } } From 75be6b21144d6448439bc37ffdb00afdcd137033 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Fri, 6 Oct 2023 14:12:35 -0700 Subject: [PATCH 02/27] add generalize method to query --- crates/bevy_ecs/src/query/mod.rs | 25 +++++++++++++++++++ crates/bevy_ecs/src/query/state.rs | 9 +++---- crates/bevy_ecs/src/system/query.rs | 38 +++++++++++++++++++++++++++-- 3 files changed, 64 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index c30292cb76a2d..0571a4898cc86 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -801,4 +801,29 @@ mod tests { let values = world.query::<&B>().iter(&world).collect::>(); assert_eq!(values, vec![&B(2)]); } + + #[test] + fn generalize() { + let mut world = World::new(); + + world.spawn((A(10), B(5))); + + fn function_that_takes_a_query(query: &Query<&A>) { + assert_eq!(query.single().0, 10); + } + + fn system_1(query: Query<&A>) { + function_that_takes_a_query(&query); + } + + fn system_2(query: Query<(&A, &B)>) { + let gen_q = query.generalize::<&A>(); + let q = gen_q.query(); + function_that_takes_a_query(&q); + } + + let mut schedule = Schedule::default(); + schedule.add_systems((system_1, system_2)); + schedule.run(&mut world); + } } diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index a862eebf951fb..98ea83855c1d5 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -142,10 +142,7 @@ impl QueryState { /// not have the same access this will panic. /// Probably should not call update archetype generation on this query state as the results will /// be very unpredictable as new archetypes could be added that don't match old query - pub fn generalize( - self, - world: UnsafeWorldCell, - ) -> QueryState { + pub fn generalize(&self, world: UnsafeWorldCell) -> QueryState { if !Q::IS_ARCHETYPAL || !F::IS_ARCHETYPAL || !NewQ::IS_ARCHETYPAL { panic!("generalizing is not allow with queries that use `Changed` or `Added`"); } @@ -173,8 +170,8 @@ impl QueryState { QueryState { world_id: self.world_id, archetype_generation: self.archetype_generation, - matched_table_ids: self.matched_table_ids, - matched_archetype_ids: self.matched_archetype_ids, + matched_table_ids: self.matched_table_ids.clone(), + matched_archetype_ids: self.matched_archetype_ids.clone(), fetch_state, filter_state, component_access, diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 36f822232a80d..f90288bfc6756 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -320,11 +320,11 @@ use std::{any::TypeId, borrow::Borrow}; /// [`single_mut`]: Self::single_mut /// [`SparseSet`]: crate::storage::SparseSet /// [System parameter]: crate::system::SystemParam -/// [`Table`]: crate::storage::Table /// [`With`]: crate::query::With +/// [`Table`]: crate::storage::Table /// [`Without`]: crate::query::Without +// SAFETY: Must have access to the components registered in `state`. pub struct Query<'world, 'state, Q: WorldQuery, F: ReadOnlyWorldQuery = ()> { - // SAFETY: Must have access to the components registered in `state`. world: UnsafeWorldCell<'world>, state: &'state QueryState, last_run: Tick, @@ -400,6 +400,19 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } } + /// Returns another `Query` that returns a subset of the data that the original query does. This can be + /// useful for passing the query to another function. + pub fn generalize(self) -> GeneralizedQuery<'w, NewQ> { + let new_state = self.state.generalize(self.world); + + GeneralizedQuery { + state: new_state, + world: self.world, + last_run: self.last_run, + this_run: self.this_run, + } + } + /// Returns an [`Iterator`] over the read-only query items. /// /// # Example @@ -1519,3 +1532,24 @@ impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } } } + +/// A type used to store the generalized query's state. +pub struct GeneralizedQuery<'world, NewQ: WorldQuery> { + world: UnsafeWorldCell<'world>, + state: QueryState, + last_run: Tick, + this_run: Tick, +} + +impl<'world, NewQ: WorldQuery> GeneralizedQuery<'world, NewQ> { + /// get the query associated with Generalized Query + pub fn query(&self) -> Query<'world, '_, NewQ, ()> { + Query { + world: self.world, + state: &self.state, + last_run: self.last_run, + this_run: self.this_run, + force_read_only_component_access: false, + } + } +} From 6fcf755173272da80214314ce5d14b7eb34ae031 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Fri, 6 Oct 2023 14:38:41 -0700 Subject: [PATCH 03/27] cargo fmt --- crates/bevy_ecs/src/query/access.rs | 1253 ++++++++++++++------------- crates/bevy_ecs/src/query/fetch.rs | 4 +- crates/bevy_ecs/src/query/filter.rs | 2 +- 3 files changed, 631 insertions(+), 628 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index b55e6efb48a1d..70a77f7759508 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -1,750 +1,753 @@ - 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, - } +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(Clone, Eq, PartialEq)] +pub struct Access { + /// 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, + /// Is `true` if this has mutable access to all elements in the collection. + /// If this is true, then `reads_all` must also be true. + writes_all: bool, + 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) + .field("writes_all", &self.writes_all) + .finish() + } +} - 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() +impl Default for Access { + fn default() -> Self { + Self::new() + } +} + +impl Access { + /// Creates an empty [`Access`] collection. + pub const fn new() -> Self { + Self { + reads_all: false, + writes_all: false, + reads_and_writes: FixedBitSet::new(), + writes: FixedBitSet::new(), + marker: PhantomData, } } - /// Tracks read and write access to specific elements in a collection. + /// Increases the set capacity to the specified amount. /// - /// 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(Clone, Eq, PartialEq)] - pub struct Access { - /// 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, - /// Is `true` if this has mutable access to all elements in the collection. - /// If this is true, then `reads_all` must also be true. - writes_all: bool, - 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) - .field("writes_all", &self.writes_all) - .finish() - } + /// 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); } - impl Default for Access { - fn default() -> Self { - Self::new() - } + /// 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()); } - impl Access { - /// Creates an empty [`Access`] collection. - pub const fn new() -> Self { - Self { - reads_all: false, - writes_all: false, - reads_and_writes: FixedBitSet::new(), - writes: FixedBitSet::new(), - marker: PhantomData, - } - } + /// 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.reads_and_writes.insert(index.sparse_set_index()); + self.writes.grow(index.sparse_set_index() + 1); + self.writes.insert(index.sparse_set_index()); + } - /// 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); - } + /// Returns `true` if this can access the element given by `index`. + pub fn has_read(&self, index: T) -> bool { + self.reads_all || self.reads_and_writes.contains(index.sparse_set_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()); - } + /// Returns `true` if this can access anything. + pub fn has_any_read(&self) -> bool { + self.reads_all || !self.reads_and_writes.is_clear() + } - /// 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.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 can exclusively access the element given by `index`. + pub fn has_write(&self, index: T) -> bool { + self.writes_all || self.writes.contains(index.sparse_set_index()) + } - /// Returns `true` if this can access the element given by `index`. - pub fn has_read(&self, index: T) -> bool { - self.reads_all || self.reads_and_writes.contains(index.sparse_set_index()) - } + /// Returns `true` if this accesses anything mutably. + pub fn has_any_write(&self) -> bool { + self.writes_all || !self.writes.is_clear() + } - /// Returns `true` if this can access anything. - pub fn has_any_read(&self) -> bool { - self.reads_all || !self.reads_and_writes.is_clear() - } + /// 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 can exclusively access the element given by `index`. - pub fn has_write(&self, index: T) -> bool { - self.writes_all || self.writes.contains(index.sparse_set_index()) - } + /// Sets this as having mutable access to all indexed elements (i.e. `EntityMut`). + pub fn write_all(&mut self) { + self.reads_all = true; + self.writes_all = true; + } - /// Returns `true` if this accesses anything mutably. - pub fn has_any_write(&self) -> bool { - self.writes_all || !self.writes.is_clear() - } + /// Returns `true` if this has access to all indexed elements (i.e. `&World`). + pub fn has_read_all(&self) -> bool { + self.reads_all + } - /// 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 has write access to all indexed elements (i.e. `EntityMut`). + pub fn has_write_all(&self) -> bool { + self.writes_all + } - /// Sets this as having mutable access to all indexed elements (i.e. `EntityMut`). - pub fn write_all(&mut self) { - self.reads_all = true; - self.writes_all = true; - } + /// Removes all accesses. + pub fn clear(&mut self) { + self.reads_all = false; + self.writes_all = false; + self.reads_and_writes.clear(); + self.writes.clear(); + } - /// Returns `true` if this has access to all indexed elements (i.e. `&World`). - pub fn has_read_all(&self) -> bool { - self.reads_all - } + /// Adds all access from `other`. + pub fn extend(&mut self, other: &Access) { + self.reads_all = self.reads_all || other.reads_all; + self.writes_all = self.writes_all || other.writes_all; + self.reads_and_writes.union_with(&other.reads_and_writes); + self.writes.union_with(&other.writes); + } - /// Returns `true` if this has write access to all indexed elements (i.e. `EntityMut`). - pub fn has_write_all(&self) -> bool { - self.writes_all + /// Returns `true` if the access and `other` can be active at the same time. + /// + /// [`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 { + if self.writes_all { + return !other.has_any_read(); } - /// Removes all accesses. - pub fn clear(&mut self) { - self.reads_all = false; - self.writes_all = false; - self.reads_and_writes.clear(); - self.writes.clear(); + if other.writes_all { + return !self.has_any_read(); } - /// Adds all access from `other`. - pub fn extend(&mut self, other: &Access) { - self.reads_all = self.reads_all || other.reads_all; - self.writes_all = self.writes_all || other.writes_all; - self.reads_and_writes.union_with(&other.reads_and_writes); - self.writes.union_with(&other.writes); + if self.reads_all { + return !other.has_any_write(); } - /// Returns `true` if the access and `other` can be active at the same time. - /// - /// [`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 { - if self.writes_all { - return !other.has_any_read(); - } - - if other.writes_all { - return !self.has_any_read(); - } - - if self.reads_all { - return !other.has_any_write(); - } - - if other.reads_all { - return !self.has_any_write(); - } - - self.writes.is_disjoint(&other.reads_and_writes) - && other.writes.is_disjoint(&self.reads_and_writes) + if other.reads_all { + return !self.has_any_write(); } - /// [`Access`] is subset of `other` access. - pub fn is_subset(&self, other: &Access) -> bool { - if self.writes_all { - return other.writes_all; - } - - if other.writes_all { - return true; - } - - if self.reads_all { - return other.reads_all || other.writes_all; - } - - if other.reads_all { - return self.has_any_write(); - } + self.writes.is_disjoint(&other.reads_and_writes) + && other.writes.is_disjoint(&self.reads_and_writes) + } - let reads = self.reads_and_writes.difference(&self.writes).collect::(); + /// [`Access`] is subset of `other` access. + pub fn is_subset(&self, other: &Access) -> bool { + if self.writes_all { + return other.writes_all; + } - reads.is_subset(&other.reads_and_writes) && self.writes.is_subset(&other.writes) + if other.writes_all { + return true; } - /// 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 { - // QUESTION: How to handle `other.writes_all`? - conflicts.extend(other.writes.ones()); - } + if self.reads_all { + return other.reads_all || other.writes_all; + } - if other.reads_all { - // QUESTION: How to handle `self.writes_all`. - conflicts.extend(self.writes.ones()); - } + if other.reads_all { + return self.has_any_write(); + } - if self.writes_all { - conflicts.extend(other.reads_and_writes.ones()); - } + let reads = self + .reads_and_writes + .difference(&self.writes) + .collect::(); - if other.writes_all { - conflicts.extend(self.reads_and_writes.ones()); - } + reads.is_subset(&other.reads_and_writes) && self.writes.is_subset(&other.writes) + } - conflicts.extend(self.writes.intersection(&other.reads_and_writes)); - conflicts.extend(self.reads_and_writes.intersection(&other.writes)); - conflicts - .ones() - .map(SparseSetIndex::get_sparse_set_index) - .collect() + /// 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 { + // QUESTION: How to handle `other.writes_all`? + conflicts.extend(other.writes.ones()); } - /// 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) + if other.reads_all { + // QUESTION: How to handle `self.writes_all`. + conflicts.extend(self.writes.ones()); } - /// 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) + if self.writes_all { + conflicts.extend(other.reads_and_writes.ones()); } - /// 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) + if other.writes_all { + conflicts.extend(self.reads_and_writes.ones()); } + + conflicts.extend(self.writes.intersection(&other.reads_and_writes)); + conflicts.extend(self.reads_and_writes.intersection(&other.writes)); + conflicts + .ones() + .map(SparseSetIndex::get_sparse_set_index) + .collect() } - /// An [`Access`] that has been filtered to include and exclude certain combinations of elements. - /// - /// Used internally to statically check if queries are disjoint. - /// - /// Subtle: a `read` or `write` in `access` should not be considered to imply a - /// `with` access. - /// - /// For example consider `Query>` this only has a `read` of `T` as doing - /// 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. - /// - /// In order to solve this the actual access that `Query<(&mut T, Option<&U>)>` has - /// is read/write `T`, read `U`. It must still have a read `U` access otherwise the following - /// queries would be incorrectly considered disjoint: - /// - `Query<&mut T>` read/write `T` - /// - `Query>` accesses nothing - /// - /// See comments the [`WorldQuery`](super::WorldQuery) impls of [`AnyOf`](super::AnyOf)/`Option`/[`Or`](super::Or) for more information. - #[derive(Debug, Clone, Eq, PartialEq)] - pub struct FilteredAccess { - access: Access, - // An array of filter sets to express `With` or `Without` clauses in disjunctive normal form, for example: `Or<(With, With)>`. - // Filters like `(With, Or<(With, Without)>` are expanded into `Or<((With, With), (With, Without))>`. - filter_sets: Vec>, - } - - impl Default for FilteredAccess { - fn default() -> Self { - Self { - access: Access::default(), - filter_sets: vec![AccessFilters::default()], - } - } + /// 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) } - impl From> for FilteredAccessSet { - fn from(filtered_access: FilteredAccess) -> Self { - let mut base = FilteredAccessSet::::default(); - base.add(filtered_access); - base - } + /// 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) } - impl FilteredAccess { - /// Returns a reference to the underlying unfiltered access. - #[inline] - pub fn access(&self) -> &Access { - &self.access + /// 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) + } +} + +/// An [`Access`] that has been filtered to include and exclude certain combinations of elements. +/// +/// Used internally to statically check if queries are disjoint. +/// +/// Subtle: a `read` or `write` in `access` should not be considered to imply a +/// `with` access. +/// +/// For example consider `Query>` this only has a `read` of `T` as doing +/// 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. +/// +/// In order to solve this the actual access that `Query<(&mut T, Option<&U>)>` has +/// is read/write `T`, read `U`. It must still have a read `U` access otherwise the following +/// queries would be incorrectly considered disjoint: +/// - `Query<&mut T>` read/write `T` +/// - `Query>` accesses nothing +/// +/// See comments the [`WorldQuery`](super::WorldQuery) impls of [`AnyOf`](super::AnyOf)/`Option`/[`Or`](super::Or) for more information. +#[derive(Debug, Clone, Eq, PartialEq)] +pub struct FilteredAccess { + access: Access, + // An array of filter sets to express `With` or `Without` clauses in disjunctive normal form, for example: `Or<(With, With)>`. + // Filters like `(With, Or<(With, Without)>` are expanded into `Or<((With, With), (With, Without))>`. + filter_sets: Vec>, +} + +impl Default for FilteredAccess { + fn default() -> Self { + Self { + access: Access::default(), + filter_sets: vec![AccessFilters::default()], } + } +} - /// Returns a mutable reference to the underlying unfiltered access. - #[inline] - pub fn access_mut(&mut self) -> &mut Access { - &mut self.access - } +impl From> for FilteredAccessSet { + fn from(filtered_access: FilteredAccess) -> Self { + let mut base = FilteredAccessSet::::default(); + base.add(filtered_access); + base + } +} - /// Adds access to the element given by `index`. - pub fn add_read(&mut self, index: T) { - self.access.add_read(index.clone()); - self.and_with(index); - } +impl FilteredAccess { + /// Returns a reference to the underlying unfiltered access. + #[inline] + pub fn access(&self) -> &Access { + &self.access + } - /// Adds exclusive access to the element given by `index`. - pub fn add_write(&mut self, index: T) { - self.access.add_write(index.clone()); - self.and_with(index); - } + /// Returns a mutable reference to the underlying unfiltered access. + #[inline] + pub fn access_mut(&mut self) -> &mut Access { + &mut self.access + } - /// Adds a `With` filter: corresponds to a conjunction (AND) operation. - /// - /// Suppose we begin with `Or<(With, With)>`, which is represented by an array of two `AccessFilter` instances. - /// Adding `AND With` via this method transforms it into the equivalent of `Or<((With, With), (With, With))>`. - pub fn and_with(&mut self, index: T) { - let index = index.sparse_set_index(); - for filter in &mut self.filter_sets { - filter.with.grow(index + 1); - filter.with.insert(index); - } - } + /// Adds access to the element given by `index`. + pub fn add_read(&mut self, index: T) { + self.access.add_read(index.clone()); + self.and_with(index); + } - /// Adds a `Without` filter: corresponds to a conjunction (AND) operation. - /// - /// Suppose we begin with `Or<(With, With)>`, which is represented by an array of two `AccessFilter` instances. - /// Adding `AND Without` via this method transforms it into the equivalent of `Or<((With, Without), (With, Without))>`. - pub fn and_without(&mut self, index: T) { - let index = index.sparse_set_index(); - for filter in &mut self.filter_sets { - filter.without.grow(index + 1); - filter.without.insert(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.and_with(index); + } - /// Appends an array of filters: corresponds to a disjunction (OR) operation. - /// - /// As the underlying array of filters represents a disjunction, - /// where each element (`AccessFilters`) represents a conjunction, - /// we can simply append to the array. - pub fn append_or(&mut self, other: &FilteredAccess) { - self.filter_sets.append(&mut other.filter_sets.clone()); + /// Adds a `With` filter: corresponds to a conjunction (AND) operation. + /// + /// Suppose we begin with `Or<(With, With)>`, which is represented by an array of two `AccessFilter` instances. + /// Adding `AND With` via this method transforms it into the equivalent of `Or<((With, With), (With, With))>`. + pub fn and_with(&mut self, index: T) { + let index = index.sparse_set_index(); + for filter in &mut self.filter_sets { + filter.with.grow(index + 1); + filter.with.insert(index); } + } - /// Adds all of the accesses from `other` to `self`. - pub fn extend_access(&mut self, other: &FilteredAccess) { - self.access.extend(&other.access); + /// Adds a `Without` filter: corresponds to a conjunction (AND) operation. + /// + /// Suppose we begin with `Or<(With, With)>`, which is represented by an array of two `AccessFilter` instances. + /// Adding `AND Without` via this method transforms it into the equivalent of `Or<((With, Without), (With, Without))>`. + pub fn and_without(&mut self, index: T) { + let index = index.sparse_set_index(); + for filter in &mut self.filter_sets { + filter.without.grow(index + 1); + filter.without.insert(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) { - return true; - } - - // If the access instances are incompatible, we want to check that whether filters can - // guarantee that queries are disjoint. - // Since the `filter_sets` array represents a Disjunctive Normal Form formula ("ORs of ANDs"), - // we need to make sure that each filter set (ANDs) rule out every filter set from the `other` instance. - // - // For example, `Query<&mut C, Or<(With, Without)>>` is compatible `Query<&mut C, (With, Without)>`, - // but `Query<&mut C, Or<(Without, Without)>>` isn't compatible with `Query<&mut C, Or<(With, With)>>`. - self.filter_sets.iter().all(|filter| { - other - .filter_sets - .iter() - .all(|other_filter| filter.is_ruled_out_by(other_filter)) - }) - } + /// Appends an array of filters: corresponds to a disjunction (OR) operation. + /// + /// As the underlying array of filters represents a disjunction, + /// where each element (`AccessFilters`) represents a conjunction, + /// we can simply append to the array. + pub fn append_or(&mut self, other: &FilteredAccess) { + self.filter_sets.append(&mut other.filter_sets.clone()); + } - /// access is compatible with other access. This does not take into account the filtered access. - pub fn is_subset(&self, other: &FilteredAccess) -> bool { - self.access.is_subset(&other.access) - } + /// Adds all of the accesses from `other` to `self`. + pub fn extend_access(&mut self, other: &FilteredAccess) { + self.access.extend(&other.access); + } - /// 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() - } + /// 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) { + return true; + } + + // If the access instances are incompatible, we want to check that whether filters can + // guarantee that queries are disjoint. + // Since the `filter_sets` array represents a Disjunctive Normal Form formula ("ORs of ANDs"), + // we need to make sure that each filter set (ANDs) rule out every filter set from the `other` instance. + // + // For example, `Query<&mut C, Or<(With, Without)>>` is compatible `Query<&mut C, (With, Without)>`, + // but `Query<&mut C, Or<(Without, Without)>>` isn't compatible with `Query<&mut C, Or<(With, With)>>`. + self.filter_sets.iter().all(|filter| { + other + .filter_sets + .iter() + .all(|other_filter| filter.is_ruled_out_by(other_filter)) + }) + } - /// Adds all access and filters from `other`. - /// - /// Corresponds to a conjunction operation (AND) for filters. - /// - /// Extending `Or<(With, Without)>` with `Or<(With, Without)>` will result in - /// `Or<((With, With), (With, Without), (Without, With), (Without, Without))>`. - pub fn extend(&mut self, other: &FilteredAccess) { - self.access.extend(&other.access); - - // We can avoid allocating a new array of bitsets if `other` contains just a single set of filters: - // in this case we can short-circuit by performing an in-place union for each bitset. - if other.filter_sets.len() == 1 { - for filter in &mut self.filter_sets { - filter.with.union_with(&other.filter_sets[0].with); - filter.without.union_with(&other.filter_sets[0].without); - } - return; - } + /// access is compatible with other access. This does not take into account the filtered access. + pub fn is_subset(&self, other: &FilteredAccess) -> bool { + self.access.is_subset(&other.access) + } - let mut new_filters = Vec::with_capacity(self.filter_sets.len() * other.filter_sets.len()); - for filter in &self.filter_sets { - for other_filter in &other.filter_sets { - let mut new_filter = filter.clone(); - new_filter.with.union_with(&other_filter.with); - new_filter.without.union_with(&other_filter.without); - new_filters.push(new_filter); - } - } - self.filter_sets = new_filters; + /// 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() + } - /// Sets the underlying unfiltered access as having access to all indexed elements. - pub fn read_all(&mut self) { - self.access.read_all(); + /// Adds all access and filters from `other`. + /// + /// Corresponds to a conjunction operation (AND) for filters. + /// + /// Extending `Or<(With, Without)>` with `Or<(With, Without)>` will result in + /// `Or<((With, With), (With, Without), (Without, With), (Without, Without))>`. + pub fn extend(&mut self, other: &FilteredAccess) { + self.access.extend(&other.access); + + // We can avoid allocating a new array of bitsets if `other` contains just a single set of filters: + // in this case we can short-circuit by performing an in-place union for each bitset. + if other.filter_sets.len() == 1 { + for filter in &mut self.filter_sets { + filter.with.union_with(&other.filter_sets[0].with); + filter.without.union_with(&other.filter_sets[0].without); + } + return; } - /// Sets the underlying unfiltered access as having mutable access to all indexed elements. - pub fn write_all(&mut self) { - self.access.write_all(); + let mut new_filters = Vec::with_capacity(self.filter_sets.len() * other.filter_sets.len()); + for filter in &self.filter_sets { + for other_filter in &other.filter_sets { + let mut new_filter = filter.clone(); + new_filter.with.union_with(&other_filter.with); + new_filter.without.union_with(&other_filter.without); + new_filters.push(new_filter); + } } + self.filter_sets = new_filters; } - #[derive(Clone, Eq, PartialEq)] - struct AccessFilters { - with: FixedBitSet, - without: FixedBitSet, - _index_type: PhantomData, + /// Sets the underlying unfiltered access as having access to all indexed elements. + pub fn read_all(&mut self) { + self.access.read_all(); } - impl fmt::Debug for AccessFilters { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("AccessFilters") - .field("with", &FormattedBitSet::::new(&self.with)) - .field("without", &FormattedBitSet::::new(&self.without)) - .finish() - } + /// Sets the underlying unfiltered access as having mutable access to all indexed elements. + pub fn write_all(&mut self) { + self.access.write_all(); } - - impl Default for AccessFilters { - fn default() -> Self { - Self { - with: FixedBitSet::default(), - without: FixedBitSet::default(), - _index_type: PhantomData, - } - } +} + +#[derive(Clone, Eq, PartialEq)] +struct AccessFilters { + with: FixedBitSet, + without: FixedBitSet, + _index_type: PhantomData, +} + +impl fmt::Debug for AccessFilters { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("AccessFilters") + .field("with", &FormattedBitSet::::new(&self.with)) + .field("without", &FormattedBitSet::::new(&self.without)) + .finish() } +} - impl AccessFilters { - fn is_ruled_out_by(&self, other: &Self) -> bool { - // Although not technically complete, we don't consider the case when `AccessFilters`'s - // `without` bitset contradicts its own `with` bitset (e.g. `(With, Without)`). - // Such query would be considered compatible with any other query, but as it's almost - // always an error, we ignore this case instead of treating such query as compatible - // with others. - !self.with.is_disjoint(&other.without) || !self.without.is_disjoint(&other.with) +impl Default for AccessFilters { + fn default() -> Self { + Self { + with: FixedBitSet::default(), + without: FixedBitSet::default(), + _index_type: PhantomData, } } +} + +impl AccessFilters { + fn is_ruled_out_by(&self, other: &Self) -> bool { + // Although not technically complete, we don't consider the case when `AccessFilters`'s + // `without` bitset contradicts its own `with` bitset (e.g. `(With, Without)`). + // Such query would be considered compatible with any other query, but as it's almost + // always an error, we ignore this case instead of treating such query as compatible + // with others. + !self.with.is_disjoint(&other.without) || !self.without.is_disjoint(&other.with) + } +} + +/// 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, + 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 + } - /// A collection of [`FilteredAccess`] instances. - /// - /// Used internally to statically check if systems have conflicting access. + /// Returns `true` if this and `other` can be active at the same time. /// - /// 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, - 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 `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`](super::With) or [`Without`](super::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; - } - for filtered in &self.filtered_accesses { - for other_filtered in &other.filtered_accesses { - if !filtered.is_compatible(other_filtered) { - return false; - } + /// 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`](super::With) or [`Without`](super::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; + } + for filtered in &self.filtered_accesses { + for other_filtered in &other.filtered_accesses { + if !filtered.is_compatible(other_filtered) { + return false; } } - true } + 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 { - for other_filtered in &other.filtered_accesses { - conflicts.extend(filtered.get_conflicts(other_filtered).into_iter()); - } + /// 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 { + for other_filtered in &other.filtered_accesses { + conflicts.extend(filtered.get_conflicts(other_filtered).into_iter()); } } - conflicts.into_iter().collect() } + 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 { - conflicts.extend(filtered.get_conflicts(filtered_access).into_iter()); - } + /// 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 { + conflicts.extend(filtered.get_conflicts(filtered_access).into_iter()); } - conflicts.into_iter().collect() } + 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); - } + /// 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); + } - /// 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 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); - } + /// 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); + } - /// Adds all of the accesses from the passed set to `self`. - pub fn extend(&mut self, filtered_access_set: FilteredAccessSet) { - self.combined_access - .extend(&filtered_access_set.combined_access); - self.filtered_accesses - .extend(filtered_access_set.filtered_accesses); - } + /// Adds all of the accesses from the passed set to `self`. + pub fn extend(&mut self, filtered_access_set: FilteredAccessSet) { + self.combined_access + .extend(&filtered_access_set.combined_access); + self.filtered_accesses + .extend(filtered_access_set.filtered_accesses); + } - /// Removes all accesses stored in this set. - pub fn clear(&mut self) { - self.combined_access.clear(); - self.filtered_accesses.clear(); - } + /// Removes all accesses stored in this set. + pub fn clear(&mut self) { + self.combined_access.clear(); + self.filtered_accesses.clear(); } +} - impl Default for FilteredAccessSet { - fn default() -> Self { - Self { - combined_access: Default::default(), - filtered_accesses: Vec::new(), - } +impl Default for FilteredAccessSet { + fn default() -> Self { + Self { + combined_access: Default::default(), + filtered_accesses: Vec::new(), } } +} - #[cfg(test)] - mod tests { - use crate::query::access::AccessFilters; - use crate::query::{Access, FilteredAccess, FilteredAccessSet}; - use fixedbitset::FixedBitSet; - use std::marker::PhantomData; +#[cfg(test)] +mod tests { + use crate::query::access::AccessFilters; + use crate::query::{Access, FilteredAccess, FilteredAccessSet}; + use fixedbitset::FixedBitSet; + use std::marker::PhantomData; - #[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); + #[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(); + let mut access_b = Access::::default(); + access_b.read_all(); - assert!(!access_b.is_compatible(&access_a)); + 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(); + // 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(); + let mut access_b = Access::::default(); + access_b.read_all(); - assert!(access_b.is_compatible(&access_a)); - } + assert!(access_b.is_compatible(&access_a)); + } - #[test] - fn access_get_conflicts() { - let mut access_a = Access::::default(); - access_a.add_read(0); - access_a.add_read(1); + #[test] + fn access_get_conflicts() { + let mut access_a = Access::::default(); + access_a.add_read(0); + access_a.add_read(1); - let mut access_b = Access::::default(); - access_b.add_read(0); - access_b.add_write(1); + let mut access_b = Access::::default(); + access_b.add_read(0); + access_b.add_write(1); - assert_eq!(access_a.get_conflicts(&access_b), vec![1]); + assert_eq!(access_a.get_conflicts(&access_b), vec![1]); - let mut access_c = Access::::default(); - access_c.add_write(0); - access_c.add_write(1); + let mut access_c = Access::::default(); + access_c.add_write(0); + access_c.add_write(1); - assert_eq!(access_a.get_conflicts(&access_c), vec![0, 1]); - assert_eq!(access_b.get_conflicts(&access_c), vec![0, 1]); + assert_eq!(access_a.get_conflicts(&access_c), vec![0, 1]); + assert_eq!(access_b.get_conflicts(&access_c), vec![0, 1]); - let mut access_d = Access::::default(); - access_d.add_read(0); + let mut access_d = Access::::default(); + access_d.add_read(0); - assert_eq!(access_d.get_conflicts(&access_a), vec![]); - assert_eq!(access_d.get_conflicts(&access_b), vec![]); - assert_eq!(access_d.get_conflicts(&access_c), vec![0]); - } + assert_eq!(access_d.get_conflicts(&access_a), vec![]); + assert_eq!(access_d.get_conflicts(&access_b), vec![]); + 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); + #[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 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:?}" - ); - } + 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(); - access_a.add_read(0); - access_a.add_read(1); - access_a.and_with(2); + #[test] + fn filtered_access_extend() { + let mut access_a = FilteredAccess::::default(); + access_a.add_read(0); + access_a.add_read(1); + access_a.and_with(2); - let mut access_b = FilteredAccess::::default(); - access_b.add_read(0); - access_b.add_write(3); - access_b.and_without(4); + let mut access_b = FilteredAccess::::default(); + access_b.add_read(0); + access_b.add_write(3); + access_b.and_without(4); - access_a.extend(&access_b); + access_a.extend(&access_b); - let mut expected = FilteredAccess::::default(); - expected.add_read(0); - expected.add_read(1); - expected.and_with(2); - expected.add_write(3); - expected.and_without(4); + let mut expected = FilteredAccess::::default(); + expected.add_read(0); + expected.add_read(1); + expected.and_with(2); + expected.add_write(3); + expected.and_without(4); - assert!(access_a.eq(&expected)); - } + assert!(access_a.eq(&expected)); + } - #[test] - fn filtered_access_extend_or() { - let mut access_a = FilteredAccess::::default(); - // Exclusive access to `(&mut A, &mut B)`. - access_a.add_write(0); - access_a.add_write(1); - - // Filter by `With`. - let mut access_b = FilteredAccess::::default(); - access_b.and_with(2); - - // Filter by `(With, Without)`. - let mut access_c = FilteredAccess::::default(); - access_c.and_with(3); - access_c.and_without(4); - - // Turns `access_b` into `Or<(With, (With, Without))>`. - access_b.append_or(&access_c); - // Applies the filters to the initial query, which corresponds to the FilteredAccess' - // representation of `Query<(&mut A, &mut B), Or<(With, (With, Without))>>`. - access_a.extend(&access_b); - - // Construct the expected `FilteredAccess` struct. - // The intention here is to test that exclusive access implied by `add_write` - // forms correct normalized access structs when extended with `Or` filters. - let mut expected = FilteredAccess::::default(); - expected.add_write(0); - expected.add_write(1); - // The resulted access is expected to represent `Or<((With, With, With), (With, With, With, Without))>`. - expected.filter_sets = vec![ - AccessFilters { - with: FixedBitSet::with_capacity_and_blocks(3, [0b111]), - without: FixedBitSet::default(), - _index_type: PhantomData, - }, - AccessFilters { - with: FixedBitSet::with_capacity_and_blocks(4, [0b1011]), - without: FixedBitSet::with_capacity_and_blocks(5, [0b10000]), - _index_type: PhantomData, - }, - ]; - - assert_eq!(access_a, expected); - } + #[test] + fn filtered_access_extend_or() { + let mut access_a = FilteredAccess::::default(); + // Exclusive access to `(&mut A, &mut B)`. + access_a.add_write(0); + access_a.add_write(1); + + // Filter by `With`. + let mut access_b = FilteredAccess::::default(); + access_b.and_with(2); + + // Filter by `(With, Without)`. + let mut access_c = FilteredAccess::::default(); + access_c.and_with(3); + access_c.and_without(4); + + // Turns `access_b` into `Or<(With, (With, Without))>`. + access_b.append_or(&access_c); + // Applies the filters to the initial query, which corresponds to the FilteredAccess' + // representation of `Query<(&mut A, &mut B), Or<(With, (With, Without))>>`. + access_a.extend(&access_b); + + // Construct the expected `FilteredAccess` struct. + // The intention here is to test that exclusive access implied by `add_write` + // forms correct normalized access structs when extended with `Or` filters. + let mut expected = FilteredAccess::::default(); + expected.add_write(0); + expected.add_write(1); + // The resulted access is expected to represent `Or<((With, With, With), (With, With, With, Without))>`. + expected.filter_sets = vec![ + AccessFilters { + with: FixedBitSet::with_capacity_and_blocks(3, [0b111]), + without: FixedBitSet::default(), + _index_type: PhantomData, + }, + AccessFilters { + with: FixedBitSet::with_capacity_and_blocks(4, [0b1011]), + without: FixedBitSet::with_capacity_and_blocks(5, [0b10000]), + _index_type: PhantomData, + }, + ]; + + assert_eq!(access_a, expected); } +} diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index ab5f6855a2be3..92fbbce2b3958 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1,7 +1,7 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId}, change_detection::{Ticks, TicksMut}, - component::{Component, ComponentId, ComponentStorage, StorageType, Tick, Components}, + component::{Component, ComponentId, ComponentStorage, Components, StorageType, Tick}, entity::Entity, query::{Access, DebugCheckedUnwrap, FilteredAccess}, storage::{ComponentSparseSet, Table, TableRow}, @@ -1406,7 +1406,7 @@ unsafe impl WorldQuery for Has { fn init_state(world: &mut World) -> ComponentId { world.init_component::() } - + fn new_state(components: &Components) -> ComponentId { components.component_id::().unwrap() } diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index e283c36652b4d..4bb41f14cde3b 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -1,6 +1,6 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId}, - component::{Component, ComponentId, ComponentStorage, StorageType, Tick, Components}, + component::{Component, ComponentId, ComponentStorage, Components, StorageType, Tick}, entity::Entity, query::{Access, DebugCheckedUnwrap, FilteredAccess, WorldQuery}, storage::{Column, ComponentSparseSet, Table, TableRow}, From 75617652deaec75ae77aa75da7c3db1260341a99 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Fri, 6 Oct 2023 15:02:15 -0700 Subject: [PATCH 04/27] ci fixes --- crates/bevy_ecs/src/query/state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 98ea83855c1d5..93fc3ed114132 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -182,7 +182,7 @@ impl QueryState { par_iter_span: bevy_utils::tracing::info_span!( "par_for_each", query = std::any::type_name::(), - filter = std::any::type_name::(), + filter = std::any::type_name::<()>(), ), } } From 851028d085c6424d76a9ea9b6a39e22c1a92d344 Mon Sep 17 00:00:00 2001 From: Mike Date: Sat, 7 Oct 2023 10:29:14 -0700 Subject: [PATCH 05/27] Switch to safe methods. Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com> --- crates/bevy_ecs/src/query/state.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 93fc3ed114132..364b0e5c44c92 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -146,12 +146,10 @@ impl QueryState { if !Q::IS_ARCHETYPAL || !F::IS_ARCHETYPAL || !NewQ::IS_ARCHETYPAL { panic!("generalizing is not allow with queries that use `Changed` or `Added`"); } - // SAFETY: we are using unsafe world to access &Components which should always be safe - let fetch_state = unsafe { NewQ::new_state(world.world().components()) }; + let fetch_state = NewQ::new_state(world.components()); #[allow(clippy::let_unit_value)] // the archetypal filters have already been applied, so we don't need them. - // SAFETY: we are using unsafe world to access &Components which should always be safe - let filter_state = unsafe { <()>::new_state(world.world().components()) }; + let filter_state = <()>::new_state(world.components()); let mut component_access = FilteredAccess::default(); NewQ::update_component_access(&fetch_state, &mut component_access); From d50c654097a47f545dbaa1c7d68c3b76ee9734c3 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Sat, 7 Oct 2023 10:35:06 -0700 Subject: [PATCH 06/27] revert mistaken change --- crates/bevy_ecs/src/system/query.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index f90288bfc6756..4f9493ee88021 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -320,11 +320,11 @@ use std::{any::TypeId, borrow::Borrow}; /// [`single_mut`]: Self::single_mut /// [`SparseSet`]: crate::storage::SparseSet /// [System parameter]: crate::system::SystemParam -/// [`With`]: crate::query::With /// [`Table`]: crate::storage::Table +/// [`With`]: crate::query::With /// [`Without`]: crate::query::Without -// SAFETY: Must have access to the components registered in `state`. pub struct Query<'world, 'state, Q: WorldQuery, F: ReadOnlyWorldQuery = ()> { + // SAFETY: Must have access to the components registered in `state`. world: UnsafeWorldCell<'world>, state: &'state QueryState, last_run: Tick, From cb74ccc79f6ac3ae4651eee6f9a4cb489f78b15d Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Sat, 7 Oct 2023 12:01:00 -0700 Subject: [PATCH 07/27] rename some stuff and improve doc --- crates/bevy_ecs/src/query/state.rs | 2 +- crates/bevy_ecs/src/system/query.rs | 56 +++++++++++++++++++++++++---- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 364b0e5c44c92..80e6624b3c571 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -138,7 +138,7 @@ impl QueryState { state } - /// Transform a query into a more generic query. If the original query state did + /// Transform a [`QueryState`] into a more generic [`QueryState`]. If the original query state did /// not have the same access this will panic. /// Probably should not call update archetype generation on this query state as the results will /// be very unpredictable as new archetypes could be added that don't match old query diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 4f9493ee88021..76dd025f7b872 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -400,12 +400,51 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } } - /// Returns another `Query` that returns a subset of the data that the original query does. This can be - /// useful for passing the query to another function. - pub fn generalize(self) -> GeneralizedQuery<'w, NewQ> { + /// Returns a [`QueryLens`] that can be used to get a query with a more restricted fetch. + /// i.e. Tranform a `Query<(&A, &mut B)>` to a `Query<&B>`. + /// This can be useful for passing the query to another function. + /// + /// ## Panics + /// + /// This will panic if `NewQ` is not a subset of `Q` or if `F` includes `Added` or `Changed`. + /// + /// ## Example + /// + /// ```rust + /// # use bevy_ecs::prelude::*; + /// # use bevy_ecs::system::QueryLens; + /// # + /// # #[derive(Component)] + /// # struct A(usize); + /// # + /// # #[derive(Component)] + /// # struct B(usize); + /// # + /// # let mut world = World::new(); + /// # + /// # world.spawn((A(10), B(5))); + /// + /// fn function_that_uses_a_query(lens: &QueryLens<&A>) { + /// assert_eq!(lens.query().single().0, 10); + /// } + /// + /// fn system_1(query: Query<&A>) { + /// function_that_uses_a_query(&query.into_query_lens()); + /// } + /// + /// fn system_2(query: Query<(&mut A, &B)>) { + /// let lens = query.restrict_fetch::<&A>(); + /// function_that_uses_a_query(&lens); + /// } + /// + /// # let mut schedule = Schedule::default(); + /// # schedule.add_systems((system_1, system_2)); + /// # schedule.run(&mut world); + /// ``` + pub fn restrict_fetch(&mut self) -> QueryLens<'_, NewQ> { let new_state = self.state.generalize(self.world); - GeneralizedQuery { + QueryLens { state: new_state, world: self.world, last_run: self.last_run, @@ -413,6 +452,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } } + /// helper method to get a QueryLens with the same fetch as the existing query + pub fn into_query_lens(&mut self) -> QueryLens<'_, Q> { + self.restrict_fetch() + } + /// Returns an [`Iterator`] over the read-only query items. /// /// # Example @@ -1534,14 +1578,14 @@ impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } /// A type used to store the generalized query's state. -pub struct GeneralizedQuery<'world, NewQ: WorldQuery> { +pub struct QueryLens<'world, NewQ: WorldQuery> { world: UnsafeWorldCell<'world>, state: QueryState, last_run: Tick, this_run: Tick, } -impl<'world, NewQ: WorldQuery> GeneralizedQuery<'world, NewQ> { +impl<'world, NewQ: WorldQuery> QueryLens<'world, NewQ> { /// get the query associated with Generalized Query pub fn query(&self) -> Query<'world, '_, NewQ, ()> { Query { From 689772f417922ecafa1100f1a352aa2ec1987b45 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Sat, 7 Oct 2023 13:07:36 -0700 Subject: [PATCH 08/27] change new_state to return `Option` --- crates/bevy_ecs/macros/src/fetch.rs | 8 ++--- crates/bevy_ecs/src/query/fetch.rs | 51 ++++++++++++++++++----------- crates/bevy_ecs/src/query/filter.rs | 16 ++++----- crates/bevy_ecs/src/query/mod.rs | 27 ++++++++++----- crates/bevy_ecs/src/query/state.rs | 4 +-- 5 files changed, 63 insertions(+), 43 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 4a804bab9e10d..bf6e518949d58 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -352,10 +352,10 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { } } - fn new_state(components: &#path::component::Components) -> #state_struct_name #user_ty_generics { - #state_struct_name { - #(#named_field_idents: <#field_types>::new_state(components),)* - } + fn new_state(components: &#path::component::Components) -> Option<#state_struct_name #user_ty_generics> { + Some(#state_struct_name { + #(#named_field_idents: <#field_types>::new_state(components)?,)* + }) } fn matches_component_set(state: &Self::State, _set_contains_id: &impl Fn(#path::component::ComponentId) -> bool) -> bool { diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 92fbbce2b3958..666ea9be5fe69 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -440,8 +440,9 @@ pub unsafe trait WorldQuery { /// Creates and initializes a [`State`](WorldQuery::State) for this [`WorldQuery`] type. fn init_state(world: &mut World) -> Self::State; - /// Creates a [`State`](WorldQuery::State) for this [`WorldQuery`] type - fn new_state(components: &Components) -> Self::State; + /// Creates a [`State`](WorldQuery::State) for this [`WorldQuery`] type. This should return + /// the same value as [`init_state`](Self::init_state). + fn new_state(components: &Components) -> Option; /// Returns `true` if this query matches a set of components. Otherwise, returns `false`. fn matches_component_set( @@ -518,7 +519,9 @@ unsafe impl WorldQuery for Entity { fn init_state(_world: &mut World) {} - fn new_state(_components: &Components) {} + fn new_state(_components: &Components) -> Option<()> { + Some(()) + } fn matches_component_set( _state: &Self::State, @@ -599,7 +602,10 @@ unsafe impl WorldQuery for EntityRef<'_> { } fn init_state(_world: &mut World) {} - fn new_state(_components: &Components) {} + + fn new_state(_components: &Components) -> Option<()> { + Some(()) + } fn matches_component_set( _state: &Self::State, @@ -680,7 +686,10 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> { } fn init_state(_world: &mut World) {} - fn new_state(_components: &Components) {} + + fn new_state(_components: &Components) -> Option<()> { + Some(()) + } fn matches_component_set( _state: &Self::State, @@ -822,8 +831,8 @@ unsafe impl WorldQuery for &T { world.init_component::() } - fn new_state(components: &Components) -> ComponentId { - components.component_id::().unwrap() + fn new_state(components: &Components) -> Option { + components.component_id::() } fn matches_component_set( @@ -987,8 +996,8 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { world.init_component::() } - fn new_state(components: &Components) -> ComponentId { - components.component_id::().unwrap() + fn new_state(components: &Components) -> Option { + components.component_id::() } fn matches_component_set( @@ -1152,8 +1161,8 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { world.init_component::() } - fn new_state(components: &Components) -> ComponentId { - components.component_id::().unwrap() + fn new_state(components: &Components) -> Option { + components.component_id::() } fn matches_component_set( @@ -1268,7 +1277,7 @@ unsafe impl WorldQuery for Option { T::init_state(world) } - fn new_state(components: &Components) -> T::State { + fn new_state(components: &Components) -> Option { T::new_state(components) } @@ -1407,8 +1416,8 @@ unsafe impl WorldQuery for Has { world.init_component::() } - fn new_state(components: &Components) -> ComponentId { - components.component_id::().unwrap() + fn new_state(components: &Components) -> Option { + components.component_id::() } fn matches_component_set( @@ -1507,8 +1516,8 @@ macro_rules! impl_tuple_fetch { ($($name::init_state(_world),)*) } - fn new_state(_components: &Components) -> Self::State { - ($($name::new_state(_components),)*) + fn new_state(_components: &Components) -> Option { + Some(($($name::new_state(_components)?,)*)) } fn matches_component_set(state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { @@ -1634,8 +1643,8 @@ macro_rules! impl_anytuple_fetch { ($($name::init_state(_world),)*) } - fn new_state(_components: &Components) -> Self::State { - ($($name::new_state(_components),)*) + fn new_state(_components: &Components) -> Option { + Some(($($name::new_state(_components)?,)*)) } fn matches_component_set(_state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { @@ -1712,7 +1721,7 @@ unsafe impl WorldQuery for NopWorldQuery { Q::init_state(world) } - fn new_state(components: &Components) -> Self::State { + fn new_state(components: &Components) -> Option { Q::new_state(components) } @@ -1779,7 +1788,9 @@ unsafe impl WorldQuery for PhantomData { fn init_state(_world: &mut World) -> Self::State {} - fn new_state(_components: &Components) -> Self::State {} + fn new_state(_components: &Components) -> Option { + Some(()) + } fn matches_component_set( _state: &Self::State, diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 4bb41f14cde3b..c7c1419919fcb 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -105,8 +105,8 @@ unsafe impl WorldQuery for With { world.init_component::() } - fn new_state(components: &Components) -> Self::State { - components.component_id::().unwrap() + fn new_state(components: &Components) -> Option { + components.component_id::() } fn matches_component_set( @@ -210,8 +210,8 @@ unsafe impl WorldQuery for Without { world.init_component::() } - fn new_state(components: &Components) -> Self::State { - components.component_id::().unwrap() + fn new_state(components: &Components) -> Option { + components.component_id::() } fn matches_component_set( @@ -378,8 +378,8 @@ macro_rules! impl_query_filter_tuple { ($($filter::init_state(world),)*) } - fn new_state(components: &Components) -> Self::State { - ($($filter::new_state(components),)*) + fn new_state(components: &Components) -> Option { + Some(($($filter::new_state(components)?,)*)) } fn matches_component_set(_state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { @@ -545,8 +545,8 @@ macro_rules! impl_tick_filter { world.init_component::() } - fn new_state(components: &Components) -> ComponentId { - components.component_id::().unwrap() + fn new_state(components: &Components) -> Option { + components.component_id::() } fn matches_component_set(&id: &ComponentId, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index 0571a4898cc86..1799722011980 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -803,23 +803,32 @@ mod tests { } #[test] - fn generalize() { + fn restrict_fetch() { + use bevy_ecs::prelude::*; + use bevy_ecs::system::QueryLens; + #[derive(Component)] + struct A(usize); + + #[derive(Component)] + struct B(usize); + let mut world = World::new(); world.spawn((A(10), B(5))); - fn function_that_takes_a_query(query: &Query<&A>) { - assert_eq!(query.single().0, 10); + fn function_that_uses_a_query(lens: &QueryLens<&mut A>) { + let mut q = lens.query(); + q.single_mut().0 = 15; + assert_eq!(q.single().0, 15); } - fn system_1(query: Query<&A>) { - function_that_takes_a_query(&query); + fn system_1(mut query: Query<&mut A>) { + function_that_uses_a_query(&query.into_query_lens()); } - fn system_2(query: Query<(&A, &B)>) { - let gen_q = query.generalize::<&A>(); - let q = gen_q.query(); - function_that_takes_a_query(&q); + fn system_2(mut query: Query<(&mut A, &B)>) { + let lens = query.restrict_fetch::<&mut A>(); + function_that_uses_a_query(&lens); } let mut schedule = Schedule::default(); diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 80e6624b3c571..21460b8d0bf1f 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -146,10 +146,10 @@ impl QueryState { if !Q::IS_ARCHETYPAL || !F::IS_ARCHETYPAL || !NewQ::IS_ARCHETYPAL { panic!("generalizing is not allow with queries that use `Changed` or `Added`"); } - let fetch_state = NewQ::new_state(world.components()); + let fetch_state = NewQ::new_state(world.components()).unwrap(); #[allow(clippy::let_unit_value)] // the archetypal filters have already been applied, so we don't need them. - let filter_state = <()>::new_state(world.components()); + let filter_state = <()>::new_state(world.components()).unwrap(); let mut component_access = FilteredAccess::default(); NewQ::update_component_access(&fetch_state, &mut component_access); From 1d174db3647686b319f4ad48ff698c41f8c348c1 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Sat, 7 Oct 2023 15:45:35 -0700 Subject: [PATCH 09/27] more renaming --- crates/bevy_ecs/src/query/state.rs | 21 ++++++++++++++------- crates/bevy_ecs/src/system/query.rs | 2 +- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 21460b8d0bf1f..4954d638a9930 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -142,14 +142,20 @@ impl QueryState { /// not have the same access this will panic. /// Probably should not call update archetype generation on this query state as the results will /// be very unpredictable as new archetypes could be added that don't match old query - pub fn generalize(&self, world: UnsafeWorldCell) -> QueryState { + pub fn restrict_fetch(&self, world: UnsafeWorldCell) -> QueryState { if !Q::IS_ARCHETYPAL || !F::IS_ARCHETYPAL || !NewQ::IS_ARCHETYPAL { panic!("generalizing is not allow with queries that use `Changed` or `Added`"); } - let fetch_state = NewQ::new_state(world.components()).unwrap(); + let fetch_state = NewQ::new_state(world.components()).expect( + "Could not create fetch_state. Make \ + sure you initialize the QueryState before calling this method.", + ); #[allow(clippy::let_unit_value)] // the archetypal filters have already been applied, so we don't need them. - let filter_state = <()>::new_state(world.components()).unwrap(); + let filter_state = <()>::new_state(world.components()).expect( + "Could not create filter_state. Make \ + sure you initialize the QueryState before calling this method.", + ); let mut component_access = FilteredAccess::default(); NewQ::update_component_access(&fetch_state, &mut component_access); @@ -1590,7 +1596,7 @@ mod tests { world.spawn((A(22), B)); let query_state = world.query::<(&A, &B)>(); - let mut new_query_state = query_state.generalize::<&A>(world.as_unsafe_world_cell()); + let mut new_query_state = query_state.restrict_fetch::<&A>(world.as_unsafe_world_cell()); assert_eq!(new_query_state.iter(&world).len(), 1); let a = new_query_state.single(&world); @@ -1613,7 +1619,7 @@ mod tests { world.spawn((A(23), B, C)); let query_state = world.query_filtered::<(&A, &B), Without>(); - let mut new_query_state = query_state.generalize::<&A>(world.as_unsafe_world_cell()); + let mut new_query_state = query_state.restrict_fetch::<&A>(world.as_unsafe_world_cell()); // even though we change the query to not have Without, we cannot get the component with C. let a = new_query_state.single(&world); @@ -1635,7 +1641,8 @@ mod tests { world.spawn(A(22)); let query_state = world.query::<&A>(); - let mut _new_query_state = query_state.generalize::<(&A, &B)>(world.as_unsafe_world_cell()); + let mut _new_query_state = + query_state.restrict_fetch::<(&A, &B)>(world.as_unsafe_world_cell()); } #[test] @@ -1653,6 +1660,6 @@ mod tests { world.spawn(A(22)); let query_state = world.query_filtered::<(&A, &B), Added>(); - let mut _new_query_state = query_state.generalize::<&A>(world.as_unsafe_world_cell()); + let mut _new_query_state = query_state.restrict_fetch::<&A>(world.as_unsafe_world_cell()); } } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 76dd025f7b872..206460074ae8e 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -442,7 +442,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// # schedule.run(&mut world); /// ``` pub fn restrict_fetch(&mut self) -> QueryLens<'_, NewQ> { - let new_state = self.state.generalize(self.world); + let new_state = self.state.restrict_fetch(self.world); QueryLens { state: new_state, From e69dcec974da7b81d59cbf98904b1e5f227e0cfc Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Sat, 7 Oct 2023 21:23:13 -0700 Subject: [PATCH 10/27] change restrict_fetch to take &Components make it pub crate - some other cleanup. --- crates/bevy_ecs/src/query/state.rs | 138 ++++++++++++++-------------- crates/bevy_ecs/src/system/query.rs | 2 +- 2 files changed, 69 insertions(+), 71 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 4954d638a9930..7e92269a47f95 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1,7 +1,7 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId, ArchetypeGeneration, ArchetypeId}, change_detection::Mut, - component::{ComponentId, Tick}, + component::{ComponentId, Components, Tick}, entity::Entity, prelude::{Component, FromWorld}, query::{ @@ -138,23 +138,28 @@ impl QueryState { state } - /// Transform a [`QueryState`] into a more generic [`QueryState`]. If the original query state did - /// not have the same access this will panic. - /// Probably should not call update archetype generation on this query state as the results will - /// be very unpredictable as new archetypes could be added that don't match old query - pub fn restrict_fetch(&self, world: UnsafeWorldCell) -> QueryState { + /// This is used to transform a [`Query`] into a more generic [`Query`]. This can be useful for passsing to another function that + /// might take the more generalize query. See [`Query::restrict_fetch`] for more details. + /// + /// You should not call `update_archetypes` on the returned QueryState as the result will be unpredictable. + /// You might end up with a mix of archetypes that only matched the original query + archetypes that only match + /// the new [`QueryState`]. Most of the safe methods on [`QueryState`] call [`update_archetypes`] internally. + pub(crate) fn restrict_fetch( + &self, + components: &Components, + ) -> QueryState { if !Q::IS_ARCHETYPAL || !F::IS_ARCHETYPAL || !NewQ::IS_ARCHETYPAL { - panic!("generalizing is not allow with queries that use `Changed` or `Added`"); + panic!("`restrict_fetch` is not allow with queries that use `Changed` or `Added`"); } - let fetch_state = NewQ::new_state(world.components()).expect( - "Could not create fetch_state. Make \ - sure you initialize the QueryState before calling this method.", + let fetch_state = NewQ::new_state(components).expect( + "Could not create fetch_state. This should not be reachable as the components should have been \ + initialized when creating the original QueryState.", ); #[allow(clippy::let_unit_value)] // the archetypal filters have already been applied, so we don't need them. - let filter_state = <()>::new_state(world.components()).expect( - "Could not create filter_state. Make \ - sure you initialize the QueryState before calling this method.", + let filter_state = <()>::new_state(components).expect( + "Could not create filter_state. This should not be reachable as the components should have been \ + initialized when creating the original QueryState.", ); let mut component_access = FilteredAccess::default(); @@ -1584,27 +1589,9 @@ mod tests { let _panics = query_state.get_many_mut(&mut world_2, []); } - #[test] - fn generalize_query_state() { - #[derive(Component)] - struct A(pub usize); + mod restrict_fetch { + use super::*; - #[derive(Component)] - struct B; - - let mut world = World::new(); - world.spawn((A(22), B)); - - let query_state = world.query::<(&A, &B)>(); - let mut new_query_state = query_state.restrict_fetch::<&A>(world.as_unsafe_world_cell()); - assert_eq!(new_query_state.iter(&world).len(), 1); - let a = new_query_state.single(&world); - - assert_eq!(a.0, 22); - } - - #[test] - fn generalize_query_cannot_get_unmatched_data() { #[derive(Component)] struct A(pub usize); @@ -1614,52 +1601,63 @@ mod tests { #[derive(Component)] struct C; - let mut world = World::new(); - world.spawn((A(22), B)); - world.spawn((A(23), B, C)); + #[test] + fn can_restrict() { + let mut world = World::new(); + world.spawn((A(22), B)); - let query_state = world.query_filtered::<(&A, &B), Without>(); - let mut new_query_state = query_state.restrict_fetch::<&A>(world.as_unsafe_world_cell()); - // even though we change the query to not have Without, we cannot get the component with C. - let a = new_query_state.single(&world); + let query_state = world.query::<(&A, &B)>(); + let mut new_query_state = query_state.restrict_fetch::<&A>(world.components()); + assert_eq!(new_query_state.iter(&world).len(), 1); + let a = new_query_state.single(&world); - assert_eq!(a.0, 22); - } + assert_eq!(a.0, 22); + } - #[test] - #[should_panic] - fn generalize_to_no_included_components_not_allowed() { - #[derive(Component)] - struct A(pub usize); + #[test] + fn cannot_get_unmatched_data() { + let mut world = World::new(); + world.spawn((A(22), B)); + world.spawn((A(23), B, C)); - #[derive(Component)] - struct B; + let query_state = world.query_filtered::<(&A, &B), Without>(); + let mut new_query_state = query_state.restrict_fetch::<&A>(world.components()); + // even though we change the query to not have Without, we do not get the component with C. + let a = new_query_state.single(&world); - let mut world = World::new(); - world.init_component::(); - world.init_component::(); - world.spawn(A(22)); + assert_eq!(a.0, 22); + } - let query_state = world.query::<&A>(); - let mut _new_query_state = - query_state.restrict_fetch::<(&A, &B)>(world.as_unsafe_world_cell()); - } + #[test] + #[should_panic] + fn not_included_components_not_allowed() { + let mut world = World::new(); + world.init_component::(); + world.init_component::(); + world.spawn(A(22)); - #[test] - #[should_panic] - fn generalize_non_archtypal_not_allowed() { - #[derive(Component)] - struct A(pub usize); + let query_state = world.query::<&A>(); + let mut _new_query_state = query_state.restrict_fetch::<(&A, &B)>(world.components()); + } - #[derive(Component)] - struct B; + #[test] + #[should_panic] + fn non_archtypal_not_allowed() { + let mut world = World::new(); + world.spawn(A(22)); - let mut world = World::new(); - world.init_component::(); - world.init_component::(); - world.spawn(A(22)); + let query_state = world.query_filtered::<(&A, &B), Added>(); + let mut _new_query_state = query_state.restrict_fetch::<&A>(world.components()); + } - let query_state = world.query_filtered::<(&A, &B), Added>(); - let mut _new_query_state = query_state.restrict_fetch::<&A>(world.as_unsafe_world_cell()); + #[test] + #[should_panic] + fn immut_to_mut_not_allowed() { + let mut world = World::new(); + world.spawn(A(22)); + + let query_state = world.query::<&A>(); + let mut _new_query_state = query_state.restrict_fetch::<&mut A>(world.components()); + } } } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 206460074ae8e..d165c7850bd0e 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -442,7 +442,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// # schedule.run(&mut world); /// ``` pub fn restrict_fetch(&mut self) -> QueryLens<'_, NewQ> { - let new_state = self.state.restrict_fetch(self.world); + let new_state = self.state.restrict_fetch(self.world.components()); QueryLens { state: new_state, From 60751fbbafcdfeec1db7feb2b67818661f7ca20b Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Sat, 7 Oct 2023 21:23:52 -0700 Subject: [PATCH 11/27] allowing getting 2 queries from a &QueryLens is unsafe --- crates/bevy_ecs/src/query/mod.rs | 34 ----------------------------- crates/bevy_ecs/src/system/query.rs | 16 +++++++------- 2 files changed, 8 insertions(+), 42 deletions(-) diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index 1799722011980..c30292cb76a2d 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -801,38 +801,4 @@ mod tests { let values = world.query::<&B>().iter(&world).collect::>(); assert_eq!(values, vec![&B(2)]); } - - #[test] - fn restrict_fetch() { - use bevy_ecs::prelude::*; - use bevy_ecs::system::QueryLens; - #[derive(Component)] - struct A(usize); - - #[derive(Component)] - struct B(usize); - - let mut world = World::new(); - - world.spawn((A(10), B(5))); - - fn function_that_uses_a_query(lens: &QueryLens<&mut A>) { - let mut q = lens.query(); - q.single_mut().0 = 15; - assert_eq!(q.single().0, 15); - } - - fn system_1(mut query: Query<&mut A>) { - function_that_uses_a_query(&query.into_query_lens()); - } - - fn system_2(mut query: Query<(&mut A, &B)>) { - let lens = query.restrict_fetch::<&mut A>(); - function_that_uses_a_query(&lens); - } - - let mut schedule = Schedule::default(); - schedule.add_systems((system_1, system_2)); - schedule.run(&mut world); - } } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index d165c7850bd0e..421fbfa38ffb7 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -424,17 +424,17 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// # /// # world.spawn((A(10), B(5))); /// - /// fn function_that_uses_a_query(lens: &QueryLens<&A>) { + /// fn function_that_uses_a_query_lens(lens: &mut QueryLens<&A>) { /// assert_eq!(lens.query().single().0, 10); /// } /// - /// fn system_1(query: Query<&A>) { - /// function_that_uses_a_query(&query.into_query_lens()); + /// fn system_1(mut query: Query<&A>) { + /// function_that_uses_a_query_lens(&mut query.into_query_lens()); /// } /// - /// fn system_2(query: Query<(&mut A, &B)>) { - /// let lens = query.restrict_fetch::<&A>(); - /// function_that_uses_a_query(&lens); + /// fn system_2(mut query: Query<(&mut A, &B)>) { + /// let mut lens = query.restrict_fetch::<&A>(); + /// function_that_uses_a_query_lens(&mut lens); /// } /// /// # let mut schedule = Schedule::default(); @@ -1586,8 +1586,8 @@ pub struct QueryLens<'world, NewQ: WorldQuery> { } impl<'world, NewQ: WorldQuery> QueryLens<'world, NewQ> { - /// get the query associated with Generalized Query - pub fn query(&self) -> Query<'world, '_, NewQ, ()> { + /// Get the [`Query`] associated with the [`QueryLens`] + pub fn query(&mut self) -> Query<'world, '_, NewQ, ()> { Query { world: self.world, state: &self.state, From 3a4184e750a9757557667def9dff2e4e55ffffa3 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Sat, 7 Oct 2023 22:13:29 -0700 Subject: [PATCH 12/27] clippy --- crates/bevy_ecs/src/query/state.rs | 9 +++++---- crates/bevy_ecs/src/system/query.rs | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 7e92269a47f95..e4c090677402c 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -138,12 +138,13 @@ impl QueryState { state } - /// This is used to transform a [`Query`] into a more generic [`Query`]. This can be useful for passsing to another function that - /// might take the more generalize query. See [`Query::restrict_fetch`] for more details. + /// This is used to transform a [`Query`](crate::system::Query) into a more generic [`Query`](crate::system::Query). + /// This can be useful for passsing to another function tha might take the more generalize query. + /// See [`Query::restrict_fetch`](crate::system::Query::restrict_fetch) for more details. /// - /// You should not call `update_archetypes` on the returned QueryState as the result will be unpredictable. + /// You should not call `update_archetypes` on the returned [`QueryState`] as the result will be unpredictable. /// You might end up with a mix of archetypes that only matched the original query + archetypes that only match - /// the new [`QueryState`]. Most of the safe methods on [`QueryState`] call [`update_archetypes`] internally. + /// the new [`QueryState`]. Most of the safe methods on [`QueryState`] call [`QueryState::update_archetypes`] internally. pub(crate) fn restrict_fetch( &self, components: &Components, diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 421fbfa38ffb7..ef36581177fdf 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -452,7 +452,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } } - /// helper method to get a QueryLens with the same fetch as the existing query + /// helper method to get a [`QueryLens`] with the same fetch as the existing query pub fn into_query_lens(&mut self) -> QueryLens<'_, Q> { self.restrict_fetch() } From 90d878de9949dd291d866bfa60deaf3b02b96c65 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Sun, 8 Oct 2023 22:17:56 -0700 Subject: [PATCH 13/27] don't allow Option<&T> transmutes --- crates/bevy_ecs/macros/src/fetch.rs | 2 ++ crates/bevy_ecs/src/query/fetch.rs | 28 ++++++++++++++++++++++++++ crates/bevy_ecs/src/query/filter.rs | 8 ++++++++ crates/bevy_ecs/src/query/state.rs | 31 +++++++++++++++++++++++------ 4 files changed, 63 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index bf6e518949d58..966d88dea81e8 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -288,6 +288,8 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { const IS_DENSE: bool = true #(&& <#field_types>::IS_DENSE)*; const IS_ARCHETYPAL: bool = true #(&& <#field_types>::IS_ARCHETYPAL)*; + + const IS_EXACT: bool = true #(&& <#field_types>::IS_EXACT)*; /// SAFETY: we call `set_archetype` for each member that implements `Fetch` #[inline] diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 666ea9be5fe69..3b5df6beafe46 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -360,6 +360,10 @@ pub unsafe trait WorldQuery { /// many elements are being iterated (such as `Iterator::collect()`). const IS_ARCHETYPAL: bool; + /// Returns true if (and only if) every archetype returned has data for the underlying types. + /// i.e. [`Self::State`] = Option<&T> would return false, but &T would return true. + const IS_EXACT: bool; + /// Adjusts internal state to account for the next [`Archetype`]. This will always be called on /// archetypes that match this [`WorldQuery`]. /// @@ -478,6 +482,8 @@ unsafe impl WorldQuery for Entity { const IS_ARCHETYPAL: bool = true; + const IS_EXACT: bool = true; + unsafe fn init_fetch<'w>( _world: UnsafeWorldCell<'w>, _state: &Self::State, @@ -549,6 +555,8 @@ unsafe impl WorldQuery for EntityRef<'_> { const IS_ARCHETYPAL: bool = true; + const IS_EXACT: bool = true; + unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, _state: &Self::State, @@ -633,6 +641,8 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> { const IS_ARCHETYPAL: bool = true; + const IS_EXACT: bool = true; + unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, _state: &Self::State, @@ -734,6 +744,8 @@ unsafe impl WorldQuery for &T { const IS_ARCHETYPAL: bool = true; + const IS_EXACT: bool = true; + #[inline] unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, @@ -888,6 +900,8 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { const IS_ARCHETYPAL: bool = true; + const IS_EXACT: bool = true; + #[inline] unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, @@ -1053,6 +1067,8 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { const IS_ARCHETYPAL: bool = true; + const IS_EXACT: bool = true; + #[inline] unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, @@ -1203,6 +1219,8 @@ unsafe impl WorldQuery for Option { const IS_ARCHETYPAL: bool = T::IS_ARCHETYPAL; + const IS_EXACT: bool = false; + #[inline] unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, @@ -1367,6 +1385,8 @@ unsafe impl WorldQuery for Has { const IS_ARCHETYPAL: bool = true; + const IS_EXACT: bool = false; + #[inline] unsafe fn init_fetch<'w>( _world: UnsafeWorldCell<'w>, @@ -1461,6 +1481,8 @@ macro_rules! impl_tuple_fetch { const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*; + const IS_EXACT: bool = true $(&& $name::IS_EXACT)*; + #[inline] unsafe fn set_archetype<'w>( _fetch: &mut Self::Fetch<'w>, @@ -1568,6 +1590,8 @@ macro_rules! impl_anytuple_fetch { const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*; + const IS_EXACT: bool = false; + #[inline] unsafe fn set_archetype<'w>( _fetch: &mut Self::Fetch<'w>, @@ -1679,6 +1703,8 @@ unsafe impl WorldQuery for NopWorldQuery { const IS_ARCHETYPAL: bool = true; + const IS_EXACT: bool = Q::IS_EXACT; + #[inline(always)] unsafe fn init_fetch( _world: UnsafeWorldCell, @@ -1758,6 +1784,8 @@ unsafe impl WorldQuery for PhantomData { const IS_DENSE: bool = true; // `PhantomData` matches every entity in each archetype. const IS_ARCHETYPAL: bool = true; + // `PhantomData` does not access any data, so it is exact. + const IS_EXACT: bool = true; unsafe fn set_archetype<'w>( _fetch: &mut Self::Fetch<'w>, diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index c7c1419919fcb..dad8e375d7e17 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -68,6 +68,8 @@ unsafe impl WorldQuery for With { const IS_ARCHETYPAL: bool = true; + const IS_EXACT: bool = false; + #[inline] unsafe fn set_table(_fetch: &mut (), _state: &ComponentId, _table: &Table) {} @@ -173,6 +175,8 @@ unsafe impl WorldQuery for Without { const IS_ARCHETYPAL: bool = true; + const IS_EXACT: bool = false; + #[inline] unsafe fn set_table(_fetch: &mut (), _state: &Self::State, _table: &Table) {} @@ -292,6 +296,8 @@ macro_rules! impl_query_filter_tuple { const IS_ARCHETYPAL: bool = true $(&& $filter::IS_ARCHETYPAL)*; + const IS_EXACT: bool = false; + #[inline] unsafe fn init_fetch<'w>(world: UnsafeWorldCell<'w>, state: &Self::State, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> { let ($($filter,)*) = state; @@ -458,6 +464,8 @@ macro_rules! impl_tick_filter { const IS_ARCHETYPAL: bool = false; + const IS_EXACT: bool = true; + #[inline] unsafe fn set_table<'w>( fetch: &mut Self::Fetch<'w>, diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index e4c090677402c..dcc9068ecb071 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -150,7 +150,14 @@ impl QueryState { components: &Components, ) -> QueryState { if !Q::IS_ARCHETYPAL || !F::IS_ARCHETYPAL || !NewQ::IS_ARCHETYPAL { - panic!("`restrict_fetch` is not allow with queries that use `Changed` or `Added`"); + panic!("`restrict_fetch` is not allowed with queries that use `Changed` or `Added`"); + } + // TODO: Figure out how to relax this restriction. This is overly restrictive as Option<&T> -> Option<&T> should be allowed. + // However the information needed to understand that is not available in this step. i.e. We try to fetch data that an archetype doesn't have. + // It is available during fetch, but that would introduce branching in release builds. + // This is undesirable as fetching is in the hot path for query iteration. + if !Q::IS_EXACT { + panic!("`restrict_fetch` from types that return archetypes that do not include the underlying data type like `Option<&T>` are not allowed.") } let fetch_state = NewQ::new_state(components).expect( "Could not create fetch_state. This should not be reachable as the components should have been \ @@ -1600,7 +1607,7 @@ mod tests { struct B; #[derive(Component)] - struct C; + struct C(pub usize); #[test] fn can_restrict() { @@ -1619,7 +1626,7 @@ mod tests { fn cannot_get_unmatched_data() { let mut world = World::new(); world.spawn((A(22), B)); - world.spawn((A(23), B, C)); + world.spawn((A(23), B, C(5))); let query_state = world.query_filtered::<(&A, &B), Without>(); let mut new_query_state = query_state.restrict_fetch::<&A>(world.components()); @@ -1630,7 +1637,7 @@ mod tests { } #[test] - #[should_panic] + #[should_panic(expected = "access is not compatible with new query type")] fn not_included_components_not_allowed() { let mut world = World::new(); world.init_component::(); @@ -1642,7 +1649,7 @@ mod tests { } #[test] - #[should_panic] + #[should_panic(expected = "`restrict_fetch` is not allowed with queries that use `Changed` or `Added`")] fn non_archtypal_not_allowed() { let mut world = World::new(); world.spawn(A(22)); @@ -1652,7 +1659,7 @@ mod tests { } #[test] - #[should_panic] + #[should_panic(expected = "access is not compatible with new query type")] fn immut_to_mut_not_allowed() { let mut world = World::new(); world.spawn(A(22)); @@ -1660,5 +1667,17 @@ mod tests { let query_state = world.query::<&A>(); let mut _new_query_state = query_state.restrict_fetch::<&mut A>(world.components()); } + + #[test] + #[should_panic(expected = "`restrict_fetch` from types that return archetypes")] + fn option_to_immut_not_allowed() { + let mut world = World::new(); + world.spawn(C(22)); + + let query_state = world.query::>(); + let mut new_query_state = query_state.restrict_fetch::<&A>(world.components()); + let x = new_query_state.single(&world); + assert_eq!(x.0, 1234); + } } } From ec28e8520fc89b908f50e6e02a3cdd3ff0baab11 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 9 Oct 2023 12:45:52 -0700 Subject: [PATCH 14/27] switch to using a fixed bit set for optional checking --- crates/bevy_ecs/macros/src/fetch.rs | 10 ++- crates/bevy_ecs/src/query/access.rs | 7 +- crates/bevy_ecs/src/query/fetch.rs | 105 ++++++++++++++++++++++++++++ crates/bevy_ecs/src/query/filter.rs | 30 ++++++++ crates/bevy_ecs/src/query/state.rs | 16 +++-- 5 files changed, 159 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 966d88dea81e8..296bd984338b3 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -288,7 +288,7 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { const IS_DENSE: bool = true #(&& <#field_types>::IS_DENSE)*; const IS_ARCHETYPAL: bool = true #(&& <#field_types>::IS_ARCHETYPAL)*; - + const IS_EXACT: bool = true #(&& <#field_types>::IS_EXACT)*; /// SAFETY: we call `set_archetype` for each member that implements `Fetch` @@ -348,6 +348,14 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { )* } + fn optional_access( + state: &Self::State, + _access: &mut #path::query::Access<#path::component::ComponentId>, + parent_is_optional: bool, + ) { + #( <#field_types>::optional_access(&state.#named_field_idents, _access, parent_is_optional); )* + } + fn init_state(world: &mut #path::world::World) -> #state_struct_name #user_ty_generics { #state_struct_name { #(#named_field_idents: <#field_types>::init_state(world),)* diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 70a77f7759508..3878fe8fdc8a2 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -269,6 +269,11 @@ impl Access { pub fn writes(&self) -> impl Iterator + '_ { self.writes.ones().map(T::get_sparse_set_index) } + + /// + pub fn difference_is_empty(&self, other: &Access) -> bool { + self.reads_and_writes.difference(&other.reads_and_writes).count() == 0 + } } /// An [`Access`] that has been filtered to include and exclude certain combinations of elements. @@ -400,7 +405,7 @@ impl FilteredAccess { }) } - /// access is compatible with other access. This does not take into account the filtered access. + /// `other` contains all the same access as `self`. This does not take into account the filtered access. pub fn is_subset(&self, other: &FilteredAccess) -> bool { self.access.is_subset(&other.access) } diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 3b5df6beafe46..d83b6b3253030 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -441,6 +441,15 @@ pub unsafe trait WorldQuery { access: &mut Access, ); + /// Adds component access for when [`WorldQuery`] is not an exact match. + /// i.e. Option<&T> where the matched archetypes don't necessarily contain + /// the data the WorldQuery takes access to in [`update_component_access`] + fn optional_access( + state: &Self::State, + access: &mut Access, + parent_is_optional: bool, + ); + /// Creates and initializes a [`State`](WorldQuery::State) for this [`WorldQuery`] type. fn init_state(world: &mut World) -> Self::State; @@ -523,6 +532,13 @@ unsafe impl WorldQuery for Entity { ) { } + fn optional_access( + _state: &Self::State, + _access: &mut Access, + _parent_is_optional: bool, + ) { + } + fn init_state(_world: &mut World) {} fn new_state(_components: &Components) -> Option<()> { @@ -609,6 +625,16 @@ unsafe impl WorldQuery for EntityRef<'_> { } } + fn optional_access( + _state: &Self::State, + access: &mut Access, + parent_is_optional: bool, + ) { + if parent_is_optional { + access.read_all() + } + } + fn init_state(_world: &mut World) {} fn new_state(_components: &Components) -> Option<()> { @@ -695,6 +721,16 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> { } } + fn optional_access( + _state: &Self::State, + access: &mut Access, + parent_is_optional: bool, + ) { + if parent_is_optional { + access.write_all(); + } + } + fn init_state(_world: &mut World) {} fn new_state(_components: &Components) -> Option<()> { @@ -839,6 +875,16 @@ unsafe impl WorldQuery for &T { } } + fn optional_access( + &component_id: &ComponentId, + access: &mut Access, + parent_is_optional: bool, + ) { + if parent_is_optional { + access.add_read(component_id); + } + } + fn init_state(world: &mut World) -> ComponentId { world.init_component::() } @@ -1006,6 +1052,16 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { } } + fn optional_access( + &component_id: &ComponentId, + access: &mut Access, + parent_is_optional: bool, + ) { + if parent_is_optional { + access.add_read(component_id); + } + } + fn init_state(world: &mut World) -> ComponentId { world.init_component::() } @@ -1173,6 +1229,16 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { } } + fn optional_access( + &component_id: &ComponentId, + access: &mut Access, + parent_is_optional: bool, + ) { + if parent_is_optional { + access.add_write(component_id); + } + } + fn init_state(world: &mut World) -> ComponentId { world.init_component::() } @@ -1291,6 +1357,14 @@ unsafe impl WorldQuery for Option { } } + fn optional_access( + state: &T::State, + access: &mut Access, + _parent_is_optional: bool, + ) { + T::optional_access(state, access, true); + } + fn init_state(world: &mut World) -> T::State { T::init_state(world) } @@ -1432,6 +1506,13 @@ unsafe impl WorldQuery for Has { ) { } + fn optional_access( + _state: &Self::State, + _access: &mut Access, + _parent_is_optional: bool, + ) { + } + fn init_state(world: &mut World) -> ComponentId { world.init_component::() } @@ -1533,6 +1614,11 @@ macro_rules! impl_tuple_fetch { $($name::update_archetype_component_access($name, _archetype, _access);)* } + fn optional_access(state: &Self::State, _access: &mut Access, _parent_is_optional: bool) { + let ($($name,)*) = state; + $($name::optional_access($name, _access, _parent_is_optional);)* + } + fn init_state(_world: &mut World) -> Self::State { ($($name::init_state(_world),)*) @@ -1663,6 +1749,11 @@ macro_rules! impl_anytuple_fetch { )* } + fn optional_access(state: &Self::State, _access: &mut Access, _parent_is_optional: bool) { + let ($($name,)*) = state; + $($name::optional_access($name, _access, true);)* + } + fn init_state(_world: &mut World) -> Self::State { ($($name::init_state(_world),)*) } @@ -1743,6 +1834,13 @@ unsafe impl WorldQuery for NopWorldQuery { ) { } + fn optional_access( + _state: &Self::State, + _access: &mut Access, + _parent_is_optional: bool, + ) { + } + fn init_state(world: &mut World) -> Self::State { Q::init_state(world) } @@ -1814,6 +1912,13 @@ unsafe impl WorldQuery for PhantomData { ) { } + fn optional_access( + _state: &Self::State, + _access: &mut Access, + _parent_is_optional: bool, + ) { + } + fn init_state(_world: &mut World) -> Self::State {} fn new_state(_components: &Components) -> Option { diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index dad8e375d7e17..b9b4593dedccb 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -103,6 +103,13 @@ unsafe impl WorldQuery for With { ) { } + fn optional_access( + _state: &Self::State, + _access: &mut Access, + _parent_is_optional: bool, + ) { + } + fn init_state(world: &mut World) -> ComponentId { world.init_component::() } @@ -210,6 +217,14 @@ unsafe impl WorldQuery for Without { ) { } + #[inline] + fn optional_access( + _state: &Self::State, + _access: &mut Access, + _parent_is_optional: bool, + ) { + } + fn init_state(world: &mut World) -> ComponentId { world.init_component::() } @@ -380,6 +395,11 @@ macro_rules! impl_query_filter_tuple { $($filter::update_archetype_component_access($filter, archetype, access);)* } + fn optional_access(state: &Self::State, access: &mut Access, parent_is_optional: bool) { + let ($($filter,)*) = state; + $($filter::optional_access($filter, access, true);)* + } + fn init_state(world: &mut World) -> Self::State { ($($filter::init_state(world),)*) } @@ -549,6 +569,16 @@ macro_rules! impl_tick_filter { } } + fn optional_access( + &id: &ComponentId, + access: &mut Access, + parent_is_optional: bool, + ) { + if parent_is_optional { + access.add_read(id); + } + } + fn init_state(world: &mut World) -> ComponentId { world.init_component::() } diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index dcc9068ecb071..0a3d485af5c4e 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -152,13 +152,6 @@ impl QueryState { if !Q::IS_ARCHETYPAL || !F::IS_ARCHETYPAL || !NewQ::IS_ARCHETYPAL { panic!("`restrict_fetch` is not allowed with queries that use `Changed` or `Added`"); } - // TODO: Figure out how to relax this restriction. This is overly restrictive as Option<&T> -> Option<&T> should be allowed. - // However the information needed to understand that is not available in this step. i.e. We try to fetch data that an archetype doesn't have. - // It is available during fetch, but that would introduce branching in release builds. - // This is undesirable as fetching is in the hot path for query iteration. - if !Q::IS_EXACT { - panic!("`restrict_fetch` from types that return archetypes that do not include the underlying data type like `Option<&T>` are not allowed.") - } let fetch_state = NewQ::new_state(components).expect( "Could not create fetch_state. This should not be reachable as the components should have been \ initialized when creating the original QueryState.", @@ -176,6 +169,15 @@ impl QueryState { let mut filter_component_access = FilteredAccess::default(); <()>::update_component_access(&filter_state, &mut filter_component_access); + // check that the transmute doesn't go from a + let mut original_optional_access = Access::default(); + Q::optional_access(&self.fetch_state, &mut original_optional_access, false); + let mut new_optional_access = Access::default(); + NewQ::optional_access(&fetch_state, &mut new_optional_access, false); + if new_optional_access.difference_is_empty(&original_optional_access) { + panic!("`tranmute` does not allow going from a broader query to a more narrow one. i.e. from Option<&T> -> &T"); + } + // Merge the temporary filter access with the main access. This ensures that filter access is // properly considered in a global "cross-query" context (both within systems and across systems). component_access.extend(&filter_component_access); From f4e455c6b590dbaf98d2ce545582cd582a6f0b67 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 9 Oct 2023 14:38:36 -0700 Subject: [PATCH 15/27] add some more tests --- crates/bevy_ecs/src/query/access.rs | 5 +- crates/bevy_ecs/src/query/state.rs | 91 ++++++++++++++++++++++++----- 2 files changed, 82 insertions(+), 14 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 3878fe8fdc8a2..0fe85cde25916 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -270,10 +270,13 @@ impl Access { self.writes.ones().map(T::get_sparse_set_index) } - /// pub fn difference_is_empty(&self, other: &Access) -> bool { self.reads_and_writes.difference(&other.reads_and_writes).count() == 0 } + + pub fn takes_no_access(&self) -> bool { + self.reads_and_writes.is_empty() + } } /// An [`Access`] that has been filtered to include and exclude certain combinations of elements. diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 0a3d485af5c4e..20d2e395b03ea 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -169,13 +169,15 @@ impl QueryState { let mut filter_component_access = FilteredAccess::default(); <()>::update_component_access(&filter_state, &mut filter_component_access); - // check that the transmute doesn't go from a + // if we have optional parameters, make sure we don't go from a broader query to a more narrow one let mut original_optional_access = Access::default(); Q::optional_access(&self.fetch_state, &mut original_optional_access, false); - let mut new_optional_access = Access::default(); - NewQ::optional_access(&fetch_state, &mut new_optional_access, false); - if new_optional_access.difference_is_empty(&original_optional_access) { - panic!("`tranmute` does not allow going from a broader query to a more narrow one. i.e. from Option<&T> -> &T"); + if !original_optional_access.takes_no_access() { + let mut new_optional_access = Access::default(); + NewQ::optional_access(&fetch_state, &mut new_optional_access, false); + if new_optional_access.difference_is_empty(&original_optional_access) { + panic!("`tranmute` does not allow going from a broader query to a more narrow one. i.e. from Option<&T> -> &T"); + } } // Merge the temporary filter access with the main access. This ensures that filter access is @@ -1599,7 +1601,7 @@ mod tests { let _panics = query_state.get_many_mut(&mut world_2, []); } - mod restrict_fetch { + mod transmute_fetch { use super::*; #[derive(Component)] @@ -1612,7 +1614,7 @@ mod tests { struct C(pub usize); #[test] - fn can_restrict() { + fn can_transmute_to_more_general() { let mut world = World::new(); world.spawn((A(22), B)); @@ -1625,7 +1627,7 @@ mod tests { } #[test] - fn cannot_get_unmatched_data() { + fn cannot_get_data_not_in_original_query() { let mut world = World::new(); world.spawn((A(22), B)); world.spawn((A(23), B, C(5))); @@ -1638,9 +1640,53 @@ mod tests { assert_eq!(a.0, 22); } + + #[test] + fn can_transmute_empty_tuple() { + let mut world = World::new(); + world.init_component::(); + let entity = world.spawn(A(10)).id(); + + let q = world.query::<()>(); + let mut q = q.restrict_fetch::(world.components()); + assert_eq!(q.single(&world), entity); + } + + #[test] + fn can_transmute_immut_fetch() { + let mut world = World::new(); + world.spawn(A(10)); + + let q = world.query::<&A>(); + let mut new_q = q.restrict_fetch::>(world.components()); + assert!(new_q.single(&world).is_added()); + + let q = world.query::>(); + let _ = q.restrict_fetch::<&A>(world.components()); + } + + #[test] + fn can_transmute_mut_fetch() { + let mut world = World::new(); + world.spawn(A(10)); + + let q = world.query::<&mut A>(); + let _ = q.restrict_fetch::>(world.components()); + let _ = q.restrict_fetch::<&A>(world.components()); + } + + #[test] + fn can_transmute_entity_mut() { + let mut world = World::new(); + + let q = world.query::(); + let _ = q.restrict_fetch::(world.components()); + } + + #[test] #[should_panic(expected = "access is not compatible with new query type")] - fn not_included_components_not_allowed() { + fn cannot_transmute_to_include_data_not_in_original_query() { let mut world = World::new(); world.init_component::(); world.init_component::(); @@ -1652,7 +1698,7 @@ mod tests { #[test] #[should_panic(expected = "`restrict_fetch` is not allowed with queries that use `Changed` or `Added`")] - fn non_archtypal_not_allowed() { + fn cannot_transmute_non_archtypal_queries() { let mut world = World::new(); world.spawn(A(22)); @@ -1662,7 +1708,7 @@ mod tests { #[test] #[should_panic(expected = "access is not compatible with new query type")] - fn immut_to_mut_not_allowed() { + fn cannot_transmute_immut_to_mut() { let mut world = World::new(); world.spawn(A(22)); @@ -1671,8 +1717,8 @@ mod tests { } #[test] - #[should_panic(expected = "`restrict_fetch` from types that return archetypes")] - fn option_to_immut_not_allowed() { + #[should_panic(expected = "`tranmute` does not allow going from a broader query to a more narrow one.")] + fn cannot_transmute_option_to_immut() { let mut world = World::new(); world.spawn(C(22)); @@ -1681,5 +1727,24 @@ mod tests { let x = new_query_state.single(&world); assert_eq!(x.0, 1234); } + + #[test] + #[should_panic(expected = "access is not compatible with new query type")] + fn cannot_transmute_entity_ref_to_wrong_data() { + let mut world = World::new(); + world.init_component::(); + + let q = world.query_filtered::>(); + let _ = q.restrict_fetch::<&B>(world.components()); + } + + #[test] + #[should_panic(expected = "access is not compatible with new query type")] + fn cannot_transmute_entity_ref_with_or_filter() { + let mut world = World::new(); + + let q = world.query_filtered::, With)>>(); + let _ = q.restrict_fetch::<&A>(world.components()); + } } } From 216d90eb660f7abcd0651237906d5decba889a57 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 9 Oct 2023 14:49:20 -0700 Subject: [PATCH 16/27] remove IS_EXACT --- crates/bevy_ecs/macros/src/fetch.rs | 4 +--- crates/bevy_ecs/src/query/access.rs | 5 ++++- crates/bevy_ecs/src/query/fetch.rs | 27 --------------------------- crates/bevy_ecs/src/query/filter.rs | 8 -------- crates/bevy_ecs/src/query/state.rs | 12 +++++++----- 5 files changed, 12 insertions(+), 44 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 296bd984338b3..bef7c1a1a1fde 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -289,8 +289,6 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { const IS_ARCHETYPAL: bool = true #(&& <#field_types>::IS_ARCHETYPAL)*; - const IS_EXACT: bool = true #(&& <#field_types>::IS_EXACT)*; - /// SAFETY: we call `set_archetype` for each member that implements `Fetch` #[inline] unsafe fn set_archetype<'__w>( @@ -349,7 +347,7 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { } fn optional_access( - state: &Self::State, + state: &Self::State, _access: &mut #path::query::Access<#path::component::ComponentId>, parent_is_optional: bool, ) { diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 0fe85cde25916..7a48438fea232 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -271,7 +271,10 @@ impl Access { } pub fn difference_is_empty(&self, other: &Access) -> bool { - self.reads_and_writes.difference(&other.reads_and_writes).count() == 0 + self.reads_and_writes + .difference(&other.reads_and_writes) + .count() + == 0 } pub fn takes_no_access(&self) -> bool { diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index d83b6b3253030..a546ed53460e0 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -360,10 +360,6 @@ pub unsafe trait WorldQuery { /// many elements are being iterated (such as `Iterator::collect()`). const IS_ARCHETYPAL: bool; - /// Returns true if (and only if) every archetype returned has data for the underlying types. - /// i.e. [`Self::State`] = Option<&T> would return false, but &T would return true. - const IS_EXACT: bool; - /// Adjusts internal state to account for the next [`Archetype`]. This will always be called on /// archetypes that match this [`WorldQuery`]. /// @@ -491,8 +487,6 @@ unsafe impl WorldQuery for Entity { const IS_ARCHETYPAL: bool = true; - const IS_EXACT: bool = true; - unsafe fn init_fetch<'w>( _world: UnsafeWorldCell<'w>, _state: &Self::State, @@ -571,8 +565,6 @@ unsafe impl WorldQuery for EntityRef<'_> { const IS_ARCHETYPAL: bool = true; - const IS_EXACT: bool = true; - unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, _state: &Self::State, @@ -667,8 +659,6 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> { const IS_ARCHETYPAL: bool = true; - const IS_EXACT: bool = true; - unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, _state: &Self::State, @@ -780,8 +770,6 @@ unsafe impl WorldQuery for &T { const IS_ARCHETYPAL: bool = true; - const IS_EXACT: bool = true; - #[inline] unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, @@ -946,8 +934,6 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { const IS_ARCHETYPAL: bool = true; - const IS_EXACT: bool = true; - #[inline] unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, @@ -1123,8 +1109,6 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { const IS_ARCHETYPAL: bool = true; - const IS_EXACT: bool = true; - #[inline] unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, @@ -1285,8 +1269,6 @@ unsafe impl WorldQuery for Option { const IS_ARCHETYPAL: bool = T::IS_ARCHETYPAL; - const IS_EXACT: bool = false; - #[inline] unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, @@ -1459,8 +1441,6 @@ unsafe impl WorldQuery for Has { const IS_ARCHETYPAL: bool = true; - const IS_EXACT: bool = false; - #[inline] unsafe fn init_fetch<'w>( _world: UnsafeWorldCell<'w>, @@ -1562,8 +1542,6 @@ macro_rules! impl_tuple_fetch { const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*; - const IS_EXACT: bool = true $(&& $name::IS_EXACT)*; - #[inline] unsafe fn set_archetype<'w>( _fetch: &mut Self::Fetch<'w>, @@ -1676,8 +1654,6 @@ macro_rules! impl_anytuple_fetch { const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*; - const IS_EXACT: bool = false; - #[inline] unsafe fn set_archetype<'w>( _fetch: &mut Self::Fetch<'w>, @@ -1794,8 +1770,6 @@ unsafe impl WorldQuery for NopWorldQuery { const IS_ARCHETYPAL: bool = true; - const IS_EXACT: bool = Q::IS_EXACT; - #[inline(always)] unsafe fn init_fetch( _world: UnsafeWorldCell, @@ -1883,7 +1857,6 @@ unsafe impl WorldQuery for PhantomData { // `PhantomData` matches every entity in each archetype. const IS_ARCHETYPAL: bool = true; // `PhantomData` does not access any data, so it is exact. - const IS_EXACT: bool = true; unsafe fn set_archetype<'w>( _fetch: &mut Self::Fetch<'w>, diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index b9b4593dedccb..71ed3ed2fb6f6 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -68,8 +68,6 @@ unsafe impl WorldQuery for With { const IS_ARCHETYPAL: bool = true; - const IS_EXACT: bool = false; - #[inline] unsafe fn set_table(_fetch: &mut (), _state: &ComponentId, _table: &Table) {} @@ -182,8 +180,6 @@ unsafe impl WorldQuery for Without { const IS_ARCHETYPAL: bool = true; - const IS_EXACT: bool = false; - #[inline] unsafe fn set_table(_fetch: &mut (), _state: &Self::State, _table: &Table) {} @@ -311,8 +307,6 @@ macro_rules! impl_query_filter_tuple { const IS_ARCHETYPAL: bool = true $(&& $filter::IS_ARCHETYPAL)*; - const IS_EXACT: bool = false; - #[inline] unsafe fn init_fetch<'w>(world: UnsafeWorldCell<'w>, state: &Self::State, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> { let ($($filter,)*) = state; @@ -484,8 +478,6 @@ macro_rules! impl_tick_filter { const IS_ARCHETYPAL: bool = false; - const IS_EXACT: bool = true; - #[inline] unsafe fn set_table<'w>( fetch: &mut Self::Fetch<'w>, diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 20d2e395b03ea..141af00ca692d 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1640,7 +1640,6 @@ mod tests { assert_eq!(a.0, 22); } - #[test] fn can_transmute_empty_tuple() { let mut world = World::new(); @@ -1683,7 +1682,6 @@ mod tests { let _ = q.restrict_fetch::(world.components()); } - #[test] #[should_panic(expected = "access is not compatible with new query type")] fn cannot_transmute_to_include_data_not_in_original_query() { @@ -1697,7 +1695,9 @@ mod tests { } #[test] - #[should_panic(expected = "`restrict_fetch` is not allowed with queries that use `Changed` or `Added`")] + #[should_panic( + expected = "`restrict_fetch` is not allowed with queries that use `Changed` or `Added`" + )] fn cannot_transmute_non_archtypal_queries() { let mut world = World::new(); world.spawn(A(22)); @@ -1717,7 +1717,9 @@ mod tests { } #[test] - #[should_panic(expected = "`tranmute` does not allow going from a broader query to a more narrow one.")] + #[should_panic( + expected = "`tranmute` does not allow going from a broader query to a more narrow one." + )] fn cannot_transmute_option_to_immut() { let mut world = World::new(); world.spawn(C(22)); @@ -1742,7 +1744,7 @@ mod tests { #[should_panic(expected = "access is not compatible with new query type")] fn cannot_transmute_entity_ref_with_or_filter() { let mut world = World::new(); - + let q = world.query_filtered::, With)>>(); let _ = q.restrict_fetch::<&A>(world.components()); } From 6f2e9c01b82fda3ab54f8333e7a1827036df751e Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 9 Oct 2023 16:51:50 -0700 Subject: [PATCH 17/27] add some tests and fix some bugs --- crates/bevy_ecs/src/query/access.rs | 11 +++--- crates/bevy_ecs/src/query/fetch.rs | 15 ++++---- crates/bevy_ecs/src/query/state.rs | 55 ++++++++++++++--------------- 3 files changed, 40 insertions(+), 41 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 7a48438fea232..62c4c9cd06890 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -213,7 +213,7 @@ impl Access { } if other.reads_all { - return self.has_any_write(); + return !self.has_any_write(); } let reads = self @@ -270,15 +270,18 @@ impl Access { self.writes.ones().map(T::get_sparse_set_index) } - pub fn difference_is_empty(&self, other: &Access) -> bool { + pub fn read_and_writes_difference_is_empty(&self, other: &Access) -> bool { + if self.reads_all || self.writes_all { + return other.reads_all || other.writes_all; + } self.reads_and_writes .difference(&other.reads_and_writes) .count() == 0 } - pub fn takes_no_access(&self) -> bool { - self.reads_and_writes.is_empty() + pub fn has_access(&self) -> bool { + self.writes_all || self.reads_all || !self.reads_and_writes.is_clear() } } diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index a546ed53460e0..f13b878bb0e30 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -620,11 +620,10 @@ unsafe impl WorldQuery for EntityRef<'_> { fn optional_access( _state: &Self::State, access: &mut Access, - parent_is_optional: bool, + _parent_is_optional: bool, ) { - if parent_is_optional { - access.read_all() - } + // it's ambiguous what data an entity ref can get + access.read_all() } fn init_state(_world: &mut World) {} @@ -714,11 +713,11 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> { fn optional_access( _state: &Self::State, access: &mut Access, - parent_is_optional: bool, + _parent_is_optional: bool, ) { - if parent_is_optional { - access.write_all(); - } + // Entity Ref may or may not have data for a component T + access.write_all(); + } fn init_state(_world: &mut World) {} diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 141af00ca692d..e6bf4f115df40 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -153,14 +153,12 @@ impl QueryState { panic!("`restrict_fetch` is not allowed with queries that use `Changed` or `Added`"); } let fetch_state = NewQ::new_state(components).expect( - "Could not create fetch_state. This should not be reachable as the components should have been \ - initialized when creating the original QueryState.", + "Could not create fetch_state. Please initialize any components needed before trying to `transmute`", ); #[allow(clippy::let_unit_value)] // the archetypal filters have already been applied, so we don't need them. let filter_state = <()>::new_state(components).expect( - "Could not create filter_state. This should not be reachable as the components should have been \ - initialized when creating the original QueryState.", + "Could not create filter_state. Please initialize any components needed before trying to `transmute`", ); let mut component_access = FilteredAccess::default(); @@ -169,25 +167,23 @@ impl QueryState { let mut filter_component_access = FilteredAccess::default(); <()>::update_component_access(&filter_state, &mut filter_component_access); + component_access.extend(&filter_component_access); + + if !component_access.is_subset(&self.component_access) { + panic!("access is not compatible with new query type"); + } + // if we have optional parameters, make sure we don't go from a broader query to a more narrow one let mut original_optional_access = Access::default(); Q::optional_access(&self.fetch_state, &mut original_optional_access, false); - if !original_optional_access.takes_no_access() { + if original_optional_access.has_access() { let mut new_optional_access = Access::default(); NewQ::optional_access(&fetch_state, &mut new_optional_access, false); - if new_optional_access.difference_is_empty(&original_optional_access) { - panic!("`tranmute` does not allow going from a broader query to a more narrow one. i.e. from Option<&T> -> &T"); + if !original_optional_access.read_and_writes_difference_is_empty(&new_optional_access) { + panic!("`transmute` does not allow going from a broader query to a more narrow one. i.e. from Option<&T> -> &T"); } } - // Merge the temporary filter access with the main access. This ensures that filter access is - // properly considered in a global "cross-query" context (both within systems and across systems). - component_access.extend(&filter_component_access); - - if !component_access.is_subset(&self.component_access) { - panic!("access is not compatible with new query type"); - } - QueryState { world_id: self.world_id, archetype_generation: self.archetype_generation, @@ -1677,11 +1673,21 @@ mod tests { #[test] fn can_transmute_entity_mut() { let mut world = World::new(); + world.spawn(A(10)); let q = world.query::(); let _ = q.restrict_fetch::(world.components()); } + #[test] + fn can_generalize_option() { + let mut world = World::new(); + world.spawn((A(22), B)); + + let query_state = world.query::<(Option<&A>, &B)>(); + let _ = query_state.restrict_fetch::>(world.components()); + } + #[test] #[should_panic(expected = "access is not compatible with new query type")] fn cannot_transmute_to_include_data_not_in_original_query() { @@ -1718,7 +1724,7 @@ mod tests { #[test] #[should_panic( - expected = "`tranmute` does not allow going from a broader query to a more narrow one." + expected = "`transmute` does not allow going from a broader query to a more narrow one." )] fn cannot_transmute_option_to_immut() { let mut world = World::new(); @@ -1731,21 +1737,12 @@ mod tests { } #[test] - #[should_panic(expected = "access is not compatible with new query type")] - fn cannot_transmute_entity_ref_to_wrong_data() { - let mut world = World::new(); - world.init_component::(); - - let q = world.query_filtered::>(); - let _ = q.restrict_fetch::<&B>(world.components()); - } - - #[test] - #[should_panic(expected = "access is not compatible with new query type")] - fn cannot_transmute_entity_ref_with_or_filter() { + #[should_panic(expected = "`transmute` does not allow going from a broader query to a more narrow one.")] + fn cannot_transmute_entity_ref() { let mut world = World::new(); + world.init_component::(); - let q = world.query_filtered::, With)>>(); + let q = world.query::(); let _ = q.restrict_fetch::<&A>(world.components()); } } From 227dc00dd1502880b99e6404915a08b1f91ec1a4 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 9 Oct 2023 17:48:49 -0700 Subject: [PATCH 18/27] document new access methods --- crates/bevy_ecs/src/query/access.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 62c4c9cd06890..cc16da551bdbf 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -270,6 +270,7 @@ impl Access { self.writes.ones().map(T::get_sparse_set_index) } + /// checks if the difference of `self` with `other` is empty pub fn read_and_writes_difference_is_empty(&self, other: &Access) -> bool { if self.reads_all || self.writes_all { return other.reads_all || other.writes_all; @@ -280,6 +281,7 @@ impl Access { == 0 } + /// checks if there is any access set pub fn has_access(&self) -> bool { self.writes_all || self.reads_all || !self.reads_and_writes.is_clear() } From 62284915fd77c0407b69ca9af0e0806a449cc7e1 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 9 Oct 2023 18:44:57 -0700 Subject: [PATCH 19/27] restrict_fetch -> transmute_fetch --- crates/bevy_ecs/src/query/fetch.rs | 3 +-- crates/bevy_ecs/src/query/state.rs | 38 +++++++++++++++-------------- crates/bevy_ecs/src/system/query.rs | 8 +++--- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index f13b878bb0e30..b9160fc7c9416 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -439,7 +439,7 @@ pub unsafe trait WorldQuery { /// Adds component access for when [`WorldQuery`] is not an exact match. /// i.e. Option<&T> where the matched archetypes don't necessarily contain - /// the data the WorldQuery takes access to in [`update_component_access`] + /// the data the WorldQuery takes access to in [`Self::update_component_access`] fn optional_access( state: &Self::State, access: &mut Access, @@ -717,7 +717,6 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> { ) { // Entity Ref may or may not have data for a component T access.write_all(); - } fn init_state(_world: &mut World) {} diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index e6bf4f115df40..eae80bb056e8d 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -140,12 +140,12 @@ impl QueryState { /// This is used to transform a [`Query`](crate::system::Query) into a more generic [`Query`](crate::system::Query). /// This can be useful for passsing to another function tha might take the more generalize query. - /// See [`Query::restrict_fetch`](crate::system::Query::restrict_fetch) for more details. + /// See [`Query::transmute_fetch`](crate::system::Query::transmute_fetch) for more details. /// - /// You should not call `update_archetypes` on the returned [`QueryState`] as the result will be unpredictable. + /// You should not call [`update_archetypes`](Self::update_archetypes) on the returned [`QueryState`] as the result will be unpredictable. /// You might end up with a mix of archetypes that only matched the original query + archetypes that only match /// the new [`QueryState`]. Most of the safe methods on [`QueryState`] call [`QueryState::update_archetypes`] internally. - pub(crate) fn restrict_fetch( + pub(crate) fn transmute_fetch( &self, components: &Components, ) -> QueryState { @@ -1615,7 +1615,7 @@ mod tests { world.spawn((A(22), B)); let query_state = world.query::<(&A, &B)>(); - let mut new_query_state = query_state.restrict_fetch::<&A>(world.components()); + let mut new_query_state = query_state.transmute_fetch::<&A>(world.components()); assert_eq!(new_query_state.iter(&world).len(), 1); let a = new_query_state.single(&world); @@ -1629,7 +1629,7 @@ mod tests { world.spawn((A(23), B, C(5))); let query_state = world.query_filtered::<(&A, &B), Without>(); - let mut new_query_state = query_state.restrict_fetch::<&A>(world.components()); + let mut new_query_state = query_state.transmute_fetch::<&A>(world.components()); // even though we change the query to not have Without, we do not get the component with C. let a = new_query_state.single(&world); @@ -1643,7 +1643,7 @@ mod tests { let entity = world.spawn(A(10)).id(); let q = world.query::<()>(); - let mut q = q.restrict_fetch::(world.components()); + let mut q = q.transmute_fetch::(world.components()); assert_eq!(q.single(&world), entity); } @@ -1653,11 +1653,11 @@ mod tests { world.spawn(A(10)); let q = world.query::<&A>(); - let mut new_q = q.restrict_fetch::>(world.components()); + let mut new_q = q.transmute_fetch::>(world.components()); assert!(new_q.single(&world).is_added()); let q = world.query::>(); - let _ = q.restrict_fetch::<&A>(world.components()); + let _ = q.transmute_fetch::<&A>(world.components()); } #[test] @@ -1666,8 +1666,8 @@ mod tests { world.spawn(A(10)); let q = world.query::<&mut A>(); - let _ = q.restrict_fetch::>(world.components()); - let _ = q.restrict_fetch::<&A>(world.components()); + let _ = q.transmute_fetch::>(world.components()); + let _ = q.transmute_fetch::<&A>(world.components()); } #[test] @@ -1676,7 +1676,7 @@ mod tests { world.spawn(A(10)); let q = world.query::(); - let _ = q.restrict_fetch::(world.components()); + let _ = q.transmute_fetch::(world.components()); } #[test] @@ -1685,7 +1685,7 @@ mod tests { world.spawn((A(22), B)); let query_state = world.query::<(Option<&A>, &B)>(); - let _ = query_state.restrict_fetch::>(world.components()); + let _ = query_state.transmute_fetch::>(world.components()); } #[test] @@ -1697,7 +1697,7 @@ mod tests { world.spawn(A(22)); let query_state = world.query::<&A>(); - let mut _new_query_state = query_state.restrict_fetch::<(&A, &B)>(world.components()); + let mut _new_query_state = query_state.transmute_fetch::<(&A, &B)>(world.components()); } #[test] @@ -1709,7 +1709,7 @@ mod tests { world.spawn(A(22)); let query_state = world.query_filtered::<(&A, &B), Added>(); - let mut _new_query_state = query_state.restrict_fetch::<&A>(world.components()); + let mut _new_query_state = query_state.transmute_fetch::<&A>(world.components()); } #[test] @@ -1719,7 +1719,7 @@ mod tests { world.spawn(A(22)); let query_state = world.query::<&A>(); - let mut _new_query_state = query_state.restrict_fetch::<&mut A>(world.components()); + let mut _new_query_state = query_state.transmute_fetch::<&mut A>(world.components()); } #[test] @@ -1731,19 +1731,21 @@ mod tests { world.spawn(C(22)); let query_state = world.query::>(); - let mut new_query_state = query_state.restrict_fetch::<&A>(world.components()); + let mut new_query_state = query_state.transmute_fetch::<&A>(world.components()); let x = new_query_state.single(&world); assert_eq!(x.0, 1234); } #[test] - #[should_panic(expected = "`transmute` does not allow going from a broader query to a more narrow one.")] + #[should_panic( + expected = "`transmute` does not allow going from a broader query to a more narrow one." + )] fn cannot_transmute_entity_ref() { let mut world = World::new(); world.init_component::(); let q = world.query::(); - let _ = q.restrict_fetch::<&A>(world.components()); + let _ = q.transmute_fetch::<&A>(world.components()); } } } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index ef36581177fdf..ca97f4e9f3a52 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -433,7 +433,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// } /// /// fn system_2(mut query: Query<(&mut A, &B)>) { - /// let mut lens = query.restrict_fetch::<&A>(); + /// let mut lens = query.transmute_fetch::<&A>(); /// function_that_uses_a_query_lens(&mut lens); /// } /// @@ -441,8 +441,8 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// # schedule.add_systems((system_1, system_2)); /// # schedule.run(&mut world); /// ``` - pub fn restrict_fetch(&mut self) -> QueryLens<'_, NewQ> { - let new_state = self.state.restrict_fetch(self.world.components()); + pub fn transmute_fetch(&mut self) -> QueryLens<'_, NewQ> { + let new_state = self.state.transmute_fetch(self.world.components()); QueryLens { state: new_state, @@ -454,7 +454,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// helper method to get a [`QueryLens`] with the same fetch as the existing query pub fn into_query_lens(&mut self) -> QueryLens<'_, Q> { - self.restrict_fetch() + self.transmute_fetch() } /// Returns an [`Iterator`] over the read-only query items. From b1b2f6c84be3dd30fe88e00a92e45afeeb0a0350 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 9 Oct 2023 20:28:55 -0700 Subject: [PATCH 20/27] use existing method --- crates/bevy_ecs/src/query/access.rs | 7 +------ crates/bevy_ecs/src/query/state.rs | 2 +- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index cc16da551bdbf..aa7a9c53e776e 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -270,7 +270,7 @@ impl Access { self.writes.ones().map(T::get_sparse_set_index) } - /// checks if the difference of `self` with `other` is empty + /// Returns true if the difference of `self` with `other` is empty pub fn read_and_writes_difference_is_empty(&self, other: &Access) -> bool { if self.reads_all || self.writes_all { return other.reads_all || other.writes_all; @@ -280,11 +280,6 @@ impl Access { .count() == 0 } - - /// checks if there is any access set - pub fn has_access(&self) -> bool { - self.writes_all || self.reads_all || !self.reads_and_writes.is_clear() - } } /// An [`Access`] that has been filtered to include and exclude certain combinations of elements. diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index eae80bb056e8d..d3e7910e10dbc 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -176,7 +176,7 @@ impl QueryState { // if we have optional parameters, make sure we don't go from a broader query to a more narrow one let mut original_optional_access = Access::default(); Q::optional_access(&self.fetch_state, &mut original_optional_access, false); - if original_optional_access.has_access() { + if original_optional_access.has_any_read() { let mut new_optional_access = Access::default(); NewQ::optional_access(&fetch_state, &mut new_optional_access, false); if !original_optional_access.read_and_writes_difference_is_empty(&new_optional_access) { From e7271c86af3af8054ab9ce59888aca1df4f8d27c Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 9 Oct 2023 20:56:58 -0700 Subject: [PATCH 21/27] add compile fail tests --- .../tests/ui/query_transmute_safety.rs | 39 +++++++++++++++++++ .../tests/ui/query_transmute_safety.stderr | 21 ++++++++++ 2 files changed, 60 insertions(+) create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/query_transmute_safety.rs create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/query_transmute_safety.stderr diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_transmute_safety.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_transmute_safety.rs new file mode 100644 index 0000000000000..071570a06cab2 --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_transmute_safety.rs @@ -0,0 +1,39 @@ +use bevy_ecs::prelude::*; +use bevy_ecs::system::SystemState; + +#[derive(Component, Eq, PartialEq, Debug)] +struct Foo(u32); + +#[derive(Component)] +struct Bar; + +fn main() { + let mut world = World::default(); + world.spawn(Foo(10)); + + let mut system_state = SystemState::>::new(&mut world); + let mut query = system_state.get_mut(&mut world); + + { + let mut lens1 = query.transmute_fetch::<&mut Foo>(); + let mut lens2 = query.transmute_fetch::<&mut Foo>(); + + let mut query1 = lens1.query(); + let mut query2 = lens2.query(); + + let f1 = query1.single_mut(); + let f2 = query2.single_mut(); // oops 2 mutable references to same Foo + assert_eq!(*f1, *f2); + } + + { + let mut lens = query.transmute_fetch::<&mut Foo>(); + + let mut query1 = lens.query(); + let mut query2 = lens.query(); + + let f1 = query1.single_mut(); + let f2 = query2.single_mut(); // oops 2 mutable references to same Foo + assert_eq!(*f1, *f2); + } +} diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_transmute_safety.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_transmute_safety.stderr new file mode 100644 index 0000000000000..cc5890b41d4ee --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_transmute_safety.stderr @@ -0,0 +1,21 @@ +error[E0499]: cannot borrow `query` as mutable more than once at a time + --> tests/ui/query_transmute_safety.rs:19:25 + | +18 | let mut lens1 = query.transmute_fetch::<&mut Foo>(); + | ----- first mutable borrow occurs here +19 | let mut lens2 = query.transmute_fetch::<&mut Foo>(); + | ^^^^^ second mutable borrow occurs here +20 | +21 | let mut query1 = lens1.query(); + | ----- first borrow later used here + +error[E0499]: cannot borrow `lens` as mutable more than once at a time + --> tests/ui/query_transmute_safety.rs:33:26 + | +32 | let mut query1 = lens.query(); + | ---- first mutable borrow occurs here +33 | let mut query2 = lens.query(); + | ^^^^ second mutable borrow occurs here +34 | +35 | let f1 = query1.single_mut(); + | ------ first borrow later used here From e4b408bf134cf161d413f1b2a1501c754e3a080b Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 9 Oct 2023 21:38:37 -0700 Subject: [PATCH 22/27] clippy --- crates/bevy_ecs/src/query/fetch.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index b9160fc7c9416..3d4288345328f 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -439,7 +439,7 @@ pub unsafe trait WorldQuery { /// Adds component access for when [`WorldQuery`] is not an exact match. /// i.e. Option<&T> where the matched archetypes don't necessarily contain - /// the data the WorldQuery takes access to in [`Self::update_component_access`] + /// the data the [`WorldQuery`] takes access to in [`Self::update_component_access`] fn optional_access( state: &Self::State, access: &mut Access, @@ -623,7 +623,7 @@ unsafe impl WorldQuery for EntityRef<'_> { _parent_is_optional: bool, ) { // it's ambiguous what data an entity ref can get - access.read_all() + access.read_all(); } fn init_state(_world: &mut World) {} From 47ce67726d8935325c324a3c2387e61292198f14 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 9 Oct 2023 21:55:01 -0700 Subject: [PATCH 23/27] add a failing option test --- crates/bevy_ecs/src/query/state.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index d3e7910e10dbc..6cba8893eb6af 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1680,12 +1680,13 @@ mod tests { } #[test] - fn can_generalize_option() { + fn can_generalize_with_option() { let mut world = World::new(); world.spawn((A(22), B)); let query_state = world.query::<(Option<&A>, &B)>(); let _ = query_state.transmute_fetch::>(world.components()); + let _ = query_state.transmute_fetch::<&B>(world.components()); } #[test] From 9d3e5266eb829fca1642ae6f39c2074931520094 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Tue, 10 Oct 2023 15:55:32 -0700 Subject: [PATCH 24/27] get new test passing --- crates/bevy_ecs/src/query/access.rs | 63 +++++++++++++++++++++++++++++ crates/bevy_ecs/src/query/state.rs | 14 +++---- 2 files changed, 69 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index aa7a9c53e776e..3a9bb7e5b4270 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -224,6 +224,53 @@ impl Access { reads.is_subset(&other.reads_and_writes) && self.writes.is_subset(&other.writes) } + /// modifies self with the intersection with `other` + pub fn intersect(&mut self, other: &Access) { + if self.writes_all { + if other.writes_all { + // we intersect everything so we're done. + return; + } + + self.writes_all = false; + + if other.reads_all { + self.reads_all = true; + return; + } + + self.reads_all = false; + + self.reads_and_writes = other.reads_and_writes.clone(); + self.writes = other.writes.clone(); + return; + } + + if self.reads_all { + if other.writes_all || other.reads_all { + return; + } + + self.reads_and_writes = other.reads_and_writes.clone(); + self.writes.clear(); + return; + } + + if other.writes_all { + // reads_and_writes and writes are already correct + return; + } + + if other.reads_all { + self.writes.clear(); + return; + } + + self.reads_and_writes + .intersect_with(&other.reads_and_writes); + self.writes.intersect_with(&other.writes); + } + /// 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(); @@ -416,6 +463,22 @@ impl FilteredAccess { self.access.is_subset(&other.access) } + /// returns true if optional access has not changed + pub fn is_optional_compatible( + &self, + mut original_optional: Access, + new_optional: &Access, + ) -> bool { + if original_optional.has_any_read() { + original_optional.intersect(&self.access); + if !original_optional.read_and_writes_difference_is_empty(new_optional) { + return false; + } + } + + true + } + /// 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) { diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 6cba8893eb6af..249ca5dfdd401 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -174,14 +174,12 @@ impl QueryState { } // if we have optional parameters, make sure we don't go from a broader query to a more narrow one - let mut original_optional_access = Access::default(); - Q::optional_access(&self.fetch_state, &mut original_optional_access, false); - if original_optional_access.has_any_read() { - let mut new_optional_access = Access::default(); - NewQ::optional_access(&fetch_state, &mut new_optional_access, false); - if !original_optional_access.read_and_writes_difference_is_empty(&new_optional_access) { - panic!("`transmute` does not allow going from a broader query to a more narrow one. i.e. from Option<&T> -> &T"); - } + let mut original_optional = Access::default(); + Q::optional_access(&self.fetch_state, &mut original_optional, false); + let mut new_optional = Access::default(); + NewQ::optional_access(&fetch_state, &mut new_optional, false); + if !component_access.is_optional_compatible(original_optional, &new_optional) { + panic!("`transmute` does not allow going from a broader query to a more narrow one. i.e. from Option<&T> -> &T"); } QueryState { From 4814f4d9f858d91a9a041766e8e31d7e46ce11f0 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Tue, 10 Oct 2023 15:55:59 -0700 Subject: [PATCH 25/27] add track caller --- crates/bevy_ecs/src/query/state.rs | 1 + crates/bevy_ecs/src/system/query.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 249ca5dfdd401..f3acfa2968175 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -145,6 +145,7 @@ impl QueryState { /// You should not call [`update_archetypes`](Self::update_archetypes) on the returned [`QueryState`] as the result will be unpredictable. /// You might end up with a mix of archetypes that only matched the original query + archetypes that only match /// the new [`QueryState`]. Most of the safe methods on [`QueryState`] call [`QueryState::update_archetypes`] internally. + #[track_caller] pub(crate) fn transmute_fetch( &self, components: &Components, diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index ca97f4e9f3a52..40a8c4235a4c6 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -441,6 +441,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// # schedule.add_systems((system_1, system_2)); /// # schedule.run(&mut world); /// ``` + #[track_caller] pub fn transmute_fetch(&mut self) -> QueryLens<'_, NewQ> { let new_state = self.state.transmute_fetch(self.world.components()); From 8e0cf6449dc30b815d86ee67d11104b733574748 Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 23 Oct 2023 19:55:39 -0700 Subject: [PATCH 26/27] rename new_state -> get_state --- crates/bevy_ecs/macros/src/fetch.rs | 4 ++-- crates/bevy_ecs/src/query/fetch.rs | 34 ++++++++++++++--------------- crates/bevy_ecs/src/query/filter.rs | 10 ++++----- crates/bevy_ecs/src/query/state.rs | 4 ++-- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index bef7c1a1a1fde..2eb23662c64af 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -360,9 +360,9 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { } } - fn new_state(components: &#path::component::Components) -> Option<#state_struct_name #user_ty_generics> { + fn get_state(components: &#path::component::Components) -> Option<#state_struct_name #user_ty_generics> { Some(#state_struct_name { - #(#named_field_idents: <#field_types>::new_state(components)?,)* + #(#named_field_idents: <#field_types>::get_state(components)?,)* }) } diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 3d4288345328f..42ebb342d175f 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -451,7 +451,7 @@ pub unsafe trait WorldQuery { /// Creates a [`State`](WorldQuery::State) for this [`WorldQuery`] type. This should return /// the same value as [`init_state`](Self::init_state). - fn new_state(components: &Components) -> Option; + fn get_state(components: &Components) -> Option; /// Returns `true` if this query matches a set of components. Otherwise, returns `false`. fn matches_component_set( @@ -535,7 +535,7 @@ unsafe impl WorldQuery for Entity { fn init_state(_world: &mut World) {} - fn new_state(_components: &Components) -> Option<()> { + fn get_state(_components: &Components) -> Option<()> { Some(()) } @@ -628,7 +628,7 @@ unsafe impl WorldQuery for EntityRef<'_> { fn init_state(_world: &mut World) {} - fn new_state(_components: &Components) -> Option<()> { + fn get_state(_components: &Components) -> Option<()> { Some(()) } @@ -721,7 +721,7 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> { fn init_state(_world: &mut World) {} - fn new_state(_components: &Components) -> Option<()> { + fn get_state(_components: &Components) -> Option<()> { Some(()) } @@ -875,7 +875,7 @@ unsafe impl WorldQuery for &T { world.init_component::() } - fn new_state(components: &Components) -> Option { + fn get_state(components: &Components) -> Option { components.component_id::() } @@ -1050,7 +1050,7 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { world.init_component::() } - fn new_state(components: &Components) -> Option { + fn get_state(components: &Components) -> Option { components.component_id::() } @@ -1225,7 +1225,7 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { world.init_component::() } - fn new_state(components: &Components) -> Option { + fn get_state(components: &Components) -> Option { components.component_id::() } @@ -1349,8 +1349,8 @@ unsafe impl WorldQuery for Option { T::init_state(world) } - fn new_state(components: &Components) -> Option { - T::new_state(components) + fn get_state(components: &Components) -> Option { + T::get_state(components) } fn matches_component_set( @@ -1495,7 +1495,7 @@ unsafe impl WorldQuery for Has { world.init_component::() } - fn new_state(components: &Components) -> Option { + fn get_state(components: &Components) -> Option { components.component_id::() } @@ -1600,8 +1600,8 @@ macro_rules! impl_tuple_fetch { ($($name::init_state(_world),)*) } - fn new_state(_components: &Components) -> Option { - Some(($($name::new_state(_components)?,)*)) + fn get_state(_components: &Components) -> Option { + Some(($($name::get_state(_components)?,)*)) } fn matches_component_set(state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { @@ -1732,8 +1732,8 @@ macro_rules! impl_anytuple_fetch { ($($name::init_state(_world),)*) } - fn new_state(_components: &Components) -> Option { - Some(($($name::new_state(_components)?,)*)) + fn get_state(_components: &Components) -> Option { + Some(($($name::get_state(_components)?,)*)) } fn matches_component_set(_state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { @@ -1817,8 +1817,8 @@ unsafe impl WorldQuery for NopWorldQuery { Q::init_state(world) } - fn new_state(components: &Components) -> Option { - Q::new_state(components) + fn get_state(components: &Components) -> Option { + Q::get_state(components) } fn matches_component_set( @@ -1892,7 +1892,7 @@ unsafe impl WorldQuery for PhantomData { fn init_state(_world: &mut World) -> Self::State {} - fn new_state(_components: &Components) -> Option { + fn get_state(_components: &Components) -> Option { Some(()) } diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 71ed3ed2fb6f6..611861e867bb9 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -112,7 +112,7 @@ unsafe impl WorldQuery for With { world.init_component::() } - fn new_state(components: &Components) -> Option { + fn get_state(components: &Components) -> Option { components.component_id::() } @@ -225,7 +225,7 @@ unsafe impl WorldQuery for Without { world.init_component::() } - fn new_state(components: &Components) -> Option { + fn get_state(components: &Components) -> Option { components.component_id::() } @@ -398,8 +398,8 @@ macro_rules! impl_query_filter_tuple { ($($filter::init_state(world),)*) } - fn new_state(components: &Components) -> Option { - Some(($($filter::new_state(components)?,)*)) + fn get_state(components: &Components) -> Option { + Some(($($filter::get_state(components)?,)*)) } fn matches_component_set(_state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { @@ -575,7 +575,7 @@ macro_rules! impl_tick_filter { world.init_component::() } - fn new_state(components: &Components) -> Option { + fn get_state(components: &Components) -> Option { components.component_id::() } diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index f3acfa2968175..7f7bd4c7e36ef 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -153,12 +153,12 @@ impl QueryState { if !Q::IS_ARCHETYPAL || !F::IS_ARCHETYPAL || !NewQ::IS_ARCHETYPAL { panic!("`restrict_fetch` is not allowed with queries that use `Changed` or `Added`"); } - let fetch_state = NewQ::new_state(components).expect( + let fetch_state = NewQ::get_state(components).expect( "Could not create fetch_state. Please initialize any components needed before trying to `transmute`", ); #[allow(clippy::let_unit_value)] // the archetypal filters have already been applied, so we don't need them. - let filter_state = <()>::new_state(components).expect( + let filter_state = <()>::get_state(components).expect( "Could not create filter_state. Please initialize any components needed before trying to `transmute`", ); From f90bc256870c90b04aec54a66a7dcda46d5c740e Mon Sep 17 00:00:00 2001 From: Mike Hsu Date: Mon, 23 Oct 2023 21:31:19 -0700 Subject: [PATCH 27/27] clean up docs. --- crates/bevy_ecs/src/query/access.rs | 8 +++--- crates/bevy_ecs/src/query/fetch.rs | 11 ++++---- crates/bevy_ecs/src/query/state.rs | 9 ++++--- crates/bevy_ecs/src/system/query.rs | 39 +++++++++++++++++++++-------- 4 files changed, 43 insertions(+), 24 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 3a9bb7e5b4270..1c15b8dc259ab 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -224,7 +224,7 @@ impl Access { reads.is_subset(&other.reads_and_writes) && self.writes.is_subset(&other.writes) } - /// modifies self with the intersection with `other` + /// Modify `self` with the intersection with `other` pub fn intersect(&mut self, other: &Access) { if self.writes_all { if other.writes_all { @@ -317,7 +317,7 @@ impl Access { self.writes.ones().map(T::get_sparse_set_index) } - /// Returns true if the difference of `self` with `other` is empty + /// Returns `true` if the difference of `self` with `other` is empty pub fn read_and_writes_difference_is_empty(&self, other: &Access) -> bool { if self.reads_all || self.writes_all { return other.reads_all || other.writes_all; @@ -458,12 +458,12 @@ impl FilteredAccess { }) } - /// `other` contains all the same access as `self`. This does not take into account the filtered access. + /// `other` contains all the same access as `self`. This does not take into account the `filter_sets`. pub fn is_subset(&self, other: &FilteredAccess) -> bool { self.access.is_subset(&other.access) } - /// returns true if optional access has not changed + /// Returns `true` if optional access has not changed. pub fn is_optional_compatible( &self, mut original_optional: Access, diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 42ebb342d175f..7143ca17cc12a 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -437,9 +437,8 @@ pub unsafe trait WorldQuery { access: &mut Access, ); - /// Adds component access for when [`WorldQuery`] is not an exact match. - /// i.e. Option<&T> where the matched archetypes don't necessarily contain - /// the data the [`WorldQuery`] takes access to in [`Self::update_component_access`] + /// Adds component access to `access` for when [`WorldQuery`] is not an exact match. + /// i.e. With `Option<&T>` the matched archetypes do not necessarily contain a `T`. fn optional_access( state: &Self::State, access: &mut Access, @@ -449,8 +448,9 @@ pub unsafe trait WorldQuery { /// Creates and initializes a [`State`](WorldQuery::State) for this [`WorldQuery`] type. fn init_state(world: &mut World) -> Self::State; - /// Creates a [`State`](WorldQuery::State) for this [`WorldQuery`] type. This should return - /// the same value as [`init_state`](Self::init_state). + /// Creates a [`State`](WorldQuery::State) for this [`WorldQuery`] type. When successful, this returns + /// the same value as [`init_state`](Self::init_state). When the state cannot be created, this return `None`. + /// This might happen if we try to get the state for a component has not been registered in the [`World`]. fn get_state(components: &Components) -> Option; /// Returns `true` if this query matches a set of components. Otherwise, returns `false`. @@ -1854,7 +1854,6 @@ unsafe impl WorldQuery for PhantomData { const IS_DENSE: bool = true; // `PhantomData` matches every entity in each archetype. const IS_ARCHETYPAL: bool = true; - // `PhantomData` does not access any data, so it is exact. unsafe fn set_archetype<'w>( _fetch: &mut Self::Fetch<'w>, diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 7f7bd4c7e36ef..aaa83417a68bc 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -138,13 +138,14 @@ impl QueryState { state } - /// This is used to transform a [`Query`](crate::system::Query) into a more generic [`Query`](crate::system::Query). - /// This can be useful for passsing to another function tha might take the more generalize query. + /// Use this to transform a [`QueryState`] into a more generic [`QueryState`]. + /// This can be useful for passing to another function that might take the more general form. /// See [`Query::transmute_fetch`](crate::system::Query::transmute_fetch) for more details. /// /// You should not call [`update_archetypes`](Self::update_archetypes) on the returned [`QueryState`] as the result will be unpredictable. /// You might end up with a mix of archetypes that only matched the original query + archetypes that only match - /// the new [`QueryState`]. Most of the safe methods on [`QueryState`] call [`QueryState::update_archetypes`] internally. + /// the new [`QueryState`]. Most of the safe methods on [`QueryState`] call [`QueryState::update_archetypes`] internally, so this + /// best used through a [`Query`](crate::system::Query). #[track_caller] pub(crate) fn transmute_fetch( &self, @@ -157,7 +158,7 @@ impl QueryState { "Could not create fetch_state. Please initialize any components needed before trying to `transmute`", ); #[allow(clippy::let_unit_value)] - // the archetypal filters have already been applied, so we don't need them. + // the archetypal filters have already been applied, so we discard them to make the query type more general. let filter_state = <()>::get_state(components).expect( "Could not create filter_state. Please initialize any components needed before trying to `transmute`", ); diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 40a8c4235a4c6..9784024d863dd 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -400,13 +400,17 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } } - /// Returns a [`QueryLens`] that can be used to get a query with a more restricted fetch. - /// i.e. Tranform a `Query<(&A, &mut B)>` to a `Query<&B>`. - /// This can be useful for passing the query to another function. + /// Returns a [`QueryLens`] that can be used to get a query with a more general fetch. + /// For example, this can transform a `Query<(&A, &mut B)>` to a `Query<&B>`. + /// This can be useful for passing the query to another function. Note that this will + /// not return all the entities in the world that match the new query. This will only + /// return the entities that matched the original query. /// /// ## Panics /// - /// This will panic if `NewQ` is not a subset of `Q` or if `F` includes `Added` or `Changed`. + /// This will panic if `NewQ` is not a subset of the original fetch `Q` + /// or if the original filter `F` includes [`Added`](crate::query::Added) or + /// [`Changed`](crate::query::Changed). /// /// ## Example /// @@ -423,24 +427,39 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// # let mut world = World::new(); /// # /// # world.spawn((A(10), B(5))); - /// - /// fn function_that_uses_a_query_lens(lens: &mut QueryLens<&A>) { + /// # + /// fn reusable_function(lens: &mut QueryLens<&A>) { /// assert_eq!(lens.query().single().0, 10); /// } /// + /// // We can use the function in a system that takes the exact query. /// fn system_1(mut query: Query<&A>) { - /// function_that_uses_a_query_lens(&mut query.into_query_lens()); + /// reusable_function(&mut query.into_query_lens()); /// } /// + /// // We can also use it with a query that does not match exactly + /// // by transmuting it. /// fn system_2(mut query: Query<(&mut A, &B)>) { /// let mut lens = query.transmute_fetch::<&A>(); - /// function_that_uses_a_query_lens(&mut lens); + /// reusable_function(&mut lens); /// } /// /// # let mut schedule = Schedule::default(); /// # schedule.add_systems((system_1, system_2)); /// # schedule.run(&mut world); /// ``` + /// + /// ## Allowed Transmutes + /// + /// Besides removing parameters from the query, you can also + /// make limited changes to the types of paramters. + /// + /// * Can always add/remove `Entity` + /// * `Ref` <-> `&T` + /// * `&mut T` -> `&T` + /// * `&mut T` -> `Ref` + /// * [`EntityMut`](crate::world::EntityMut) -> [`EntityRef`](crate::world::EntityRef) + /// #[track_caller] pub fn transmute_fetch(&mut self) -> QueryLens<'_, NewQ> { let new_state = self.state.transmute_fetch(self.world.components()); @@ -453,7 +472,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } } - /// helper method to get a [`QueryLens`] with the same fetch as the existing query + /// Gets a [`QueryLens`] with the same fetch as the existing query. pub fn into_query_lens(&mut self) -> QueryLens<'_, Q> { self.transmute_fetch() } @@ -1578,7 +1597,7 @@ impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } } -/// A type used to store the generalized query's state. +/// A type used to store the new [`QueryState`] from [`Query::transmute_fetch`]. pub struct QueryLens<'world, NewQ: WorldQuery> { world: UnsafeWorldCell<'world>, state: QueryState,