From 9110910d8060f1512e6e36507cf74d86eb86459b Mon Sep 17 00:00:00 2001 From: Ellen Date: Sat, 6 Aug 2022 14:25:40 +0100 Subject: [PATCH 1/3] do it --- crates/bevy_ecs/macros/src/fetch.rs | 14 ++- crates/bevy_ecs/src/query/fetch.rs | 150 ++++++++++++++++++---------- crates/bevy_ecs/src/query/filter.rs | 65 ++++++------ crates/bevy_ecs/src/query/iter.rs | 39 +++----- crates/bevy_ecs/src/query/mod.rs | 6 +- 5 files changed, 161 insertions(+), 113 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 6d0e3d509428c..734057b7bf463 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -186,7 +186,6 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { #(#(#ignored_field_attrs)* #ignored_field_visibilities #ignored_field_idents: #ignored_field_types,)* } - #[derive(Clone)] #[doc(hidden)] #visibility struct #fetch_struct_name #user_impl_generics_with_world #user_where_clauses_with_world { #(#field_idents: <#field_types as #path::query::WorldQueryGats<'__w>>::Fetch,)* @@ -239,6 +238,19 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { } } + unsafe fn clone_fetch<'__w>( + _fetch: &>::Fetch + ) -> >::Fetch { + #fetch_struct_name { + #( + #field_idents: <#field_types>::clone_fetch(& _fetch. #field_idents), + )* + #( + #ignored_field_idents: Default::default(), + )* + } + } + const IS_DENSE: bool = true #(&& <#field_types>::IS_DENSE)*; const IS_ARCHETYPAL: bool = true #(&& <#field_types>::IS_ARCHETYPAL)*; diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index a4670944807f8..991f224f12a7a 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -327,6 +327,16 @@ pub unsafe trait WorldQuery: for<'w> WorldQueryGats<'w> { change_tick: u32, ) -> >::Fetch; + /// This function is safe to call if `Self:ReadOnlyWorldQuery` holds. + /// + /// # Safety + /// While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure + /// that the returned value is not used in any way that would cause two `QueryItem` for the same + /// `archetype_index` or `table_row` to be alive at the same time. + unsafe fn clone_fetch<'w>( + fetch: &>::Fetch, + ) -> >::Fetch; + /// Returns true if (and only if) every table of every archetype matched by this fetch contains /// all of the matched components. This is used to select a more efficient "table iterator" /// for "dense" queries. If this returns true, [`WorldQuery::set_table`] and [`WorldQuery::table_fetch`] @@ -461,7 +471,6 @@ pub type ROQueryFetch<'w, Q> = QueryFetch<'w, ::ReadOnly>; pub type ROQueryItem<'w, Q> = QueryItem<'w, ::ReadOnly>; #[doc(hidden)] -#[derive(Clone)] pub struct EntityFetch<'w> { entities: Option>, } @@ -488,6 +497,14 @@ unsafe impl WorldQuery for Entity { EntityFetch { entities: None } } + unsafe fn clone_fetch<'w>( + fetch: &>::Fetch, + ) -> >::Fetch { + EntityFetch { + entities: fetch.entities, + } + } + #[inline] unsafe fn set_archetype<'w>( fetch: &mut EntityFetch<'w>, @@ -595,6 +612,17 @@ unsafe impl WorldQuery for &T { } } + unsafe fn clone_fetch<'w>( + fetch: &>::Fetch, + ) -> >::Fetch { + ReadFetch { + table_components: fetch.table_components, + entity_table_rows: fetch.entity_table_rows, + entities: fetch.entities, + sparse_set: fetch.sparse_set, + } + } + #[inline] unsafe fn set_archetype<'w>( fetch: &mut ReadFetch<'w, T>, @@ -692,17 +720,6 @@ unsafe impl WorldQuery for &T { } } -impl Clone for ReadFetch<'_, T> { - fn clone(&self) -> Self { - Self { - table_components: self.table_components, - entity_table_rows: self.entity_table_rows, - entities: self.entities, - sparse_set: self.sparse_set, - } - } -} - /// SAFETY: access is read only unsafe impl ReadOnlyWorldQuery for &T {} @@ -761,6 +778,20 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { } } + unsafe fn clone_fetch<'w>( + fetch: &>::Fetch, + ) -> >::Fetch { + WriteFetch { + table_components: fetch.table_components, + table_ticks: fetch.table_ticks, + entities: fetch.entities, + entity_table_rows: fetch.entity_table_rows, + sparse_set: fetch.sparse_set, + last_change_tick: fetch.last_change_tick, + change_tick: fetch.change_tick, + } + } + #[inline] unsafe fn set_archetype<'w>( fetch: &mut WriteFetch<'w, T>, @@ -887,20 +918,6 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { } } -impl Clone for WriteFetch<'_, T> { - fn clone(&self) -> Self { - Self { - table_components: self.table_components, - table_ticks: self.table_ticks, - entities: self.entities, - entity_table_rows: self.entity_table_rows, - sparse_set: self.sparse_set, - last_change_tick: self.last_change_tick, - change_tick: self.change_tick, - } - } -} - impl<'w, T: Component> WorldQueryGats<'w> for &mut T { type Fetch = WriteFetch<'w, T>; type Item = Mut<'w, T>; @@ -911,17 +928,6 @@ pub struct OptionFetch<'w, T: WorldQuery> { fetch: >::Fetch, matches: bool, } -impl<'w, T: WorldQuery> Clone for OptionFetch<'w, T> -where - >::Fetch: Clone, -{ - fn clone(&self) -> Self { - Self { - fetch: self.fetch.clone(), - matches: self.matches, - } - } -} // SAFETY: defers to soundness of `T: WorldQuery` impl unsafe impl WorldQuery for Option { @@ -948,6 +954,15 @@ unsafe impl WorldQuery for Option { } } + unsafe fn clone_fetch<'w>( + fetch: &>::Fetch, + ) -> >::Fetch { + OptionFetch { + fetch: T::clone_fetch(&fetch.fetch), + matches: fetch.matches, + } + } + #[inline] unsafe fn set_archetype<'w>( fetch: &mut OptionFetch<'w, T>, @@ -1065,7 +1080,6 @@ impl<'w, T: WorldQuery> WorldQueryGats<'w> for Option { /// } /// # bevy_ecs::system::assert_is_system(print_moving_objects_system); /// ``` -#[derive(Clone)] pub struct ChangeTrackers { pub(crate) component_ticks: ComponentTicks, pub(crate) last_change_tick: u32, @@ -1073,6 +1087,18 @@ pub struct ChangeTrackers { marker: PhantomData, } +impl Clone for ChangeTrackers { + fn clone(&self) -> Self { + Self { + component_ticks: self.component_ticks, + last_change_tick: self.last_change_tick, + change_tick: self.change_tick, + marker: PhantomData, + } + } +} +impl Copy for ChangeTrackers {} + impl std::fmt::Debug for ChangeTrackers { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("ChangeTrackers") @@ -1111,20 +1137,6 @@ pub struct ChangeTrackersFetch<'w, T> { change_tick: u32, } -impl Clone for ChangeTrackersFetch<'_, T> { - fn clone(&self) -> Self { - Self { - table_ticks: self.table_ticks, - entity_table_rows: self.entity_table_rows, - entities: self.entities, - sparse_set: self.sparse_set, - marker: self.marker, - last_change_tick: self.last_change_tick, - change_tick: self.change_tick, - } - } -} - // SAFETY: `ROQueryFetch` is the same as `QueryFetch` unsafe impl WorldQuery for ChangeTrackers { type ReadOnly = Self; @@ -1161,6 +1173,20 @@ unsafe impl WorldQuery for ChangeTrackers { } } + unsafe fn clone_fetch<'w>( + fetch: &>::Fetch, + ) -> >::Fetch { + ChangeTrackersFetch { + table_ticks: fetch.table_ticks, + entity_table_rows: fetch.entity_table_rows, + entities: fetch.entities, + sparse_set: fetch.sparse_set, + marker: fetch.marker, + last_change_tick: fetch.last_change_tick, + change_tick: fetch.change_tick, + } + } + #[inline] unsafe fn set_archetype<'w>( fetch: &mut ChangeTrackersFetch<'w, T>, @@ -1317,6 +1343,13 @@ macro_rules! impl_tuple_fetch { ($($name::init_fetch(_world, $name, _last_change_tick, _change_tick),)*) } + unsafe fn clone_fetch<'w>( + fetch: &>::Fetch, + ) -> >::Fetch { + let ($($name,)*) = &fetch; + ($($name::clone_fetch($name),)*) + } + const IS_DENSE: bool = true $(&& $name::IS_DENSE)*; const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*; @@ -1395,7 +1428,6 @@ macro_rules! impl_tuple_fetch { /// `Query>` is equivalent to `Query<(Option<&A>, Option<&B>, Option<&mut C>), Or<(With, With, With)>>`. /// Each of the components in `T` is returned as an `Option`, as with `Option` queries. /// Entities are guaranteed to have at least one of the components in `T`. -#[derive(Clone)] pub struct AnyOf(PhantomData); macro_rules! impl_anytuple_fetch { @@ -1427,6 +1459,13 @@ macro_rules! impl_anytuple_fetch { ($(($name::init_fetch(_world, $name, _last_change_tick, _change_tick), false),)*) } + unsafe fn clone_fetch<'w>( + fetch: &>::Fetch, + ) -> >::Fetch { + let ($($name,)*) = &fetch; + ($(($name::clone_fetch(& $name.0), $name.1),)*) + } + const IS_DENSE: bool = true $(&& $name::IS_DENSE)*; const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*; @@ -1559,6 +1598,11 @@ unsafe impl WorldQuery for NopWorldQuery { ) { } + unsafe fn clone_fetch<'w>( + _fetch: &>::Fetch, + ) -> >::Fetch { + } + #[inline(always)] unsafe fn set_archetype( _fetch: &mut (), diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 960c6d4bcd7f5..238964aaa909b 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -66,6 +66,11 @@ unsafe impl WorldQuery for With { ) { } + unsafe fn clone_fetch<'w>( + _fetch: &>::Fetch, + ) -> >::Fetch { + } + const IS_DENSE: bool = { match T::Storage::STORAGE_TYPE { StorageType::Table => true, @@ -173,6 +178,11 @@ unsafe impl WorldQuery for Without { ) { } + unsafe fn clone_fetch<'w>( + _fetch: &>::Fetch, + ) -> >::Fetch { + } + const IS_DENSE: bool = { match T::Storage::STORAGE_TYPE { StorageType::Table => true, @@ -271,7 +281,6 @@ unsafe impl ReadOnlyWorldQuery for Without {} /// } /// # bevy_ecs::system::assert_is_system(print_cool_entity_system); /// ``` -#[derive(Clone, Copy)] pub struct Or(pub T); #[doc(hidden)] @@ -279,18 +288,6 @@ pub struct OrFetch<'w, T: WorldQuery> { fetch: QueryFetch<'w, T>, matches: bool, } -impl<'w, T: WorldQuery> Copy for OrFetch<'w, T> where QueryFetch<'w, T>: Copy {} -impl<'w, T: WorldQuery> Clone for OrFetch<'w, T> -where - QueryFetch<'w, T>: Clone, -{ - fn clone(&self) -> Self { - Self { - fetch: self.fetch.clone(), - matches: self.matches, - } - } -} macro_rules! impl_query_filter_tuple { ($(($filter: ident, $state: ident)),*) => { @@ -326,6 +323,18 @@ macro_rules! impl_query_filter_tuple { },)*) } + unsafe fn clone_fetch<'w>( + fetch: &>::Fetch, + ) -> >::Fetch { + let ($($filter,)*) = &fetch; + ($( + OrFetch { + fetch: $filter::clone_fetch(&$filter.fetch), + matches: $filter.matches, + }, + )*) + } + #[inline] unsafe fn set_table<'w>(fetch: &mut >::Fetch, state: &Self::State, table: &'w Table) { let ($($filter,)*) = fetch; @@ -471,6 +480,20 @@ macro_rules! impl_tick_filter { } } + unsafe fn clone_fetch<'w>( + fetch: &>::Fetch, + ) -> >::Fetch { + $fetch_name { + table_ticks: fetch.table_ticks, + entity_table_rows: fetch.entity_table_rows, + entities: fetch.entities, + sparse_set: fetch.sparse_set, + last_change_tick: fetch.last_change_tick, + change_tick: fetch.change_tick, + marker: PhantomData, + } + } + const IS_DENSE: bool = { match T::Storage::STORAGE_TYPE { StorageType::Table => true, @@ -565,22 +588,6 @@ macro_rules! impl_tick_filter { /// SAFETY: read-only access unsafe impl ReadOnlyWorldQuery for $name {} - - impl Clone for $fetch_name<'_, T> { - fn clone(&self) -> Self { - Self { - table_ticks: self.table_ticks.clone(), - entity_table_rows: self.entity_table_rows.clone(), - marker: self.marker.clone(), - entities: self.entities.clone(), - sparse_set: self.sparse_set.clone(), - last_change_tick: self.last_change_tick.clone(), - change_tick: self.change_tick.clone(), - } - } - } - - impl Copy for $fetch_name<'_, T> {} }; } diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 2ae0e1cda7273..174048f3b40b7 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -339,11 +339,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize> /// references to the same component, leading to unique reference aliasing. ///. /// It is always safe for shared access. - unsafe fn fetch_next_aliased_unchecked(&mut self) -> Option<[QueryItem<'w, Q>; K]> - where - QueryFetch<'w, Q>: Clone, - QueryFetch<'w, F>: Clone, - { + unsafe fn fetch_next_aliased_unchecked(&mut self) -> Option<[QueryItem<'w, Q>; K]> { if K == 0 { return None; } @@ -354,7 +350,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize> Some(_) => { // walk forward up to last element, propagating cursor state forward for j in (i + 1)..K { - self.cursors[j] = self.cursors[j - 1].clone(); + self.cursors[j] = self.cursors[j - 1].clone_cursor(); match self.cursors[j].next(self.tables, self.archetypes, self.query_state) { Some(_) => {} None if i > 0 => continue 'outer, @@ -380,11 +376,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize> /// Get next combination of queried components #[inline] - pub fn fetch_next(&mut self) -> Option<[QueryItem<'_, Q>; K]> - where - for<'a> QueryFetch<'a, Q>: Clone, - for<'a> QueryFetch<'a, F>: Clone, - { + pub fn fetch_next(&mut self) -> Option<[QueryItem<'_, Q>; K]> { // SAFETY: we are limiting the returned reference to self, // making sure this method cannot be called multiple times without getting rid // of any previously returned unique references first, thus preventing aliasing. @@ -400,9 +392,6 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize> // multiple times would allow multiple owned references to the same data to exist. impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery, const K: usize> Iterator for QueryCombinationIter<'w, 's, Q, F, K> -where - QueryFetch<'w, Q>: Clone, - QueryFetch<'w, F>: Clone, { type Item = [QueryItem<'w, Q>; K]; @@ -467,9 +456,6 @@ where // This is correct as [`QueryCombinationIter`] always returns `None` once exhausted. impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery, const K: usize> FusedIterator for QueryCombinationIter<'w, 's, Q, F, K> -where - QueryFetch<'w, Q>: Clone, - QueryFetch<'w, F>: Clone, { } @@ -485,17 +471,20 @@ struct QueryIterationCursor<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> { phantom: PhantomData, } -impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Clone for QueryIterationCursor<'w, 's, Q, F> -where - QueryFetch<'w, Q>: Clone, - QueryFetch<'w, F>: Clone, -{ - fn clone(&self) -> Self { +impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, Q, F> { + /// This function is safe to call if `(Q, F): ReadOnlyWorldQuery` holds. + /// + /// # Safety + /// While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure + /// that the returned value is not used in any way that would cause two `QueryItem` for the same + /// `archetype_index` or `table_row` to be alive at the same time. + unsafe fn clone_cursor(&self) -> Self { Self { table_id_iter: self.table_id_iter.clone(), archetype_id_iter: self.archetype_id_iter.clone(), - fetch: self.fetch.clone(), - filter: self.filter.clone(), + // SAFETY: upheld by caller invariants + fetch: Q::clone_fetch(&self.fetch), + filter: F::clone_fetch(&self.filter), current_len: self.current_len, current_index: self.current_index, phantom: PhantomData, diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index 4adfbd2e369ee..92b33428ff8c0 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -21,7 +21,7 @@ pub(crate) unsafe fn debug_checked_unreachable() -> ! { mod tests { use super::{ReadOnlyWorldQuery, WorldQuery}; use crate::prelude::{AnyOf, Entity, Or, QueryState, With, Without}; - use crate::query::{ArchetypeFilter, QueryCombinationIter, QueryFetch}; + use crate::query::{ArchetypeFilter, QueryCombinationIter}; use crate::system::{IntoSystem, Query, System, SystemState}; use crate::{self as bevy_ecs, component::Component, world::World}; use std::any::type_name; @@ -166,8 +166,6 @@ mod tests { Q: WorldQuery, F: ReadOnlyWorldQuery, F::ReadOnly: ArchetypeFilter, - for<'w> QueryFetch<'w, Q::ReadOnly>: Clone, - for<'w> QueryFetch<'w, F::ReadOnly>: Clone, { let mut query = world.query_filtered::(); let iter = query.iter_combinations::(world); @@ -179,8 +177,6 @@ mod tests { Q: WorldQuery, F: ReadOnlyWorldQuery, F::ReadOnly: ArchetypeFilter, - for<'w> QueryFetch<'w, Q::ReadOnly>: Clone, - for<'w> QueryFetch<'w, F::ReadOnly>: Clone, { let mut query = world.query_filtered::(); let iter = query.iter(world); From bad65d3468181c8676924db8617c378cf9f6db5f Mon Sep 17 00:00:00 2001 From: Boxy Date: Tue, 25 Oct 2022 21:46:48 +0100 Subject: [PATCH 2/3] rebase --- crates/bevy_ecs/src/query/mod.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index 92b33428ff8c0..dff4b9a251f41 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -88,8 +88,6 @@ mod tests { Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery, F::ReadOnly: ArchetypeFilter, - for<'w> QueryFetch<'w, Q::ReadOnly>: Clone, - for<'w> QueryFetch<'w, F::ReadOnly>: Clone, { let mut query = world.query_filtered::(); let iter = query.iter(world); From 173bcc8f3969ba3990df4154b7871263a18ecd5f Mon Sep 17 00:00:00 2001 From: Boxy Date: Wed, 26 Oct 2022 02:43:28 +0100 Subject: [PATCH 3/3] Update crates/bevy_ecs/src/query/fetch.rs Co-authored-by: Alice Cecile --- crates/bevy_ecs/src/query/fetch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 991f224f12a7a..510420aa4600c 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -327,7 +327,7 @@ pub unsafe trait WorldQuery: for<'w> WorldQueryGats<'w> { change_tick: u32, ) -> >::Fetch; - /// This function is safe to call if `Self:ReadOnlyWorldQuery` holds. + /// While this function can be called for any query, it is always safe to call if `Self: ReadOnlyWorldQuery` holds. /// /// # Safety /// While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure