Combining multiple run criteria

[alice-i-cecile]

What problem does this solve or what need does it fill?

As programs grow in complexity, it becomes increasingly common to need to combine more than one run criteria.

This is particularly true with FixedTimeStep.

Currently, RunCriteria cannot be chained ergonomically. Instead, you have to wrap the internal logic in a custom run criteria. This is trickier than expected, and limits code reuse.

What solution would you like?

Run criteria should be able to be chained together in the obvious way: ideally by calling .with_run_criteria more than once in a builder pattern.

While the correct behavior is clearly a logical AND for simple boolean outputs, the presence of YesAndLoop (and soon NoAndLoop) complicates the desired logic here.

This behavior is currently used by our State logic, but not really anywhere else; changing how they work may resolve this difficulty for us.

What alternative(s) have you considered?

Currently, RunCriteria cannot be chained ergonomically. Instead, you have to wrap the internal logic in a custom run criteria. This is trickier than expected, and limits code reuse.

Additional context

This issue was prompted by discussion on Discord about the problem, ultimately caused by a user wanting to add an extra run criteria to a system that was running on a FixedTimeStep.

The inability to nicely chain run criteria prevents us from using them for more complex core functionality down the line.

[mystal]

I'm the mentioned user from Discord 👋

For my use case, I'm working on a game with basically all game systems on a fixed timestep. And I want to be able to pause gameplay and single step frames with a key. So I want to add a second run criteria that checks an enum resource that can be Running, Paused, or Step. The enum value is updated by a system running in the normal update stage that checks for key presses.

So the end result is that if FixedTimestep says ShouldRun::Yes and my new criteria says Yes, then we run all the systems.

[cart]

I acknowledge that this isn't exactly the same as the suggested "multiple run criterias", which I wouldn't personally call "chained criterias". But I think @mystal's use case is currently covered by this (which I would call chained criterias):

[alice-i-cecile]

That solution works for the particular use case :) It's not terribly composable though: you need to write a custom version of your run criteria as a chain system, rather than being able to use seperate run criteria directly. It's certainly a good workaround.

There are three proposed syntaxes for a general version of this so far:

1. .with_run_criteria(A.system()).with_run_criteria(B.system()): this would only permit one logical structure for combining them
2. .with_run_criteria(A.system() | B.system()): simple and explicit, but requires operator overloading in unintuitive ways for *AndLoop. Implementing this may also be annoying
3. .with_run_criteria(A.system().or(B.system())): Explicit and flexible, and allows the extension to more complex or noncommutative operators.
4. .with_run_criteria(or(A.system(), B.system()): As with 3, but a bit nicer for nested logic. Requires importing RunCriteriaCombinator::or into your namespace to work.

[mystal]

@cart Your solution looks good, though I think I need ShouldRun::NoAndLoop to make sure the FixedTimtestep's accumulator is drained as much as possible this tick. Basically replacing the second match arm with:

MyEnum::Y => if should_run == ShouldRun::YesAndLoop { ShouldRun::NoAndLoop } else { ShouldRun::No },

Edited because I realized we need to return ShouldRun::No at some point.

[cart]

@alice-i-cecile @Ratysz

Yeah the big issue with those solutions (as touched on in discord) will be determining the right way to resolve YesAndLoop and NoAndLoop in the context of an "or" operation.

The "chaining" approach requires that you resolve that ambiguity yourself instead of creating arbitrary rules in the "or" implementation. And has the extremely valuable property of not requiring new features.

I don't want our reflex to be "add a new abstraction" every time a new problem presents itself. I would much prefer to keep it simple first, then build higher level abstractions when we've proven they're needed in common cases. Building a "maximally capable and flexible" api isn't our only criteria, and building toward that often comes at the cost of other things (api complexity, legibility, etc).

We have a ton of new ECS features in the pipeline and each new thing we add increases the complexity of what a "bevy app" can be. We've come a long way from app.add_system(foo.system()). Which is a good thing in many ways, but at the same time I can't help but look at this and feel like we've lost something in the process:

enum Label {
    Foo,
    Bar,
}

app.add_stage(
    "MyStage"
    SystemStage::parallel()
        .with_system_set(SystemSet::default()
            .run_criteria(some_criteria.system())
            .with_system(foo.system().label(Label::Foo).before(other_crate::OtherLabels::X))
            .with_system(
                bar.system()
                    .run_criteria(FixedTimeStep::step(0.2).or(CustomCriteria::default()))
                    .label(Label::Bar)
                    .depends_on(Label::Foo)
                )
            )
    )
)

To be clear, I'm not pointing this out to say any of the code above is "wrong". Imo the complexity cost has been worth it so far. But at the same time, parsing out what that code does isn't trivial and requires a lot of context. I'm just using a contrived example to illustrate that abstractions have a price.

[alice-i-cecile]

(ping @TheRawMeatball as well)

I agree with your assessment <3

