Skip to content

Commit

Permalink
Support systems that take references as input (#15184)
Browse files Browse the repository at this point in the history
# Objective

- Fixes #14924
- Closes #9584

## Solution

- We introduce a new trait, `SystemInput`, that serves as a type
function from the `'static` form of the input, to its lifetime'd
version, similarly to `SystemParam` or `WorldQuery`.
- System functions now take the lifetime'd wrapped version,
`SystemInput::Param<'_>`, which prevents the issue presented in #14924
(i.e. `InRef<T>`).
- Functions for running systems now take the lifetime'd unwrapped
version, `SystemInput::Inner<'_>` (i.e. `&T`).
- Due to the above change, system piping had to be re-implemented as a
standalone type, rather than `CombinatorSystem` as it was previously.
- Removes the `Trigger<'static, E, B>` transmute in observer runner
code.

## Testing

- All current tests pass.
- Added additional tests and doc-tests.

---

## Showcase

```rust
let mut world = World::new();

let mut value = 2;

// Currently possible:
fn square(In(input): In<usize>) -> usize {
    input * input
}
value = world.run_system_once_with(value, square);

// Now possible:
fn square_mut(InMut(input): InMut<usize>) {
    *input *= *input;
}
world.run_system_once_with(&mut value, square_mut);

// Or:
fn square_ref(InRef(input): InRef<usize>) -> usize {
    *input * *input
}
value = world.run_system_once_with(&value, square_ref);
```

## Migration Guide

- All current explicit usages of the following types must be changed in
the way specified:
    - `SystemId<I, O>` to `SystemId<In<I>, O>`
    - `System<In = T>` to `System<In = In<T>>`
    - `IntoSystem<I, O, M>` to `IntoSystem<In<I>, O, M>`
    - `Condition<M, T>` to `Condition<M, In<T>>`
- `In<Trigger<E, B>>` is no longer a valid input parameter type. Use
`Trigger<E, B>` directly, instead.

---------

Co-authored-by: Giacomo Stevanato <giaco.stevanato@gmail.com>
  • Loading branch information
ItsDoot and SkiFire13 authored Sep 23, 2024
1 parent 4c087da commit c7ec456
Show file tree
Hide file tree
Showing 17 changed files with 745 additions and 294 deletions.
12 changes: 8 additions & 4 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use bevy_ecs::{
intern::Interned,
prelude::*,
schedule::{ScheduleBuildSettings, ScheduleLabel},
system::{IntoObserverSystem, SystemId},
system::{IntoObserverSystem, SystemId, SystemInput},
};
#[cfg(feature = "trace")]
use bevy_utils::tracing::info_span;
Expand Down Expand Up @@ -302,10 +302,14 @@ impl App {
/// This allows for running systems in a push-based fashion.
/// Using a [`Schedule`] is still preferred for most cases
/// due to its better performance and ability to run non-conflicting systems simultaneously.
pub fn register_system<I: 'static, O: 'static, M, S: IntoSystem<I, O, M> + 'static>(
pub fn register_system<I, O, M>(
&mut self,
system: S,
) -> SystemId<I, O> {
system: impl IntoSystem<I, O, M> + 'static,
) -> SystemId<I, O>
where
I: SystemInput + 'static,
O: 'static,
{
self.main_mut().register_system(system)
}

Expand Down
12 changes: 8 additions & 4 deletions crates/bevy_app/src/sub_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use bevy_ecs::{
event::EventRegistry,
prelude::*,
schedule::{InternedScheduleLabel, ScheduleBuildSettings, ScheduleLabel},
system::SystemId,
system::{SystemId, SystemInput},
};

#[cfg(feature = "trace")]
Expand Down Expand Up @@ -189,10 +189,14 @@ impl SubApp {
}

/// See [`App::register_system`].
pub fn register_system<I: 'static, O: 'static, M, S: IntoSystem<I, O, M> + 'static>(
pub fn register_system<I, O, M>(
&mut self,
system: S,
) -> SystemId<I, O> {
system: impl IntoSystem<I, O, M> + 'static,
) -> SystemId<I, O>
where
I: SystemInput + 'static,
O: 'static,
{
self.world.register_system(system)
}

Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ pub mod prelude {
IntoSystemSetConfigs, Schedule, Schedules, SystemSet,
},
system::{
Commands, Deferred, EntityCommand, EntityCommands, In, IntoSystem, Local, NonSend,
NonSendMut, ParallelCommands, ParamSet, Query, ReadOnlySystem, Res, ResMut, Resource,
System, SystemParamBuilder, SystemParamFunction,
Commands, Deferred, EntityCommand, EntityCommands, In, InMut, InRef, IntoSystem, Local,
NonSend, NonSendMut, ParallelCommands, ParamSet, Query, ReadOnlySystem, Res, ResMut,
Resource, System, SystemIn, SystemInput, SystemParamBuilder, SystemParamFunction,
},
world::{
Command, EntityMut, EntityRef, EntityWorldMut, FromWorld, OnAdd, OnInsert, OnRemove,
Expand Down
15 changes: 15 additions & 0 deletions crates/bevy_ecs/src/observer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::{archetype::ArchetypeFlags, system::IntoObserverSystem, world::*};
use crate::{component::ComponentId, prelude::*, world::DeferredWorld};
use bevy_ptr::Ptr;
use bevy_utils::HashMap;
use std::ops::{Deref, DerefMut};
use std::{fmt::Debug, marker::PhantomData};

/// Type containing triggered [`Event`] information for a given run of an [`Observer`]. This contains the
Expand Down Expand Up @@ -122,6 +123,20 @@ impl<'w, E: Debug, B: Bundle> Debug for Trigger<'w, E, B> {
}
}

impl<'w, E, B: Bundle> Deref for Trigger<'w, E, B> {
type Target = E;

fn deref(&self) -> &Self::Target {
self.event
}
}

impl<'w, E, B: Bundle> DerefMut for Trigger<'w, E, B> {
fn deref_mut(&mut self) -> &mut Self::Target {
self.event
}
}

/// A description of what an [`Observer`] observes.
#[derive(Default, Clone)]
pub struct ObserverDescriptor {
Expand Down
7 changes: 0 additions & 7 deletions crates/bevy_ecs/src/observer/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,13 +357,6 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
propagate,
observer_trigger,
);
// SAFETY: the static lifetime is encapsulated in Trigger / cannot leak out.
// Additionally, IntoObserverSystem is only implemented for functions starting
// with for<'a> Trigger<'a>, meaning users cannot specify Trigger<'static> manually,
// allowing the Trigger<'static> to be moved outside of the context of the system.
// This transmute is obviously not ideal, but it is safe. Ideally we can remove the
// static constraint from ObserverSystem, but so far we have not found a way.
let trigger: Trigger<'static, E, B> = unsafe { std::mem::transmute(trigger) };
// SAFETY:
// - observer was triggered so must have an `Observer` component.
// - observer cannot be dropped or mutated until after the system pointer is already dropped.
Expand Down
101 changes: 53 additions & 48 deletions crates/bevy_ecs/src/schedule/condition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ use std::borrow::Cow;
use std::ops::Not;

use crate::system::{
Adapt, AdapterSystem, CombinatorSystem, Combine, IntoSystem, ReadOnlySystem, System,
Adapt, AdapterSystem, CombinatorSystem, Combine, IntoSystem, ReadOnlySystem, System, SystemIn,
SystemInput,
};

/// A type-erased run condition stored in a [`Box`].
Expand Down Expand Up @@ -57,7 +58,7 @@ pub type BoxedCondition<In = ()> = Box<dyn ReadOnlySystem<In = In, Out = bool>>;
///
/// ```
/// # use bevy_ecs::prelude::*;
/// fn identity() -> impl Condition<(), bool> {
/// fn identity() -> impl Condition<(), In<bool>> {
/// IntoSystem::into_system(|In(x)| x)
/// }
///
Expand All @@ -70,7 +71,7 @@ pub type BoxedCondition<In = ()> = Box<dyn ReadOnlySystem<In = In, Out = bool>>;
/// # world.insert_resource(DidRun(false));
/// # app.run(&mut world);
/// # assert!(world.resource::<DidRun>().0);
pub trait Condition<Marker, In = ()>: sealed::Condition<Marker, In> {
pub trait Condition<Marker, In: SystemInput = ()>: sealed::Condition<Marker, In> {
/// Returns a new run condition that only returns `true`
/// if both this one and the passed `and` return `true`.
///
Expand Down Expand Up @@ -466,20 +467,20 @@ pub trait Condition<Marker, In = ()>: sealed::Condition<Marker, In> {
}
}

impl<Marker, In, F> Condition<Marker, In> for F where F: sealed::Condition<Marker, In> {}
impl<Marker, In: SystemInput, F> Condition<Marker, In> for F where F: sealed::Condition<Marker, In> {}

mod sealed {
use crate::system::{IntoSystem, ReadOnlySystem};
use crate::system::{IntoSystem, ReadOnlySystem, SystemInput};

pub trait Condition<Marker, In>:
pub trait Condition<Marker, In: SystemInput>:
IntoSystem<In, bool, Marker, System = Self::ReadOnlySystem>
{
// This associated type is necessary to let the compiler
// know that `Self::System` is `ReadOnlySystem`.
type ReadOnlySystem: ReadOnlySystem<In = In, Out = bool>;
}

impl<Marker, In, F> Condition<Marker, In> for F
impl<Marker, In: SystemInput, F> Condition<Marker, In> for F
where
F: IntoSystem<In, bool, Marker>,
F::System: ReadOnlySystem,
Expand All @@ -496,7 +497,7 @@ pub mod common_conditions {
event::{Event, EventReader},
prelude::{Component, Query, With},
removal_detection::RemovedComponents,
system::{In, IntoSystem, Local, Res, Resource, System},
system::{In, IntoSystem, Local, Res, Resource, System, SystemInput},
};

/// A [`Condition`]-satisfying system that returns `true`
Expand Down Expand Up @@ -1110,10 +1111,12 @@ pub mod common_conditions {
/// app.run(&mut world);
/// assert_eq!(world.resource::<Counter>().0, 2);
/// ```
pub fn condition_changed<Marker, CIn, C: Condition<Marker, CIn>>(
condition: C,
) -> impl Condition<(), CIn> {
condition.pipe(|In(new): In<bool>, mut prev: Local<bool>| -> bool {
pub fn condition_changed<Marker, CIn, C>(condition: C) -> impl Condition<(), CIn>
where
CIn: SystemInput,
C: Condition<Marker, CIn>,
{
condition.pipe(|In(new): In<bool>, mut prev: Local<bool>| {
let changed = *prev != new;
*prev = new;
changed
Expand Down Expand Up @@ -1164,10 +1167,11 @@ pub mod common_conditions {
/// app.run(&mut world);
/// assert_eq!(world.resource::<Counter>().0, 2);
/// ```
pub fn condition_changed_to<Marker, CIn, C: Condition<Marker, CIn>>(
to: bool,
condition: C,
) -> impl Condition<(), CIn> {
pub fn condition_changed_to<Marker, CIn, C>(to: bool, condition: C) -> impl Condition<(), CIn>
where
CIn: SystemInput,
C: Condition<Marker, CIn>,
{
condition.pipe(move |In(new): In<bool>, mut prev: Local<bool>| -> bool {
let now_true = *prev != new && new == to;
*prev = new;
Expand All @@ -1179,21 +1183,22 @@ pub mod common_conditions {
/// Invokes [`Not`] with the output of another system.
///
/// See [`common_conditions::not`] for examples.
pub type NotSystem<T> = AdapterSystem<NotMarker, T>;
pub type NotSystem<S> = AdapterSystem<NotMarker, S>;

/// Used with [`AdapterSystem`] to negate the output of a system via the [`Not`] operator.
#[doc(hidden)]
#[derive(Clone, Copy)]
pub struct NotMarker;

impl<T: System> Adapt<T> for NotMarker
where
T::Out: Not,
{
type In = T::In;
type Out = <T::Out as Not>::Output;
impl<S: System<Out: Not>> Adapt<S> for NotMarker {
type In = S::In;
type Out = <S::Out as Not>::Output;

fn adapt(&mut self, input: Self::In, run_system: impl FnOnce(T::In) -> T::Out) -> Self::Out {
fn adapt(
&mut self,
input: <Self::In as SystemInput>::Inner<'_>,
run_system: impl FnOnce(SystemIn<'_, S>) -> S::Out,
) -> Self::Out {
!run_system(input)
}
}
Expand Down Expand Up @@ -1221,17 +1226,17 @@ pub struct AndMarker;

impl<In, A, B> Combine<A, B> for AndMarker
where
In: Copy,
for<'a> In: SystemInput<Inner<'a>: Copy>,
A: System<In = In, Out = bool>,
B: System<In = In, Out = bool>,
{
type In = In;
type Out = bool;

fn combine(
input: Self::In,
a: impl FnOnce(<A as System>::In) -> <A as System>::Out,
b: impl FnOnce(<B as System>::In) -> <B as System>::Out,
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, A>) -> B::Out,
) -> Self::Out {
a(input) && b(input)
}
Expand All @@ -1242,17 +1247,17 @@ pub struct NandMarker;

impl<In, A, B> Combine<A, B> for NandMarker
where
In: Copy,
for<'a> In: SystemInput<Inner<'a>: Copy>,
A: System<In = In, Out = bool>,
B: System<In = In, Out = bool>,
{
type In = In;
type Out = bool;

fn combine(
input: Self::In,
a: impl FnOnce(<A as System>::In) -> <A as System>::Out,
b: impl FnOnce(<B as System>::In) -> <B as System>::Out,
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, B>) -> B::Out,
) -> Self::Out {
!(a(input) && b(input))
}
Expand All @@ -1263,17 +1268,17 @@ pub struct NorMarker;

impl<In, A, B> Combine<A, B> for NorMarker
where
In: Copy,
for<'a> In: SystemInput<Inner<'a>: Copy>,
A: System<In = In, Out = bool>,
B: System<In = In, Out = bool>,
{
type In = In;
type Out = bool;

fn combine(
input: Self::In,
a: impl FnOnce(<A as System>::In) -> <A as System>::Out,
b: impl FnOnce(<B as System>::In) -> <B as System>::Out,
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, B>) -> B::Out,
) -> Self::Out {
!(a(input) || b(input))
}
Expand All @@ -1284,17 +1289,17 @@ pub struct OrMarker;

impl<In, A, B> Combine<A, B> for OrMarker
where
In: Copy,
for<'a> In: SystemInput<Inner<'a>: Copy>,
A: System<In = In, Out = bool>,
B: System<In = In, Out = bool>,
{
type In = In;
type Out = bool;

fn combine(
input: Self::In,
a: impl FnOnce(<A as System>::In) -> <A as System>::Out,
b: impl FnOnce(<B as System>::In) -> <B as System>::Out,
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, B>) -> B::Out,
) -> Self::Out {
a(input) || b(input)
}
Expand All @@ -1305,17 +1310,17 @@ pub struct XnorMarker;

impl<In, A, B> Combine<A, B> for XnorMarker
where
In: Copy,
for<'a> In: SystemInput<Inner<'a>: Copy>,
A: System<In = In, Out = bool>,
B: System<In = In, Out = bool>,
{
type In = In;
type Out = bool;

fn combine(
input: Self::In,
a: impl FnOnce(<A as System>::In) -> <A as System>::Out,
b: impl FnOnce(<B as System>::In) -> <B as System>::Out,
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, B>) -> B::Out,
) -> Self::Out {
!(a(input) ^ b(input))
}
Expand All @@ -1326,17 +1331,17 @@ pub struct XorMarker;

impl<In, A, B> Combine<A, B> for XorMarker
where
In: Copy,
for<'a> In: SystemInput<Inner<'a>: Copy>,
A: System<In = In, Out = bool>,
B: System<In = In, Out = bool>,
{
type In = In;
type Out = bool;

fn combine(
input: Self::In,
a: impl FnOnce(<A as System>::In) -> <A as System>::Out,
b: impl FnOnce(<B as System>::In) -> <B as System>::Out,
input: <Self::In as SystemInput>::Inner<'_>,
a: impl FnOnce(SystemIn<'_, A>) -> A::Out,
b: impl FnOnce(SystemIn<'_, B>) -> B::Out,
) -> Self::Out {
a(input) ^ b(input)
}
Expand Down
Loading

0 comments on commit c7ec456

Please sign in to comment.