From 20577c85e024efaa2560b7499d64d79e9c2f810c Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Wed, 1 May 2024 20:26:19 -0400 Subject: [PATCH 1/2] Replace BuildableSystemParam trait with SystemParamBuilder trait to allow better composition of builders. --- crates/bevy_ecs/src/lib.rs | 2 +- crates/bevy_ecs/src/system/builder.rs | 447 ++++++++++++++---- crates/bevy_ecs/src/system/function_system.rs | 47 +- crates/bevy_ecs/src/system/system_param.rs | 213 ++++++--- 4 files changed, 516 insertions(+), 193 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 307c7a94a16e5..dd9ac8bcd3762 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -56,7 +56,7 @@ pub mod prelude { }, system::{ Commands, Deferred, In, IntoSystem, Local, NonSend, NonSendMut, ParallelCommands, - ParamSet, Query, ReadOnlySystem, Res, ResMut, Resource, System, SystemBuilder, + ParamSet, Query, ReadOnlySystem, Res, ResMut, Resource, System, SystemParamBuilder, SystemParamFunction, }, world::{ diff --git a/crates/bevy_ecs/src/system/builder.rs b/crates/bevy_ecs/src/system/builder.rs index f3daf8fb06e31..bb4584b31c32f 100644 --- a/crates/bevy_ecs/src/system/builder.rs +++ b/crates/bevy_ecs/src/system/builder.rs @@ -1,18 +1,24 @@ -use bevy_utils::all_tuples; +use bevy_utils::{all_tuples, synccell::SyncCell}; -use super::{ - BuildableSystemParam, FunctionSystem, Local, Res, ResMut, Resource, SystemMeta, SystemParam, - SystemParamFunction, SystemState, +use crate::{ + prelude::QueryBuilder, + query::{QueryData, QueryFilter, QueryState}, + system::{ + system_param::{Local, ParamSet, SystemParam}, + Query, SystemMeta, + }, + world::{FromWorld, World}, }; -use crate::prelude::{FromWorld, Query, World}; -use crate::query::{QueryData, QueryFilter}; +use std::fmt::Debug; -/// Builder struct used to construct state for [`SystemParam`] passed to a system. +use super::{init_query_param, Res, ResMut, Resource, SystemState}; + +/// A builder that can create a [`SystemParam`] /// /// ``` /// # use bevy_ecs::prelude::*; /// # use bevy_ecs_macros::SystemParam; -/// # use bevy_ecs::system::RunSystemOnce; +/// # use bevy_ecs::system::{RunSystemOnce, ParamBuilder, LocalBuilder, QueryParamBuilder}; /// # /// # #[derive(Component)] /// # struct A; @@ -28,121 +34,280 @@ use crate::query::{QueryData, QueryFilter}; /// # /// # let mut world = World::new(); /// # world.insert_resource(R); -/// +/// # /// fn my_system(res: Res, query: Query<&A>, param: MyParam) { /// // ... /// } /// -/// // Create a builder from the world, helper methods exist to add `SystemParam`, -/// // alternatively use `.param::()` for any other `SystemParam` types. -/// let system = SystemBuilder::<()>::new(&mut world) -/// .resource::() -/// .query::<&A>() -/// .param::() -/// .build(my_system); +/// // To build a system, create a tuple of `SystemParamBuilder`s with a builder for each param. +/// // `ParamBuilder` can be used to build a parameter using its default initialization, +/// // and has helper methods to create typed builders. +/// let system = ( +/// ParamBuilder, +/// ParamBuilder::query::<&A>(), +/// ParamBuilder::of::(), +/// ) +/// .build_state(&mut world) +/// .build_system(my_system); /// -/// // Parameters that the builder is initialised with will appear first in the arguments. -/// let system = SystemBuilder::<(Res, Query<&A>)>::new(&mut world) -/// .param::() -/// .build(my_system); +/// // Other implementations of `SystemParamBuilder` can be used to configure the parameters. +/// let system = ( +/// ParamBuilder, +/// QueryParamBuilder::new::<&A, ()>(|builder| { +/// builder.with::(); +/// }), +/// ParamBuilder, +/// ) +/// .build_state(&mut world) +/// .build_system(my_system); +/// +/// fn single_parameter_system(local: Local) { +/// // ... +/// } /// -/// // Parameters that implement `BuildableSystemParam` can use `.builder::()` to build in place. -/// let system = SystemBuilder::<()>::new(&mut world) -/// .resource::() -/// .builder::>(|builder| { builder.with::(); }) -/// .param::() -/// .build(my_system); +/// // Note that the builder for a system must be a tuple, even if there is only one parameter. +/// let system = (LocalBuilder(2),) +/// .build_state(&mut world) +/// .build_system(single_parameter_system); /// /// world.run_system_once(system); ///``` -pub struct SystemBuilder<'w, T: SystemParam = ()> { - pub(crate) meta: SystemMeta, - pub(crate) state: T::State, - pub(crate) world: &'w mut World, +/// +/// # Safety +/// +/// The implementor must ensure the following is true. +/// - [`SystemParamBuilder::build`] correctly registers all [`World`] accesses used +/// by [`SystemParam::get_param`] with the provided [`system_meta`](SystemMeta). +/// - None of the world accesses may conflict with any prior accesses registered +/// on `system_meta`. +/// +/// Note that this depends on the implementation of [`SystemParam::get_param`], +/// so if `Self` is not a local type then you must call [`SystemParam::init_state`] +/// or another [`SystemParamBuilder::build`] +pub unsafe trait SystemParamBuilder: Sized { + /// Registers any [`World`] access used by this [`SystemParam`] + /// and creates a new instance of this param's [`State`](SystemParam::State). + fn build(self, world: &mut World, meta: &mut SystemMeta) -> P::State; + + /// Create a [`SystemState`] from a [`SystemParamBuilder`]. + /// To create a system, call [`SystemState::build_system`] on the result. + fn build_state(self, world: &mut World) -> SystemState

{ + SystemState::from_builder(world, self) + } } -impl<'w, T: SystemParam> SystemBuilder<'w, T> { - /// Construct a new builder with the default state for `T` - pub fn new(world: &'w mut World) -> Self { - let mut meta = SystemMeta::new::(); - Self { - state: T::init_state(world, &mut meta), - meta, - world, - } +/// A [`SystemParamBuilder`] for any [`SystemParam`] that uses its default initialization. +#[derive(Default, Debug, Copy, Clone)] +pub struct ParamBuilder; + +// SAFETY: Calls `SystemParam::init_state` +unsafe impl SystemParamBuilder

for ParamBuilder { + fn build(self, world: &mut World, meta: &mut SystemMeta) -> P::State { + P::init_state(world, meta) } +} - /// Construct the a system with the built params - pub fn build(self, func: F) -> FunctionSystem - where - F: SystemParamFunction, +impl ParamBuilder { + /// Creates a [`SystemParamBuilder`] for any [`SystemParam`] that uses its default initialization. + pub fn of() -> impl SystemParamBuilder { + Self + } + + /// Helper method for reading a [`Resource`] as a param, equivalent to `of::>()` + pub fn resource<'w, T: Resource>() -> impl SystemParamBuilder> { + Self + } + + /// Helper method for mutably accessing a [`Resource`] as a param, equivalent to `of::>()` + pub fn resource_mut<'w, T: Resource>() -> impl SystemParamBuilder> { + Self + } + + /// Helper method for adding a [`Local`] as a param, equivalent to `of::>()` + pub fn local<'s, T: FromWorld + Send + 'static>() -> impl SystemParamBuilder> { + Self + } + + /// Helper method for adding a [`Query`] as a param, equivalent to `of::>()` + pub fn query<'w, 's, D: QueryData + 'static>() -> impl SystemParamBuilder> { - FunctionSystem::from_builder(self, func) + Self } - /// Return the constructed [`SystemState`] - pub fn state(self) -> SystemState { - SystemState::from_builder(self) + /// Helper method for adding a filtered [`Query`] as a param, equivalent to `of::>()` + pub fn query_filtered<'w, 's, D: QueryData + 'static, F: QueryFilter + 'static>( + ) -> impl SystemParamBuilder> { + Self } } -macro_rules! impl_system_builder { - ($($curr: ident),*) => { - impl<'w, $($curr: SystemParam,)*> SystemBuilder<'w, ($($curr,)*)> { - /// Add `T` as a parameter built from the world - pub fn param(mut self) -> SystemBuilder<'w, ($($curr,)* T,)> { - #[allow(non_snake_case)] - let ($($curr,)*) = self.state; - SystemBuilder { - state: ($($curr,)* T::init_state(self.world, &mut self.meta),), - meta: self.meta, - world: self.world, - } - } +// SAFETY: Calls `init_query_param`, just like `Query::init_state`. +unsafe impl<'w, 's, D: QueryData + 'static, F: QueryFilter + 'static> + SystemParamBuilder> for QueryState +{ + fn build(self, world: &mut World, system_meta: &mut SystemMeta) -> QueryState { + self.validate_world(world.id()); + init_query_param(world, system_meta, &self); + self + } +} - /// Helper method for reading a [`Resource`] as a param, equivalent to `.param::>()` - pub fn resource(self) -> SystemBuilder<'w, ($($curr,)* Res<'static, T>,)> { - self.param::>() - } +/// A [`SystemParamBuilder`] for a [`Query`]. +pub struct QueryParamBuilder(T); - /// Helper method for mutably accessing a [`Resource`] as a param, equivalent to `.param::>()` - pub fn resource_mut(self) -> SystemBuilder<'w, ($($curr,)* ResMut<'static, T>,)> { - self.param::>() - } +impl QueryParamBuilder { + /// Creates a [`SystemParamBuilder`] for a [`Query`] that accepts a callback to configure the [`QueryBuilder`]. + pub fn new(f: T) -> Self + where + T: FnOnce(&mut QueryBuilder), + { + Self(f) + } +} - /// Helper method for adding a [`Local`] as a param, equivalent to `.param::>()` - pub fn local(self) -> SystemBuilder<'w, ($($curr,)* Local<'static, T>,)> { - self.param::>() - } +impl<'a, D: QueryData, F: QueryFilter> + QueryParamBuilder) + 'a>> +{ + /// Creates a [`SystemParamBuilder`] for a [`Query`] that accepts a callback to configure the [`QueryBuilder`]. + /// This boxes the callback so that it has a common type and can be put in a `Vec`. + pub fn new_box(f: impl FnOnce(&mut QueryBuilder) + 'a) -> Self { + Self(Box::new(f)) + } +} - /// Helper method for adding a [`Query`] as a param, equivalent to `.param::>()` - pub fn query(self) -> SystemBuilder<'w, ($($curr,)* Query<'static, 'static, D, ()>,)> { - self.query_filtered::() - } +// SAFETY: Calls `init_query_param`, just like `Query::init_state`. +unsafe impl< + 'w, + 's, + D: QueryData + 'static, + F: QueryFilter + 'static, + T: FnOnce(&mut QueryBuilder), + > SystemParamBuilder> for QueryParamBuilder +{ + fn build(self, world: &mut World, system_meta: &mut SystemMeta) -> QueryState { + let mut builder = QueryBuilder::new(world); + (self.0)(&mut builder); + let state = builder.build(); + init_query_param(world, system_meta, &state); + state + } +} - /// Helper method for adding a filtered [`Query`] as a param, equivalent to `.param::>()` - pub fn query_filtered(self) -> SystemBuilder<'w, ($($curr,)* Query<'static, 'static, D, F>,)> { - self.param::>() +macro_rules! impl_system_param_builder_tuple { + ($(($param: ident, $builder: ident)),*) => { + // SAFETY: implementors of each `SystemParamBuilder` in the tuple have validated their impls + unsafe impl<$($param: SystemParam,)* $($builder: SystemParamBuilder<$param>,)*> SystemParamBuilder<($($param,)*)> for ($($builder,)*) { + fn build(self, _world: &mut World, _meta: &mut SystemMeta) -> <($($param,)*) as SystemParam>::State { + #[allow(non_snake_case)] + let ($($builder,)*) = self; + #[allow(clippy::unused_unit)] + ($($builder.build(_world, _meta),)*) } + } + }; +} - /// Add `T` as a parameter built with the given function - pub fn builder( - mut self, - func: impl FnOnce(&mut T::Builder<'_>), - ) -> SystemBuilder<'w, ($($curr,)* T,)> { - #[allow(non_snake_case)] - let ($($curr,)*) = self.state; - SystemBuilder { - state: ($($curr,)* T::build(self.world, &mut self.meta, func),), - meta: self.meta, - world: self.world, +all_tuples!(impl_system_param_builder_tuple, 0, 16, P, B); + +// SAFETY: implementors of each `SystemParamBuilder` in the vec have validated their impls +unsafe impl> SystemParamBuilder> for Vec { + fn build(self, world: &mut World, meta: &mut SystemMeta) -> as SystemParam>::State { + self.into_iter() + .map(|builder| builder.build(world, meta)) + .collect() + } +} + +/// A [`SystemParamBuilder`] for a [`ParamSet`]. +/// To build a [`ParamSet`] with a tuple of system parameters, pass a tuple of matching [`SystemParamBuilder`]s. +/// To build a [`ParamSet`] with a `Vec` of system parameters, pass a `Vec` of matching [`SystemParamBuilder`]s. +pub struct ParamSetBuilder(T); + +macro_rules! impl_param_set_builder_tuple { + ($(($param: ident, $builder: ident, $meta: ident)),*) => { + // SAFETY: implementors of each `SystemParamBuilder` in the tuple have validated their impls + unsafe impl<'w, 's, $($param: SystemParam,)* $($builder: SystemParamBuilder<$param>,)*> SystemParamBuilder> for ParamSetBuilder<($($builder,)*)> { + #[allow(non_snake_case)] + fn build(self, _world: &mut World, _system_meta: &mut SystemMeta) -> <($($param,)*) as SystemParam>::State { + let ParamSetBuilder(($($builder,)*)) = self; + // Note that this is slightly different from `init_state`, which calls `init_state` on each param twice. + // One call populates an empty `SystemMeta` with the new access, while the other runs against a cloned `SystemMeta` to check for conflicts. + // Builders can only be invoked once, so we do both in a single call here. + // That means that any `filtered_accesses` in the `component_access_set` will get copied to every `$meta` + // and will appear multiple times in the final `SystemMeta`. + $( + let mut $meta = _system_meta.clone(); + let $param = $builder.build(_world, &mut $meta); + )* + // Make the ParamSet non-send if any of its parameters are non-send. + if false $(|| !$meta.is_send())* { + _system_meta.set_non_send(); } + $( + _system_meta + .component_access_set + .extend($meta.component_access_set); + _system_meta + .archetype_component_access + .extend(&$meta.archetype_component_access); + )* + #[allow(clippy::unused_unit)] + ($($param,)*) } } }; } -all_tuples!(impl_system_builder, 0, 15, P); +all_tuples!(impl_param_set_builder_tuple, 1, 8, P, B, meta); + +// SAFETY: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts +// with any prior access, a panic will occur. +unsafe impl<'w, 's, P: SystemParam, B: SystemParamBuilder

> + SystemParamBuilder>> for ParamSetBuilder> +{ + fn build( + self, + world: &mut World, + system_meta: &mut SystemMeta, + ) -> as SystemParam>::State { + let mut states = Vec::with_capacity(self.0.len()); + let mut metas = Vec::with_capacity(self.0.len()); + for builder in self.0 { + let mut meta = system_meta.clone(); + states.push(builder.build(world, &mut meta)); + metas.push(meta); + } + if metas.iter().any(|m| !m.is_send()) { + system_meta.set_non_send(); + } + for meta in metas { + system_meta + .component_access_set + .extend(meta.component_access_set); + system_meta + .archetype_component_access + .extend(&meta.archetype_component_access); + } + states + } +} + +/// A [`SystemParamBuilder`] for a [`Local`]. +/// The provided value will be used as the initial value of the `Local`. +pub struct LocalBuilder(pub T); + +// SAFETY: `Local` performs no world access. +unsafe impl<'s, T: FromWorld + Send + 'static> SystemParamBuilder> + for LocalBuilder +{ + fn build( + self, + _world: &mut World, + _meta: &mut SystemMeta, + ) -> as SystemParam>::State { + SyncCell::new(self.0) + } +} #[cfg(test)] mod tests { @@ -155,6 +320,12 @@ mod tests { #[derive(Component)] struct A; + #[derive(Component)] + struct B; + + #[derive(Component)] + struct C; + fn local_system(local: Local) -> u64 { *local } @@ -171,9 +342,9 @@ mod tests { fn local_builder() { let mut world = World::new(); - let system = SystemBuilder::<()>::new(&mut world) - .builder::>(|x| *x = 10) - .build(local_system); + let system = (LocalBuilder(10),) + .build_state(&mut world) + .build_system(local_system); let result = world.run_system_once(system); assert_eq!(result, 10); @@ -186,11 +357,26 @@ mod tests { world.spawn(A); world.spawn_empty(); - let system = SystemBuilder::<()>::new(&mut world) - .builder::>(|query| { - query.with::(); - }) - .build(query_system); + let system = (QueryParamBuilder::new(|query| { + query.with::(); + }),) + .build_state(&mut world) + .build_system(query_system); + + let result = world.run_system_once(system); + assert_eq!(result, 1); + } + + #[test] + fn query_builder_state() { + let mut world = World::new(); + + world.spawn(A); + world.spawn_empty(); + + let state = QueryBuilder::new(&mut world).with::().build(); + + let system = (state,).build_state(&mut world).build_system(query_system); let result = world.run_system_once(system); assert_eq!(result, 1); @@ -203,12 +389,67 @@ mod tests { world.spawn(A); world.spawn_empty(); - let system = SystemBuilder::<()>::new(&mut world) - .local::() - .param::>() - .build(multi_param_system); + let system = (LocalBuilder(0), ParamBuilder) + .build_state(&mut world) + .build_system(multi_param_system); let result = world.run_system_once(system); assert_eq!(result, 1); } + + #[test] + fn param_set_builder() { + let mut world = World::new(); + + world.spawn((A, B, C)); + world.spawn((A, B)); + world.spawn((A, C)); + world.spawn((A, C)); + world.spawn_empty(); + + let system = (ParamSetBuilder(( + QueryParamBuilder::new(|builder| { + builder.with::(); + }), + QueryParamBuilder::new(|builder| { + builder.with::(); + }), + )),) + .build_state(&mut world) + .build_system(|mut params: ParamSet<(Query<&mut A>, Query<&mut A>)>| { + params.p0().iter().count() + params.p1().iter().count() + }); + + let result = world.run_system_once(system); + assert_eq!(result, 5); + } + + #[test] + fn param_set_vec_builder() { + let mut world = World::new(); + + world.spawn((A, B, C)); + world.spawn((A, B)); + world.spawn((A, C)); + world.spawn((A, C)); + world.spawn_empty(); + + let system = (ParamSetBuilder(vec![ + QueryParamBuilder::new_box(|builder| { + builder.with::(); + }), + QueryParamBuilder::new_box(|builder| { + builder.with::(); + }), + ]),) + .build_state(&mut world) + .build_system(|mut params: ParamSet>>| { + let mut count = 0; + params.for_each(|mut query| count += query.iter_mut().count()); + count + }); + + let result = world.run_system_once(system); + assert_eq!(result, 5); + } } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index bf2b2c4fd5dbc..5f11300beba88 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -14,7 +14,7 @@ use std::{borrow::Cow, marker::PhantomData}; #[cfg(feature = "trace")] use bevy_utils::tracing::{info_span, Span}; -use super::{In, IntoSystem, ReadOnlySystem, SystemBuilder}; +use super::{In, IntoSystem, ReadOnlySystem, SystemParamBuilder}; /// The metadata of a [`System`]. #[derive(Clone)] @@ -202,16 +202,34 @@ impl SystemState { } } - // Create a [`SystemState`] from a [`SystemBuilder`] - pub(crate) fn from_builder(builder: SystemBuilder) -> Self { + /// Create a [`SystemState`] from a [`SystemParamBuilder`] + pub(crate) fn from_builder(world: &mut World, builder: impl SystemParamBuilder) -> Self { + let mut meta = SystemMeta::new::(); + meta.last_run = world.change_tick().relative_to(Tick::MAX); + let param_state = builder.build(world, &mut meta); Self { - meta: builder.meta, - param_state: builder.state, - world_id: builder.world.id(), + meta, + param_state, + world_id: world.id(), archetype_generation: ArchetypeGeneration::initial(), } } + /// Create a [`FunctionSystem`] from a [`SystemState`]. + pub fn build_system>( + self, + func: F, + ) -> FunctionSystem { + FunctionSystem { + func, + param_state: Some(self.param_state), + system_meta: self.meta, + world_id: Some(self.world_id), + archetype_generation: self.archetype_generation, + marker: PhantomData, + } + } + /// Gets the metadata for this instance. #[inline] pub fn meta(&self) -> &SystemMeta { @@ -407,23 +425,6 @@ where marker: PhantomData Marker>, } -impl FunctionSystem -where - F: SystemParamFunction, -{ - // Create a [`FunctionSystem`] from a [`SystemBuilder`] - pub(crate) fn from_builder(builder: SystemBuilder, func: F) -> Self { - Self { - func, - param_state: Some(builder.state), - system_meta: builder.meta, - world_id: Some(builder.world.id()), - archetype_generation: ArchetypeGeneration::initial(), - marker: PhantomData, - } - } -} - // De-initializes the cloned system. impl Clone for FunctionSystem where diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index aae8e4e2e26f3..b1811d9830011 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -5,7 +5,6 @@ use crate::{ change_detection::{Ticks, TicksMut}, component::{ComponentId, ComponentTicks, Components, Tick}, entity::Entities, - prelude::QueryBuilder, query::{ Access, FilteredAccess, FilteredAccessSet, QueryData, QueryFilter, QueryState, ReadOnlyQueryData, @@ -181,19 +180,6 @@ pub unsafe trait SystemParam: Sized { ) -> Self::Item<'world, 'state>; } -/// A parameter that can be built with [`SystemBuilder`](crate::system::builder::SystemBuilder) -pub trait BuildableSystemParam: SystemParam { - /// A mutable reference to this type will be passed to the builder function - type Builder<'b>; - - /// Constructs [`SystemParam::State`] for `Self` using a given builder function - fn build( - world: &mut World, - meta: &mut SystemMeta, - func: impl FnOnce(&mut Self::Builder<'_>), - ) -> Self::State; -} - /// A [`SystemParam`] that only reads a given [`World`]. /// /// # Safety @@ -217,17 +203,7 @@ unsafe impl SystemParam for Qu fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { let state = QueryState::new_with_access(world, &mut system_meta.archetype_component_access); - assert_component_access_compatibility( - &system_meta.name, - std::any::type_name::(), - std::any::type_name::(), - &system_meta.component_access_set, - &state.component_access, - world, - ); - system_meta - .component_access_set - .add(state.component_access.clone()); + init_query_param(world, system_meta, &state); state } @@ -253,33 +229,22 @@ unsafe impl SystemParam for Qu } } -impl<'w, 's, D: QueryData + 'static, F: QueryFilter + 'static> BuildableSystemParam - for Query<'w, 's, D, F> -{ - type Builder<'b> = QueryBuilder<'b, D, F>; - - #[inline] - fn build( - world: &mut World, - system_meta: &mut SystemMeta, - build: impl FnOnce(&mut Self::Builder<'_>), - ) -> Self::State { - let mut builder = QueryBuilder::new(world); - build(&mut builder); - let state = builder.build(); - assert_component_access_compatibility( - &system_meta.name, - std::any::type_name::(), - std::any::type_name::(), - &system_meta.component_access_set, - &state.component_access, - world, - ); - system_meta - .component_access_set - .add(state.component_access.clone()); - state - } +pub(crate) fn init_query_param( + world: &mut World, + system_meta: &mut SystemMeta, + state: &QueryState, +) { + assert_component_access_compatibility( + &system_meta.name, + std::any::type_name::(), + std::any::type_name::(), + &system_meta.component_access_set, + &state.component_access, + world, + ); + system_meta + .component_access_set + .add(state.component_access.clone()); } fn assert_component_access_compatibility( @@ -851,20 +816,6 @@ unsafe impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> { } } -impl<'w, T: FromWorld + Send + 'static> BuildableSystemParam for Local<'w, T> { - type Builder<'b> = T; - - fn build( - world: &mut World, - _meta: &mut SystemMeta, - func: impl FnOnce(&mut Self::Builder<'_>), - ) -> Self::State { - let mut value = T::from_world(world); - func(&mut value); - SyncCell::new(value) - } -} - /// Types that can be used with [`Deferred`] in systems. /// This allows storing system-local data which is used to defer [`World`] mutations. /// @@ -1427,6 +1378,136 @@ unsafe impl SystemParam for SystemChangeTick { } } +// SAFETY: `init_state` does no access. The interesting safety checks are on the `SystemParamBuilder`. +unsafe impl SystemParam for Vec { + type State = Vec; + + type Item<'world, 'state> = Vec>; + + fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { + Vec::new() + } + + unsafe fn get_param<'world, 'state>( + state: &'state mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'world>, + change_tick: Tick, + ) -> Self::Item<'world, 'state> { + state + .iter_mut() + // SAFETY: + // - We initialized the state for each parameter in the builder, so the caller ensures we have access to any world data needed by each param. + // - The caller ensures this was the world used to initialize our state, and we used that world to initialize parameter states + .map(|state| unsafe { T::get_param(state, system_meta, world, change_tick) }) + .collect() + } + + unsafe fn new_archetype( + state: &mut Self::State, + archetype: &Archetype, + system_meta: &mut SystemMeta, + ) { + for state in state { + // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. + unsafe { T::new_archetype(state, archetype, system_meta) }; + } + } + + fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { + for state in state { + T::apply(state, system_meta, world); + } + } + + fn queue(state: &mut Self::State, system_meta: &SystemMeta, mut world: DeferredWorld) { + for state in state { + T::queue(state, system_meta, world.reborrow()); + } + } +} + +// SAFETY: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts +// with any prior access, a panic will occur. +unsafe impl SystemParam for ParamSet<'_, '_, Vec> { + type State = Vec; + + type Item<'world, 'state> = ParamSet<'world, 'state, Vec>; + + fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { + Vec::new() + } + + unsafe fn get_param<'world, 'state>( + state: &'state mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'world>, + change_tick: Tick, + ) -> Self::Item<'world, 'state> { + ParamSet { + param_states: state, + system_meta: system_meta.clone(), + world, + change_tick, + } + } + + unsafe fn new_archetype( + state: &mut Self::State, + archetype: &Archetype, + system_meta: &mut SystemMeta, + ) { + for state in state { + // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. + unsafe { T::new_archetype(state, archetype, system_meta) } + } + } + + fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { + for state in state { + T::apply(state, system_meta, world); + } + } + + fn queue(state: &mut Self::State, system_meta: &SystemMeta, mut world: DeferredWorld) { + for state in state { + T::queue(state, system_meta, world.reborrow()); + } + } +} + +impl ParamSet<'_, '_, Vec> { + /// Accesses the parameter at the given index. + /// No other parameters may be accessed while this one is active. + pub fn get_mut(&mut self, index: usize) -> T::Item<'_, '_> { + // SAFETY: + // - We initialized the state for each parameter in the builder, so the caller ensures we have access to any world data needed by any param. + // We have mutable access to the ParamSet, so no other params in the set are active. + // - The caller of `get_param` ensured that this was the world used to initialize our state, and we used that world to initialize parameter states + unsafe { + T::get_param( + &mut self.param_states[index], + &self.system_meta, + self.world, + self.change_tick, + ) + } + } + + /// Calls a closure for each parameter in the set. + pub fn for_each(&mut self, mut f: impl FnMut(T::Item<'_, '_>)) { + self.param_states.iter_mut().for_each(|state| { + f( + // SAFETY: + // - We initialized the state for each parameter in the builder, so the caller ensures we have access to any world data needed by any param. + // We have mutable access to the ParamSet, so no other params in the set are active. + // - The caller of `get_param` ensured that this was the world used to initialize our state, and we used that world to initialize parameter states + unsafe { T::get_param(state, &self.system_meta, self.world, self.change_tick) }, + ); + }); + } +} + macro_rules! impl_system_param_tuple { ($($param: ident),*) => { // SAFETY: tuple consists only of ReadOnlySystemParams From f63e49e91c38afab3ab7257789d67b5b20379752 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Mon, 15 Jul 2024 07:16:22 -0400 Subject: [PATCH 2/2] Remove SystemParamBuilder impls for Vec and ParamSet. --- crates/bevy_ecs/src/system/builder.rs | 70 ----------- crates/bevy_ecs/src/system/system_param.rs | 130 --------------------- 2 files changed, 200 deletions(-) diff --git a/crates/bevy_ecs/src/system/builder.rs b/crates/bevy_ecs/src/system/builder.rs index bb4584b31c32f..520d73f4e9cdc 100644 --- a/crates/bevy_ecs/src/system/builder.rs +++ b/crates/bevy_ecs/src/system/builder.rs @@ -209,15 +209,6 @@ macro_rules! impl_system_param_builder_tuple { all_tuples!(impl_system_param_builder_tuple, 0, 16, P, B); -// SAFETY: implementors of each `SystemParamBuilder` in the vec have validated their impls -unsafe impl> SystemParamBuilder> for Vec { - fn build(self, world: &mut World, meta: &mut SystemMeta) -> as SystemParam>::State { - self.into_iter() - .map(|builder| builder.build(world, meta)) - .collect() - } -} - /// A [`SystemParamBuilder`] for a [`ParamSet`]. /// To build a [`ParamSet`] with a tuple of system parameters, pass a tuple of matching [`SystemParamBuilder`]s. /// To build a [`ParamSet`] with a `Vec` of system parameters, pass a `Vec` of matching [`SystemParamBuilder`]s. @@ -260,38 +251,6 @@ macro_rules! impl_param_set_builder_tuple { all_tuples!(impl_param_set_builder_tuple, 1, 8, P, B, meta); -// SAFETY: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts -// with any prior access, a panic will occur. -unsafe impl<'w, 's, P: SystemParam, B: SystemParamBuilder

> - SystemParamBuilder>> for ParamSetBuilder> -{ - fn build( - self, - world: &mut World, - system_meta: &mut SystemMeta, - ) -> as SystemParam>::State { - let mut states = Vec::with_capacity(self.0.len()); - let mut metas = Vec::with_capacity(self.0.len()); - for builder in self.0 { - let mut meta = system_meta.clone(); - states.push(builder.build(world, &mut meta)); - metas.push(meta); - } - if metas.iter().any(|m| !m.is_send()) { - system_meta.set_non_send(); - } - for meta in metas { - system_meta - .component_access_set - .extend(meta.component_access_set); - system_meta - .archetype_component_access - .extend(&meta.archetype_component_access); - } - states - } -} - /// A [`SystemParamBuilder`] for a [`Local`]. /// The provided value will be used as the initial value of the `Local`. pub struct LocalBuilder(pub T); @@ -423,33 +382,4 @@ mod tests { let result = world.run_system_once(system); assert_eq!(result, 5); } - - #[test] - fn param_set_vec_builder() { - let mut world = World::new(); - - world.spawn((A, B, C)); - world.spawn((A, B)); - world.spawn((A, C)); - world.spawn((A, C)); - world.spawn_empty(); - - let system = (ParamSetBuilder(vec![ - QueryParamBuilder::new_box(|builder| { - builder.with::(); - }), - QueryParamBuilder::new_box(|builder| { - builder.with::(); - }), - ]),) - .build_state(&mut world) - .build_system(|mut params: ParamSet>>| { - let mut count = 0; - params.for_each(|mut query| count += query.iter_mut().count()); - count - }); - - let result = world.run_system_once(system); - assert_eq!(result, 5); - } } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index b1811d9830011..afa42b9a76c65 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1378,136 +1378,6 @@ unsafe impl SystemParam for SystemChangeTick { } } -// SAFETY: `init_state` does no access. The interesting safety checks are on the `SystemParamBuilder`. -unsafe impl SystemParam for Vec { - type State = Vec; - - type Item<'world, 'state> = Vec>; - - fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { - Vec::new() - } - - unsafe fn get_param<'world, 'state>( - state: &'state mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'world>, - change_tick: Tick, - ) -> Self::Item<'world, 'state> { - state - .iter_mut() - // SAFETY: - // - We initialized the state for each parameter in the builder, so the caller ensures we have access to any world data needed by each param. - // - The caller ensures this was the world used to initialize our state, and we used that world to initialize parameter states - .map(|state| unsafe { T::get_param(state, system_meta, world, change_tick) }) - .collect() - } - - unsafe fn new_archetype( - state: &mut Self::State, - archetype: &Archetype, - system_meta: &mut SystemMeta, - ) { - for state in state { - // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { T::new_archetype(state, archetype, system_meta) }; - } - } - - fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { - for state in state { - T::apply(state, system_meta, world); - } - } - - fn queue(state: &mut Self::State, system_meta: &SystemMeta, mut world: DeferredWorld) { - for state in state { - T::queue(state, system_meta, world.reborrow()); - } - } -} - -// SAFETY: Relevant parameter ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any ParamState conflicts -// with any prior access, a panic will occur. -unsafe impl SystemParam for ParamSet<'_, '_, Vec> { - type State = Vec; - - type Item<'world, 'state> = ParamSet<'world, 'state, Vec>; - - fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { - Vec::new() - } - - unsafe fn get_param<'world, 'state>( - state: &'state mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'world>, - change_tick: Tick, - ) -> Self::Item<'world, 'state> { - ParamSet { - param_states: state, - system_meta: system_meta.clone(), - world, - change_tick, - } - } - - unsafe fn new_archetype( - state: &mut Self::State, - archetype: &Archetype, - system_meta: &mut SystemMeta, - ) { - for state in state { - // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { T::new_archetype(state, archetype, system_meta) } - } - } - - fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { - for state in state { - T::apply(state, system_meta, world); - } - } - - fn queue(state: &mut Self::State, system_meta: &SystemMeta, mut world: DeferredWorld) { - for state in state { - T::queue(state, system_meta, world.reborrow()); - } - } -} - -impl ParamSet<'_, '_, Vec> { - /// Accesses the parameter at the given index. - /// No other parameters may be accessed while this one is active. - pub fn get_mut(&mut self, index: usize) -> T::Item<'_, '_> { - // SAFETY: - // - We initialized the state for each parameter in the builder, so the caller ensures we have access to any world data needed by any param. - // We have mutable access to the ParamSet, so no other params in the set are active. - // - The caller of `get_param` ensured that this was the world used to initialize our state, and we used that world to initialize parameter states - unsafe { - T::get_param( - &mut self.param_states[index], - &self.system_meta, - self.world, - self.change_tick, - ) - } - } - - /// Calls a closure for each parameter in the set. - pub fn for_each(&mut self, mut f: impl FnMut(T::Item<'_, '_>)) { - self.param_states.iter_mut().for_each(|state| { - f( - // SAFETY: - // - We initialized the state for each parameter in the builder, so the caller ensures we have access to any world data needed by any param. - // We have mutable access to the ParamSet, so no other params in the set are active. - // - The caller of `get_param` ensured that this was the world used to initialize our state, and we used that world to initialize parameter states - unsafe { T::get_param(state, &self.system_meta, self.world, self.change_tick) }, - ); - }); - } -} - macro_rules! impl_system_param_tuple { ($($param: ident),*) => { // SAFETY: tuple consists only of ReadOnlySystemParams