Skip to content

Enable skipping/running SystemSets during Stepping#13219

Open
cBournhonesque wants to merge 8 commits intobevyengine:mainfrom
cBournhonesque:cb/run-system-set-init
Open

Enable skipping/running SystemSets during Stepping#13219
cBournhonesque wants to merge 8 commits intobevyengine:mainfrom
cBournhonesque:cb/run-system-set-init

Conversation

@cBournhonesque
Copy link
Contributor

Objective

Solution

  • Instead, I think it's possible to extend the existing system stepping machinery to achieve what I want. It should not be a problem to require system-stepping since run_system_set() would mostly be used inside tests.
    As a bonus, the new solution provides more control, since it's possible to run all the systems in a set except for one, etc.

  • Add functions in SystemStepping to be able to add SystemStepping instructions (AlwaysRun, NeverRun) for an entire SystemSet. In those cases, system-stepping will issue instructions for every system that is contained inside the Set.

  • For ease of computation I had to add a variable sets_to_systems in the ScheduleGraph (which is gated behind the system-stepping feature), but it's requiring a lot of #[cfg(feature(bevy_debug_stepping))] statements elsewhere in the code. I wonder if there's a better solution?

Testing

  • Added some unit tests to check that system-set stepping instructions work properly

Changelog

Added

Extended SystemStepping to be able to Skip/Step-through all the systems of a SystemSet

@cBournhonesque cBournhonesque added A-ECS Entities, components, systems, and events D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 3, 2024
@cBournhonesque cBournhonesque added the X-Contentious There are nontrivial implications that should be thought through label May 19, 2024
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Haven't looked too closely at the implementation details yet, but this feels much better as a feature of system stepping than the approach in the previous pr.

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

What happens if you configure conflicting behavior for two different sets that contain the same system? i.e. one set is set to always run and the other is set to never run.

@janhohenheim janhohenheim added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jul 17, 2024
@cBournhonesque
Copy link
Contributor Author

cBournhonesque commented Aug 3, 2024

What happens if you configure conflicting behavior for two different sets that contain the same system? i.e. one set is set to always run and the other is set to never run.

Great question!
The current behavior is undefined, because there is no ordering:

  1. here when we access the system sets inside the schedule

I think this could probably updated; instead the list of behaviour_updates could be a BTreeMap to have an ordering of updates, and I could just go through all the updates in order of the behaviour_updates.
We would have to figure out something similar to what's listed here.
Is it possible to have a SystemSet be present multiple times inside a schedule?

Also, AFAIK every system creates an anonymous systemSet with the same name, so maybe an extension of this PR would be to get rid of the system-based stepping and to only keep the set-based API

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introduce an app::run_system_set() function

4 participants