From 474b55a29c0dc0875735e51c0e994bf27c8fc5ab Mon Sep 17 00:00:00 2001 From: Joseph <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 28 Aug 2023 09:36:46 -0700 Subject: [PATCH] Add `system.map(...)` for transforming the output of a system (#8526) # Objective Any time we wish to transform the output of a system, we currently use system piping to do so: ```rust my_system.pipe(|In(x)| do_something(x)) ``` Unfortunately, system piping is not a zero cost abstraction. Each call to `.pipe` requires allocating two extra access sets: one for the second system and one for the combined accesses of both systems. This also adds extra work to each call to `update_archetype_component_access`, which stacks as one adds multiple layers of system piping. ## Solution Add the `AdapterSystem` abstraction: similar to `CombinatorSystem`, this allows you to implement a trait to generically control how a system is run and how its inputs and outputs are processed. Unlike `CombinatorSystem`, this does not have any overhead when computing world accesses which makes it ideal for simple operations such as inverting or ignoring the output of a system. Add the extension method `.map(...)`: this is similar to `.pipe(...)`, only it accepts a closure as an argument instead of an `In` system. ```rust my_system.map(do_something) ``` This has the added benefit of making system names less messy: a system that ignores its output will just be called `my_system`, instead of `Pipe(my_system, ignore)` --- ## Changelog TODO ## Migration Guide The `system_adapter` functions have been deprecated: use `.map` instead, which is a lightweight alternative to `.pipe`. ```rust // Before: my_system.pipe(system_adapter::ignore) my_system.pipe(system_adapter::unwrap) my_system.pipe(system_adapter::new(T::from)) // After: my_system.map(std::mem::drop) my_system.map(Result::unwrap) my_system.map(T::from) // Before: my_system.pipe(system_adapter::info) my_system.pipe(system_adapter::dbg) my_system.pipe(system_adapter::warn) my_system.pipe(system_adapter::error) // After: my_system.map(bevy_utils::info) my_system.map(bevy_utils::dbg) my_system.map(bevy_utils::warn) my_system.map(bevy_utils::error) ``` --------- Co-authored-by: Alice Cecile --- crates/bevy_ecs/src/lib.rs | 6 +- crates/bevy_ecs/src/schedule/condition.rs | 99 ++--------- crates/bevy_ecs/src/system/adapter_system.rs | 170 +++++++++++++++++++ crates/bevy_ecs/src/system/mod.rs | 48 +++++- crates/bevy_utils/src/lib.rs | 24 +++ examples/ecs/system_piping.rs | 12 +- 6 files changed, 261 insertions(+), 98 deletions(-) create mode 100644 crates/bevy_ecs/src/system/adapter_system.rs diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index cb91dae603..c60356f4da 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -30,6 +30,10 @@ pub mod prelude { #[doc(hidden)] #[cfg(feature = "bevy_reflect")] pub use crate::reflect::{AppTypeRegistry, ReflectComponent, ReflectResource}; + #[allow(deprecated)] + pub use crate::system::adapter::{ + self as system_adapter, dbg, error, ignore, info, unwrap, warn, + }; #[doc(hidden)] pub use crate::{ bundle::Bundle, @@ -45,8 +49,6 @@ pub mod prelude { OnEnter, OnExit, OnTransition, Schedule, Schedules, State, States, SystemSet, }, system::{ - adapter as system_adapter, - adapter::{dbg, error, ignore, info, unwrap, warn}, Commands, Deferred, In, IntoSystem, Local, NonSend, NonSendMut, ParallelCommands, ParamSet, Query, ReadOnlySystem, Res, ResMut, Resource, System, SystemParamFunction, }, diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs index abaf9bbf62..8186819ccd 100644 --- a/crates/bevy_ecs/src/schedule/condition.rs +++ b/crates/bevy_ecs/src/schedule/condition.rs @@ -1,12 +1,9 @@ -use std::any::TypeId; use std::borrow::Cow; use std::ops::Not; -use crate::component::{self, ComponentId}; -use crate::query::Access; -use crate::system::{CombinatorSystem, Combine, IntoSystem, ReadOnlySystem, System}; -use crate::world::unsafe_world_cell::UnsafeWorldCell; -use crate::world::World; +use crate::system::{ + Adapt, AdapterSystem, CombinatorSystem, Combine, IntoSystem, ReadOnlySystem, System, +}; /// A type-erased run condition stored in a [`Box`]. pub type BoxedCondition = Box>; @@ -181,8 +178,6 @@ mod sealed { /// A collection of [run conditions](Condition) that may be useful in any bevy app. pub mod common_conditions { - use std::borrow::Cow; - use super::NotSystem; use crate::{ change_detection::DetectChanges, @@ -987,96 +982,30 @@ pub mod common_conditions { { let condition = IntoSystem::into_system(condition); let name = format!("!{}", condition.name()); - NotSystem:: { - condition, - name: Cow::Owned(name), - } + NotSystem::new(super::NotMarker, condition, name.into()) } } /// Invokes [`Not`] with the output of another system. /// /// See [`common_conditions::not`] for examples. -#[derive(Clone)] -pub struct NotSystem -where - T::Out: Not, -{ - condition: T, - name: Cow<'static, str>, -} -impl System for NotSystem +pub type NotSystem = AdapterSystem; + +/// Used with [`AdapterSystem`] to negate the output of a system via the [`Not`] operator. +#[doc(hidden)] +#[derive(Clone, Copy)] +pub struct NotMarker; + +impl Adapt for NotMarker where T::Out: Not, { type In = T::In; type Out = ::Output; - fn name(&self) -> Cow<'static, str> { - self.name.clone() + fn adapt(&mut self, input: Self::In, run_system: impl FnOnce(T::In) -> T::Out) -> Self::Out { + !run_system(input) } - - fn type_id(&self) -> TypeId { - TypeId::of::() - } - - fn component_access(&self) -> &Access { - self.condition.component_access() - } - - fn archetype_component_access(&self) -> &Access { - self.condition.archetype_component_access() - } - - fn is_send(&self) -> bool { - self.condition.is_send() - } - - fn is_exclusive(&self) -> bool { - self.condition.is_exclusive() - } - - unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { - // SAFETY: The inner condition system asserts its own safety. - !self.condition.run_unsafe(input, world) - } - - fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out { - !self.condition.run(input, world) - } - - fn apply_deferred(&mut self, world: &mut World) { - self.condition.apply_deferred(world); - } - - fn initialize(&mut self, world: &mut World) { - self.condition.initialize(world); - } - - fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) { - self.condition.update_archetype_component_access(world); - } - - fn check_change_tick(&mut self, change_tick: component::Tick) { - self.condition.check_change_tick(change_tick); - } - - fn get_last_run(&self) -> component::Tick { - self.condition.get_last_run() - } - - fn set_last_run(&mut self, last_run: component::Tick) { - self.condition.set_last_run(last_run); - } -} - -// SAFETY: This trait is only implemented when the inner system is read-only. -// The `Not` condition does not modify access, and thus cannot transform a read-only system into one that is not. -unsafe impl ReadOnlySystem for NotSystem -where - T: ReadOnlySystem, - T::Out: Not, -{ } /// Combines the outputs of two systems using the `&&` operator. diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs new file mode 100644 index 0000000000..288e911388 --- /dev/null +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -0,0 +1,170 @@ +use std::borrow::Cow; + +use super::{ReadOnlySystem, System}; +use crate::world::unsafe_world_cell::UnsafeWorldCell; + +/// Customizes the behavior of an [`AdapterSystem`] +/// +/// # Examples +/// +/// ``` +/// # use bevy_ecs::prelude::*; +/// use bevy_ecs::system::{Adapt, AdapterSystem}; +/// +/// // A system adapter that inverts the result of a system. +/// // NOTE: Instead of manually implementing this, you can just use `bevy_ecs::schedule::common_conditions::not`. +/// pub type NotSystem = AdapterSystem; +/// +/// // This struct is used to customize the behavior of our adapter. +/// pub struct NotMarker; +/// +/// impl Adapt for NotMarker +/// where +/// S: System, +/// S::Out: std::ops::Not, +/// { +/// type In = S::In; +/// type Out = ::Output; +/// +/// fn adapt( +/// &mut self, +/// input: Self::In, +/// run_system: impl FnOnce(S::In) -> S::Out, +/// ) -> Self::Out { +/// !run_system(input) +/// } +/// } +/// # let mut world = World::new(); +/// # let mut system = NotSystem::new(NotMarker, IntoSystem::into_system(|| false), "".into()); +/// # system.initialize(&mut world); +/// # assert!(system.run((), &mut world)); +/// ``` +pub trait Adapt: Send + Sync + 'static { + /// The [input](System::In) type for an [`AdapterSystem`]. + type In; + /// The [output](System::Out) type for an [`AdapterSystem`]. + type Out; + + /// When used in an [`AdapterSystem`], this function customizes how the system + /// is run and how its inputs/outputs are adapted. + fn adapt(&mut self, input: Self::In, run_system: impl FnOnce(S::In) -> S::Out) -> Self::Out; +} + +/// A [`System`] that takes the output of `S` and transforms it by applying `Func` to it. +#[derive(Clone)] +pub struct AdapterSystem { + func: Func, + system: S, + name: Cow<'static, str>, +} + +impl AdapterSystem +where + Func: Adapt, + S: System, +{ + /// Creates a new [`System`] that uses `func` to adapt `system`, via the [`Adapt`] trait. + pub const fn new(func: Func, system: S, name: Cow<'static, str>) -> Self { + Self { func, system, name } + } +} + +impl System for AdapterSystem +where + Func: Adapt, + S: System, +{ + type In = Func::In; + type Out = Func::Out; + + fn name(&self) -> Cow<'static, str> { + self.name.clone() + } + + fn type_id(&self) -> std::any::TypeId { + std::any::TypeId::of::() + } + + fn component_access(&self) -> &crate::query::Access { + self.system.component_access() + } + + #[inline] + fn archetype_component_access( + &self, + ) -> &crate::query::Access { + self.system.archetype_component_access() + } + + fn is_send(&self) -> bool { + self.system.is_send() + } + + fn is_exclusive(&self) -> bool { + self.system.is_exclusive() + } + + #[inline] + unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { + // SAFETY: `system.run_unsafe` has the same invariants as `self.run_unsafe`. + self.func + .adapt(input, |input| self.system.run_unsafe(input, world)) + } + + #[inline] + fn run(&mut self, input: Self::In, world: &mut crate::prelude::World) -> Self::Out { + self.func + .adapt(input, |input| self.system.run(input, world)) + } + + #[inline] + fn apply_deferred(&mut self, world: &mut crate::prelude::World) { + self.system.apply_deferred(world); + } + + fn initialize(&mut self, world: &mut crate::prelude::World) { + self.system.initialize(world); + } + + #[inline] + fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) { + self.system.update_archetype_component_access(world); + } + + fn check_change_tick(&mut self, change_tick: crate::component::Tick) { + self.system.check_change_tick(change_tick); + } + + fn get_last_run(&self) -> crate::component::Tick { + self.system.get_last_run() + } + + fn set_last_run(&mut self, last_run: crate::component::Tick) { + self.system.set_last_run(last_run); + } + + fn default_system_sets(&self) -> Vec> { + self.system.default_system_sets() + } +} + +// SAFETY: The inner system is read-only. +unsafe impl ReadOnlySystem for AdapterSystem +where + Func: Adapt, + S: ReadOnlySystem, +{ +} + +impl Adapt for F +where + S: System, + F: Send + Sync + 'static + FnMut(S::Out) -> Out, +{ + type In = S::In; + type Out = Out; + + fn adapt(&mut self, input: S::In, run_system: impl FnOnce(S::In) -> S::Out) -> Out { + self(run_system(input)) + } +} diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 3900dd68ac..051795fd56 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -102,6 +102,7 @@ //! - All tuples between 1 to 16 elements where each element implements [`SystemParam`] //! - [`()` (unit primitive type)](https://doc.rust-lang.org/stable/std/primitive.unit.html) +mod adapter_system; mod combinator; mod commands; mod exclusive_function_system; @@ -114,6 +115,7 @@ mod system_param; use std::borrow::Cow; +pub use adapter_system::*; pub use combinator::*; pub use commands::*; pub use exclusive_function_system::*; @@ -162,6 +164,34 @@ pub trait IntoSystem: Sized { let name = format!("Pipe({}, {})", system_a.name(), system_b.name()); PipeSystem::new(system_a, system_b, Cow::Owned(name)) } + + /// Pass the output of this system into the passed function `f`, creating a new system that + /// outputs the value returned from the function. + /// + /// ``` + /// # use bevy_ecs::prelude::*; + /// # let mut schedule = Schedule::new(); + /// // Ignores the output of a system that may fail. + /// schedule.add_systems(my_system.map(std::mem::drop)); + /// # let mut world = World::new(); + /// # world.insert_resource(T); + /// # schedule.run(&mut world); + /// + /// # #[derive(Resource)] struct T; + /// # type Err = (); + /// fn my_system(res: Res) -> Result<(), Err> { + /// // ... + /// # Err(()) + /// } + /// ``` + fn map(self, f: F) -> AdapterSystem + where + F: Send + Sync + 'static + FnMut(Out) -> T, + { + let system = Self::into_system(self); + let name = system.name(); + AdapterSystem::new(f, system, name) + } } // All systems implicitly implement IntoSystem. @@ -201,6 +231,7 @@ impl IntoSystem for T { pub struct In(pub In); /// A collection of common adapters for [piping](crate::system::PipeSystem) the result of a system. +#[deprecated = "this form of system adapter has been deprecated in favor of `system.map(...)`"] pub mod adapter { use crate::system::In; use bevy_utils::tracing; @@ -223,6 +254,7 @@ pub mod adapter { /// println!("{x:?}"); /// } /// ``` + #[deprecated = "use `.map(...)` instead"] pub fn new(mut f: impl FnMut(T) -> U) -> impl FnMut(In) -> U { move |In(x)| f(x) } @@ -260,6 +292,7 @@ pub mod adapter { /// # fn open_file(name: &str) -> Result<&'static str, std::io::Error> /// # { Ok("hello world") } /// ``` + #[deprecated = "use `.map(Result::unwrap)` instead"] pub fn unwrap(In(res): In>) -> T { res.unwrap() } @@ -287,6 +320,7 @@ pub mod adapter { /// "42".to_string() /// } /// ``` + #[deprecated = "use `.map(bevy_utils::info)` instead"] pub fn info(In(data): In) { tracing::info!("{:?}", data); } @@ -314,6 +348,7 @@ pub mod adapter { /// Ok("42".parse()?) /// } /// ``` + #[deprecated = "use `.map(bevy_utils::dbg)` instead"] pub fn dbg(In(data): In) { tracing::debug!("{:?}", data); } @@ -341,6 +376,7 @@ pub mod adapter { /// Err("Got to rusty?".to_string()) /// } /// ``` + #[deprecated = "use `.map(bevy_utils::warn)` instead"] pub fn warn(In(res): In>) { if let Err(warn) = res { tracing::warn!("{:?}", warn); @@ -369,6 +405,7 @@ pub mod adapter { /// Err("Some error".to_owned()) /// } /// ``` + #[deprecated = "use `.map(bevy_utils::error)` instead"] pub fn error(In(res): In>) { if let Err(error) = res { tracing::error!("{:?}", error); @@ -409,6 +446,7 @@ pub mod adapter { /// Some(()) /// } /// ``` + #[deprecated = "use `.map(std::mem::drop)` instead"] pub fn ignore(In(_): In) {} } @@ -510,7 +548,7 @@ mod tests { Schedule, }, system::{ - adapter::new, Commands, In, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query, + Commands, In, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query, QueryComponentError, Res, ResMut, Resource, System, SystemState, }, world::{FromWorld, World}, @@ -1785,10 +1823,10 @@ mod tests { !val } - assert_is_system(returning::>.pipe(unwrap)); - assert_is_system(returning::>.pipe(ignore)); - assert_is_system(returning::<&str>.pipe(new(u64::from_str)).pipe(unwrap)); - assert_is_system(exclusive_in_out::<(), Result<(), std::io::Error>>.pipe(error)); + assert_is_system(returning::>.map(Result::unwrap)); + assert_is_system(returning::>.map(std::mem::drop)); + assert_is_system(returning::<&str>.map(u64::from_str).map(Result::unwrap)); + assert_is_system(exclusive_in_out::<(), Result<(), std::io::Error>>.map(bevy_utils::error)); assert_is_system(returning::.pipe(exclusive_in_out::)); returning::<()>.run_if(returning::.pipe(not)); diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs index 3dc32e81d8..1c3152a043 100644 --- a/crates/bevy_utils/src/lib.rs +++ b/crates/bevy_utils/src/lib.rs @@ -298,6 +298,30 @@ impl Drop for OnDrop { } } +/// Calls the [`tracing::info!`] macro on a value. +pub fn info(data: T) { + tracing::info!("{:?}", data); +} + +/// Calls the [`tracing::debug!`] macro on a value. +pub fn dbg(data: T) { + tracing::debug!("{:?}", data); +} + +/// Processes a [`Result`] by calling the [`tracing::warn!`] macro in case of an [`Err`] value. +pub fn warn(result: Result<(), E>) { + if let Err(warn) = result { + tracing::warn!("{:?}", warn); + } +} + +/// Processes a [`Result`] by calling the [`tracing::error!`] macro in case of an [`Err`] value. +pub fn error(result: Result<(), E>) { + if let Err(error) = result { + tracing::error!("{:?}", error); + } +} + /// Like [`tracing::trace`], but conditional on cargo feature `detailed_trace`. #[macro_export] macro_rules! detailed_trace { diff --git a/examples/ecs/system_piping.rs b/examples/ecs/system_piping.rs index f2584c46ad..0863daa700 100644 --- a/examples/ecs/system_piping.rs +++ b/examples/ecs/system_piping.rs @@ -5,7 +5,7 @@ use bevy::prelude::*; use std::num::ParseIntError; use bevy::log::LogPlugin; -use bevy::utils::tracing::Level; +use bevy::utils::{dbg, error, info, tracing::Level, warn}; fn main() { App::new() @@ -19,11 +19,11 @@ fn main() { Update, ( parse_message_system.pipe(handler_system), - data_pipe_system.pipe(info), - parse_message_system.pipe(dbg), - warning_pipe_system.pipe(warn), - parse_error_message_system.pipe(error), - parse_message_system.pipe(ignore), + data_pipe_system.map(info), + parse_message_system.map(dbg), + warning_pipe_system.map(warn), + parse_error_message_system.map(error), + parse_message_system.map(std::mem::drop), ), ) .run();