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

Directly run system given world #2427

Closed
wants to merge 18 commits into from

Conversation

alice-i-cecile
Copy link
Member

Objective

Systems are an ergonomic and expressive API for performing well-encapsulated complex logic.
However, you could not easily use them while working directly with &mut World for custom commands, exclusive systems, writing tests or bare-bones bevy_ecs usage.

Solution

Adds a System::run_direct method, which immediately executes a single system on a particular world and then flushes any Commands (or similar system parameters).

The equivalent run_direct method is also created for exclusive systems.

This is also very useful for users who may want to construct their own game loop in part or whole, sacrificing parallelism for simplicity and control.

Notes

Successor to #2417; exposing an API that dodges the ownership issues created there. Closely related to #2234.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jul 2, 2021
fn run(&mut self, world: &mut World);

/// Runs the exclusive system directly on the world, initializing the world correctly
///
Copy link
Member Author

Choose a reason for hiding this comment

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

For both methods, it may be useful to describe how to persistently store and then access a system, rather than initializing it every frame.

This reduces work by enabling query caching and also allows Local resources to persist correctly.

I'm just not sure that the API docs are the right place to put that; it feels like content that might be better suited to the book.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I suppose that the way to do this is to probably store the system, run .initialize once, and then call the old .run() manually.

The new method is much more convenient, but will probably perform worse for repeated invocations.

Hmm. Maybe we also demonstrate how to store and run a stage in the same section of the book?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Enhancement A new feature and removed S-Needs-Triage This issue needs to be labelled labels Jul 2, 2021
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I'm not convinced that this API is more useful than the existing APIs

I think I'd prefer to just change the run method to initialise self; although this API could be useful for exclusive systems since they might already get called using the 'run' method.

Really, we just need to be much more explicit about System's safety conditions

/// Runs the exclusive system in the world.
///
/// Use [`run_direct`] instead if you are manually running a system outside of a schedule

fn run(&mut self, world: &mut World);
Copy link
Member

Choose a reason for hiding this comment

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

Does this imply that run has an additional safety requirement that self is initialised for this world?

If that is so, then it should be unsafe

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not unsafe in the Rust sense; it merely panics with an opaque error message.

@alice-i-cecile
Copy link
Member Author

I think I'd prefer to just change the run method to initialise self

The problem with this is that you lose all the nice caching, and have to repeat the initialization work repeatedly. This is much less of an issue in one-off systems that are being run than it is in schedules though.

I'm not convinced that this API is more useful than the existing APIs

The aim of this is actually to make working with exclusive world access easier, rather than compete directly with the way that other System methods are used. Rather than learning an entirely new API and fighting the borrow checker with cell and resource_scope, users can simply make an ordinary system and run it, then use the exclusive world access to finish up the task they had in mind.

@DJMcNab
Copy link
Member

DJMcNab commented Jul 7, 2021

But I mean that the actual body of this function seems to be:

fn apply_direct(world){
self.init(world);
self.run(world);
}

@alice-i-cecile
Copy link
Member Author

Immediately applying buffers is the other very useful trick for ordinary non-exclusive systems, which lets you bypass thinking about "have my commands applied yet".

This is largely a convenience method, but I think that it does a good job helping reduce boilerplate and mental overhead for basic uses by hiding some of the largely internal implementation details and just doing what end users would like to do when writing custom commands or using ordinary system syntax within larger exclusive systems.

run_direct for exclusive systems is mostly there to ensure API consistency; I would be very surprised to see it get heavy use.

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile alice-i-cecile added the S-Needs-RFC This issue or PR is particularly complex, and needs an approved RFC before it can be merged label Sep 22, 2021
@alice-i-cecile
Copy link
Member Author

Closing this out as I don't think it's immediately the way forward. We can easily reimplement this if needed.

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-Enhancement A new feature S-Needs-RFC This issue or PR is particularly complex, and needs an approved RFC before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants