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

Support systems that take references as input #15184

Merged
merged 38 commits into from
Sep 23, 2024

Conversation

ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Sep 13, 2024

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.

@ItsDoot ItsDoot added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 13, 2024
examples/ecs/system_piping.rs Outdated Show resolved Hide resolved
@ItsDoot ItsDoot added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 15, 2024
@ItsDoot ItsDoot marked this pull request as ready for review September 15, 2024 05:02
@ItsDoot 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 ItsDoot added this to the 0.15 milestone Sep 15, 2024
@JMS55
Copy link
Contributor

JMS55 commented Sep 15, 2024

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

@ItsDoot
Copy link
Contributor Author

ItsDoot commented Sep 15, 2024

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
Copy link
Contributor

JMS55 commented Sep 15, 2024

Makes sense, nvm then.

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
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +126 to +138
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
}
}
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.

crates/bevy_ecs/src/system/observer_system.rs Show resolved Hide resolved
crates/bevy_ecs/src/system/system_registry.rs Show resolved Hide resolved
@ItsDoot 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
@navneetankur
Copy link
Contributor

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

@ItsDoot
Copy link
Contributor Author

ItsDoot commented Sep 19, 2024

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
Copy link
Contributor

navneetankur commented Sep 19, 2024

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
Copy link
Contributor

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

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Sep 23, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 23, 2024
@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Sep 23, 2024
@ItsDoot
Copy link
Contributor Author

ItsDoot commented Sep 23, 2024

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

@alice-i-cecile
Copy link
Member

Yep, that validation is correct. Thanks!

@alice-i-cecile 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
26 checks passed
@ItsDoot ItsDoot deleted the inref branch September 23, 2024 18:54
@alice-i-cecile
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
7 participants