I would like to see some additional user stories before we decide what to do with this (including "just chain them"). That said, this solution to decoupling States and Stages requires a more powerful abstraction, and pays for it by making thinking about State a lot clearer IMO.

I think that there's a good argument for waiting on this, and the State question, until we have a good model for how to handle "deep UI" and widget communication. A lot of these problems seem deeply interrelated, and I'm optimistic that we can find something elegant.

[Ratysz]

I don't want our reflex to be "add a new abstraction" every time a new problem presents itself.

I agree. I can't speak for others, but whenever I talk about new things like these, it comes with an unspoken disclaimer of "we don't really need a new solution here, but let's brainstorm this just in case we stumble onto something great".

[alice-i-cecile]

The main functionality that I'd want for this is just the ability to write custom functions that take more than one ShouldRun, and combine them into another ShouldRun. This is straightforward to write by hand, but actually getting the internal systems to evaluate is a pain.

Suppose I were to write

.run_criteria(my_combinator(
criteria_system_a.system(), 
criteria_system_b.system()
))

Basing this off the current API, my_combinator would need to be a system with an output type of ShouldRun.

I'm pretty sure that we could write a my_combinator that works for any two internal criteria systems, and we could then nest this effectively. Then, look at a few examples, add a trait and write a derive macro to create these run criteria combinators nicely. We wouldn't need to change the existing API for this at all: this trait and derive macro can safely live in an external crate, probably with a few of the obvious combinators as well.

This seems like a nice intermediate step between system chaining (flexible, uncomplicated, tedious) and a more overwrought abstraction that would still allow us to combine existing run criteria without modification.

[TheRawMeatball]

Hmm, I'd say a function with signature

fn(impl System<Out = ShouldRun>, impl System<Out = ShouldRun>, impl Fn(ShouldRun, ShouldRun) -> ShouldRun) -> ShouldRun 

would be a cleaner way of doing things

[alice-i-cecile]

@TheRawMeatball ah, I see :) Then you can decouple the logic for combining ShouldRun from the data piping, and use only a single function, rather than a whole derive macro. I agree, thanks!

With only a single function, I don't think I'd feel bad having this live in Bevy itself.

Of course, that function would itself have to meet the signature of with_run_criteria or its modernized variant, but that's not too bad, and allows us to nest this for combining any number of run criteria if you really need to.

[cart]

Maybe we could build a generic abstraction that isn't hard-coded to ShouldRun?

Ex:

.run_criteria(
    a.system().zip(b.system()).map(|(a, b)| {
        if a == ShouldRun::Yes {
            ShouldRun::Yes
        } else {
            b
        }
    })
)

