Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Add marker traits to distinguish base sets from regular system sets #7863

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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