Add 'World::run_system_with_input' function + allow `World::run_system` to get system output #10380

Nathan-Fenner · 2023-11-05T04:57:51Z

Objective

Allows chained systems taking an In<_> input parameter to be run as one-shot systems. This API was mentioned in #8963.

In addition, run_system(_with_input) returns the system output, for any 'static output type.

Solution

A new function, World::run_system_with_input allows a SystemId<I, O> to be run by providing an I value as input and producing O as an output.

SystemId<I, O> is now generic over the input type I and output type O, along with the related functions and types RegisteredSystem, RemovedSystem, register_system, remove_system, and RegisteredSystemError. These default to (), preserving the existing API, for all of the public types.

Changelog

Added World::run_system_with_input function to allow one-shot systems that take In<_> input parameters
Changed World::run_system and World::register_system to support systems with return types beyond ()
Added Commands::run_system_with_input command that schedules a one-shot system with an In<_> input parameter

github-actions · 2023-11-05T04:58:07Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

UkoeHB · 2023-11-06T05:56:15Z

Inputs are not included in oneshots because if your system has no input you need to write () as a placeholder. There is no corresponding API problem with outputs, so IMO this PR should include system outputs.

Nathan-Fenner · 2023-11-06T06:41:42Z

Supporting output is easy enough and churns more-or-less the same API surface, so I've added this to the PR.

Nathan-Fenner · 2023-11-06T06:42:28Z

crates/bevy_ecs/src/system/system_registry.rs

-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
-pub struct SystemId(Entity);
+#[derive(Eq)]
+pub struct SystemId<I = (), O = ()>(Entity, std::marker::PhantomData<fn(I) -> O>);


The particular type in the PhantomData doesn't really matter as long as it mentions both I and O, so I figured it might as well be fn(I) -> O since that (mnemonically) describes how the parameters are used.

Trashtalk217 · 2023-11-07T14:08:39Z

This looks good and I think it's a great expansion. I'll have to do a line-by-line review later, but for now, I'd like to point out that run_system_with_input probably needs a Command version as well. Of course, the Command versions cannot return output, (and that should be documented), but it'd be nice to give it input as well.

Also, can you now register and run system_a.pipe(system_b) systems? That might've already been possible, I'm not sure.

Nathan-Fenner · 2023-11-07T17:57:29Z

Also, can you now register and run system_a.pipe(system_b) systems? That might've already been possible, I'm not sure.

This is a good question - I'm not sure. I'll check later whether it's possible. If so, it should be documented and tested.

maniwani · 2023-11-07T21:59:11Z

I haven't reviewed this closely yet, but is adding generics to SystemId necessary? I see SystemId as an ultimately temporary type. We eventually want to refer to systems with their Entity key directly and we can't add any type parameters to that.

(edit: added paragraph)
Your implementation makes sense. I guess what I mean is if/when we make all systems entities, I'd really prefer if the standard way to look one up was just using its Entity key directly (and knowing I and O) instead of using a typed wrapper.

james7132 · 2023-11-08T02:11:24Z

crates/bevy_ecs/src/system/system_registry.rs

@@ -7,18 +7,18 @@ use thiserror::Error;

 /// A small wrapper for [`BoxedSystem`] that also keeps track whether or not the system has been initialized.
 #[derive(Component)]
