Centralized run criteria #29

Ratysz · 2021-07-15T18:26:14Z

Lift run criteria and related machinery out of individual stages, and centralize them in the schedule instead.

alice-i-cecile · 2021-07-15T18:31:00Z

rfcs/centralized_run_criteria.md

+        }),
+    );
+```
+- Globally, owned by a schedule, usable by anything inside said schedule via a label:


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.

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

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.

alice-i-cecile · 2021-07-15T18:31:32Z

rfcs/centralized_run_criteria.md

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


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

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.

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.

alice-i-cecile · 2021-07-15T18:32:23Z

rfcs/centralized_run_criteria.md

+    .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):


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

Both, actually.

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

alice-i-cecile · 2021-07-15T18:34:19Z

rfcs/centralized_run_criteria.md

+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))


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

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

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

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.

rfcs/centralized_run_criteria.md

alice-i-cecile · 2021-07-15T18:36:13Z

rfcs/centralized_run_criteria.md

+
+## Drawbacks
+
+Stages will become unusable individually, outside of a schedule.


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

alice-i-cecile · 2021-07-15T18:38:39Z

rfcs/centralized_run_criteria.md

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


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.

Startup systems.

alice-i-cecile

I really like this: it's a sensible refactor that makes other critical user-facing work much easier. I have a few nits on API design and clarity (left as notes on the code), but you have my agreement in principle.

cart · 2021-07-15T20:18:30Z

rfcs/29_centralized_run_criteria.md

+- 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


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.

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.

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

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

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.

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.

Nilirad

player_has_control is referred to as run_if_player_has_control, when added to stage.

rfcs/29_centralized_run_criteria.md

Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>

cart · 2021-07-17T19:26:46Z

rfcs/29_centralized_run_criteria.md

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


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.

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.

cart · 2021-07-17T19:34:30Z

rfcs/29_centralized_run_criteria.md

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


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?

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.

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.

alice-i-cecile · 2023-02-06T01:22:18Z

Implemented as part of #45.

Ratysz added 2 commits July 15, 2021 21:23

Accidentally entire RFC.

7db4415

RFC number.

0da27c7

alice-i-cecile reviewed Jul 15, 2021

View reviewed changes

rfcs/centralized_run_criteria.md Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jul 15, 2021

View reviewed changes

cart reviewed Jul 15, 2021

View reviewed changes

Nilirad suggested changes Jul 16, 2021

View reviewed changes

rfcs/29_centralized_run_criteria.md Outdated Show resolved Hide resolved

Ratysz and others added 2 commits July 16, 2021 12:50

Update rfcs/29_centralized_run_criteria.md

39a3243

Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>

Notes on label ambiguity and same-frame state changes.

10ee296

cart reviewed Jul 17, 2021

View reviewed changes

alice-i-cecile mentioned this pull request Jul 20, 2021

Composable builder pattern for system construction and stage configuration bevyengine/bevy#2510

Closed

This was referenced Aug 5, 2021

Run criteria attaching prototype ForesightMiningSoftwareCorporation/bevy_transform_gizmo#12

Closed

Adding run criteria to SystemSet::on_exit overwrites existing one bevyengine/bevy#2609

Closed

alice-i-cecile closed this Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralized run criteria #29

Centralized run criteria #29

Ratysz commented Jul 15, 2021 •

edited

Loading

alice-i-cecile Jul 15, 2021

Ratysz Jul 16, 2021

alice-i-cecile Jul 17, 2021

alice-i-cecile Jul 15, 2021

Ratysz Jul 15, 2021

alice-i-cecile Jul 17, 2021

alice-i-cecile Jul 15, 2021

Ratysz Jul 15, 2021

Ratysz Jul 16, 2021

alice-i-cecile Jul 15, 2021

Ratysz Jul 15, 2021

alice-i-cecile Jul 17, 2021

Ratysz Jul 17, 2021

alice-i-cecile Jul 15, 2021 •

edited

Loading

alice-i-cecile Jul 15, 2021

Ratysz Jul 15, 2021

alice-i-cecile left a comment

cart Jul 15, 2021

Ratysz Jul 16, 2021

cart Jul 17, 2021

Ratysz Jul 17, 2021

cart Jul 17, 2021

Ratysz Jul 17, 2021

Nilirad left a comment

cart Jul 17, 2021

Ratysz Jul 17, 2021

cart Jul 17, 2021

Ratysz Jul 17, 2021

cart Jul 17, 2021 •

edited

Loading

alice-i-cecile commented Feb 6, 2023


		## Drawbacks

		Stages will become unusable individually, outside of a 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 run criteria always run exactly once per schedule execution.

Centralized run criteria #29

Centralized run criteria #29

Conversation

Ratysz commented Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nilirad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cart Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile commented Feb 6, 2023

Ratysz commented Jul 15, 2021 •

edited

Loading

alice-i-cecile Jul 15, 2021 •

edited

Loading

cart Jul 17, 2021 •

edited

Loading