Skip to content

Commit

Permalink
Add marker traits to distinguish base sets from regular system sets (#…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
JoJoJet committed Mar 2, 2023
1 parent 98cf0d2 commit 9733613
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 96 deletions.
11 changes: 3 additions & 8 deletions benches/benches/bevy_ecs/scheduling/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<dyn SystemSet> {
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));
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_app/src/config.rs
Original file line number Diff line number Diff line change
@@ -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,
},
};

Expand Down Expand Up @@ -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),
Expand All @@ -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),
Expand Down
22 changes: 21 additions & 1 deletion crates/bevy_ecs/macros/src/set.rs
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -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()
}
34 changes: 18 additions & 16 deletions crates/bevy_ecs/src/schedule/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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`.
Expand All @@ -115,12 +117,12 @@ impl<S: SystemSet> 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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -277,10 +279,10 @@ pub trait IntoSystemConfig<Marker, Config = SystemConfig> {
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`.
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down
67 changes: 0 additions & 67 deletions crates/bevy_ecs/src/schedule/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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]
Expand Down
10 changes: 10 additions & 0 deletions crates/bevy_ecs/src/schedule/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ pub trait SystemSet: DynHash + Debug + Send + Sync + 'static {
fn dyn_clone(&self) -> Box<dyn SystemSet>;
}

/// 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())
Expand Down

0 comments on commit 9733613

Please sign in to comment.