-
Notifications
You must be signed in to change notification settings - Fork 66
Centralized run criteria #29
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
# Feature Name: `centralized_run_criteria` | ||
|
||
## Summary | ||
|
||
This feature lifts run criteria and related machinery out of individual stages, and centralizes them in the schedule instead. | ||
|
||
## Motivation | ||
|
||
Currently, run criteria are stage-specific: it's not possible to use the same criteria across two stages without creating two copies of it. Code and behavior duplication is bad enough on its own, but here it also results in unintuitive quirks in any mechanism built on top of criteria ([bevy#1671](https://github.com/bevyengine/bevy/issues/1671), [bevy#1700](https://github.com/bevyengine/bevy/issues/1700), [bevy#1865](https://github.com/bevyengine/bevy/issues/1865)). | ||
|
||
## User-facing explanation | ||
|
||
Run criteria are utility systems that control whether regular systems should run during a given schedule execution. They can be used by entire schedules and stages, or by individual systems. Any system that returns `ShouldRun` can be used as a run criteria. | ||
|
||
Criteria can be defined in two ways: | ||
- Inline, directly attached to their user: | ||
```rust | ||
fn player_has_control(controller: Res<PlayerController>) -> ShouldRun { | ||
if controller.player_has_control() { | ||
ShouldRun::Yes | ||
} else { | ||
ShouldRun::No | ||
} | ||
} | ||
|
||
stage | ||
.add_system(player_movement_system.with_run_criteria(run_if_player_has_control)) | ||
.add_system( | ||
player_attack_system.with_run_criteria(|controller: Res<PlayerController>| { | ||
if controller.player_can_attack() { | ||
ShouldRun::Yes | ||
} else { | ||
ShouldRun::No | ||
} | ||
}), | ||
); | ||
``` | ||
- Globally, owned by a schedule, usable by anything inside said schedule via a label: | ||
```rust | ||
#[derive(Debug, Clone, PartialEq, Eq, Hash, RunCriteriaLabel)] | ||
enum PlayerControlCriteria { | ||
CanAttack, | ||
} | ||
|
||
schedule | ||
.add_run_criteria(run_if_player_can_attack.label(PlayerControlCriteria::CanAttack)) | ||
.add_system(player_attack_system.with_run_criteria(PlayerControlCriteria::CanAttack)) | ||
.add_system(player_dmg_buff_system.with_run_criteria(PlayerControlCriteria::CanAttack)) | ||
``` | ||
|
||
Since run criteria aren't forbidden from having side effects, it might become necessary to define an explicit execution order between them. This is done almost exactly as with regular systems, the main difference is that the label must be unique (to facilitate reuse, and piping - covered elsewhere): | ||
```rust | ||
schedule | ||
.add_run_criteria(run_if_player_can_attack.label(PlayerControlCriteria::CanAttack)) | ||
.add_run_criteria(run_if_player_can_move.after(PlayerControlCriteria::CanAttack)) | ||
``` | ||
|
||
It should be noted that criteria declared globally are required to have a label, because they would be useless for their purpose otherwise. Criteria declared inline can be labelled as well, but using that for anything other than defining execution order is detrimental to code readability. | ||
|
||
Run criteria are evaluated once at the start of schedule, any criteria returning `ShouldRun::*AndCheckAgain` will be evaluated again at the end of schedule. At that point, depending on the new result, the systems governed by those criteria might be ran again, followed by another criteria evaluation; this repeats until no criteria return `ShouldRun::Yes*`. Systems with no specified criteria run exactly once per schedule execution. | ||
|
||
## Implementation strategy | ||
|
||
- Move all fields related to systems' criteria from `SystemStage` to `Schedule`. | ||
- Move and adapt criteria processing machinery from `SystemStage` to `Schedule`. | ||
- Handle inline-defined criteria in system descriptors handed directly to stages. | ||
- Plumb criteria or their results from `Schedule` to `SystemStage` for execution. | ||
- Change stages' own criteria to work with criteria descriptors. | ||
- Rearrange criteria descriptor structs and related method bounds to make globally declared criteria require labels. | ||
|
||
## Drawbacks | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're missing a pretty big drawback: there is now no way to make run criteria react to state calculated this frame. This is possible in the current implementation. Ex:
This is still possible to express in the new system, but it won't behave as expected because the "can attack" criteria is calculated at the start of the frame instead of at the start of StageB. Of course, this can be done "inside" systems. But it creates the question "what is run criteria for"? "reusable conditional state that is calculated at the start of a schedule run only" is useful, but also pretty arbitrary and limited. Guiding users to the right abstraction might be challenging and I expect some amount of frustration in the event that they hit the limits of the abstraction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can still react to changes on the same frame, the same way we're doing it within a single stage: rerunning systems with the relevant criteria after the reevaluation at the end of the stage - "feedback". But this "feedforward" reaction mode will indeed be lost. Main difference between "feedforward" and schedule-wide "feedback" is that the former doesn't run state-specific systems of stages prior to the state change. I think it's possible to crowbar that back in, but it seems like more trouble than it's worth. But I'll add that to the drawbacks, for sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should come up with a list of scenarios that we want run criteria to support, then work back from there. In my head, "feedback-only" criteria only works (well) for "looping" criteria like fixed timestep and state transitions. Maybe this is fine, but I think its good to be intentional about it. And if run criteria is only useful for "some number of things countable on one hand", maybe its better to optimize for those things individually (ex: bake FSM drivers directly into the schedule executor). Also, if we really do plan on rewriting criteria and feedback again for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's exactly what
Three reasons: the intermediate is useable and useful on its own, these changes would have to be made with a full redesign anyway, and it's a smaller changeset than the full redesign would be. The latter reason further leads to it being easier and faster to design, implement, and review. I don't think that this being a separate RFC mandates that it must be its own PR: once we (well, me, probably) put together the namedropped RFCs, the actual implementation can be done in one pass, without this stopgap solution ever finding its way into a release. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Im fine with this being a separate RFC (and I think you made the right call on that). But I definitely wouldn't merge it without the wider context being established and agreed upon first. As-is this breaks too many scenarios and punts too much to "future work" which hasn't been designed yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's perfectly fine by me, in fact, I much prefer this staying in flight, rather than have it land only to get amended later if we run into a problem. |
||
|
||
Stages will become unusable individually, outside of a schedule. | ||
|
||
Performance of poorly-factored (wrt this feature) schedules might decrease. However, correctly refactoring them should result in a net increase. | ||
|
||
## Rationale and alternatives | ||
|
||
This feature is a step towards `first_class_fsm_drivers`, `stageless_api`, and `lifecycle_api` (TODO: link to RFCs). While these are attainable without implementing `centralized_run_criteria` first, they all would have to retread some amount of this work. | ||
|
||
One alternative is to provide the new API as an addition to the existing stage-local criteria. This would maintain usability of stages outside of a schedule, but could prove to be considerably harder to implement. It will definitely complicate the API, and will likely exist only until `stageless_api`. | ||
|
||
## Unresolved questions | ||
|
||
- It's unclear what the better approach is: implement this feature followed by `stageless_api`, or both of them at once. | ||
- Most of the implementation details are unknowable without actually starting on the implementation. | ||
- Should there be any special considerations for e.g. schedules nested inside stages? |
Uh oh!
There was an error while loading. Please reload this page.