[Merged by Bors] - System sets and run criteria v2

[Ratysz]

I'm opening this prematurely; consider this an RFC that predates RFCs and therefore not super-RFC-like.

This PR does two "big" things: decouple run criteria from system sets, reimagine system sets as weapons of mass system description.

What it lets us do:

• Reuse run criteria within a stage.
• Pipe output of one run criteria as input to another.
• Assign labels, dependencies, run criteria, and ambiguity sets to many systems at the same time.

Things already done:

• Decoupled run criteria from system sets.
• Mass system description superpowers to SystemSet.
• Implemented RunCriteriaDescriptor.
• Removed VirtualSystemSet.
• Centralized all run criteria of SystemStage.
• Extended system descriptors with per-system run criteria.
• .before() and .after() for run criteria.
• Explicit order between state driver and related run criteria. Fixes State-dependent system runs twice on wrong add_state call order #1672.
• Opt-in run criteria deduplication; default behavior is to panic.
• Labels (not exposed) for state run criteria; state run criteria are deduplicated.

API issues that need discussion:

• FixedTimestep::step(1.0).label("my label") and FixedTimestep::step(1.0).with_label("my label") are both valid but do very different things.

---

I will try to maintain this post up-to-date as things change. Do check the diffs in "edited" thingy from time to time.

[alice-i-cecile]

Do we want built-in run criteria "combinators"? (Although, this is best left for another PR - good candidate for battle-testing the RFC process.)

Relevant to this question: #1295. I think we leave this for another PR and keep this scope small so we can try to get this out ASAP to fix bugs.

[TheRawMeatball]

There is no way to declare a run criterion without attaching it to something. Maybe it's for the best?

FixedTimestep::step(1.0).label("my label") and FixedTimestep::step(1.0).with_label("my label") are both valid but do very different things.

Personally I think this is very important to reinforce the idea that run criteria and systems are separate things. Also, this trivially solves the second problem.

my_label.chain(my_criterion.system()) feels too arcane. Even when no inline closures are involved:

As a first pass, I'd like to propose RunCriteria::of(label).chain(something_else.system()) as a possible solution. The of method should return a type that allows for ergonomically referring to a certain run criteria and be designed with the possibility of built-in combinators in mind.

[alice-i-cecile]

Personally I think this is very important to reinforce the idea that run criteria and systems are separate things. Also, this trivially solves the second problem.

RunCriteria being cleanly distinct from systems would help a lot with explaining how to use them, creating a distinct API and ensuring we can further specialize them. Ideally they can freely take SystemParam and have their data fed to them by Bevy's internals, but otherwise share very little else with Systems.

[alice-i-cecile]

In my view, I would expect most run criteria to be reused, especially as games grow. So I think my ideal API would look like:

app
  .add_run_criteria(FixedTimeStep{ 2.0 }, MyRunCriteria::EveryTwo)
  .add_system(blinken_lights.system()).with_run_criteria(MyRunCriteria::EveryTwo)
  .add_system_set([
     forces.system(),
     physics.system(),
  ]).with_run_criteria(MyRunCriteria::EveryTwo);

With distinct RunCriteria and RunCriteriaLabel types / traits.

Edit:

Ideally, I'd love to be able to attach run criteria to system labels as well, allowing you to tie run criteria behavior to a label directly. I would prefer this to being able to attach run criteria to system sets at all, provided we can label each system in a system set (I haven't poked at the new labelling API deeply).

That might be out of scope though and I could live without it.

[Ratysz]

Personally I think this is very important to reinforce the idea that run criteria and systems are separate things.

Yes. I think we can do this and use familiar APIs.

Also, this trivially solves the second problem.

I'm not sure I see how?..

As a first pass, I'd like to propose RunCriteria::of(label).chain(something_else.system()) as a possible solution.

Maybe; I'll see how that feels in tests. What's in now already returns a type that can have special APIs on it: the RunCriterionDescriptor.

So I think my ideal API would look like: [...]

Yes, something close to that should be possible. However, arrays of systems coercing into system sets is not trivial, and not because const generics: physics.system() and forces.system() are different types. This can be solved by making .system() return a proper SystemDescriptor - which would fix quite a few things, actually, but will most likely break even more things. So that particular desire will have to remain out of scope for now.

