From b1c3e9862d06f5d8ec0fca3ce20d7318cff2d456 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Wed, 23 Mar 2022 22:53:56 +0000 Subject: [PATCH] Auto-label function systems with SystemTypeIdLabel (#4224) This adds the concept of "default labels" for systems (currently scoped to "parallel systems", but this could just as easily be implemented for "exclusive systems"). Function systems now include their function's `SystemTypeIdLabel` by default. This enables the following patterns: ```rust // ordering two systems without manually defining labels app .add_system(update_velocity) .add_system(movement.after(update_velocity)) // ordering sets of systems without manually defining labels app .add_system(foo) .add_system_set( SystemSet::new() .after(foo) .with_system(bar) .with_system(baz) ) ``` Fixes: #4219 Related to: #4220 Credit to @aevyrie @alice-i-cecile @DJMcNab (and probably others) for proposing (and supporting) this idea about a year ago. I was a big dummy that both shut down this (very good) idea and then forgot I did that. Sorry. You all were right! --- crates/bevy_ecs/src/lib.rs | 1 + crates/bevy_ecs/src/schedule/stage.rs | 46 ++++++++----- .../src/schedule/system_descriptor.rs | 27 ++++---- crates/bevy_ecs/src/system/function_system.rs | 69 ++++++++++++++++++- crates/bevy_ecs/src/system/system.rs | 5 ++ examples/ecs/system_sets.rs | 23 +------ 6 files changed, 120 insertions(+), 51 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 8d5c59adfe..cb559641d6 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -35,6 +35,7 @@ pub mod prelude { system::{ Commands, In, IntoChainSystem, IntoExclusiveSystem, IntoSystem, Local, NonSend, NonSendMut, Query, QuerySet, RemovedComponents, Res, ResMut, System, + SystemParamFunction, }, world::{FromWorld, Mut, World}, }; diff --git a/crates/bevy_ecs/src/schedule/stage.rs b/crates/bevy_ecs/src/schedule/stage.rs index 8901d24cdf..dd8d9976da 100644 --- a/crates/bevy_ecs/src/schedule/stage.rs +++ b/crates/bevy_ecs/src/schedule/stage.rs @@ -1578,15 +1578,25 @@ mod tests { fn ambiguity_detection() { use super::{find_ambiguities, SystemContainer}; - fn find_ambiguities_first_labels( + fn find_ambiguities_first_str_labels( systems: &[impl SystemContainer], ) -> Vec<(BoxedSystemLabel, BoxedSystemLabel)> { find_ambiguities(systems) .drain(..) .map(|(index_a, index_b, _conflicts)| { ( - systems[index_a].labels()[0].clone(), - systems[index_b].labels()[0].clone(), + systems[index_a] + .labels() + .iter() + .find(|a| (&***a).type_id() == std::any::TypeId::of::<&str>()) + .unwrap() + .clone(), + systems[index_b] + .labels() + .iter() + .find(|a| (&***a).type_id() == std::any::TypeId::of::<&str>()) + .unwrap() + .clone(), ) }) .collect() @@ -1616,7 +1626,7 @@ mod tests { .with_system(component.label("4")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert!( ambiguities.contains(&(Box::new("1"), Box::new("4"))) || ambiguities.contains(&(Box::new("4"), Box::new("1"))) @@ -1631,7 +1641,7 @@ mod tests { .with_system(resource.label("4")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert!( ambiguities.contains(&(Box::new("1"), Box::new("4"))) || ambiguities.contains(&(Box::new("4"), Box::new("1"))) @@ -1656,7 +1666,7 @@ mod tests { .with_system(resource.label("4")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert!( ambiguities.contains(&(Box::new("0"), Box::new("3"))) || ambiguities.contains(&(Box::new("3"), Box::new("0"))) @@ -1675,7 +1685,7 @@ mod tests { .with_system(resource.label("4").in_ambiguity_set("a")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert!( ambiguities.contains(&(Box::new("0"), Box::new("3"))) || ambiguities.contains(&(Box::new("3"), Box::new("0"))) @@ -1688,7 +1698,7 @@ mod tests { .with_system(component.label("2")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert!( ambiguities.contains(&(Box::new("0"), Box::new("1"))) || ambiguities.contains(&(Box::new("1"), Box::new("0"))) @@ -1701,7 +1711,7 @@ mod tests { .with_system(component.label("2").after("0")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert!( ambiguities.contains(&(Box::new("1"), Box::new("2"))) || ambiguities.contains(&(Box::new("2"), Box::new("1"))) @@ -1715,7 +1725,7 @@ mod tests { .with_system(component.label("3").after("1").after("2")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert!( ambiguities.contains(&(Box::new("1"), Box::new("2"))) || ambiguities.contains(&(Box::new("2"), Box::new("1"))) @@ -1729,7 +1739,7 @@ mod tests { .with_system(component.label("3").after("1").after("2")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert_eq!(ambiguities.len(), 0); let mut stage = SystemStage::parallel() @@ -1739,7 +1749,7 @@ mod tests { .with_system(component.label("3").after("1").after("2")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert!( ambiguities.contains(&(Box::new("1"), Box::new("2"))) || ambiguities.contains(&(Box::new("2"), Box::new("1"))) @@ -1769,7 +1779,7 @@ mod tests { ); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert!( ambiguities.contains(&(Box::new("1"), Box::new("2"))) || ambiguities.contains(&(Box::new("2"), Box::new("1"))) @@ -1819,7 +1829,7 @@ mod tests { ); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert_eq!(ambiguities.len(), 0); let mut stage = SystemStage::parallel() @@ -1850,7 +1860,7 @@ mod tests { ); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.parallel); + let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); assert!( ambiguities.contains(&(Box::new("1"), Box::new("4"))) || ambiguities.contains(&(Box::new("4"), Box::new("1"))) @@ -1884,7 +1894,7 @@ mod tests { .with_system(empty.exclusive_system().label("6").after("2").after("5")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.exclusive_at_start); + let ambiguities = find_ambiguities_first_str_labels(&stage.exclusive_at_start); assert!( ambiguities.contains(&(Box::new("1"), Box::new("3"))) || ambiguities.contains(&(Box::new("3"), Box::new("1"))) @@ -1921,7 +1931,7 @@ mod tests { .with_system(empty.exclusive_system().label("6").after("2").after("5")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.exclusive_at_start); + let ambiguities = find_ambiguities_first_str_labels(&stage.exclusive_at_start); assert!( ambiguities.contains(&(Box::new("2"), Box::new("3"))) || ambiguities.contains(&(Box::new("3"), Box::new("2"))) @@ -1947,7 +1957,7 @@ mod tests { .with_system(empty.exclusive_system().label("3").in_ambiguity_set("a")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_labels(&stage.exclusive_at_start); + let ambiguities = find_ambiguities_first_str_labels(&stage.exclusive_at_start); assert_eq!(ambiguities.len(), 0); } diff --git a/crates/bevy_ecs/src/schedule/system_descriptor.rs b/crates/bevy_ecs/src/schedule/system_descriptor.rs index 825b581135..405c8760f3 100644 --- a/crates/bevy_ecs/src/schedule/system_descriptor.rs +++ b/crates/bevy_ecs/src/schedule/system_descriptor.rs @@ -3,7 +3,10 @@ use crate::{ AmbiguitySetLabel, BoxedAmbiguitySetLabel, BoxedSystemLabel, IntoRunCriteria, RunCriteriaDescriptorOrLabel, SystemLabel, }, - system::{BoxedSystem, ExclusiveSystem, ExclusiveSystemCoerced, ExclusiveSystemFn, IntoSystem}, + system::{ + AsSystemLabel, BoxedSystem, ExclusiveSystem, ExclusiveSystemCoerced, ExclusiveSystemFn, + IntoSystem, + }, }; /// Encapsulates a system and information on when it run in a `SystemStage`. @@ -105,9 +108,9 @@ pub struct ParallelSystemDescriptor { fn new_parallel_descriptor(system: BoxedSystem<(), ()>) -> ParallelSystemDescriptor { ParallelSystemDescriptor { + labels: system.default_labels(), system, run_criteria: None, - labels: Vec::new(), before: Vec::new(), after: Vec::new(), ambiguity_sets: Vec::new(), @@ -126,10 +129,10 @@ pub trait ParallelSystemDescriptorCoercion { fn label(self, label: impl SystemLabel) -> ParallelSystemDescriptor; /// Specifies that the system should run before systems with the given label. - fn before(self, label: impl SystemLabel) -> ParallelSystemDescriptor; + fn before(self, label: impl AsSystemLabel) -> ParallelSystemDescriptor; /// Specifies that the system should run after systems with the given label. - fn after(self, label: impl SystemLabel) -> ParallelSystemDescriptor; + fn after(self, label: impl AsSystemLabel) -> ParallelSystemDescriptor; /// Specifies that the system is exempt from execution order ambiguity detection /// with other systems in this set. @@ -150,13 +153,13 @@ impl ParallelSystemDescriptorCoercion<()> for ParallelSystemDescriptor { self } - fn before(mut self, label: impl SystemLabel) -> ParallelSystemDescriptor { - self.before.push(Box::new(label)); + fn before(mut self, label: impl AsSystemLabel) -> ParallelSystemDescriptor { + self.before.push(Box::new(label.as_system_label())); self } - fn after(mut self, label: impl SystemLabel) -> ParallelSystemDescriptor { - self.after.push(Box::new(label)); + fn after(mut self, label: impl AsSystemLabel) -> ParallelSystemDescriptor { + self.after.push(Box::new(label.as_system_label())); self } @@ -182,11 +185,11 @@ where new_parallel_descriptor(Box::new(IntoSystem::into_system(self))).label(label) } - fn before(self, label: impl SystemLabel) -> ParallelSystemDescriptor { + fn before(self, label: impl AsSystemLabel) -> ParallelSystemDescriptor { new_parallel_descriptor(Box::new(IntoSystem::into_system(self))).before(label) } - fn after(self, label: impl SystemLabel) -> ParallelSystemDescriptor { + fn after(self, label: impl AsSystemLabel) -> ParallelSystemDescriptor { new_parallel_descriptor(Box::new(IntoSystem::into_system(self))).after(label) } @@ -207,11 +210,11 @@ impl ParallelSystemDescriptorCoercion<()> for BoxedSystem<(), ()> { new_parallel_descriptor(self).label(label) } - fn before(self, label: impl SystemLabel) -> ParallelSystemDescriptor { + fn before(self, label: impl AsSystemLabel) -> ParallelSystemDescriptor { new_parallel_descriptor(self).before(label) } - fn after(self, label: impl SystemLabel) -> ParallelSystemDescriptor { + fn after(self, label: impl AsSystemLabel) -> ParallelSystemDescriptor { new_parallel_descriptor(self).after(label) } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index b43bc5f77b..d77edf3da2 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -2,6 +2,7 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId, ArchetypeGeneration, ArchetypeId}, component::ComponentId, query::{Access, FilteredAccessSet}, + schedule::SystemLabel, system::{ check_system_change_tick, ReadOnlySystemParamFetch, System, SystemParam, SystemParamFetch, SystemParamState, @@ -9,7 +10,7 @@ use crate::{ world::{World, WorldId}, }; use bevy_ecs_macros::all_tuples; -use std::{borrow::Cow, marker::PhantomData}; +use std::{borrow::Cow, fmt::Debug, hash::Hash, marker::PhantomData}; /// The metadata of a [`System`]. pub struct SystemMeta { @@ -422,6 +423,47 @@ where self.system_meta.name.as_ref(), ); } + fn default_labels(&self) -> Vec> { + vec![Box::new(self.func.as_system_label())] + } +} + +/// A [`SystemLabel`] that was automatically generated for a system on the basis of its `TypeId`. +pub struct SystemTypeIdLabel(PhantomData T>); + +impl Debug for SystemTypeIdLabel { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_tuple("SystemTypeIdLabel") + .field(&std::any::type_name::()) + .finish() + } +} +impl Hash for SystemTypeIdLabel { + fn hash(&self, _state: &mut H) { + // All SystemTypeIds of a given type are the same. + } +} +impl Clone for SystemTypeIdLabel { + fn clone(&self) -> Self { + Self(PhantomData) + } +} + +impl Copy for SystemTypeIdLabel {} + +impl PartialEq for SystemTypeIdLabel { + #[inline] + fn eq(&self, _other: &Self) -> bool { + // All labels of a given type are equal, as they will all have the same type id + true + } +} +impl Eq for SystemTypeIdLabel {} + +impl SystemLabel for SystemTypeIdLabel { + fn dyn_clone(&self) -> Box { + Box::new(*self) + } } /// A trait implemented for all functions that can be used as [`System`]s. @@ -490,3 +532,28 @@ macro_rules! impl_system_function { } all_tuples!(impl_system_function, 0, 16, F); + +/// Used to implicitly convert systems to their default labels. For example, it will convert +/// "system functions" to their [`SystemTypeIdLabel`]. +pub trait AsSystemLabel { + type SystemLabel: SystemLabel; + fn as_system_label(&self) -> Self::SystemLabel; +} + +impl> + AsSystemLabel<(In, Out, Param, Marker)> for T +{ + type SystemLabel = SystemTypeIdLabel; + + fn as_system_label(&self) -> Self::SystemLabel { + SystemTypeIdLabel(PhantomData:: Self>) + } +} + +impl AsSystemLabel<()> for T { + type SystemLabel = T; + + fn as_system_label(&self) -> Self::SystemLabel { + self.clone() + } +} diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 28c84ecb80..3cf0f9e773 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -4,6 +4,7 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId}, component::ComponentId, query::Access, + schedule::SystemLabel, world::World, }; use std::borrow::Cow; @@ -56,6 +57,10 @@ pub trait System: Send + Sync + 'static { /// Initialize the system. fn initialize(&mut self, _world: &mut World); fn check_change_tick(&mut self, change_tick: u32); + /// The default labels for the system + fn default_labels(&self) -> Vec> { + Vec::new() + } } /// A convenience type alias for a boxed [`System`] trait object. diff --git a/examples/ecs/system_sets.rs b/examples/ecs/system_sets.rs index 26c2c025e1..fffba8b1b1 100644 --- a/examples/ecs/system_sets.rs +++ b/examples/ecs/system_sets.rs @@ -16,14 +16,6 @@ struct PostPhysics; #[derive(Default)] struct Done(bool); -/// This is used to show that within a [`SystemSet`], individual systems can also -/// be labelled, allowing further fine tuning of run ordering. -#[derive(Clone, Hash, Debug, PartialEq, Eq, SystemLabel)] -pub enum PhysicsSystem { - UpdateVelocity, - Movement, -} - /// This example realizes the following scheme: /// /// ```none @@ -63,18 +55,9 @@ fn main() { // This criteria ensures this whole system set only runs when this system's // output says so (ShouldRun::Yes) .with_run_criteria(run_for_a_second) - .with_system( - update_velocity - // Only applied to the `update_velocity` system - .label(PhysicsSystem::UpdateVelocity), - ) - .with_system( - movement - // Only applied to the `movement` system - .label(PhysicsSystem::Movement) - // Enforce order within this system by specifying this - .after(PhysicsSystem::UpdateVelocity), - ), + .with_system(update_velocity) + // Make movement run after update_velocity + .with_system(movement.after(update_velocity)), ) .add_system_set( SystemSet::new()