Skip to content
Closed
Changes from 1 commit
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
87 changes: 87 additions & 0 deletions rfcs/centralized_run_criteria.md
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:
Copy link
Member

Choose a reason for hiding this comment

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

It's not immediately clear to beginners that they can also be attached to system sets, which will be a common pattern.

Copy link
Author

Choose a reason for hiding this comment

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

System sets should be taught as "a way to attach things to multiple systems at the same time". "Criteria can be attached to systems" should click with that.

Copy link
Member

Choose a reason for hiding this comment

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

System sets should be taught as "a way to attach things to multiple systems at the same time".

Cool, I like that framing a lot. I'll try to do this in the new book.

```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:
Copy link
Member

Choose a reason for hiding this comment

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

This schedule-based API is very useful for raw consumers of bevy_ecs, but less familiar for users of bevy_ecs, who may not clue in that you can just add this to the AppBuilder as well.

Copy link
Author

Choose a reason for hiding this comment

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

I want to say that this RFC is about features of bevy_ecs, AppBuilder may as well not exist.

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely, but bevy_app is a downstream consumer, and will have to respond to these API changes.

Not essential here though, just something to bear in mind.

```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):
Copy link
Member

Choose a reason for hiding this comment

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

What precisely does "unique" mean here? Must the system only have one label? Must the label only have one system?

Copy link
Author

Choose a reason for hiding this comment

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

Both, actually.

Copy link
Author

Choose a reason for hiding this comment

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

I've revised the design in 10ee296. Now criteria can have as many labels as they want(e.g. for execution order specification), but only the labels that map to just one criteria support criteria reusing (and piping, which is irrelevant for this RFC so I omitted its mention).

```rust
schedule
.add_run_criteria(run_if_player_can_attack.label(PlayerControlCriteria::CanAttack))
Copy link
Member

Choose a reason for hiding this comment

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

If labels are required, it feels like the schedule (and appbuilder) methods like this should just take two arguments.

Copy link
Author

Choose a reason for hiding this comment

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

I'm torn on this one: two arguments is correct Rust, descriptors everywhere is consistent Bevy.

Copy link
Member

Choose a reason for hiding this comment

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

I still prefer two arguments so it's a a) compile-time error and b) immediately obvious what needs to be done.

Copy link
Author

Choose a reason for hiding this comment

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

I think I have a solution: the label passed here as the separate argument is the label that can be used for piping and reusing, labels attached to the descriptor (the criteria-bearing argument) are for execution order and whatever else. It even eliminates this restriction.

.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

Stages will become unusable individually, outside of a schedule.
Copy link
Member

@alice-i-cecile alice-i-cecile Jul 15, 2021

Choose a reason for hiding this comment

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

IMO this is fine because you can just make a schedule that contains a single stage.

In the long term, stages themselves are due for deprecation in some form so...


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

Choose a reason for hiding this comment

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

Oh right: schedules implement the Stage trait... Have we seen any practical applications of this yet? Having a sense of that will help guide our design here.

Copy link
Author

Choose a reason for hiding this comment

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

Startup systems.