Skip to content

Support systems that take references as input #15184

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
merged 38 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
0df60ca
switch input assoc type to generic param
ItsDoot Sep 12, 2024
26ed7e2
experiment for allowing refs in system input
ItsDoot Sep 12, 2024
b89e1ea
fix and comment out usage issues for now
ItsDoot Sep 13, 2024
7955555
fix pipe inference in example
ItsDoot Sep 13, 2024
1df45dd
fix ecs piping example
ItsDoot Sep 13, 2024
f017238
remove observer trigger transmute
ItsDoot Sep 13, 2024
30138bc
fix pipe system naming
ItsDoot Sep 13, 2024
61c1698
override run function for pipe systems to fix tests
ItsDoot Sep 13, 2024
8e76f43
fix inference for piping, but missing impl for now
ItsDoot Sep 14, 2024
74099bb
switch back to assoc type, and fix type inference
ItsDoot Sep 15, 2024
1b35bd5
fix example
ItsDoot Sep 15, 2024
f08681e
add documentation
ItsDoot Sep 15, 2024
caa273a
add new input types to prelude
ItsDoot Sep 15, 2024
219f15f
remove unnecessary function, rename other functions, update docs
ItsDoot Sep 15, 2024
6c071b8
fix doctests
ItsDoot Sep 15, 2024
842c92a
cleanup
ItsDoot Sep 15, 2024
66b7506
improve doctests
ItsDoot Sep 15, 2024
3661f6d
remove extra whitespace
ItsDoot Sep 15, 2024
c0a06cf
remove sized requirement for inref and inmut inner types
ItsDoot Sep 15, 2024
b1a5578
remove compilation test
ItsDoot Sep 15, 2024
8d3e89b
cleanup
ItsDoot Sep 15, 2024
9b98afe
replace generic param with impl trait
ItsDoot Sep 15, 2024
d4ddf50
fix some generic bounds
ItsDoot Sep 15, 2024
28c46a0
address feedback
ItsDoot Sep 15, 2024
3299564
fix bound on realonly pipe system
ItsDoot Sep 15, 2024
36140ed
remove unused import
ItsDoot Sep 15, 2024
0192003
fix broken doc link
ItsDoot Sep 15, 2024
261b9e3
Replace per-systeminput impls with generic impl
ItsDoot Sep 15, 2024
432b02a
impl deref and debug for all systeminputs
ItsDoot Sep 15, 2024
b68e16c
address feedback
ItsDoot Sep 15, 2024
6c46e48
add staticsysteminput to systeminput docs
ItsDoot Sep 15, 2024
c46130e
give methods on SystemInput clearer names
ItsDoot Sep 15, 2024
24f0adb
add side-effectful test demonstrating InMut
ItsDoot Sep 16, 2024
3f65788
fix ci complaint
ItsDoot Sep 16, 2024
69eae8f
remove SystemInput::unwrap (unused)
ItsDoot Sep 19, 2024
981b206
improve docs
ItsDoot Sep 20, 2024
1623043
fix errors
ItsDoot Sep 23, 2024
c08d853
add validate_param impls
ItsDoot Sep 23, 2024
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
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
}
}
Comment on lines +126 to +138
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a particular reason this was added in this PR? I don't disagree with the implementation, but it feels tangential to the goal here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added Deref/DerefMut impls for all SystemInputs for future generic usage of abstracting over mutable or readonly access to a type in the input: where I: SystemInput + DerefMut<Target = String>, for example. Since Trigger is now a SystemInput, I added it for equality. If it's a controversial change I'm fine with removing it, but thought I might as well since we're mucking around with observers here.

Copy link
Contributor

Choose a reason for hiding this comment

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

No that seems perfectly reasonable, thanks for clarifying! Just wanted to get the reasoning written down in-case anyone else had the same thought.


/// 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