Skip to content

Commit

Permalink
Remove the SystemParamState trait and remove types like ResState (b…
Browse files Browse the repository at this point in the history 
…evyengine#6919)

Spiritual successor to bevyengine#5205.
Actual successor to bevyengine#6865.

# Objective

Currently, system params are defined using three traits: `SystemParam`, `ReadOnlySystemParam`, `SystemParamState`. The behavior for each param is specified by the `SystemParamState` trait, while `SystemParam` simply defers to the state.

Splitting the traits in this way makes it easier to implement within macros, but it increases the cognitive load. Worst of all, this approach requires each `MySystemParam` to have a public `MySystemParamState` type associated with it.

## Solution

* Merge the trait `SystemParamState` into `SystemParam`.
* Remove all trivial `SystemParam` state types. 
  * `OptionNonSendMutState<T>`: you will not be missed.

---

- [x] Fix/resolve the remaining test failure.

## Changelog

* Removed the trait `SystemParamState`, merging its functionality into `SystemParam`.

## Migration Guide

**Note**: this should replace the migration guide for bevyengine#6865.
This is relative to Bevy 0.9, not main.

The traits `SystemParamState` and `SystemParamFetch` have been removed, and their functionality has been transferred to `SystemParam`.


```rust
// Before (0.9)
impl SystemParam for MyParam<'_, '_> {
    type State = MyParamState;
}
unsafe impl SystemParamState for MyParamState {
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
}
unsafe impl<'w, 's> SystemParamFetch<'w, 's> for MyParamState {
    type Item = MyParam<'w, 's>;
    fn get_param(&mut self, ...) -> Self::Item;
}
unsafe impl ReadOnlySystemParamFetch for MyParamState { }

// After (0.10)
unsafe impl SystemParam for MyParam<'_, '_> {
    type State = MyParamState;
    type Item<'w, 's> = MyParam<'w, 's>;
    fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { ... }
    fn get_param<'w, 's>(state: &mut Self::State, ...) -> Self::Item<'w, 's>;
}
unsafe impl ReadOnlySystemParam for MyParam<'_, '_> { }
```

The trait `ReadOnlySystemParamFetch` has been replaced with `ReadOnlySystemParam`.

```rust
// Before
unsafe impl ReadOnlySystemParamFetch for MyParamState {}

// After
unsafe impl ReadOnlySystemParam for MyParam<'_, '_> {}
```
  • Loading branch information
@JoJoJet @james7132
JoJoJet authored and james7132 committed Jan 21, 2023
1 parent 5fa57ab commit 563cb2e
Show file tree
Hide file tree
Showing 7 changed files with 288 additions and 527 deletions.
94 changes: 37 additions & 57 deletions crates/bevy_ecs/macros/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,6 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
let mut tokens = TokenStream::new();
let max_params = 8;
let params = get_idents(|i| format!("P{i}"), max_params);
let params_state = get_idents(|i| format!("PF{i}"), max_params);
let metas = get_idents(|i| format!("m{i}"), max_params);
let mut param_fn_muts = Vec::new();
for (i, param) in params.iter().enumerate() {
Expand All @@ -238,44 +237,37 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
// Conflicting params in ParamSet are not accessible at the same time
// ParamSets are guaranteed to not conflict with other SystemParams
unsafe {
<#param::State as SystemParamState>::get_param(&mut self.param_states.#index, &self.system_meta, self.world, self.change_tick)
#param::get_param(&mut self.param_states.#index, &self.system_meta, self.world, self.change_tick)
}
}
});
}