-struct RegisteredSystem {
+struct RegisteredSystem<I, O> {


This may significantly increase the number of registered components within a World. Every new input-output signature registered in this way will result in a new Component type and ComponentId being allocated.

Agreed, I would really like to type-erase this.

I don't see how inputs and outputs can be done without type information.

What is the harm in having a lot of registered components?

Hmm okay, I see your point.

I started looking at how this might be done- by boxing the system again, we can store a Box<dyn Any + 'static + Send + Sync> whose contents are the BoxedSystem<I, O>.

By keeping SystemId<I, O> generic, we know what to downcast to inside of the functions that access the RegisteredSystem. So it remains possible, at the cost of 1 extra box allocation per registered system.

I think that, to maintain the current API and generally make run_system{_with_input} ergonomic, in this case the downcast would be assumed to succeed, using .expect(...) - there are no reasonable strategies to handle a downcasting error. Since the API doesn't expose RegisteredSystem we can assume that with any reasonable use, the types will match at runtime, using the API border here to keep them matched up.

On the other hand, I don't really see how many types of registered components would affect performance - it's still user-bounded by how many different one-shot systems the user creates, in terms of which types are used.

This is also an internal API, so if there is a need to refactor later for performance reasons, it can be done with little user-facing impact. If this is currently "fast enough" I'd rather get extra power into developers hands sooner and then if there are specific cases where it is too slow can go back and try to benchmark and improve.

Not sure if this helps, but I have prior art in this space building off @aevyrie's work.

Yeah, at least for me, I'm fine if this is merged with the existing architecture. Like you said, it's not public, this is incredibly useful functionality, and I don't think it's likely to be a serious performance footgun.

What is the harm in having a lot of registered components?

I haven't measured it, but bloating the components metadata stores may have a negative impact on TypeId -> ComponentId lookups, which most prominently show up in the typed World::get and Entity{Ref/Mut}::get APIs, which may cause Command slowdowns. This previously wasn't too big of a concern given that the component set typically doesn't grow organically over time after initialization, or if it does, it's not reliant on the generic APIs (i.e. the *_by_id untyped APIs), but these changes might allow that to happen over the lifetime of a World.

I'll be a bit more clear and say that I don't think this is blocking. If we're treating SystemId as a temporary holdover, then this shouldn't be a huge issue, and I eventually want to find alternatives to that HashMap<TypeId, ComponentId> as well.

FYI another way this could be done would be adding a supertrait to System that's free of the In and Out associated types, and whose run_erased function takes two &mut dyn Any:

the first one is supposed to point to a Option<I> and will be .take()n by the run_erased function to get its input;

the second one is supposed to point to a Option<O> and will be filled by the run_erased function with its output.

Then run_erased internally knows what the I and O types were supposed to be and can do the needed downcasts. This way any additional boxing is avoided, and only a dynamic check that the input and output types match up is performed when downcasting the &mut dyn Anys.

crates/bevy_ecs/src/system/system_registry.rs

Trashtalk217

Some extra things worth adding, in my opinion:

Adapt the RunSystem command to become RunSystemWithInput
Clearly document that the command version does not return any output

Other than that, I am not particularly worried about SystemId getting type parameters. I think the number of components will stay relatively small as most one-shot systems will have no input nor output, and because every component is tied to at least one system, you'd have to have a lot of systems for it to become a problem. I just don't think it's likely and as discussed, SystemId is not permanent.

It's worth looking into how this would work without type parameters but given the current design, this is a great extension. At the end of the day, I really like this API and I consider the existence of SystemId to be more of an implementation detail.

crates/bevy_ecs/src/system/system_registry.rs

Trashtalk217

It seems that the CI still has some complaints, but once those are fixed, I think this PR is ready to go.

Thank you for contributing!

Nathan-Fenner · 2023-11-15T03:12:35Z

Should be all fixed up now! 🚢 Thanks for reviewing!

UkoeHB

Looks clean

alice-i-cecile · 2023-11-15T13:45:54Z

I'm happy with the final API, the docs and tests are great, and this functionality is very useful. We can refactor internals later if there are strong proposals (or demonstrated problems).

…m` to get system output (bevyengine#10380) # Objective Allows chained systems taking an `In<_>` input parameter to be run as one-shot systems. This API was mentioned in bevyengine#8963. In addition, `run_system(_with_input)` returns the system output, for any `'static` output type. ## Solution A new function, `World::run_system_with_input` allows a `SystemId<I, O>` to be run by providing an `I` value as input and producing `O` as an output. `SystemId<I, O>` is now generic over the input type `I` and output type `O`, along with the related functions and types `RegisteredSystem`, `RemovedSystem`, `register_system`, `remove_system`, and `RegisteredSystemError`. These default to `()`, preserving the existing API, for all of the public types. --- ## Changelog - Added `World::run_system_with_input` function to allow one-shot systems that take `In<_>` input parameters - Changed `World::run_system` and `World::register_system` to support systems with return types beyond `()` - Added `Commands::run_system_with_input` command that schedules a one-shot system with an `In<_>` input parameter

Nathan-Fenner force-pushed the nf/run_system_with_input branch 2 times, most recently from ea11dd5 to 7728b9f Compare November 5, 2023 06:39

Nathan-Fenner changed the title ~~Add 'World::run_system_with_input' function~~ Add 'World::run_system_with_input' function + allow World::run_system to get system output Nov 6, 2023

Nathan-Fenner commented Nov 6, 2023

View reviewed changes

Nathan-Fenner force-pushed the nf/run_system_with_input branch 3 times, most recently from 6f2e243 to e711294 Compare November 7, 2023 03:32

james7132 requested a review from maniwani November 7, 2023 17:33

james7132 reviewed Nov 8, 2023

View reviewed changes

alice-i-cecile added C-Enhancement A new feature A-ECS Entities, components, systems, and events labels Nov 8, 2023

alice-i-cecile added this to the 0.13 milestone Nov 8, 2023

alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Nov 8, 2023

alice-i-cecile reviewed Nov 8, 2023

View reviewed changes

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

Trashtalk217 suggested changes Nov 9, 2023

View reviewed changes

crates/bevy_ecs/src/system/system_registry.rs Show resolved Hide resolved

crates/bevy_ecs/src/system/system_registry.rs Show resolved Hide resolved

Trashtalk217 mentioned this pull request Nov 10, 2023

Add run_system_singleton #10469

Closed

Nathan-Fenner added 2 commits November 12, 2023 14:37

Add 'World::run_system_with_input' function

f8df323

allow 'run_system' to get system output

6346ccd

Nathan-Fenner force-pushed the nf/run_system_with_input branch from e711294 to 6346ccd Compare November 12, 2023 22:38

include phantom data in SystemId debug

7f68e79

Nathan-Fenner force-pushed the nf/run_system_with_input branch from 6982a45 to 7f68e79 Compare November 12, 2023 22:40

support the run_system_with_input Command

90eaaa9

Nathan-Fenner force-pushed the nf/run_system_with_input branch from f1cb295 to 90eaaa9 Compare November 12, 2023 23:04

Nathan-Fenner requested a review from Trashtalk217 November 12, 2023 23:04

fix up docs

dc4b380

Trashtalk217 approved these changes Nov 13, 2023

View reviewed changes

fix doc comment

53ff88d

UkoeHB approved these changes Nov 15, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 15, 2023

alice-i-cecile approved these changes Nov 15, 2023

View reviewed changes

alice-i-cecile added this pull request to the merge queue Nov 15, 2023

Merged via the queue into bevyengine:main with commit 1f05e1e Nov 15, 2023
22 checks passed

UkoeHB mentioned this pull request Nov 16, 2023

Portable Callback objects #10582

Closed

stargazing-dino mentioned this pull request Dec 31, 2023

Alt design pattern with In Out Systems zkat/big-brain#90

Open

nicopap mentioned this pull request Jan 29, 2024

0.13 announcement post bevyengine/bevy-website#891

Merged

Testare mentioned this pull request Jan 30, 2024

Proto-RFC: Composable Systems and Components #1951

Closed

Trashtalk217 mentioned this pull request Feb 4, 2024

0.13: One-Shot system Improvements bevyengine/bevy-website#909

Merged

SkiFire13 mentioned this pull request Jul 27, 2024

Don't debug SystemId's entity field twice #14499

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'World::run_system_with_input' function + allow `World::run_system` to get system output #10380

Add 'World::run_system_with_input' function + allow `World::run_system` to get system output #10380

Nathan-Fenner commented Nov 5, 2023 •

edited

Loading

github-actions bot commented Nov 5, 2023

UkoeHB commented Nov 6, 2023

Nathan-Fenner commented Nov 6, 2023

Nathan-Fenner Nov 6, 2023

Trashtalk217 commented Nov 7, 2023

Nathan-Fenner commented Nov 7, 2023

maniwani commented Nov 7, 2023 •

edited

Loading

james7132 Nov 8, 2023

alice-i-cecile Nov 8, 2023

maniwani Nov 8, 2023

alice-i-cecile Nov 8, 2023

Nathan-Fenner Nov 8, 2023 •

edited

Loading

UkoeHB Nov 8, 2023

alice-i-cecile Nov 8, 2023

james7132 Nov 9, 2023

SkiFire13 Nov 20, 2023

Trashtalk217 left a comment •

edited

Loading

Trashtalk217 left a comment •

edited

Loading

Nathan-Fenner commented Nov 15, 2023

UkoeHB left a comment

alice-i-cecile commented Nov 15, 2023

Add 'World::run_system_with_input' function + allow World::run_system to get system output #10380

Add 'World::run_system_with_input' function + allow World::run_system to get system output #10380

Conversation

Nathan-Fenner commented Nov 5, 2023 • edited Loading

Objective

Solution

Changelog

github-actions bot commented Nov 5, 2023

UkoeHB commented Nov 6, 2023

Nathan-Fenner commented Nov 6, 2023

Choose a reason for hiding this comment

Trashtalk217 commented Nov 7, 2023

Nathan-Fenner commented Nov 7, 2023

maniwani commented Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nathan-Fenner Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trashtalk217 left a comment • edited Loading

Choose a reason for hiding this comment

Trashtalk217 left a comment • edited Loading

Choose a reason for hiding this comment

Nathan-Fenner commented Nov 15, 2023

UkoeHB left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Nov 15, 2023

Add 'World::run_system_with_input' function + allow `World::run_system` to get system output #10380

Add 'World::run_system_with_input' function + allow `World::run_system` to get system output #10380

Nathan-Fenner commented Nov 5, 2023 •

edited

Loading

maniwani commented Nov 7, 2023 •

edited

Loading

Nathan-Fenner Nov 8, 2023 •

edited

Loading

Trashtalk217 left a comment •

edited

Loading

Trashtalk217 left a comment •

edited

Loading