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

Enhance SubApp schedule usage #2970

Closed
notverymoe opened this issue Oct 15, 2021 · 7 comments
Closed

Enhance SubApp schedule usage #2970

notverymoe opened this issue Oct 15, 2021 · 7 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@notverymoe
Copy link
Contributor

notverymoe commented Oct 15, 2021

What problem does this solve or what need does it fill?

Doing some work based on the subapp changes in #2535 for bevy_ecs and bevy_app, and ran into a couple things. Though I understand they're heavy WIP still.

But regardless, it doesn't seem possible to manually execute all of the stages contained in a subapp's schedule. For example, in Render2, we currently fetch all the stages explicitly and run them - since we need to run the extract stage on another world. This means that if another plugin wants to add stages to the render subapp, they won't be executed - as far as I understand.

There's also no way to run the run_criteria check for when you're explicitly executing a schedule like this. Additionally, run_criteria can't be aware of the parent/extract world currently unless you inject that world ahead of time. That might be enough, but might be something to keep in mind with any solution.

What solution would you like?

A neat solution might be to add per-stage runners, or a SystemStageWithRunner type, though I'm not certain how you'd feed temporary state into them - still learning how far lifetimes can be pushed. In the case of Render2, the logic to run the on the parent's world would be implemented in the stage runner, and then just the subapp's run method could be called - removing the issue of run_criteria and mut iteration. This would allow the stage runner to attach the world before the run_criteria check.

What alternative(s) have you considered?

Currently I've implemented some escape-valve function in a fork. They allow me to call the run_criteria manually and iterate stages mutably. There's probably a much neater way to implement what I've attempted to implement, see additional context.

Only other plausible option that comes to mind to me is to push the "extract" stage logic down into bevy_ecs, so that bevy_app and subapp implementers don't need to concerns themselves with manual schedule execution. Probably be through the introduction of add_extract_system type methods, and some method to provide the "parent" world so that the extract stages can be run. This also makes things a lot more standardized and rigid, which may or may not be a plus in this situation.

Additional context

The fork implementation I mentioned can be found here:
https://github.com/butterscotch-rs/bevy/blob/81387d3ecfb1421d288a856c21c6da8a69f71151/crates/bevy_ecs/src/schedule/mod.rs#L216

@notverymoe notverymoe added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Oct 15, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use and removed C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Oct 15, 2021
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Oct 15, 2021

This is a very helpful and detailed user story! We've been discussing related topics over in #2801; as well as in the RFC repo and in the ecs-dev channel on Discord.

@notverymoe
Copy link
Contributor Author

Oh, oops! I didn't notice the discussion section, thanks for pointing it out! Should be an interesting read. Is there any worth in closing this issue and just moving the contents over to the discussion in some form, or is this the better format for user stories with regards to that?

@alice-i-cecile
Copy link
Member

This is a good format for these user stories IMO: they're clear, actionable items that we can eventually close.

@notverymoe
Copy link
Contributor Author

notverymoe commented Oct 16, 2021

Just some more information with what I've run into.

There's no way to attach extra information to stages, for example, to mark a stage as being an "extraction stage". That's expected, but I've just quickly added that into the fork as part of the stage trait. By adding that, it's possible to attach multiple stages that will "extract" in any position. However it's a tad inefficient since it will move that subapp world in-and-out of the parent world's resources multiple times even if all the extract stages are back to back. That's a pretty straightforward fix though.

By adding a marker, there's no need for the mutable iterator to return the stage's label and the sub-app logic can be abstracted from the specific sub-app. Which can be seen here:
https://github.com/butterscotch-rs/butterscotch/blob/c7a840bcccb18cc6ae43a4f90a633071cf013db8/crates/subapp/src/subapp_runner.rs

Untested currently, but I wanted to get this up before I went off to bed

@notverymoe
Copy link
Contributor Author

notverymoe commented Oct 17, 2021

Another attempt:
https://github.com/butterscotch-rs/bevy/tree/extract_logic_test

This one adds some logic into Schedule so that a parent world can be provided, and stages can be marked as extract (runs parent world, flush to schedule's world) or insert (runs on schedule's world, flush to parent's world). Consecutive stages of the same type won't incur additional setup/teardown cost (ie. 3 extract stages in a row is fine).

Requires some plumbning upwards of course, don't look to closely at that 😅 The API/code is pretty iffy with what I've put in there, and I'm sure there's still some errors (particularly around entity reservation). And I've had to make SystemStage aware of InsertWorld / ExtractWorld resources to implement the correct flushing behaviour, which muddies the waters a little.

The bonus is that it makes the render subapp just looks like this:

        let mut render_app = App::empty();

        render_app
            .add_stage(RenderStage::Extract, SystemStage::parallel()).set_stage_kind(&RenderStage::Extract, ScheduleStageKind::Extract)
            .add_stage(RenderStage::Prepare, SystemStage::parallel())
            .add_stage(RenderStage::Render,  SystemStage::parallel().with_system(render_system.exclusive_system()))
            .add_stage(RenderStage::Cleanup, SystemStage::parallel());
            
        render_app

@alice-i-cecile alice-i-cecile added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Dec 12, 2021
@james7132
Copy link
Member

james7132 commented Feb 16, 2023

This seems to have been addressed in #6503 and the follow-up PRs. Is this issue still relevant?

@alice-i-cecile
Copy link
Member

I think this has changed enough that I'd prefer a new issue for any ongoing usability requests.

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-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

No branches or pull requests

3 participants