for param_count in 1..=max_params {
let param = &params[0..param_count];
let param_state = &params_state[0..param_count];
let meta = &metas[0..param_count];
let param_fn_mut = &param_fn_muts[0..param_count];
tokens.extend(TokenStream::from(quote! {
impl<'w, 's, #(#param: SystemParam,)*> SystemParam for ParamSet<'w, 's, (#(#param,)*)>
{
type State = ParamSetState<(#(#param::State,)*)>;
}

// SAFETY: All parameters are constrained to ReadOnlyState, so World is only read

// SAFETY: All parameters are constrained to ReadOnlySystemParam, so World is only read
unsafe impl<'w, 's, #(#param,)*> ReadOnlySystemParam for ParamSet<'w, 's, (#(#param,)*)>
where #(#param: ReadOnlySystemParam,)*
{ }

// 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<#(#param_state: SystemParamState,)*> SystemParamState for ParamSetState<(#(#param_state,)*)>
unsafe impl<'_w, '_s, #(#param: SystemParam,)*> SystemParam for ParamSet<'_w, '_s, (#(#param,)*)>
{
type Item<'w, 's> = ParamSet<'w, 's, (#(<#param_state as SystemParamState>::Item::<'w, 's>,)*)>;
type State = (#(#param::State,)*);
type Item<'w, 's> = ParamSet<'w, 's, (#(#param,)*)>;

fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self {
fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State {
#(
// Pretend to add each param to the system alone, see if it conflicts
let mut #meta = system_meta.clone();
#meta.component_access_set.clear();
#meta.archetype_component_access.clear();
#param_state::init(world, &mut #meta);
let #param = #param_state::init(world, &mut system_meta.clone());
#param::init_state(world, &mut #meta);
let #param = #param::init_state(world, &mut system_meta.clone());
)*
#(
system_meta
Expand All @@ -285,29 +277,26 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
.archetype_component_access
.extend(&#meta.archetype_component_access);
)*
ParamSetState((#(#param,)*))
(#(#param,)*)
}

fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta) {
let (#(#param,)*) = &mut self.0;
#(
#param.new_archetype(archetype, system_meta);
)*
fn new_archetype(state: &mut Self::State, archetype: &Archetype, system_meta: &mut SystemMeta) {
<(#(#param,)*) as SystemParam>::new_archetype(state, archetype, system_meta);
}

fn apply(&mut self, system_meta: &SystemMeta, world: &mut World) {
self.0.apply(system_meta, world)
fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) {
<(#(#param,)*) as SystemParam>::apply(state, system_meta, world);
}

#[inline]
unsafe fn get_param<'w, 's>(
state: &'s mut Self,
state: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
change_tick: u32,
) -> Self::Item<'w, 's> {
ParamSet {
param_states: &mut state.0,
param_states: state,
system_meta: system_meta.clone(),
world,
change_tick,
Expand All @@ -317,7 +306,6 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {

impl<'w, 's, #(#param: SystemParam,)*> ParamSet<'w, 's, (#(#param,)*)>
{

#(#param_fn_mut)*
}
}));
Expand Down Expand Up @@ -411,14 +399,20 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
}
}

let (impl_generics, ty_generics, where_clause) = generics.split_for_impl();
let (_impl_generics, ty_generics, where_clause) = generics.split_for_impl();

let lifetimeless_generics: Vec<_> = generics
.params
.iter()
.filter(|g| !matches!(g, GenericParam::Lifetime(_)))
.collect();

let mut shadowed_lifetimes: Vec<_> = generics.lifetimes().map(|x| x.lifetime.clone()).collect();
for lifetime in &mut shadowed_lifetimes {
let shadowed_ident = format_ident!("_{}", lifetime.ident);
lifetime.ident = shadowed_ident;
}

let mut punctuated_generics = Punctuated::<_, Token![,]>::new();
punctuated_generics.extend(lifetimeless_generics.iter().map(|g| match g {
GenericParam::Type(g) => GenericParam::Type(TypeParam {
Expand All @@ -432,15 +426,6 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
_ => unreachable!(),
}));

let mut punctuated_generics_no_bounds = punctuated_generics.clone();
for g in &mut punctuated_generics_no_bounds {
match g {
GenericParam::Type(g) => g.bounds.clear(),
GenericParam::Lifetime(g) => g.bounds.clear(),
GenericParam::Const(_) => {}
}
}

let mut punctuated_generic_idents = Punctuated::<_, Token![,]>::new();
punctuated_generic_idents.extend(lifetimeless_generics.iter().map(|g| match g {
GenericParam::Type(g) => &g.ident,
Expand Down Expand Up @@ -479,10 +464,6 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
// The struct can still be accessed via SystemParam::State, e.g. EventReaderState can be accessed via
// <EventReader<'static, 'static, T> as SystemParam>::State
const _: () = {
impl #impl_generics #path::system::SystemParam for #struct_name #ty_generics #where_clause {
type State = FetchState<'static, 'static, #punctuated_generic_idents>;
}

#[doc(hidden)]
#state_struct_visibility struct FetchState <'w, 's, #(#lifetimeless_generics,)*> {
state: (#(<#tuple_types as #path::system::SystemParam>::State,)*),
Expand All @@ -492,34 +473,33 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
)>,
}

unsafe impl<#punctuated_generics> #path::system::SystemParamState for
FetchState<'static, 'static, #punctuated_generic_idents>
#where_clause {
type Item<'w, 's> = #struct_name #ty_generics;
unsafe impl<'w, 's, #punctuated_generics> #path::system::SystemParam for #struct_name #ty_generics #where_clause {
type State = FetchState<'static, 'static, #punctuated_generic_idents>;
type Item<'_w, '_s> = #struct_name <#(#shadowed_lifetimes,)* #punctuated_generic_idents>;

fn init(world: &mut #path::world::World, system_meta: &mut #path::system::SystemMeta) -> Self {
Self {
state: #path::system::SystemParamState::init(world, system_meta),
fn init_state(world: &mut #path::world::World, system_meta: &mut #path::system::SystemMeta) -> Self::State {
FetchState {
state: <(#(#tuple_types,)*) as #path::system::SystemParam>::init_state(world, system_meta),
marker: std::marker::PhantomData,
}
}

fn new_archetype(&mut self, archetype: &#path::archetype::Archetype, system_meta: &mut #path::system::SystemMeta) {
self.state.new_archetype(archetype, system_meta)
fn new_archetype(state: &mut Self::State, archetype: &#path::archetype::Archetype, system_meta: &mut #path::system::SystemMeta) {
<(#(#tuple_types,)*) as #path::system::SystemParam>::new_archetype(&mut state.state, archetype, system_meta)
}

fn apply(&mut self, system_meta: &#path::system::SystemMeta, world: &mut #path::world::World) {
self.state.apply(system_meta, world)
fn apply(state: &mut Self::State, system_meta: &#path::system::SystemMeta, world: &mut #path::world::World) {
<(#(#tuple_types,)*) as #path::system::SystemParam>::apply(&mut state.state, system_meta, world);
}

unsafe fn get_param<'w, 's>(
state: &'s mut Self,
unsafe fn get_param<'w2, 's2>(
state: &'s2 mut Self::State,
system_meta: &#path::system::SystemMeta,
world: &'w #path::world::World,
world: &'w2 #path::world::World,
change_tick: u32,
) -> Self::Item<'w, 's> {
) -> Self::Item<'w2, 's2> {
let (#(#tuple_patterns,)*) = <
<(#(#tuple_types,)*) as #path::system::SystemParam>::State as #path::system::SystemParamState
(#(#tuple_types,)*) as #path::system::SystemParam
>::get_param(&mut state.state, system_meta, world, change_tick);
#struct_name {
#(#fields: #field_locals,)*
Expand Down
21 changes: 9 additions & 12 deletions crates/bevy_ecs/src/system/commands/parallel_scope.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ use thread_local::ThreadLocal;
use crate::{
entity::Entities,
prelude::World,
system::{SystemMeta, SystemParam, SystemParamState},
system::{SystemMeta, SystemParam},
};

use super::{CommandQueue, Commands};

/// The internal [`SystemParam`] state of the [`ParallelCommands`] type
#[doc(hidden)]
#[derive(Default)]
/// The internal [`SystemParamState`] of the [`ParallelCommands`] type
pub struct ParallelCommandsState {
thread_local_storage: ThreadLocal<Cell<CommandQueue>>,
}
Expand Down Expand Up @@ -48,30 +48,27 @@ pub struct ParallelCommands<'w, 's> {
entities: &'w Entities,
}

impl SystemParam for ParallelCommands<'_, '_> {
type State = ParallelCommandsState;
}

// SAFETY: no component or resource access to report
unsafe impl SystemParamState for ParallelCommandsState {
unsafe impl SystemParam for ParallelCommands<'_, '_> {
type State = ParallelCommandsState;
type Item<'w, 's> = ParallelCommands<'w, 's>;

fn init(_: &mut World, _: &mut crate::system::SystemMeta) -> Self {
Self::default()
fn init_state(_: &mut World, _: &mut crate::system::SystemMeta) -> Self::State {
ParallelCommandsState::default()
}

fn apply(&mut self, _system_meta: &SystemMeta, world: &mut World) {
fn apply(state: &mut Self::State, _system_meta: &SystemMeta, world: &mut World) {
#[cfg(feature = "trace")]
let _system_span =
bevy_utils::tracing::info_span!("system_commands", name = _system_meta.name())
.entered();
for cq in &mut self.thread_local_storage {
for cq in &mut state.thread_local_storage {
cq.get_mut().apply(world);
}
}

unsafe fn get_param<'w, 's>(
state: &'s mut Self,
state: &'s mut Self::State,
_: &crate::system::SystemMeta,
world: &'w World,
_: u32,
Expand Down
11 changes: 4 additions & 7 deletions crates/bevy_ecs/src/system/exclusive_function_system.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
schedule::{SystemLabel, SystemLabelId},
system::{
check_system_change_tick, AsSystemLabel, ExclusiveSystemParam, ExclusiveSystemParamItem,
ExclusiveSystemParamState, IntoSystem, System, SystemMeta, SystemTypeIdLabel,
IntoSystem, System, SystemMeta, SystemTypeIdLabel,
},
world::{World, WorldId},
};
Expand Down Expand Up @@ -94,7 +94,7 @@ where
let saved_last_tick = world.last_change_tick;
world.last_change_tick = self.system_meta.last_change_tick;

let params = <Param as ExclusiveSystemParam>::State::get_param(
let params = Param::get_param(
self.param_state.as_mut().expect(PARAM_MESSAGE),
&self.system_meta,
);
Expand Down Expand Up @@ -122,17 +122,14 @@ where
#[inline]
fn apply_buffers(&mut self, world: &mut World) {
let param_state = self.param_state.as_mut().expect(PARAM_MESSAGE);
param_state.apply(world);
Param::apply(param_state, world);
}

#[inline]
fn initialize(&mut self, world: &mut World) {
self.world_id = Some(world.id());
self.system_meta.last_change_tick = world.change_tick().wrapping_sub(MAX_CHANGE_AGE);
self.param_state = Some(<Param::State as ExclusiveSystemParamState>::init(
world,
&mut self.system_meta,
));
self.param_state = Some(Param::init(world, &mut self.system_meta));
}

fn update_archetype_component_access(&mut self, _world: &World) {}
Expand Down
Loading

0 comments on commit 563cb2e

Please sign in to comment.