From 2cf790b2dd79ae56206a09f220b394ab3a03331a Mon Sep 17 00:00:00 2001 From: vil'mo Date: Sun, 13 Oct 2024 12:45:21 +0700 Subject: [PATCH 01/10] Implemented DataSet --- crates/bevy_ecs/macros/src/lib.rs | 156 +++++++++++++++++++++++++++++ crates/bevy_ecs/src/query/fetch.rs | 9 ++ 2 files changed, 165 insertions(+) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 10f794075466c..944430c204649 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -415,6 +415,162 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { tokens } +#[proc_macro] +pub fn impl_data_set(_input: TokenStream) -> TokenStream { + let mut tokens = TokenStream::new(); + let max_members = 8; + let datas = get_idents(|i| format!("D{i}"), max_members); + let accesses = get_idents(|i| format!("access{i}"), max_members); + let mut member_fn_muts = Vec::new(); + for (i, data) in datas.iter().enumerate() { + let fn_name = Ident::new(&format!("d{i}"), Span::call_site()); + let index = Index::from(i); + let ordinal = match i { + 1 => "1st".to_owned(), + 2 => "2nd".to_owned(), + 3 => "3rd".to_owned(), + x => format!("{x}th"), + }; + let comment = + format!("Gets exclusive access to the {ordinal} member of this [`DataSet`]."); + member_fn_muts.push(quote! { + #[doc = #comment] + /// No other members may be accessed while this one is active. + pub fn #fn_name(&mut self) -> QueryItem<'_, #data> { + // SAFETY: since it's only possible to create instance of `DataSet` using + // `::fetch`, following safety rules were upheld by the caller. + // - [`WorldQuery::set_table`] or [`WorldQuery::set_archetype`] have been called for `DataSet`, + // so it was called for each `data` (as implementation of those functions for `DataSet` suggests). + // - `entity` and `table_row` are in the range of the current table and archetype. + #data::shrink(unsafe { + #data::fetch(&mut self.fetch.#index, self.entity, self.table_row) + }) + } + }); + } + + for member_count in 1..=max_members { + let data = &datas[0..member_count]; + let access = &accesses[0..member_count]; + let member_fn_mut = &member_fn_muts[0..member_count]; + tokens.extend(TokenStream::from(quote! { + // SAFETY: each item in set is read only + unsafe impl<'__w, #(#data: ReadOnlyQueryData,)*> ReadOnlyQueryData for DataSet<'__w, (#(#data,)*)> {} + + // SAFETY: deferes to soundness of `#data: WorldQuery` impl + unsafe impl<'__w, #(#data: QueryData,)*> QueryData for DataSet<'__w, (#(#data,)*)> { + type ReadOnly = DataSet<'__w, (#(#data::ReadOnly,)*)>; + } + + // SAFETY: + // for each member of the set accessed by `fetch`, [`update_component_access`] + // - adds corresponding access to `filtered_access` + // - panics if it's access conflicts with access that has already been added before calling `update_component_access` + // + // If `fetch` mutably accesses a member of the set, it is impossible to access any other members. + unsafe impl<'__w, #(#data: QueryData,)*> WorldQuery for DataSet<'__w, (#(#data,)*)> { + type Item<'w> = DataSet<'w, (#(#data,)*)>; + type Fetch<'w> = (#(#data::Fetch<'w>,)*); + type State = (#(#data::State,)*); + + fn shrink<'wlong: 'wshort, 'wshort>( + item: Self::Item<'wlong>, + ) -> Self::Item<'wshort> { + DataSet { + fetch: Self::shrink_fetch(item.fetch), + entity: item.entity, + table_row: item.table_row, + } + } + + fn shrink_fetch<'wlong: 'wshort, 'wshort>( + fetch: Self::Fetch<'wlong>, + ) -> Self::Fetch<'wshort> { + <(#(#data,)*) as WorldQuery>::shrink_fetch(fetch) + } + + unsafe fn init_fetch<'w>( + world: UnsafeWorldCell<'w>, + state: &Self::State, + last_run: Tick, + this_run: Tick, + ) -> Self::Fetch<'w> { + // SAFETY: The invariants are uphold by the caller. + unsafe { <(#(#data,)*) as WorldQuery>::init_fetch(world, state, last_run, this_run) } + } + + const IS_DENSE: bool = <(#(#data,)*) as WorldQuery>::IS_DENSE; + + unsafe fn set_archetype<'w>( + fetch: &mut Self::Fetch<'w>, + state: &Self::State, + archetype: &'w Archetype, + table: &'w Table, + ) { + // SAFETY: The invariants are uphold by the caller. + unsafe { <(#(#data,)*) as WorldQuery>::set_archetype(fetch, state, archetype, table); } + } + + unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table) { + // SAFETY: The invariants are uphold by the caller. + unsafe { <(#(#data,)*) as WorldQuery>::set_table(fetch, state, table); } + } + + unsafe fn fetch<'w>( + fetch: &mut Self::Fetch<'w>, + entity: Entity, + table_row: TableRow, + ) -> Self::Item<'w> { + DataSet { + fetch: fetch.clone(), + entity, + table_row, + } + } + + fn update_component_access(state: &Self::State, filtered_access: &mut FilteredAccess) { + let (#(#data,)*) = state; + + #( + // Making sure each individual member of the set doesn't conflict with other query access. + // Panics if one of the members conflicts with previous access. + #data::update_component_access(#data, &mut filtered_access.clone()); + )* + #( + // Updating empty [`FilteredAccess`] and then extending passed filtered_access. + // This is done to avoid conflicts with other members of the set. + let mut #access = FilteredAccess::default(); + #data::update_component_access(#data, &mut #access); + filtered_access.extend(&#access); + )* + } + + fn init_state(world: &mut World) -> Self::State { + <(#(#data,)*) as WorldQuery>::init_state(world) + } + + fn get_state(components: &Components) -> Option { + <(#(#data,)*) as WorldQuery>::get_state(components) + } + + fn matches_component_set( + state: &Self::State, + set_contains_id: &impl Fn(ComponentId) -> bool, + ) -> bool { + <(#(#data,)*) as WorldQuery>::matches_component_set(state, set_contains_id) + } + } + + impl<'w, #(#data: QueryData,)*> DataSet<'w, (#(#data,)*)> + { + #(#member_fn_mut)* + } + })); + } + + tokens +} + /// Implement `SystemParam` to use a struct as a parameter in a system #[proc_macro_derive(SystemParam, attributes(system_param))] pub fn derive_system_param(input: TokenStream) -> TokenStream { diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 17d92fc97930a..ffd855c927ddd 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -11,6 +11,7 @@ use crate::{ FilteredEntityMut, FilteredEntityRef, Mut, Ref, World, }, }; +use bevy_ecs_macros::impl_data_set; use bevy_ptr::{ThinSlicePtr, UnsafeCellDeref}; use bevy_utils::all_tuples; use core::{cell::UnsafeCell, marker::PhantomData}; @@ -1716,6 +1717,14 @@ unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { type ReadOnly = Ref<'__w, T>; } +pub struct DataSet<'a, T: QueryData> { + fetch: T::Fetch<'a>, + entity: Entity, + table_row: TableRow, +} + +impl_data_set!(); + #[doc(hidden)] pub struct OptionFetch<'w, T: WorldQuery> { fetch: T::Fetch<'w>, From 041ed7c49c0968556679f7143bea9ddc10b2edd3 Mon Sep 17 00:00:00 2001 From: vil'mo Date: Sun, 13 Oct 2024 14:13:28 +0700 Subject: [PATCH 02/10] Added doumentation --- crates/bevy_ecs/src/query/fetch.rs | 139 +++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index ffd855c927ddd..f44efcbd0b55a 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1717,6 +1717,145 @@ unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { type ReadOnly = Ref<'__w, T>; } +/// A collection of potentially conflicting [`QueryData`]s allowed by disjoint access. + +/// Allows queries to safely access and interact with up to 8 mutually exclusive [`QueryData`]s in a single iteration. +/// +/// Each individual [`QueryData`] can be accessed by using the functions `d0()`, `d1()`, ..., `d7()`, +/// according to the order they are defined in the `DataSet`. This ensures that there's +/// only one mutable reference to the member of the set at a time. +/// +/// # Examples +/// +/// This can be useful when you have a lot of complex [`QueryData`]s that conflict with each other. +/// +/// Imagine you have a system that calculates desired health and then changes it. +/// You also have a helper [`QueryData`] that calculates this for you. +/// The following system has conflicting access to the two [`QueryData`]s, +/// which is not allowed due to rust's mutability rules. +/// +/// ```should_panic +/// # use bevy_ecs::prelude::*; +/// # use bevy_ecs::query::QueryData; +/// # +/// # #[derive(Component)] +/// # struct Health; +/// # +/// # #[derive(Component)] +/// # struct Regen; +/// # +/// #[derive(QueryData)] +/// struct HealthChangeCalculation { +/// health: &'static Health, +/// regen: &'static Regen, +/// // ... +/// } +/// +/// // This will panic at runtime when the system gets initialized. +/// fn bad_system( +/// mut query: Query<(HealthChangeCalculation, &mut Health)>, +/// ) { +/// // ... +/// } +/// # +/// # let mut bad_system_system = IntoSystem::into_system(bad_system); +/// # let mut world = World::new(); +/// # bad_system_system.initialize(&mut world); +/// # bad_system_system.run((), &mut world); +/// ``` +/// +/// Conflicting [`QueryData`]s like these can be placed in a [`DataSet`], +/// which leverages the borrow checker to ensure that only one of the contained members are accessed at a given time. +/// +/// ``` +/// # use bevy_ecs::prelude::*; +/// # use bevy_ecs::query::QueryData; +/// # use bevy_ecs::query::DataSet; +/// # +/// # #[derive(Component)] +/// # struct Health; +/// # +/// # #[derive(Component)] +/// # struct Regen; +/// # +/// #[derive(QueryData)] +/// struct HealthChangeCalculation { +/// health: &'static Health, +/// regen: &'static Regen, +/// // ... +/// } +/// +/// fn health_change_system( +/// mut query: Query>, +/// ) { +/// for mut set in query.iter_mut() { +/// // This will access the first `QueryData`. +/// let health_change = set.d0(); +/// // Calculating health change... +/// +/// // The second `QueryData`. +/// // This would fail to compile if the previous parameter was still borrowed. +/// let health = set.d1(); +/// // Updating health... +/// } +/// } +/// # bevy_ecs::system::assert_is_system(health_change_system); +/// ``` +/// +/// [`DataSet`] can also be used in generic contexts that require a [`QueryData`]. +/// +/// ``` +/// # use bevy_ecs::prelude::*; +/// # use bevy_ecs::query::QueryData; +/// # use bevy_ecs::query::DataSet; +/// # +/// # #[derive(Component)] +/// # struct Transform; +/// # +/// trait Behavior { +/// type Data: QueryData; +/// fn update(&mut self, data: &mut QueryItem<'_, Self::Data>); +/// } +/// +/// struct Run; +/// +/// impl Behavior for Run { +/// type Data = &'static mut Transform; +/// +/// fn update(&mut self, data: &mut Transform) { +/// // Do something... +/// } +/// } +/// +/// struct Jump; +/// +/// impl Behavior for Jump { +/// type Data = &'static mut Transform; +/// +/// fn update(&mut self, data: &mut Transform) { +/// // Do something different... +/// } +/// } +/// +/// struct Selector; +/// +/// impl Behavior for Selector { +/// // `update` would not be possible to use, to select between `Run` and `Jump`. +/// // since both `Run` and `Jump` data have mutually exclusive access. +/// type Data = DataSet<'static, ( +/// ::Data, +/// ::Data, +/// //... +/// )>; +/// +/// fn update(&mut self, data: &mut QueryItem<'_, Self::Data>) { +/// // Select the behavior to use... +/// } +/// } +/// ``` pub struct DataSet<'a, T: QueryData> { fetch: T::Fetch<'a>, entity: Entity, From 504a500bfca19abd046b3f0dfe24771218f54d92 Mon Sep 17 00:00:00 2001 From: vil'mo Date: Sun, 13 Oct 2024 15:01:53 +0700 Subject: [PATCH 03/10] Added tests --- crates/bevy_ecs/src/query/fetch.rs | 5 +++-- crates/bevy_ecs/src/system/mod.rs | 28 +++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index f44efcbd0b55a..843b209ce1e2f 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1811,6 +1811,7 @@ unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { /// # use bevy_ecs::prelude::*; /// # use bevy_ecs::query::QueryData; /// # use bevy_ecs::query::DataSet; +/// # use bevy_ecs::query::QueryItem; /// # /// # #[derive(Component)] /// # struct Transform; @@ -1825,7 +1826,7 @@ unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { /// impl Behavior for Run { /// type Data = &'static mut Transform; /// -/// fn update(&mut self, data: &mut Transform) { +/// fn update(&mut self, data: &mut QueryItem<'_, Self::Data>) { /// // Do something... /// } /// } @@ -1835,7 +1836,7 @@ unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { /// impl Behavior for Jump { /// type Data = &'static mut Transform; /// -/// fn update(&mut self, data: &mut Transform) { +/// fn update(&mut self, data: &mut QueryItem<'_, Self::Data>) { /// // Do something different... /// } /// } diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index dd6a950f72a8e..ca6a59f058b02 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -320,7 +320,7 @@ mod tests { component::{Component, Components, Tick}, entity::{Entities, Entity}, prelude::{AnyOf, EntityRef}, - query::{Added, Changed, Or, With, Without}, + query::{Added, Changed, Or, With, Without, DataSet}, removal_detection::RemovedComponents, schedule::{ apply_deferred, common_conditions::resource_exists, Condition, IntoSystemConfigs, @@ -810,6 +810,32 @@ mod tests { run_system(&mut world, sys); } + + #[test] + fn data_set_system() { + fn sys(mut _query: Query>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic] + fn conflicting_query_with_data_set_system() { + fn sys(_query_1: Query<&mut A>, _query_2: Query>) {} + + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic] + fn conflicting_data_sets_system() { + fn sys(_query_1: Query>, _query_2: Query>) {} + + let mut world = World::default(); + run_system(&mut world, sys); + } + #[derive(Default, Resource)] struct BufferRes { _buffer: Vec, From 1d47074192a2e1776358d796d5aa1be24d1b0e0d Mon Sep 17 00:00:00 2001 From: vil'mo Date: Sun, 13 Oct 2024 17:50:23 +0700 Subject: [PATCH 04/10] Fixed style and typos --- crates/bevy_ecs/macros/src/lib.rs | 31 +++++++++++++++--------------- crates/bevy_ecs/src/query/fetch.rs | 6 +++--- crates/bevy_ecs/src/system/mod.rs | 3 +-- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 944430c204649..542f96cab6e53 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -419,10 +419,10 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { pub fn impl_data_set(_input: TokenStream) -> TokenStream { let mut tokens = TokenStream::new(); let max_members = 8; - let datas = get_idents(|i| format!("D{i}"), max_members); + let data_types = get_idents(|i| format!("D{i}"), max_members); let accesses = get_idents(|i| format!("access{i}"), max_members); let mut member_fn_muts = Vec::new(); - for (i, data) in datas.iter().enumerate() { + for (i, data) in data_types.iter().enumerate() { let fn_name = Ident::new(&format!("d{i}"), Span::call_site()); let index = Index::from(i); let ordinal = match i { @@ -431,13 +431,12 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { 3 => "3rd".to_owned(), x => format!("{x}th"), }; - let comment = - format!("Gets exclusive access to the {ordinal} member of this [`DataSet`]."); + let comment = format!("Gets exclusive access to the {ordinal} member of this [`DataSet`]."); member_fn_muts.push(quote! { #[doc = #comment] /// No other members may be accessed while this one is active. pub fn #fn_name(&mut self) -> QueryItem<'_, #data> { - // SAFETY: since it's only possible to create instance of `DataSet` using + // SAFETY: since it's only possible to create instance of `DataSet` using // `::fetch`, following safety rules were upheld by the caller. // - [`WorldQuery::set_table`] or [`WorldQuery::set_archetype`] have been called for `DataSet`, // so it was called for each `data` (as implementation of those functions for `DataSet` suggests). @@ -450,7 +449,7 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { } for member_count in 1..=max_members { - let data = &datas[0..member_count]; + let data = &data_types[0..member_count]; let access = &accesses[0..member_count]; let member_fn_mut = &member_fn_muts[0..member_count]; tokens.extend(TokenStream::from(quote! { @@ -472,10 +471,10 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { type Item<'w> = DataSet<'w, (#(#data,)*)>; type Fetch<'w> = (#(#data::Fetch<'w>,)*); type State = (#(#data::State,)*); - + fn shrink<'wlong: 'wshort, 'wshort>( item: Self::Item<'wlong>, - ) -> Self::Item<'wshort> { + ) -> Self::Item<'wshort> { DataSet { fetch: Self::shrink_fetch(item.fetch), entity: item.entity, @@ -488,7 +487,7 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { ) -> Self::Fetch<'wshort> { <(#(#data,)*) as WorldQuery>::shrink_fetch(fetch) } - + unsafe fn init_fetch<'w>( world: UnsafeWorldCell<'w>, state: &Self::State, @@ -500,7 +499,7 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { } const IS_DENSE: bool = <(#(#data,)*) as WorldQuery>::IS_DENSE; - + unsafe fn set_archetype<'w>( fetch: &mut Self::Fetch<'w>, state: &Self::State, @@ -510,12 +509,12 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { // SAFETY: The invariants are uphold by the caller. unsafe { <(#(#data,)*) as WorldQuery>::set_archetype(fetch, state, archetype, table); } } - + unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table) { // SAFETY: The invariants are uphold by the caller. unsafe { <(#(#data,)*) as WorldQuery>::set_table(fetch, state, table); } } - + unsafe fn fetch<'w>( fetch: &mut Self::Fetch<'w>, entity: Entity, @@ -527,7 +526,7 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { table_row, } } - + fn update_component_access(state: &Self::State, filtered_access: &mut FilteredAccess) { let (#(#data,)*) = state; @@ -544,15 +543,15 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { filtered_access.extend(&#access); )* } - + fn init_state(world: &mut World) -> Self::State { <(#(#data,)*) as WorldQuery>::init_state(world) } - + fn get_state(components: &Components) -> Option { <(#(#data,)*) as WorldQuery>::get_state(components) } - + fn matches_component_set( state: &Self::State, set_contains_id: &impl Fn(ComponentId) -> bool, diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 843b209ce1e2f..c36939dd08767 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1753,7 +1753,7 @@ unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { /// /// // This will panic at runtime when the system gets initialized. /// fn bad_system( -/// mut query: Query<(HealthChangeCalculation, &mut Health)>, +/// mut query: Query<(HealthChangeCalculation, &mut Health)>, /// ) { /// // ... /// } @@ -1806,7 +1806,7 @@ unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { /// ``` /// /// [`DataSet`] can also be used in generic contexts that require a [`QueryData`]. -/// +/// /// ``` /// # use bevy_ecs::prelude::*; /// # use bevy_ecs::query::QueryData; @@ -1845,7 +1845,7 @@ unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { /// /// impl Behavior for Selector { /// // `update` would not be possible to use, to select between `Run` and `Jump`. -/// // since both `Run` and `Jump` data have mutually exclusive access. +/// // since both `Run` and `Jump` data have mutually exclusive access. /// type Data = DataSet<'static, ( /// ::Data, /// ::Data, diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index ca6a59f058b02..545402a4540ec 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -320,7 +320,7 @@ mod tests { component::{Component, Components, Tick}, entity::{Entities, Entity}, prelude::{AnyOf, EntityRef}, - query::{Added, Changed, Or, With, Without, DataSet}, + query::{Added, Changed, DataSet, Or, With, Without}, removal_detection::RemovedComponents, schedule::{ apply_deferred, common_conditions::resource_exists, Condition, IntoSystemConfigs, @@ -810,7 +810,6 @@ mod tests { run_system(&mut world, sys); } - #[test] fn data_set_system() { fn sys(mut _query: Query>) {} From fed9cfb2fe859cb5a332656e387a97725e10520b Mon Sep 17 00:00:00 2001 From: vil'mo Date: Mon, 14 Oct 2024 11:26:41 +0700 Subject: [PATCH 05/10] Removed unnececcary variable in macro implementation --- crates/bevy_ecs/macros/src/lib.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 542f96cab6e53..32fe19bdd4577 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -420,7 +420,6 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { let mut tokens = TokenStream::new(); let max_members = 8; let data_types = get_idents(|i| format!("D{i}"), max_members); - let accesses = get_idents(|i| format!("access{i}"), max_members); let mut member_fn_muts = Vec::new(); for (i, data) in data_types.iter().enumerate() { let fn_name = Ident::new(&format!("d{i}"), Span::call_site()); @@ -450,7 +449,6 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { for member_count in 1..=max_members { let data = &data_types[0..member_count]; - let access = &accesses[0..member_count]; let member_fn_mut = &member_fn_muts[0..member_count]; tokens.extend(TokenStream::from(quote! { // SAFETY: each item in set is read only @@ -463,7 +461,7 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { // SAFETY: // for each member of the set accessed by `fetch`, [`update_component_access`] - // - adds corresponding access to `filtered_access` + // - adds corresponding access to `access` // - panics if it's access conflicts with access that has already been added before calling `update_component_access` // // If `fetch` mutably accesses a member of the set, it is impossible to access any other members. @@ -527,20 +525,21 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { } } - fn update_component_access(state: &Self::State, filtered_access: &mut FilteredAccess) { + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { let (#(#data,)*) = state; #( // Making sure each individual member of the set doesn't conflict with other query access. // Panics if one of the members conflicts with previous access. - #data::update_component_access(#data, &mut filtered_access.clone()); + #data::update_component_access(#data, &mut access.clone()); )* + let mut current_access; #( - // Updating empty [`FilteredAccess`] and then extending passed filtered_access. + // Updating empty [`FilteredAccess`] and then extending passed access. // This is done to avoid conflicts with other members of the set. - let mut #access = FilteredAccess::default(); - #data::update_component_access(#data, &mut #access); - filtered_access.extend(&#access); + current_access = FilteredAccess::default(); + #data::update_component_access(#data, &mut current_access); + access.extend(¤t_access); )* } From d2150b0344d79ec0fdf5ff0d057daa8e61fc3cad Mon Sep 17 00:00:00 2001 From: vil'mo Date: Thu, 17 Oct 2024 09:07:48 +0700 Subject: [PATCH 06/10] Review changes --- crates/bevy_ecs/macros/src/lib.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 32fe19bdd4577..28977b04e923d 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -454,7 +454,7 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { // SAFETY: each item in set is read only unsafe impl<'__w, #(#data: ReadOnlyQueryData,)*> ReadOnlyQueryData for DataSet<'__w, (#(#data,)*)> {} - // SAFETY: deferes to soundness of `#data: WorldQuery` impl + // SAFETY: defers to soundness of `#data: WorldQuery` impl unsafe impl<'__w, #(#data: QueryData,)*> QueryData for DataSet<'__w, (#(#data,)*)> { type ReadOnly = DataSet<'__w, (#(#data::ReadOnly,)*)>; } @@ -533,11 +533,10 @@ pub fn impl_data_set(_input: TokenStream) -> TokenStream { // Panics if one of the members conflicts with previous access. #data::update_component_access(#data, &mut access.clone()); )* - let mut current_access; #( // Updating empty [`FilteredAccess`] and then extending passed access. // This is done to avoid conflicts with other members of the set. - current_access = FilteredAccess::default(); + let mut current_access = FilteredAccess::default(); #data::update_component_access(#data, &mut current_access); access.extend(¤t_access); )* From 095145b46e18d80844b3eed89c52873cf2b6c74d Mon Sep 17 00:00:00 2001 From: vil'mo Date: Wed, 1 Jan 2025 22:14:33 +0700 Subject: [PATCH 07/10] From proc macro to declarative --- crates/bevy_ecs/src/query/fetch.rs | 141 ++++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 649c244048de9..d30369599bbb7 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -11,11 +11,10 @@ use crate::{ FilteredEntityMut, FilteredEntityRef, Mut, Ref, World, }, }; -use bevy_ecs_macros::impl_data_set; use bevy_ptr::{ThinSlicePtr, UnsafeCellDeref}; use core::{cell::UnsafeCell, marker::PhantomData}; use smallvec::SmallVec; -use variadics_please::all_tuples; +use variadics_please::{all_tuples, all_tuples_enumerated}; /// Types that can be fetched from a [`World`] using a [`Query`]. /// @@ -1719,8 +1718,8 @@ unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { } /// A collection of potentially conflicting [`QueryData`]s allowed by disjoint access. - /// Allows queries to safely access and interact with up to 8 mutually exclusive [`QueryData`]s in a single iteration. +/// This query only matches entity if it is possible to access all members of the set for this entity. /// /// Each individual [`QueryData`] can be accessed by using the functions `d0()`, `d1()`, ..., `d7()`, /// according to the order they are defined in the `DataSet`. This ensures that there's @@ -1864,7 +1863,141 @@ pub struct DataSet<'a, T: QueryData> { table_row: TableRow, } -impl_data_set!(); +macro_rules! impl_data_set { + ($(($index: tt, $data: ident, $detupled: ident, $fn_name: ident)),*) => { + // SAFETY: All members are constrained to ReadOnlyQueryData, so World is only read + unsafe impl<'a, $($data,)*> ReadOnlyQueryData for DataSet<'a, ($($data,)*)> + where $($data: ReadOnlyQueryData,)* + { } + + // SAFETY: defers to soundness of `#data: WorldQuery` impl + unsafe impl<'a, $($data: QueryData,)*> QueryData for DataSet<'a, ($($data,)*)> { + type ReadOnly = DataSet<'a, ($($data::ReadOnly,)*)>; + } + + // SAFETY: + // for each member of the set accessed by `fetch`, [`update_component_access`] + // - adds corresponding access to `access` + // - panics if it's access conflicts with access that has already been added before calling `update_component_access` + // + // If `fetch` mutably accesses a member of the set, it is impossible to access any other members. + unsafe impl<'_a, $($data: QueryData,)*> WorldQuery for DataSet<'_a, ($($data,)*)> + { + type Item<'w> = DataSet<'w, ($($data,)*)>; + type Fetch<'w> = ($($data::Fetch<'w>,)*); + type State = ($($data::State,)*); + + fn shrink<'wlong: 'wshort, 'wshort>( + item: Self::Item<'wlong>, + ) -> Self::Item<'wshort> { + DataSet { + fetch: Self::shrink_fetch(item.fetch), + entity: item.entity, + table_row: item.table_row, + } + } + + fn shrink_fetch<'wlong: 'wshort, 'wshort>( + fetch: Self::Fetch<'wlong>, + ) -> Self::Fetch<'wshort> { + <($($data,)*) as WorldQuery>::shrink_fetch(fetch) + } + + unsafe fn init_fetch<'w>( + world: UnsafeWorldCell<'w>, + state: &Self::State, + last_run: Tick, + this_run: Tick, + ) -> Self::Fetch<'w> { + // SAFETY: The invariants are uphold by the caller. + unsafe { <($($data,)*) as WorldQuery>::init_fetch(world, state, last_run, this_run) } + } + + const IS_DENSE: bool = <($($data,)*) as WorldQuery>::IS_DENSE; + + unsafe fn set_archetype<'w>( + fetch: &mut Self::Fetch<'w>, + state: &Self::State, + archetype: &'w Archetype, + table: &'w Table, + ) { + // SAFETY: The invariants are uphold by the caller. + unsafe { <($($data,)*) as WorldQuery>::set_archetype(fetch, state, archetype, table); } + } + + unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table) { + // SAFETY: The invariants are uphold by the caller. + unsafe { <($($data,)*) as WorldQuery>::set_table(fetch, state, table); } + } + + unsafe fn fetch<'w>( + fetch: &mut Self::Fetch<'w>, + entity: Entity, + table_row: TableRow, + ) -> Self::Item<'w> { + DataSet { + fetch: fetch.clone(), + entity, + table_row, + } + } + + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { + let ($($detupled,)*) = state; + + $( + // Making sure each individual member of the set doesn't conflict with other query access. + // Panics if one of the members conflicts with previous access. + $data::update_component_access($detupled, &mut access.clone()); + )* + $( + // Updating empty [`FilteredAccess`] and then extending passed access. + // This is done to avoid conflicts with other members of the set. + let mut current_access = FilteredAccess::default(); + $data::update_component_access($detupled, &mut current_access); + access.extend(¤t_access); + )* + } + + fn init_state(world: &mut World) -> Self::State { + <($($data,)*) as WorldQuery>::init_state(world) + } + + fn get_state(components: &Components) -> Option { + <($($data,)*) as WorldQuery>::get_state(components) + } + + fn matches_component_set( + state: &Self::State, + set_contains_id: &impl Fn(ComponentId) -> bool, + ) -> bool { + <($($data,)*) as WorldQuery>::matches_component_set(state, set_contains_id) + } + } + + impl<$($data: QueryData,)*> DataSet<'_, ($($data,)*)> + { + $( + /// Gets exclusive access to the data set member at index + #[doc = stringify!($index)] + /// . + /// No other members may be accessed while this one is active. + pub fn $fn_name<'a>(&'a mut self) -> QueryItem<'a, $data> { + // SAFETY: since it's only possible to create instance of `DataSet` using + // `::fetch`, following safety rules were upheld by the caller. + // - [`WorldQuery::set_table`] or [`WorldQuery::set_archetype`] have been called for `DataSet`, + // so it was called for each `data` (as implementation of those functions for `DataSet` suggests). + // - `entity` and `table_row` are in the range of the current table and archetype. + $data::shrink(unsafe { + $data::fetch(&mut self.fetch.$index, self.entity, self.table_row) + }) + } + )* + } + } +} + +all_tuples_enumerated!(impl_data_set, 1, 8, P, detupled, d); #[doc(hidden)] pub struct OptionFetch<'w, T: WorldQuery> { From b474ca3a1c66cd6bd286b2432ca502d95f4be6d3 Mon Sep 17 00:00:00 2001 From: vil'mo Date: Wed, 1 Jan 2025 22:31:20 +0700 Subject: [PATCH 08/10] Added tests --- crates/bevy_ecs/src/query/fetch.rs | 2 +- crates/bevy_ecs/src/system/mod.rs | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index d30369599bbb7..7ad7f7e1f692b 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1951,7 +1951,7 @@ macro_rules! impl_data_set { $data::update_component_access($detupled, &mut access.clone()); )* $( - // Updating empty [`FilteredAccess`] and then extending passed access. + // Updating empty [`FilteredAccess`] and then extending main access. // This is done to avoid conflicts with other members of the set. let mut current_access = FilteredAccess::default(); $data::update_component_access($detupled, &mut current_access); diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 475df56fb24fc..0d6b05d4372d1 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -835,6 +835,24 @@ mod tests { run_system(&mut world, sys); } + #[test] + #[should_panic] + fn conflicting_data_set_with_query_system() { + fn sys(_query_1: Query>, _query_2: Query<&mut A>) {} + + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic] + fn conflicting_data_set_in_second_item_system() { + fn sys(_query_1: Query>, _query_2: Query<&mut A>) {} + + let mut world = World::default(); + run_system(&mut world, sys); + } + #[test] #[should_panic] fn conflicting_data_sets_system() { From 075b615b1ccc1035a58cda68fedb232c51fe8100 Mon Sep 17 00:00:00 2001 From: vil'mo Date: Thu, 2 Jan 2025 00:24:01 +0700 Subject: [PATCH 09/10] Resource access fix --- crates/bevy_ecs/src/query/fetch.rs | 4 ++++ crates/bevy_ecs/src/query/mod.rs | 32 +++++++++++++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 7ad7f7e1f692b..84d3fff1d3569 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1955,6 +1955,10 @@ macro_rules! impl_data_set { // This is done to avoid conflicts with other members of the set. let mut current_access = FilteredAccess::default(); $data::update_component_access($detupled, &mut current_access); + assert!(current_access.access().is_resources_compatible(&access.access()), + "{} in `DataSet` conflicts with a previous resource access in the same `DataSet`. Resources are fetched all at once, so their access can't conflict even if they are different members of `DataSet`.", + core::any::type_name::<$data>() + ); access.extend(¤t_access); )* } diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index c6c1383ceb7b9..e2b3ec6b45cb2 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -109,7 +109,7 @@ mod tests { component::{Component, ComponentId, Components, Tick}, prelude::{AnyOf, Changed, Entity, Or, QueryState, Res, ResMut, Resource, With, Without}, query::{ - ArchetypeFilter, FilteredAccess, Has, QueryCombinationIter, QueryData, + ArchetypeFilter, DataSet, FilteredAccess, Has, QueryCombinationIter, QueryData, ReadOnlyQueryData, WorldQuery, }, schedule::{IntoSystemConfigs, Schedule}, @@ -1028,4 +1028,34 @@ mod tests { .archetype_component_access() .is_compatible(write_res.archetype_component_access())); } + + #[test] + #[should_panic] + fn data_set_resource_access_conflicts() { + fn system( + _q1: Query>, + ) { + } + assert_is_system(system); + } + + #[test] + fn data_set_resource_access_without_conflicts() { + fn system( + _q1: Query>, + ) { + } + assert_is_system(system); + } + + #[test] + #[should_panic] + fn data_set_other_resource_access() { + fn system( + _r: ResMut, + _q1: Query>, + ) { + } + assert_is_system(system); + } } From d1dee59ef80007421e089d9276b0c4b0b3b1c846 Mon Sep 17 00:00:00 2001 From: vil'mo Date: Thu, 2 Jan 2025 00:26:31 +0700 Subject: [PATCH 10/10] Fmt --- crates/bevy_ecs/src/query/fetch.rs | 2 +- crates/bevy_ecs/src/query/mod.rs | 16 +++------------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 84d3fff1d3569..f6ab8bc645a85 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1956,7 +1956,7 @@ macro_rules! impl_data_set { let mut current_access = FilteredAccess::default(); $data::update_component_access($detupled, &mut current_access); assert!(current_access.access().is_resources_compatible(&access.access()), - "{} in `DataSet` conflicts with a previous resource access in the same `DataSet`. Resources are fetched all at once, so their access can't conflict even if they are different members of `DataSet`.", + "{} in `DataSet` conflicts with a previous resource access in the same `DataSet`. Resources are fetched all at once, so their access can't conflict even if they are different members of `DataSet`.", core::any::type_name::<$data>() ); access.extend(¤t_access); diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index e2b3ec6b45cb2..7bc0c9a8ad13a 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -1032,30 +1032,20 @@ mod tests { #[test] #[should_panic] fn data_set_resource_access_conflicts() { - fn system( - _q1: Query>, - ) { - } + fn system(_q1: Query>) {} assert_is_system(system); } #[test] fn data_set_resource_access_without_conflicts() { - fn system( - _q1: Query>, - ) { - } + fn system(_q1: Query>) {} assert_is_system(system); } #[test] #[should_panic] fn data_set_other_resource_access() { - fn system( - _r: ResMut, - _q1: Query>, - ) { - } + fn system(_r: ResMut, _q1: Query>) {} assert_is_system(system); } }