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

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

Merged

Conversation

Nathan-Fenner
Copy link
Contributor

@Nathan-Fenner Nathan-Fenner commented Nov 5, 2023

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

Copy link
Contributor

github-actions bot commented Nov 5, 2023

Welcome, new contributor!

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

@Nathan-Fenner 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
@UkoeHB
Copy link
Contributor

UkoeHB commented Nov 6, 2023

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

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

#[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>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@Nathan-Fenner 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
@Trashtalk217
Copy link
Contributor

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

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

maniwani commented Nov 7, 2023

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.

@@ -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> {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm okay, I see your point.

Copy link
Contributor Author

@Nathan-Fenner Nathan-Fenner Nov 8, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Nov 8, 2023
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Nov 8, 2023
@alice-i-cecile alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Nov 8, 2023
Copy link
Contributor

@Trashtalk217 Trashtalk217 left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Trashtalk217 Trashtalk217 left a comment

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

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

Looks clean

@alice-i-cecile 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 alice-i-cecile added this pull request to the merge queue Nov 15, 2023
@alice-i-cecile
Copy link
Member

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).

Merged via the queue into bevyengine:main with commit 1f05e1e Nov 15, 2023
22 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…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
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-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! 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
Development

Successfully merging this pull request may close these issues.

7 participants