Support systems that take references as input by ItsDoot · Pull Request #15184 · bevyengine/bevy

ItsDoot · 2024-09-13T07:21:23Z

Objective

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 Observers get a 'static Trigger and that's unsound #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

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.

examples/ecs/system_piping.rs

JMS55 · 2024-09-15T05:26:01Z

I'd personally rather InMut<Foo> be named InRefMut<Foo> to match Rust's ref foo and ref mut foo syntax.

ItsDoot · 2024-09-15T05:36:52Z

I'd personally rather InMut<Foo> be named InRefMut<Foo> to match Rust's ref foo and ref mut foo syntax.

But it doesn't match how bevy names things:

JMS55 · 2024-09-15T05:40:43Z

Makes sense, nvm then.

crates/bevy_ecs/src/system/adapter_system.rs

crates/bevy_ecs/src/system/function_system.rs

crates/bevy_ecs/src/system/input.rs

bushrat011899

I really like this change and I'm excited to see what we can do once this is merged. Excellent work handling the early DX regressions in type inference.

bushrat011899 · 2024-09-15T23:08:57Z

crates/bevy_ecs/src/observer/mod.rs

+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
+    }
+}


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.

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.

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

crates/bevy_ecs/src/system/observer_system.rs

crates/bevy_ecs/src/system/system_registry.rs

navneetankur · 2024-09-19T11:30:17Z

Do we need SystemInput::unwrap method? It seems only wrap is needed.

ItsDoot · 2024-09-19T15:42:58Z

Do we need SystemInput::unwrap method? It seems only wrap is needed.

It's not currently used anywhere but might prove useful in the future.

navneetankur · 2024-09-19T16:23:09Z

Do we need SystemInput::unwrap method? It seems only wrap is needed.

I asked because:
Currently it creates an artificial limitation than SystemInput::Inner has to be able to converted back to SystemInput. It will prevent us from implementing SystemInput for some weird objects, which only makes sense to convert in one way but not back.
Anyway It's generally better to keep Trait methods minimal.

It's not currently used anywhere but might prove useful in the future.

Okay. ~~Though I feel it would have been better to add it later when needed~~. Also thanks, for the work.
Edit: On second thought, I am not sure :). If you think it will be useful in future, having it would prevent users from implementing it on such types.

SkiFire13 · 2024-09-19T16:44:25Z

My guess is that unwrap was needed in an earlier iteration but the code that was using it has been removed/changed.

Co-authored-by: Giacomo Stevanato <giaco.stevanato@gmail.com>

ItsDoot · 2024-09-23T17:31:58Z

