From 45112a2dae85da0b9e2284c64069aa91562b3f3d Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 2 Mar 2023 13:22:58 +0000 Subject: [PATCH] Add marker traits to distinguish base sets from regular system sets (#7863) # Objective Base sets, added in #7466 are a special type of system set. Systems can only be added to base sets via `in_base_set`, while non-base sets can only be added via `in_set`. Unfortunately this is currently guarded by a runtime panic, which presents an unfortunate toe-stub when the wrong method is used. The delayed response between writing code and encountering the error (possibly hours) makes the distinction between base sets and other sets much more difficult to learn. ## Solution Add the marker traits `BaseSystemSet` and `FreeSystemSet`. `in_base_set` and `in_set` now respectively accept these traits, which moves the runtime panic to a compile time error. --- ## Changelog + Added the marker trait `BaseSystemSet`, which is distinguished from a `FreeSystemSet`. These are both subtraits of `SystemSet`. ## Migration Guide None if merged with 0.10 --- .../benches/bevy_ecs/scheduling/schedule.rs | 11 +-- crates/bevy_app/src/config.rs | 8 +-- crates/bevy_ecs/macros/src/set.rs | 22 +++++- crates/bevy_ecs/src/schedule/config.rs | 34 +++++----- crates/bevy_ecs/src/schedule/mod.rs | 67 ------------------- crates/bevy_ecs/src/schedule/set.rs | 10 +++ 6 files changed, 56 insertions(+), 96 deletions(-) diff --git a/benches/benches/bevy_ecs/scheduling/schedule.rs b/benches/benches/bevy_ecs/scheduling/schedule.rs index 18a2699becad9a..3117c0fff2bc76 100644 --- a/benches/benches/bevy_ecs/scheduling/schedule.rs +++ b/benches/benches/bevy_ecs/scheduling/schedule.rs @@ -63,16 +63,11 @@ pub fn build_schedule(criterion: &mut Criterion) { // Use multiple different kinds of label to ensure that dynamic dispatch // doesn't somehow get optimized away. - #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] + #[derive(SystemSet, Debug, Clone, Copy, PartialEq, Eq, Hash)] struct NumSet(usize); - #[derive(Debug, Clone, Copy, SystemSet, PartialEq, Eq, Hash)] - struct DummySet; - impl SystemSet for NumSet { - fn dyn_clone(&self) -> Box { - Box::new(self.clone()) - } - } + #[derive(SystemSet, Debug, Clone, Copy, PartialEq, Eq, Hash)] + struct DummySet; let mut group = criterion.benchmark_group("build_schedule"); group.warm_up_time(std::time::Duration::from_millis(500)); diff --git a/crates/bevy_app/src/config.rs b/crates/bevy_app/src/config.rs index 81d35f85744459..3a5f30a50453b5 100644 --- a/crates/bevy_app/src/config.rs +++ b/crates/bevy_app/src/config.rs @@ -1,8 +1,8 @@ use bevy_ecs::{ all_tuples, schedule::{ - BoxedScheduleLabel, Condition, IntoSystemConfig, IntoSystemSet, ScheduleLabel, - SystemConfig, SystemConfigs, SystemSet, + BaseSystemSet, BoxedScheduleLabel, Condition, FreeSystemSet, IntoSystemConfig, + IntoSystemSet, ScheduleLabel, SystemConfig, SystemConfigs, }, }; @@ -84,7 +84,7 @@ impl IntoSystemConfig<(), Self> for SystemAppConfig { } #[track_caller] - fn in_set(self, set: impl SystemSet) -> Self { + fn in_set(self, set: impl FreeSystemSet) -> Self { let Self { system, schedule } = self; Self { system: system.in_set(set), @@ -93,7 +93,7 @@ impl IntoSystemConfig<(), Self> for SystemAppConfig { } #[track_caller] - fn in_base_set(self, set: impl SystemSet) -> Self { + fn in_base_set(self, set: impl BaseSystemSet) -> Self { let Self { system, schedule } = self; Self { system: system.in_base_set(set), diff --git a/crates/bevy_ecs/macros/src/set.rs b/crates/bevy_ecs/macros/src/set.rs index 545de5ef87eb46..054735c8586530 100644 --- a/crates/bevy_ecs/macros/src/set.rs +++ b/crates/bevy_ecs/macros/src/set.rs @@ -1,5 +1,5 @@ use proc_macro::TokenStream; -use quote::{quote, ToTokens}; +use quote::{format_ident, quote, ToTokens}; use syn::parse::{Parse, ParseStream}; pub static SYSTEM_SET_ATTRIBUTE_NAME: &str = "system_set"; @@ -12,6 +12,14 @@ pub static BASE_ATTRIBUTE_NAME: &str = "base"; /// - `input`: The [`syn::DeriveInput`] for the struct that we want to derive the set trait for /// - `trait_path`: The [`syn::Path`] to the set trait pub fn derive_set(input: syn::DeriveInput, trait_path: &syn::Path) -> TokenStream { + let mut base_trait_path = trait_path.clone(); + let ident = &mut base_trait_path.segments.last_mut().unwrap().ident; + *ident = format_ident!("Base{ident}"); + + let mut free_trait_path = trait_path.clone(); + let ident = &mut free_trait_path.segments.last_mut().unwrap().ident; + *ident = format_ident!("Free{ident}"); + let ident = input.ident; let mut is_base = false; @@ -65,6 +73,16 @@ pub fn derive_set(input: syn::DeriveInput, trait_path: &syn::Path) -> TokenStrea .unwrap(), ); + let marker_impl = if is_base { + quote! { + impl #impl_generics #base_trait_path for #ident #ty_generics #where_clause {} + } + } else { + quote! { + impl #impl_generics #free_trait_path for #ident #ty_generics #where_clause {} + } + }; + (quote! { impl #impl_generics #trait_path for #ident #ty_generics #where_clause { fn is_base(&self) -> bool { @@ -75,6 +93,8 @@ pub fn derive_set(input: syn::DeriveInput, trait_path: &syn::Path) -> TokenStrea std::boxed::Box::new(std::clone::Clone::clone(self)) } } + + #marker_impl }) .into() } diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index 15dd3f1f79933d..899985fb22b438 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -9,6 +9,8 @@ use crate::{ system::{BoxedSystem, IntoSystem, System}, }; +use super::{BaseSystemSet, FreeSystemSet}; + /// A [`SystemSet`] with scheduling metadata. pub struct SystemSetConfig { pub(super) set: BoxedSystemSet, @@ -86,10 +88,10 @@ pub trait IntoSystemSetConfig { fn into_config(self) -> SystemSetConfig; /// Add to the provided `set`. #[track_caller] - fn in_set(self, set: impl SystemSet) -> SystemSetConfig; + fn in_set(self, set: impl FreeSystemSet) -> SystemSetConfig; /// Add to the provided "base" `set`. For more information on base sets, see [`SystemSet::is_base`]. #[track_caller] - fn in_base_set(self, set: impl SystemSet) -> SystemSetConfig; + fn in_base_set(self, set: impl BaseSystemSet) -> SystemSetConfig; /// Add this set to the schedules's default base set. fn in_default_base_set(self) -> SystemSetConfig; /// Run before all systems in `set`. @@ -115,12 +117,12 @@ impl IntoSystemSetConfig for S { } #[track_caller] - fn in_set(self, set: impl SystemSet) -> SystemSetConfig { + fn in_set(self, set: impl FreeSystemSet) -> SystemSetConfig { self.into_config().in_set(set) } #[track_caller] - fn in_base_set(self, set: impl SystemSet) -> SystemSetConfig { + fn in_base_set(self, set: impl BaseSystemSet) -> SystemSetConfig { self.into_config().in_base_set(set) } @@ -155,12 +157,12 @@ impl IntoSystemSetConfig for BoxedSystemSet { } #[track_caller] - fn in_set(self, set: impl SystemSet) -> SystemSetConfig { + fn in_set(self, set: impl FreeSystemSet) -> SystemSetConfig { self.into_config().in_set(set) } #[track_caller] - fn in_base_set(self, set: impl SystemSet) -> SystemSetConfig { + fn in_base_set(self, set: impl BaseSystemSet) -> SystemSetConfig { self.into_config().in_base_set(set) } @@ -277,10 +279,10 @@ pub trait IntoSystemConfig { fn into_config(self) -> Config; /// Add to `set` membership. #[track_caller] - fn in_set(self, set: impl SystemSet) -> Config; + fn in_set(self, set: impl FreeSystemSet) -> Config; /// Add to the provided "base" `set`. For more information on base sets, see [`SystemSet::is_base`]. #[track_caller] - fn in_base_set(self, set: impl SystemSet) -> Config; + fn in_base_set(self, set: impl BaseSystemSet) -> Config; /// Don't add this system to the schedules's default set. fn no_default_base_set(self) -> Config; /// Run before all systems in `set`. @@ -309,12 +311,12 @@ where } #[track_caller] - fn in_set(self, set: impl SystemSet) -> SystemConfig { + fn in_set(self, set: impl FreeSystemSet) -> SystemConfig { self.into_config().in_set(set) } #[track_caller] - fn in_base_set(self, set: impl SystemSet) -> SystemConfig { + fn in_base_set(self, set: impl BaseSystemSet) -> SystemConfig { self.into_config().in_base_set(set) } @@ -349,12 +351,12 @@ impl IntoSystemConfig<()> for BoxedSystem<(), ()> { } #[track_caller] - fn in_set(self, set: impl SystemSet) -> SystemConfig { + fn in_set(self, set: impl FreeSystemSet) -> SystemConfig { self.into_config().in_set(set) } #[track_caller] - fn in_base_set(self, set: impl SystemSet) -> SystemConfig { + fn in_base_set(self, set: impl BaseSystemSet) -> SystemConfig { self.into_config().in_base_set(set) } @@ -471,13 +473,13 @@ where /// Add these systems to the provided `set`. #[track_caller] - fn in_set(self, set: impl SystemSet) -> SystemConfigs { + fn in_set(self, set: impl FreeSystemSet) -> SystemConfigs { self.into_configs().in_set(set) } /// Add these systems to the provided "base" `set`. For more information on base sets, see [`SystemSet::is_base`]. #[track_caller] - fn in_base_set(self, set: impl SystemSet) -> SystemConfigs { + fn in_base_set(self, set: impl BaseSystemSet) -> SystemConfigs { self.into_configs().in_base_set(set) } @@ -657,13 +659,13 @@ where /// Add these system sets to the provided `set`. #[track_caller] - fn in_set(self, set: impl SystemSet) -> SystemSetConfigs { + fn in_set(self, set: impl FreeSystemSet) -> SystemSetConfigs { self.into_configs().in_set(set) } /// Add these system sets to the provided "base" `set`. For more information on base sets, see [`SystemSet::is_base`]. #[track_caller] - fn in_base_set(self, set: impl SystemSet) -> SystemSetConfigs { + fn in_base_set(self, set: impl BaseSystemSet) -> SystemSetConfigs { self.into_configs().in_base_set(set) } diff --git a/crates/bevy_ecs/src/schedule/mod.rs b/crates/bevy_ecs/src/schedule/mod.rs index fa7a2b72844a9d..f886d815d1d37c 100644 --- a/crates/bevy_ecs/src/schedule/mod.rs +++ b/crates/bevy_ecs/src/schedule/mod.rs @@ -567,16 +567,6 @@ mod tests { )); } - #[test] - #[should_panic] - fn in_system_type_set() { - fn foo() {} - fn bar() {} - - let mut schedule = Schedule::new(); - schedule.add_system(foo.in_set(bar.into_system_set())); - } - #[test] #[should_panic] fn configure_system_type_set() { @@ -688,63 +678,6 @@ mod tests { enum Normal { X, Y, - Z, - } - - #[test] - #[should_panic] - fn disallow_adding_base_sets_to_system_with_in_set() { - let mut schedule = Schedule::new(); - schedule.add_system(named_system.in_set(Base::A)); - } - - #[test] - #[should_panic] - fn disallow_adding_sets_to_system_with_in_base_set() { - let mut schedule = Schedule::new(); - schedule.add_system(named_system.in_base_set(Normal::X)); - } - - #[test] - #[should_panic] - fn disallow_adding_base_sets_to_systems_with_in_set() { - let mut schedule = Schedule::new(); - schedule.add_systems((named_system, named_system).in_set(Base::A)); - } - - #[test] - #[should_panic] - fn disallow_adding_sets_to_systems_with_in_base_set() { - let mut schedule = Schedule::new(); - schedule.add_systems((named_system, named_system).in_base_set(Normal::X)); - } - - #[test] - #[should_panic] - fn disallow_adding_base_sets_to_set_with_in_set() { - let mut schedule = Schedule::new(); - schedule.configure_set(Normal::Y.in_set(Base::A)); - } - - #[test] - #[should_panic] - fn disallow_adding_sets_to_set_with_in_base_set() { - let mut schedule = Schedule::new(); - schedule.configure_set(Normal::Y.in_base_set(Normal::X)); - } - - #[test] - #[should_panic] - fn disallow_adding_base_sets_to_sets_with_in_set() { - let mut schedule = Schedule::new(); - schedule.configure_sets((Normal::X, Normal::Y).in_set(Base::A)); - } - - #[test] - #[should_panic] - fn disallow_adding_sets_to_sets_with_in_base_set() { - let mut schedule = Schedule::new(); - schedule.configure_sets((Normal::X, Normal::Y).in_base_set(Normal::Z)); } #[test] diff --git a/crates/bevy_ecs/src/schedule/set.rs b/crates/bevy_ecs/src/schedule/set.rs index 06a162c7549b9c..ad882f11c1897c 100644 --- a/crates/bevy_ecs/src/schedule/set.rs +++ b/crates/bevy_ecs/src/schedule/set.rs @@ -37,6 +37,16 @@ pub trait SystemSet: DynHash + Debug + Send + Sync + 'static { fn dyn_clone(&self) -> Box; } +/// A system set that is never contained in another set. +/// Systems and other system sets may only belong to one base set. +/// +/// This should only be implemented for types that return `true` from [`SystemSet::is_base`]. +pub trait BaseSystemSet: SystemSet {} + +/// System sets that are *not* base sets. This should not be implemented for +/// any types that implement [`BaseSystemSet`]. +pub trait FreeSystemSet: SystemSet {} + impl PartialEq for dyn SystemSet { fn eq(&self, other: &Self) -> bool { self.dyn_eq(other.as_dyn_eq())