Ideally, I'd love to be able to attach run criteria to system labels as well, allowing you to tie run criteria behavior to a label directly.

"Define stuff by doing things to a label" is on the docket as "future API R&D" - after stageless, most likely.

[TheRawMeatball]

Yes, something close to that should be possible.

While arrays are definitely problematic, we should be able to make something that works using tuples. With a trait for these systems, and tuples of these systems, we could have far more elegant description syntax.

[alice-i-cecile]

While arrays are definitely problematic, we should be able to make something that works using tuples. With a trait for these systems, and tuples of these systems, we could have far more elegant description syntax.

More "I wish we had variadic generics" macros ;) I like this idea though: tuples are better than nothing.

[cart]

This generally looks good so far. I don't think we should merge it before 0.5 unless it:

1. passes existing state tests (as written)
2. solves State-dependent system runs twice on wrong add_state call order #1672 (i don't think Hard lock using State across stages #1671 needs solving before 0.5 and i dont really see how this pr could help there). It looks like the State-dependent system runs twice on wrong add_state call order #1672 solution would be to:
   • Add a new StateDriver<T> run criteria label, which is automatically added to the driver criteria
   • Make State::<T>::on_enter() return a RunCriteriaDescriptor (StateDriverLabel<T>::new().chain(State::<T>::current_on_enter_impl())

Its a little odd that we're making criteria chaining kind of like after() and kind of like chain(). It feels like consistency would be a good thing there.

If criteria can be ordered (and use labels), imo we should use before() and after() for those cases and leave chain() for system chaining.

[Ratysz]

Its a little odd that we're making criteria chaining kind of like after() and kind of like chain(). It feels like consistency would be a good thing there.

If criteria can be ordered (and use labels), imo we should use before() and after() for those cases and leave chain() for system chaining.

Yes, it is a bit odd, but it's doing something that we don't have a precedent for: "chaining" systems without welding them together into one system. Without a syntax for this the alternative is to use the regular decoupling techniques, like events or channels or any other kind of buffer resource, all of which are more cumbersome.

It bears mentioning that this does not actually replace regular chaining, so I think a more distinct name would be better.

I already have after() and before() locally. Cleaning it up; I'll try a few more things just in case, then it should be ready.

[alice-i-cecile]

I definitely dislike using the chain name here when the behaviour is so different. It's also not super clear what "chain" might do at all to a new user.

If we need the behaviour, what about immediately_before, directly_before or just_before? And the same for an after variant.

[Ratysz]

It bears mentioning that this does not actually replace regular chaining, so I think a more distinct name would be better.

Discussion on Discord so far seems to converge towards .map(), .pipe(), .map_criteria(), or .pipe_criteria(). I'm partial to the last one.

[alice-i-cecile]

Since piping doesn't make sense if run criteria labels are many-to-many, the labels are soft-required to be unique. In practice it means that any criteria using a duplicate label is silently discarded.

I don't love this behavior :/ It's very implicit and hard to wrap your head around.

Which run criteria label is the "first" one in the case of duplicates?

This is silent because state run criteria would produce unsilencable noise otherwise; not deduplicating them automatically is an odd papercut to have.

Why do we have duplicate run criteria labels at all? This being silent seems very dangerous and hard to debug: I'd prefer a panic over it working (unless there's a good reason the duplicates can't be avoided).

Can we avoid using labels at all here? E.g. from this snippet:

   .with_run_criteria(RunCriteria::pipe("every other time", eot_piped.system())),

Why not just use

   .with_run_criteria(eot.system().pipe(eot_piped.system()))),

and duplicate the run criteria?

[TheRawMeatball]

The silentness allows for the auto-labelling of state run criteria, which fixes ordering bugs and deduplicates unnecessary run criteria.

For "why labels", these run criteria aren't guaranteed to be side effect free and the state driver is an example of a run criteria with side effects.

[alice-i-cecile]

The silentness allows for the auto-labelling of state run criteria, which fixes ordering bugs and deduplicates unnecessary run criteria.

For "why labels", these run criteria aren't guaranteed to be side effect free and the state driver is an example of a run criteria with side effects.