@alice-i-cecile Should be good to go as long as this addition is correct: c08d853 (from #15276)

alice-i-cecile · 2024-09-23T17:37:26Z

Yep, that validation is correct. Thanks!

alice-i-cecile · 2024-10-20T14:34:09Z

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1695 if you'd like to help out.

…State::build_system_with_input (#17034) # Objective - Made certain methods public for advanced use cases. Methods that returns mutable references are marked as unsafe due to the possibility of violating internal lifetime constraint assumptions. - Fixes an issue introduced by #15184

…State::build_system_with_input (bevyengine#17034) # Objective - Made certain methods public for advanced use cases. Methods that returns mutable references are marked as unsafe due to the possibility of violating internal lifetime constraint assumptions. - Fixes an issue introduced by bevyengine#15184

@mockersf

A backport of #18840 to version 0.15.3. I was unsure whether I should also update the versions of all crates. Looking at previous backports this was not done. The code which relies on the old solver behavior was introduced in #15184 which is from version 0.15. So this is the only version which needs a backport. cc @mockersf

cart reviewed Sep 13, 2024

View reviewed changes

examples/ecs/system_piping.rs Outdated Show resolved Hide resolved

ItsDoot added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 15, 2024

ItsDoot marked this pull request as ready for review September 15, 2024 05:02

ItsDoot added S-Needs-Review Needs reviewer attention (from anyone!) to move forward P-Unsound A bug that results in undefined compiler behavior and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 15, 2024

ItsDoot added this to the 0.15 milestone Sep 15, 2024

SkiFire13 reviewed Sep 15, 2024

View reviewed changes

crates/bevy_ecs/src/system/adapter_system.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/system/adapter_system.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/system/function_system.rs Outdated Show resolved Hide resolved

SkiFire13 reviewed Sep 15, 2024

View reviewed changes

crates/bevy_ecs/src/system/input.rs Outdated Show resolved Hide resolved

bushrat011899 approved these changes Sep 15, 2024

View reviewed changes

ItsDoot force-pushed the inref branch from 639e836 to 32cc7d5 Compare September 17, 2024 02:20

SkiFire13 approved these changes Sep 17, 2024

View reviewed changes

ItsDoot added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 18, 2024

alice-i-cecile added the M-Release-Note Work that should be called out in the blog due to impact label Sep 23, 2024

alice-i-cecile approved these changes Sep 23, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Sep 23, 2024

alice-i-cecile removed this pull request from the merge queue due to a manual request Sep 23, 2024

ItsDoot and others added 13 commits September 23, 2024 12:18

remove unused import

36140ed

fix broken doc link

0192003

Replace per-systeminput impls with generic impl

261b9e3

Co-authored-by: Giacomo Stevanato <giaco.stevanato@gmail.com>

impl deref and debug for all systeminputs

432b02a

address feedback

b68e16c

add staticsysteminput to systeminput docs

6c46e48

give methods on SystemInput clearer names

c46130e

add side-effectful test demonstrating InMut

24f0adb

fix ci complaint

3f65788

remove SystemInput::unwrap (unused)

69eae8f

improve docs

981b206

fix errors

1623043

add validate_param impls

c08d853

ItsDoot force-pushed the inref branch from 9af4784 to c08d853 Compare September 23, 2024 17:21

alice-i-cecile added this pull request to the merge queue Sep 23, 2024

Merged via the queue into bevyengine:main with commit c7ec456 Sep 23, 2024

ItsDoot deleted the inref branch September 23, 2024 18:54

alice-i-cecile mentioned this pull request Oct 20, 2024

Write release notes for PR #15184: Support systems that take references as input bevyengine/bevy-website#1695

Closed

alice-i-cecile mentioned this pull request Nov 3, 2024

Release notes for reference in system input bevyengine/bevy-website#1764

Merged

PixelDust22 mentioned this pull request Dec 30, 2024

Make 8 methods public and updated input parameter generics for SystemState::build_system_with_input #17034

Merged

lcnr mentioned this pull request May 25, 2025

backport to 0.15: remove reliance on a trait solver bug #19360

Merged

mxgrey mentioned this pull request Jun 4, 2025

Provide a buffer initialization stage open-rmf/crossflow#82

Open

Uh oh!

Conversation

ItsDoot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Showcase

Migration Guide

Uh oh!

Uh oh!

JMS55 commented Sep 15, 2024

Uh oh!

ItsDoot commented Sep 15, 2024

Uh oh!

JMS55 commented Sep 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bushrat011899 left a comment

Choose a reason for hiding this comment

Uh oh!

bushrat011899 Sep 15, 2024

Choose a reason for hiding this comment

Uh oh!

ItsDoot Sep 15, 2024

Choose a reason for hiding this comment

Uh oh!

bushrat011899 Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

navneetankur commented Sep 19, 2024

Uh oh!

ItsDoot commented Sep 19, 2024

Uh oh!

navneetankur commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SkiFire13 commented Sep 19, 2024

Uh oh!

ItsDoot commented Sep 23, 2024

Uh oh!

alice-i-cecile commented Sep 23, 2024

Uh oh!

alice-i-cecile commented Oct 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ItsDoot commented Sep 13, 2024 •

edited

Loading

navneetankur commented Sep 19, 2024 •

edited

Loading