Where .zip() returns a new system that takes the output of each system and puts it in a tuple (much like rust's iterator.zip()). then map returns a new system that uses the given function to map the current System::Output to another value.

[cart]

We could also replace map with a "chained" system

[alice-i-cecile]

Maybe we could build a generic abstraction that isn't hard-coded to ShouldRun?

I can definitely see the value of this, although I'm not sure where else we'd want to use it. I also think this pattern is also much less clear compared to the combinator function pattern laid out in @TheRawMeatball's comment.

The clarity issues are particularly bad for programmers with less experience or formal CS background, which I would expect to see writing games in Bevy. Functional programming patterns like that are always really rough for less experienced audiences, and I don't think that combining run criteria is inherently a particularly abstract pattern.

[cart]

But they are still very common in the Rust world. I think I'd rather have people learn an "arbitrary Rust/CS" concept that applies everywhere than an "arbitrary Bevy run criteria" concept that applies only to that situation.

And Bevy is increasingly embracing the "functions as composable / functional behaviors" concept. If that trend continues, I expect generic methods like this to increase in value over time.

[alice-i-cecile]

There's some further discussion on this from an end user perspective at bevy-cheatbook/bevy-cheatbook#42. In particular, the hand-rolled combinators by @TehPers are compelling and worth consideration by anyone attempting a native solution to this issue.

[TehPers]

the hand-rolled combinators by @TehPers are compelling and worth consideration by anyone attempting a native solution to this issue

Even with the combinators, it's still tricky to combine run criteria. If all your run criteria are normal systems, then it's not too difficult with if_all or if_any (or even .chain), but if any of them are not systems (for example the State::on_* criteria) then you start needing to add run criteria directly to the stage (which is not intuitive and could be a lot easier).

[masonk]

Maybe we could build a generic abstraction that isn't hard-coded to ShouldRun?

Ex:

.run_criteria(
    a.system().zip(b.system()).map(|(a, b)| {
        if a == ShouldRun::Yes {
            ShouldRun::Yes
        } else {
            b
        }
    })
)

Where .zip() returns a new system that takes the output of each system and puts it in a tuple (much like rust's iterator.zip()). then map returns a new system that uses the given function to map the current System::Output to another value.

I proposed the merge function described by @TheRawMeatball and then generalized it to something like the zip() proposed here before reading this thread :)

I had one more idea for a generalization. If systems could run other systems, that's the ultimate generalization of where this is going. I think I understand whey they currently aren't allowed to, and it's because Bevy is trying hard to track the dependencies of systems at a granular level, which would go out the window with a naive implementation of systems-calling-systems.

I wonder if there's any other reason why systems don't have a way to call other systems today? One idea I had was to allow systems to inject other systems (or "SystemRunners", etc) the same way they inject all other Bevy-managed resources. Then, Bevy can continue to track dependencies granularly but give arbitrary control of how systems compose to the end-user. That would be a much deeper and more fundamental change to Bevy than what is required to solve the motivating use-case, but it feels like where Bevy is going. I've built several large frameworks in my life and they always converge towards composable extension points, so it's easy for me to spot the pattern.

[Ratysz]

Systems can run other systems: a system is just a function, after all, so it can be called like one. Same applies to run criteria - that's what I hinted at earlier in #2233. However, one will have to plumb arguments of the inner systems to signature of the outer system manually; we should be able to make that more automagic, eventually, but that needs R&D and might not be worth it until several of the more juicy upcoming features of Rust are finally stabilized (specialization, variadic tuples, fix to that one ICE, and GATs).

[masonk]

Yes, the manual plumbing is the problem. It's ugly but maybe acceptable in small examples where your composition is one level deep and the systems you're calling only have a few deps. But it gets uglier and uglier as the composition depth increases, since you have to plumb through all transitive calls.

Although - even in small cases like the TimeStep + State example I gave in the issue I filed, plumbing the deps of TimeStep into my custom run criteria system is ugly. I don't want to know the internal details of what TimeStep does. I don't want to change my app if TimeStep changes it deps. And, do I even have visibility of the deps that TimeStep injects? I would need them.

I'm not as familiar with the internals of Bevy, but I know Bevy already tracks system dependencies at a granular level today. I don't see why Bevy would need any unstable Rust features to share the functionality it already has with end-user code.

[Ratysz]

I'm not as familiar with the internals of Bevy, but I know Bevy already tracks system dependencies at a granular level today. I don't see why Bevy would need any unstable Rust features to share the functionality it already has with end-user code.

Because user-facing API is statically typed.

I'm not saying this needs unstable features, I'm insinuating that without said features it would be an incredibly brittle pile of highly-specialized macros, potentially even larger than the one we already have for implementing systems in the first place.

This can be dodged by using dynamic systems for this kind of compositing, but it doesn't feel right to me, and as far as I know we don't even have them yet anyway.

[masonk]

What about the zip() function that @cart proposed? That smaller API would yield a large fraction of the total value in this space.

[TheRawMeatball]

While .zip() and .chain() are nice, they don't share the right semantics with .pipe() - namely, they create more individual instances of the systems, which can be an issue because of how systems can hold state, and in the case of run criteria, accidentally cause unintended mutation of state. A nicer alternative would be a pipe_2 function which lets you pipe the output of other systems from the schedule, instead of creating more and more instances of the same system.

[Ratysz]

@TheRawMeatball pretty much gave my answer. I would only add that extending a form of piping to systems (as opposed to just the run criteria) is syntactic sugar that's more trouble than it's worth: it will, essentially, be a channel, just with weird API and complicated implementation.

[masonk]

they create more individual instances of the systems

I think I don't understand this part.

The way I'm imagining things, if I were to install a hypothetical zip(time_step, state_system), then I wouldn't also manually install those systems. zip would return something that installs those systems (once).

[Ratysz]

I'm assuming by "install" you mean "add it to the schedule"; if so - yes, that is what happens. But what if you wanted to use a run criteria more than once in a given stage? With zip or chain or whatever else of that kind you would have to have a copy per use. pipe lets you grab the result of a criteria without having to create another instance of it.

[masonk]

So what would pipe2 look like? I'm having a hard time imagining that. All of my ideas in that direction just head straight back to a generalized "systems can inject the outputs of other systems" API.

[Ratysz]

So what would pipe2 look like?

Instead of grabbing the result of just one other run criteria, it will grab the result of two run criteria. The user then can implement their own and/or/whatever.

The "piping from arbitrary amount of criteria" I kept bringing up in #2233 extends that to an arbitrary amount of criteria rather than just one or just two.

[masonk]

Instead of grabbing the result of just one other run criteria, it will grab the result of two run criteria.

How would that look like in code, though?

[Ratysz]

How would that look like in code, though?

However we want it to look like. We haven't started API design yet.

Extrapolating current pattern, it might be something like RunCriteria::pipe_2(MyCriteriaLabelOne, MyCriteriaLabelTwo, my_criteria_combinator.system()) and (MyCriteriaLabelOne, MyCriteriaLabelTwo).pipe_2(my_criteria_combinator.system()).

[masonk]

I see, thanks.

[alice-i-cecile]

Fixed by #7267.