Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run criteria should be composable; states should act more like other run criteria #2233

Closed
masonk opened this issue May 22, 2021 · 15 comments
Closed
Labels
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible P-High This is particularly urgent, and deserves immediate attention

Comments

@masonk
Copy link
Contributor

masonk commented May 22, 2021

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

The 0.5 StatesV2 system is a huge leap forward, but it still has some weaknesses.

One of the biggest weaknesses is that it doesn't compose well with other run criteria.

To give an example, suppose a user wants to handle input on a FixedTimeStep, but differently on a per-state basis.

Today, it's not clear how to do that. It probably involves building a custom run criteria that manually composes the logic of FixedTimeStep and the logic of Statesv2.

What solution would you like?

I would like RunCriteria to become logically composable, so that:

  • Users can group them
  • Users can AND, OR, and NOT them

Additionally, to make the motivating use-case work, it would be necessary to expose state-based run criteria as regular run criteria so they can be composed with other run criteria.

@masonk masonk added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels May 22, 2021
@masonk
Copy link
Contributor Author

masonk commented May 22, 2021

If I have Criteria1.And(Criteria2) there's a table of possible states:

In the table I will omit the states where both returned the same thing, as then it's trivial that output should return that.

No beats everything: if something says No, there is no reason to ever check again.
If one system says Yes, but another says NoAndCheckAgain, we have to try again later to see if that system should run.
If one system says Yes and others say YesAndCheckAgain, it's kind of hard to say what to do. Arguments can be made in either direction. "Yes" because it might be surprising to run a "Yes" condition more than once. "YesAndCheckAgain" because it might be surprising to not check again.

System1 ShouldRun System2 ShouldRun Output ShouldRun
Yes No No
No NoAndCheckAgain No
Yes YesAndCheckAgain Not sure
Yes NoAndCheckAgain NoAndCheckAgain
YesAndCheckAgain NoAndCheckAgain NoAndCheckAgain

@Ratysz
Copy link
Contributor

Ratysz commented May 22, 2021

Combining criteria raised quite a few arguments a while ago... We settled on a minimal version for now, criteria piping.

Criteria created by states are fully-interoperable with that, too: use the State::on_update() and similar methods that produce a criteria descriptor. It will also automatically make sure that you don't end up with multiple copies of the same state-related criteria hanging around in the stage by virtue of this descriptor feature.

We might eventually have built-in combinators, and/or piping in from arbitrary amounts of criteria without needing to express it all as a long "chain" of pipes. I don't think these plans are explicitly documented anywhere, though. cc @TheRawMeatball?

(Judging by the amount of edits your table has accrued while I was typing this up, I think you're now well aware why this isn't as straightforward as it is at first glance.)

@masonk
Copy link
Contributor Author

masonk commented May 22, 2021

It's not straightforward, but I think I found the rules that work.

So, how would you run a FixedTimeStep with State::on_update(GameState::InGame), today in bevy 0.5?

@Ratysz
Copy link
Contributor

Ratysz commented May 22, 2021

I think I found the rules that work.

What about when the first system returns any of the *AndCheckAgain variants?

So, how would you run a FixedTimeStep with State::on_update(GameState::InGame), today in bevy 0.5?

You would have to, unfortunately, partially replicate the function of one of them to create a pipeable version. This is why we want to pipe from more than one criteria.

@masonk
Copy link
Contributor Author

masonk commented May 22, 2021

The problem with the piping is that it doesn't actually compose, right?

The system you pass to pipe() doesn't have the same function signature as system you'd install as a standalone run criteria.

@masonk
Copy link
Contributor Author

masonk commented May 22, 2021

I think I found the rules that work.

What about when the first system returns any of the *AndCheckAgain variants?

Flip the table. The logic commutes.

@Ratysz
Copy link
Contributor

Ratysz commented May 22, 2021

Flip the table. The logic commutes.

Oh, right, we are talking about just and. Seems to check out so far.

The problem with the piping is that it doesn't actually compose, right?

Not in a pleasant way, no.