These are good answers; hopefully we can revisit this with a more elegant fix later.

[Ratysz]

[...] hard to wrap your head around [...]

I don't see how: "criteria labels must be unique" is as simple a rule as it gets. It should be expanded with "if you define a criteria with the same label as some other criteria it will be discarded" to fully document the behavior, though.

Looking at it now, I think this could be split to be explicit: panic on duplicate labels by default, discard criteria with duplicate labels as an opt-in.

Can we avoid using labels at all here?

I've reiterated this several times already and I was certain we were all on the same page with regards to what the piping operation does, exactly. I don't know how else I could explain why it works via a label, but I'll try again anyway:

A run criteria is defined somewhere. We want to use its result somewhere else. We attach a label to that first definition, then refer to the criteria by that label in the somewhere else. If we didn't use a label but the criteria itself again, we would have two copies of the same function running every tick. With piping, that "somewhere else" is another criteria. If we want that behavior but are okay with having duplicate code, we use a chain instead.

[Ratysz]

82db4f6 makes the deduplication behavior opt-in, default is to panic. Has appropriate errors and docs.

[cart]

I expect this behavior to be a bit surprising for users. People using a SystemSet with RunCriteria probably did that with the intent to apply the criteria to all systems (ex: run all of these systems with a fixed timestep).

As a user, I would expect adding criteria to a system inside one of these sets to apply that criteria "in addition to" the system set criteria (ex: every_other_time criteria on a system should run every other time a fixed timestep occurs).

As a Bevy internals developer, I understand the run criteria implementation and why it can't do that, but that doesn't change the fact that users will be surprised when the "every_other_run" criteria overrides the fixed timestep criteria.

After this pr I would like us to explore the idea of multiple run criteria (ShouldRun::Yes + ShouldRun::YesAndLoop == ShouldRunYesAndLoop, ShouldRun::Yes + ShouldRun::No == ShouldRun::No, etc), which would hopefully solve this problem.

In the interim, I almost want to make this panic because it goes against user expectations, which we ourselves have already set due to how things like "SystemSet labels" now work (system set labels and system labels are combined).

Users hitting the panic could just move the affected system to a separate system set (or add it without a set)

[alice-i-cecile]

As a user, I would expect adding criteria to a system inside one of these sets to apply that criteria "in addition to" the system set criteria (ex: every_other_time criteria on a system should run every other time a fixed timestep occurs).

I definitely agree here.

After this pr I would like us to explore the idea of multiple run criteria (ShouldRun::Yes + ShouldRun::YesAndLoop == ShouldRunYesAndLoop, ShouldRun::Yes + ShouldRun::No == ShouldRun::No, etc), which would hopefully solve this problem.

@TheRawMeatball, @Ratysz and I talked about this at length during #1144 on Discord. Our conclusion was that no obvious and clearly correct logic table exists for this quaternary logic :(

[Ratysz]

The rule encoded here is "systems' own descriptors are applied on top of the descriptor of their set" - which means overwriting any non-additive field; run criteria is just the only one such field we have right now. SystemSets are not intended to be any sort of a hierarchy block or a container: they are now used only for mass description. Could probably do with a rename to avoid this confusion.

[cart]

I totally understand the rule (i fully groked the "system sets are weapons of mass system description). Thats not why I left the comment. The behavior in the pr is "consistent", I just think it produces surprising behaviors. I think the FixedUpdate and State lifecycle criterias will be the two most common cases for SystemSets. People using sets under those circumstances are using them to group systems that should run under a certain condition. With that mindset, adding another criteria inside the set will likely be motivated by needing "additional" criteria (otherwise the user wouldn't be putting the system in a "fixed update" system set). Replacing the criteria defeats the purpose the set was created for.

I would rather fail in those cases to tell users they did something wrong than produce surprising results. I would also prefer to not fail (but I understand that this particular case is challenging so I'm cool with tabling the multiple run criteria conversation).

Renaming won't do anything to solve the issue above, or the fact that labels do work as "expected" by "adding" but run criteria "replace".

[Ratysz]

My previous comment had one more paragraph that got cut off 🤦 Network's been crapping out all morning. The gist was that I share your concerns but I don't have a good way of addressing them yet, hence this behavior - there can't be just no behavior. Attempt to reproduce it:

I had thought about forcing the criteria defined on systems that are inside sets to be "pipable", but that seemed more arbitrary than overwriting. It can't be strait-up forbidden because having to move a system outside of a set just to be able to set its run criteria is unergonomic - you'd end up with another copy of the descriptor, all because of the very thing that was supposed to prevent having to copy the descriptor. It also can't be "do it, but warn in console": every time we do that we have to think about how to silence the warning for legit cases.

In the short-term, until we figure it out (probably via a dedicated system set API improvement PR - system sets v3?..), I'm more okay with this overwriting behavior than I would be with forbidding this outright.

[cart]

I had thought about forcing the criteria defined on systems that are inside sets to be "pipable", but that seemed more arbitrary than overwriting.

Yeah I agree. Seems too complicated / arbitrary.

It also can't be "do it, but warn in console": every time we do that we have to think about how to silence the warning for legit cases.

Agreed

It can't be strait-up forbidden because having to move a system outside of a set just to be able to set its run criteria is unergonomic - you'd end up with another copy of the descriptor, all because of the very thing that was supposed to prevent having to copy the descriptor.

I don't think this transform is particularly unergonomic. And it significantly increases clarity:

// before
app
    .add_system_set(SystemSet::parallel()
        .with_run_criteria(FixedTimestep::step(0.4))
        .with_system(a.system())
        .with_system(b.system())
        .with_system(c.system().with_run_criteria(State::on_enter(Foo::Bar))
    )

// after
app
    .add_system_set(SystemSet::parallel()
        .with_run_criteria(FixedTimestep::step(0.4))
        .with_system(a.system())
        .with_system(b.system())
    )
    .add_system(c.system().with_run_criteria(State::on_enter(Foo::Bar)))

[Ratysz]

For a case where you only have run criteria, sure. But consider this:

// before
app
    .add_system_set(SystemSet::parallel()
        .with_run_criteria(FixedTimestep::step(0.4))
        .label(MyLabel::Logic)
        .after(MyLabel::Physics)
        .before(MyLabel::Rendering)
        .with_system(a.system())
        .with_system(b.system())
        .with_system(c.system().with_run_criteria(State::on_enter(Foo::Bar))
    )

// after
app
    .add_system_set(SystemSet::parallel()
        .with_run_criteria(FixedTimestep::step(0.4))
        .label(MyLabel::Logic)
        .after(MyLabel::Physics)
        .before(MyLabel::Rendering)
        .with_system(a.system())
        .with_system(b.system())
    )
    .add_system(c.system()
        .with_run_criteria(State::on_enter(Foo::Bar))
        .label(MyLabel::Logic)
        .after(MyLabel::Physics)
        .before(MyLabel::Rendering)
    )

[cart]

Yeah thats a fair point. I'd argue that c.system() is in a "similar" set, but not an identical set, due to the differing run criteria. Regardless it sounds like we've agreed on the implementation (at least as a short term solution while we work the details of future run criteria improvements).

[cart]

Just pushed a "proposal" that removes RunCriteriaIndex labels, which were both unnecessary/confusing (#1675 (comment)) and broken (#1675 (comment)).

The new impl relies on "fixing up" stored run criteria indices like we do for systems. I'm not a huge fan of this machinery, but I think its preferable to adding a bunch of extra (broken/meaningless) labels. We could remove the index-fixup machinery by making runcriteria indices stable upon insertion (ex: the run_criteria list only allows push ops), but that also requires keeping more things in memory and more indirection when iterating/evaluating run criteria (ex: use the "run criteria index" to look up the "packed run criteria index").

[cart]

Forgot to mention that I also included my preferred solution to #1675 (comment). I'm not putting my foot down there / its easily revertible if we opt for a different approach.

[Ratysz]

Good catch on the bug, and thanks for fixing it!

I like the other changes too - I've warmed up to the idea of forbidding for now individual criteria on systems that are inserted as part of a set: a clear behavior with a meh API is better than surprising behavior with good API.

[cart]

Awesome I think this is good to go!

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• markdownlint