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 a module for common system chain/pipe adapters #5776

Closed
wants to merge 12 commits into from
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/lib.rs
Expand Up @@ -39,9 +39,9 @@ pub mod prelude {
StageLabel, State, SystemLabel, SystemSet, SystemStage,
},
system::{
Commands, In, IntoChainSystem, IntoExclusiveSystem, IntoSystem, Local, NonSend,
NonSendMut, ParallelCommands, ParamSet, Query, RemovedComponents, Res, ResMut,
Resource, System, SystemParamFunction,
adapter as system_adapter, Commands, In, IntoChainSystem, IntoExclusiveSystem,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
adapter as system_adapter, Commands, In, IntoChainSystem, IntoExclusiveSystem,
adapter as chain_adapter, Commands, In, IntoChainSystem, IntoExclusiveSystem,

Wouldn't it be better to use chain_adapter as those adapters can only be used on ChainSystems?

Copy link
Member Author

@JoJoJet JoJoJet Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current plan is to rename .chain() to .pipe(), so I'd rather not give it a name that we'd have to change later.

Also, system_adapter feels more consistent to me. The two ways of accessing this module are bevy::ecs::system::adapter and bevy::prelude::system_adapter. Like how bevy_ecs and bevy::ecs mean the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense

IntoSystem, Local, NonSend, NonSendMut, ParallelCommands, ParamSet, Query,
RemovedComponents, Res, ResMut, Resource, System, SystemParamFunction,
},
world::{FromWorld, Mut, World},
};
Expand Down
128 changes: 128 additions & 0 deletions crates/bevy_ecs/src/system/system_chaining.rs
Expand Up @@ -142,3 +142,131 @@ where
}
}
}

/// A collection of common adapters for [chaining](super::ChainSystem) the result of a system.
pub mod adapter {
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
use crate::system::In;
use std::fmt::Debug;

/// Converts a regular function into a system adapter.
///
/// # Examples
/// ```
/// use bevy_ecs::prelude::*;
///
/// return1
/// .chain(system_adapter::new(u32::try_from))
/// .chain(system_adapter::unwrap)
/// .chain(print);
///
/// fn return1() -> u64 { 1 }
/// fn print(In(x): In<impl std::fmt::Debug>) {
/// println!("{x:?}");
/// }
/// ```
pub fn new<T, U>(mut f: impl FnMut(T) -> U) -> impl FnMut(In<T>) -> U {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO new may be confusing. In rust new is generally used to create an object.
Since this is a mapping operation, I think map would fit better.

Copy link
Member Author

@JoJoJet JoJoJet Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I respectfully disagree. The meaning behind the name system_adapter::new is "create a new system adapter from some fn", which should be clear enough IMO. And the resulting system adapter is an object, of course.

I don't think it makes sense to call it map, since the mapping is semantically being done by the .chain() call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't take into consideration the mod name. It makes sense.

move |In(x)| f(x)
}

/// System adapter that unwraps the `Ok` variant of a [`Result`].
/// This is useful for fallible systems that should panic in the case of an error.
///
/// There is no equivalent adapter for [`Option`]. Instead, it's best to provide
/// an error message and convert to a `Result` using `ok_or{_else}`.
///
/// # Examples
///
/// Panicking on error
///
/// ```
/// use bevy_ecs::prelude::*;
/// #
/// # #[derive(StageLabel)]
/// # enum CoreStage { Update };
///
/// // Building a new schedule/app...
/// # use bevy_ecs::schedule::SystemStage;
/// # let mut sched = Schedule::default(); sched
/// # .add_stage(CoreStage::Update, SystemStage::single_threaded())
/// .add_system_to_stage(
/// CoreStage::Update,
/// // Panic if the load system returns an error.
/// load_save_system.chain(system_adapter::unwrap)
/// )
/// // ...
/// # ;
/// # let mut world = World::new();
/// # sched.run(&mut world);
///
/// // A system which may fail irreparably.
/// fn load_save_system() -> Result<(), std::io::Error> {
/// let save_file = open_file("my_save.json")?;
/// dbg!(save_file);
/// Ok(())
/// }
/// # fn open_file(name: &str) -> Result<&'static str, std::io::Error>
/// # { Ok("hello world") }
/// ```
pub fn unwrap<T, E: Debug>(In(res): In<Result<T, E>>) -> T {
res.unwrap()
}

/// System adapter that ignores the output of the previous system in a chain.
/// This is useful for fallible systems that should simply return early in case of an `Err`/`None`.
///
/// # Examples
///
/// Returning early
///
/// ```
/// use bevy_ecs::prelude::*;
///
/// // Marker component for an enemy entity.
/// #[derive(Component)]
/// struct Monster;
/// #
/// # #[derive(StageLabel)]
/// # enum CoreStage { Update };
///
/// // Building a new schedule/app...
/// # use bevy_ecs::schedule::SystemStage;
/// # let mut sched = Schedule::default(); sched
/// # .add_stage(CoreStage::Update, SystemStage::single_threaded())
/// .add_system_to_stage(
/// CoreStage::Update,
/// // If the system fails, just move on and try again next frame.
/// fallible_system.chain(system_adapter::ignore)
/// )
/// // ...
/// # ;
/// # let mut world = World::new();
/// # sched.run(&mut world);
///
/// // A system which may return early. It's more convenient to use the `?` operator for this.
/// fn fallible_system(
/// q: Query<Entity, With<Monster>>
/// ) -> Option<()> {
/// let monster_id = q.iter().next()?;
/// println!("Monster entity is {monster_id:?}");
/// Some(())
/// }
/// ```
pub fn ignore<T>(In(_): In<T>) {}

#[cfg(test)]
#[test]
fn assert_systems() {
use std::str::FromStr;

use crate::{prelude::*, system::assert_is_system};

/// Mocks a system that returns a value of type `T`.
fn returning<T>() -> T {
unimplemented!()
}

assert_is_system(returning::<Result<u32, std::io::Error>>.chain(unwrap));
assert_is_system(returning::<Option<()>>.chain(ignore));
assert_is_system(returning::<&str>.chain(new(u64::from_str)).chain(unwrap));
}
}