Skip to content

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

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 96 additions & 0 deletions rfcs/29_centralized_run_criteria.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
# 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 run_if_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))
```

Note that only labels that appear on just one criteria can be utilized for reusing criteria results: the label to criteria relationship must be unambiguous.

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 exactly as with regular systems:
```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 run criteria always run exactly once per schedule execution.
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 mean that a schedule with the following stages:

UpdateStage: MoveSystem (fixed timestep criteria)
PostUpdateStage: TransformPropagateSystem
RenderStage: RenderSystem

Could execute like this for a single frame?

MoveSystem (Yes and Loop)
TransformPropagateSystem
RenderSystem
MoveSystem (No)

Isn't this, like, very wrong? MoveSystem is intended to be run X times for the fixed timestep prior to Transforms being updated and RenderSystem rendering the final transform state. Instead, only the first timestep is rendered, then subsequent timesteps happen after rendering and show up next frame (and note that the position would be out of sync with the TransformPropagate state at the start of the next frame). Am I confused?

Copy link
Author

Choose a reason for hiding this comment

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

No, you're right, however: this feature is breaking; what you outlined is a prime example of the breakage. The schedule and criteria that's okay in current implementation will most likely become nonsense - I should probably put that in drawbacks.

The intended solution is to either involve everything in a FSM, or rearrange the schedule to have all systems like "RenderSystem" at the start of the schedule. I'm not absolutely confident that's a good solution (on its own, anyway), hence the RFC.

Copy link
Member

@cart cart Jul 17, 2021

Choose a reason for hiding this comment

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

The intended solution is to either involve everything in a FSM, or rearrange the schedule to have all systems like "RenderSystem" at the start of the schedule.

I don't find the "move render system to the front" approach very satisfying. Render shouldn't run until PostUpdate runs, otherwise it will attempt to draw stuff that hasn't been "fully processed" yet. Putting it first feels wrong. We would need to create new machinery to ensure that it doesn't actually run on the first run, which feels complicated to me, especially if there are multiple stages that all rely on each other (ex: a PostRender stage). And it would mean that rendering happens for "the previous update" instead of "the current update". And managing "dependencies" between multiple stages (instead of just the Update->Render dependency) when they aren't necessarily inserted in the order they should run sounds nasty.

FSM might be a solution, but it feels like we should wait for that design before committing to this impl.


## 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
Copy link
Member

Choose a reason for hiding this comment

The 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:

  • StageA System: run a system where CanAttack is set according to some condition
  • StageB System: use run criteria to only run a system when CanAttack is set

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.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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 stageless_api and first_class_fsm_drivers, why bother with this intermediate design at all? We all have limited time to design, implement, and review things. Shouldn't we focus on designing the "final implementation"?

Copy link
Author

Choose a reason for hiding this comment

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

(ex: bake FSM drivers directly into the schedule executor)

That's exactly what first_class_fsm_drivers is referring to.

[...] why bother with this intermediate design at all?

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Right now, we have two modes of same-frame state change handling:

* "Feedforward": state was changed in a previous stage, so this stage will run only systems that aren't forbidden by the new state.
* "Feedback": a state change was detected while reevaluating criteria at the end of stage, so the stage runs again, with only the systems that are specific to the new state.
Copy link
Member

Choose a reason for hiding this comment

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

How is this:

a state change was detected while reevaluating criteria at the end of stage, so the stage runs again, with only the systems that are specific to the new state.

Compatible with this part of the RFC?

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 run criteria always run exactly once per schedule execution.

Copy link
Author

Choose a reason for hiding this comment

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

It's not. Not entirely, at least: the feedback paragraph refers to how things are now (roughly), the other one describes how they will be with this feature.


With `centralized_run_criteria`, feedforward mode is lost. Main difference between it and the feature's schedule-wide feedback is that the former doesn't run state-specific systems of stages prior to the state change. It could be possible to add that back in, but it seems like it would be more trouble than it's worth - especially considering that subsequent features (`first_class_fsm_drivers` and `stageless_api`) will likely incompatibly rework criteria and feedback.

## 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?