From 7db441546ef094bde6e08fe0f7048d4ce949fc82 Mon Sep 17 00:00:00 2001 From: Ratys Date: Thu, 15 Jul 2021 21:23:28 +0300 Subject: [PATCH 1/4] Accidentally entire RFC. --- rfcs/centralized_run_criteria.md | 87 ++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 rfcs/centralized_run_criteria.md diff --git a/rfcs/centralized_run_criteria.md b/rfcs/centralized_run_criteria.md new file mode 100644 index 00000000..74da63e5 --- /dev/null +++ b/rfcs/centralized_run_criteria.md @@ -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) -> 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| { + 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 + +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? \ No newline at end of file From 0da27c700c6e46b8dfb092ea8c3f76bbe789dc09 Mon Sep 17 00:00:00 2001 From: Ratys Date: Thu, 15 Jul 2021 21:26:50 +0300 Subject: [PATCH 2/4] RFC number. --- ...centralized_run_criteria.md => 29_centralized_run_criteria.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename rfcs/{centralized_run_criteria.md => 29_centralized_run_criteria.md} (100%) diff --git a/rfcs/centralized_run_criteria.md b/rfcs/29_centralized_run_criteria.md similarity index 100% rename from rfcs/centralized_run_criteria.md rename to rfcs/29_centralized_run_criteria.md From 39a3243feef332e2ae708c0e77a659118e3926c6 Mon Sep 17 00:00:00 2001 From: Alexander Sepity Date: Fri, 16 Jul 2021 12:50:42 +0300 Subject: [PATCH 3/4] Update rfcs/29_centralized_run_criteria.md Co-authored-by: Federico Rinaldi --- rfcs/29_centralized_run_criteria.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/29_centralized_run_criteria.md b/rfcs/29_centralized_run_criteria.md index 74da63e5..ddba362e 100644 --- a/rfcs/29_centralized_run_criteria.md +++ b/rfcs/29_centralized_run_criteria.md @@ -15,7 +15,7 @@ Run criteria are utility systems that control whether regular systems should run Criteria can be defined in two ways: - Inline, directly attached to their user: ```rust -fn player_has_control(controller: Res) -> ShouldRun { +fn run_if_player_has_control(controller: Res) -> ShouldRun { if controller.player_has_control() { ShouldRun::Yes } else { @@ -84,4 +84,4 @@ One alternative is to provide the new API as an addition to the existing stage-l - 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? \ No newline at end of file +- Should there be any special considerations for e.g. schedules nested inside stages? From 10ee29604152e566a832534f0b9aec5ce20940b0 Mon Sep 17 00:00:00 2001 From: Ratys Date: Fri, 16 Jul 2021 18:42:27 +0300 Subject: [PATCH 4/4] Notes on label ambiguity and same-frame state changes. --- rfcs/29_centralized_run_criteria.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/rfcs/29_centralized_run_criteria.md b/rfcs/29_centralized_run_criteria.md index ddba362e..c69f2721 100644 --- a/rfcs/29_centralized_run_criteria.md +++ b/rfcs/29_centralized_run_criteria.md @@ -48,7 +48,9 @@ schedule .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): +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)) @@ -57,7 +59,7 @@ schedule 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. +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. ## Implementation strategy @@ -74,6 +76,13 @@ 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. + +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.