I see two options here:

  • Generalize, then specialize: implement piping from arbitrary amount of criteria, then build some combinators on top of that.
  • Specialize, then generalize: implement some straightforward combinators, then consider generalizing them if/when we get arbitrary piping.

@Ratysz Ratysz added A-Core Common functionality for all bevy apps and removed S-Needs-Triage This issue needs to be labelled labels May 22, 2021
@masonk
Copy link
Contributor Author

masonk commented May 22, 2021

I think Yes & YesAndCheckAgain -> Yes is the most tendentious rule in the table. Everything else seems very clear to me but that might be a bad one that kills the whole idea.

@Ratysz
Copy link
Contributor

Ratysz commented May 22, 2021

It does make sense if we look at this combinator as strictly logical and - although, that doesn't check out for Yes && NoAndCheckAgain -> NoAndCheckAgain. Things like this is why we decided to omit built-in combinators back then.

@masonk
Copy link
Contributor Author

masonk commented May 22, 2021

I agree that automatic combinators is something that's quite hard to convince ourselves we can always get right and I support waiting on that (for now, but with experience we might get more confidence about what the correct truth table is (or learn that there isn't one).

But pipe() is just very limited in usefulness. It doesn't make even simple things easy.

If there were a way to just toss the logic over the wall to the dev and say, "here's two ShouldRuns; we don't know how to combine them, you combine them", that would be composable.

// These two systems are supplied by other crates, e.g. by Bevy core
fn a(..) -> ShouldRun {}
fn b(..) -> ShouldRun {}

// There is no way to pipe() a and b, they don't have the signature for that.
// Instead let's allow the user to define merge behavior
type CriteriaSystem = BoxedSystem<(), ShouldRun>;

fn merge(a: CriteriaSystem , b: CriteriaSystem , mergeFn: fn(ShouldRun, ShouldRun)  -> ShouldRun) -> CriteriaSystem {}

@Ratysz
Copy link
Contributor

Ratysz commented May 22, 2021

That's exactly what I'm referring to with "piping from arbitrary amount of criteria". The idea was to generalize the implementation to fully cover criteria with no piped input, criteria with one piped input, and criteria with N piped inputs.

@masonk
Copy link
Contributor Author

masonk commented May 22, 2021

I really like this approach. Rather than choosing one truth table we can offer combinators that implement different truth tables. And the user always has the choice of fully custom logic.

I think we only need to handle the case of merging two systems. You can handle everything almost everything that way. We could define a macro that implements merge2, merge3, ... mergeN functions if it's important to be able to do multiple-way merges.

@alice-i-cecile
Copy link
Member

See also #1295. This is one of the larger pain points outside of areas that are simply missing functionality like UI, and seems to come up again and again. I'm going to add the high-impact label to this, in the hopes that we can work out a better solution in time for 0.6.

@alice-i-cecile alice-i-cecile added the P-High This is particularly urgent, and deserves immediate attention label May 23, 2021
@masonk
Copy link
Contributor Author

masonk commented May 23, 2021

It occured to me that there's nothing really specific about run criteria in the merge function I proposed. It's just a way of combining two systems that don't fit into a pipeline. It could be generalized to this:

fn compose_systems(a: BoxedSystem<(), A>, b: BoxedSystem<(), B>, by: fn(A, B) -> C) -> BoxedSystem<(), C>;

(Or a macro that lets us do this for n systems).

When trying to articulate why this has to be a change to bevy framework rather than just clever user code , it's because user code can't run systems: bevy always calls them.

Since a lot of functionality is packaged as systems and not underlying composable functions, all the app developer can do is copypasta the logic inside the systems. That's the fundamental limitation that keeps this from being an external library crate.

So, another way to remove the blocker is to allow systems to call other systems inline, e.g., by injecting &mut World. (I recognize that this messes up the nice parallelism from strongly-typed systems by hiding system dependencies). If that were available, app devs could roll their own combinators just by calling two systems and doing logic on their output(s). That would probably be the most general way to solve the problem. However that might be too far afield.

@alice-i-cecile
Copy link
Member

Deduplicating in favor of #1295.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible P-High This is particularly urgent, and deserves immediate attention
Projects
None yet
Development

No branches or pull requests

3 participants