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] - Allow piping run conditions #7547

Closed
wants to merge 6 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
9 changes: 4 additions & 5 deletions crates/bevy_ecs/src/schedule/condition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,14 @@ pub trait Condition<Params>: sealed::Condition<Params> {}
impl<Params, F> Condition<Params> for F where F: sealed::Condition<Params> {}

mod sealed {
use crate::system::{IntoSystem, IsFunctionSystem, ReadOnlySystemParam, SystemParamFunction};
use crate::system::{IntoSystem, ReadOnlySystem};

pub trait Condition<Params>: IntoSystem<(), bool, Params> {}

impl<Params, Marker, F> Condition<(IsFunctionSystem, Params, Marker)> for F
impl<Params, F> Condition<Params> for F
where
F: SystemParamFunction<(), bool, Params, Marker> + Send + Sync + 'static,
Params: ReadOnlySystemParam + 'static,
Marker: 'static,
F: IntoSystem<(), bool, Params>,
F::System: ReadOnlySystem,
{
}
}
Expand Down
13 changes: 13 additions & 0 deletions crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ use crate::{
use bevy_ecs_macros::all_tuples;
use std::{any::TypeId, borrow::Cow, marker::PhantomData};

use super::ReadOnlySystem;

/// The metadata of a [`System`].
#[derive(Clone)]
pub struct SystemMeta {
Expand Down Expand Up @@ -528,6 +530,17 @@ where
}
}

/// SAFETY: `F`'s param is `ReadOnlySystemParam`, so this system will only read from the world.
unsafe impl<In, Out, Param, Marker, F> ReadOnlySystem for FunctionSystem<In, Out, Param, Marker, F>
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
where
In: 'static,
Out: 'static,
Param: ReadOnlySystemParam + 'static,
Marker: 'static,
F: SystemParamFunction<In, Out, Param, Marker> + Send + Sync + 'static,
{
}

/// A trait implemented for all functions that can be used as [`System`]s.
///
/// This trait can be useful for making your own systems which accept other systems,
Expand Down
20 changes: 18 additions & 2 deletions crates/bevy_ecs/src/system/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,11 @@ pub use system::*;
pub use system_param::*;
pub use system_piping::*;

/// Ensure that a given function is a system
/// Ensure that a given function is a [system](System).
///
/// This should be used when writing doc examples,
/// to confirm that systems used in an example are
/// valid systems
/// valid systems.
pub fn assert_is_system<In, Out, Params, S: IntoSystem<In, Out, Params>>(sys: S) {
if false {
// Check it can be converted into a system
Expand All @@ -130,6 +130,22 @@ pub fn assert_is_system<In, Out, Params, S: IntoSystem<In, Out, Params>>(sys: S)
}
}

/// Ensure that a given function is a [read-only system](ReadOnlySystem).
///
/// This should be used when writing doc examples,
/// to confirm that systems used in an example are
/// valid systems.
pub fn assert_is_read_only_system<In, Out, Params, S: IntoSystem<In, Out, Params>>(sys: S)
where
S::System: ReadOnlySystem,
{
if false {
// Check it can be converted into a system
// TODO: This should ensure that the system has no conflicting system params
IntoSystem::into_system(sys);
}
}

#[cfg(test)]
mod tests {
use std::any::TypeId;
Expand Down
10 changes: 10 additions & 0 deletions crates/bevy_ecs/src/system/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ pub trait System: Send + Sync + 'static {
fn set_last_change_tick(&mut self, last_change_tick: u32);
}

/// [`System`] types that do not modify the [`World`] when run.
/// This is implemented for any systems whose parameters all implement [`ReadOnlySystemParam`].
///
/// [`ReadOnlySystemParam`]: crate::system::ReadOnlySystemParam
///
/// # Safety
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
///
/// This must only be implemented for system types which do not mutate the `World`.
pub unsafe trait ReadOnlySystem: System {}
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved

/// A convenience type alias for a boxed [`System`] trait object.
pub type BoxedSystem<In = (), Out = ()> = Box<dyn System<In = In, Out = Out>>;

Expand Down
17 changes: 17 additions & 0 deletions crates/bevy_ecs/src/system/system_piping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use crate::{
};
use std::{any::TypeId, borrow::Cow};

use super::ReadOnlySystem;

/// A [`System`] created by piping the output of the first system into the input of the second.
///
/// This can be repeated indefinitely, but system pipes cannot branch: the output is consumed by the receiving system.
Expand Down Expand Up @@ -153,6 +155,15 @@ impl<SystemA: System, SystemB: System<In = SystemA::Out>> System for PipeSystem<
}
}

/// SAFETY: Both systems are read-only, so piping them together will only read from the world.
unsafe impl<SystemA: System, SystemB: System<In = SystemA::Out>> ReadOnlySystem
for PipeSystem<SystemA, SystemB>
where
SystemA: ReadOnlySystem,
SystemB: ReadOnlySystem,
{
}

/// An extension trait providing the [`IntoPipeSystem::pipe`] method to pass input from one system into the next.
///
/// The first system must have return type `T`
Expand Down Expand Up @@ -412,10 +423,16 @@ pub mod adapter {
unimplemented!()
}

fn not(In(val): In<bool>) -> bool {
!val
}
Comment on lines +426 to +428
Copy link
Contributor

@Shatur Shatur Feb 7, 2023

Choose a reason for hiding this comment

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

Maybe move it into conditions module? I think it's a useful condition. Unless we going to have an extension trait that provides .not() method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to include the not adapter in this PR, since it adds extra considerations. I figure we can save that for a follow-up PR, where we can decide whether we'd rather use .not() or .pipe(not) (I don't really want us to have both options).


assert_is_system(returning::<Result<u32, std::io::Error>>.pipe(unwrap));
assert_is_system(returning::<Option<()>>.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::<bool>.pipe(exclusive_in_out::<bool, ()>));

returning::<()>.run_if(returning::<bool>.pipe(not));
}
}