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 multipiping #2446

Closed
wants to merge 7 commits into from
Closed

Conversation

Ratysz
Copy link
Contributor

@Ratysz Ratysz commented Jul 10, 2021

Objective

  • Implement piping of results from an arbitrary amount (well, up to 16) of run criteria to enable constructing proper criteria combinators.
  • Sneak in "Optional .system(), part 5 (criteria piping)", shhh.

Solution

  • Rework RunCriteriaContainer to store a RunCriteriaTrait trait object instead of the single/piped enum.
  • Implement pseudovariadics codegen for the aforementioned trait object and piping.
  • Expand parallel_run_criteria test to cover the new cases; expand documentation of RunCriteria::pipe().

Relevant issues


Quick example:

stage.with_system_run_criteria(
    ("every second time", "every third time")
        .pipe(|input: In<(ShouldRun, ShouldRun)>| match input {
            In((ShouldRun::Yes, ShouldRun::Yes)) => ShouldRun::Yes,
            _ => ShouldRun::No,
        })
        .label("every sixth time"),
)

I'll see if I can do something about that double indirection (the RunCriteriaInner thing in the implementation). (Forgot how Rust works for a moment there.)

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jul 10, 2021
@mockersf
Copy link
Member

wouldn't that be a join rather than a pipe ?

@Ratysz
Copy link
Contributor Author

Ratysz commented Jul 10, 2021

Hm, I can see it like that. Although, as per previous discussion, I think it's pretty important to highlight the, well, piping of outputs into an input, rather than the order of operations alone.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Enhancement A new feature and removed S-Needs-Triage This issue needs to be labelled labels Jul 13, 2021
@Ratysz
Copy link
Contributor Author

Ratysz commented Jul 15, 2021

bors try

@bors
Copy link
Contributor

bors bot commented Jul 15, 2021

try

Merge conflict.

.iter()
.take(self.inner.parents_len())
.map(|label| {
*labels.get(label).unwrap_or_else(|| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can't we just use expect here?

Nope, apparently this way dodges string formatting costs: https://stackoverflow.com/a/63291643/3234149

@alice-i-cecile
Copy link
Member

Great code quality, as always. The multi-piping API is nice (and a very natural extension), the system yeet is 💯.

@alice-i-cecile
Copy link
Member

Adding to 0.6 milestone for the optional .system aspects. If the rest of the PR is controversial, we can split this out.

bors bot pushed a commit that referenced this pull request Jul 17, 2021
# Objective

- Continue work of #2398 and friends.
- Make `.system()` optional in chaining.

## Solution

- Slight change to `IntoChainSystem` signature and implementation.
- Remove some usages of `.system()` in the chaining example, to verify the implementation.

---

I swear, I'm not splitting these up on purpose, I just legit forgot about most of the things where `System` appears in public API, and my trait usage explorer mingles that with the gajillion internal uses.

In case you're wondering what happened to part 5, #2446 ate it.
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
# Objective

- Continue work of bevyengine#2398 and friends.
- Make `.system()` optional in chaining.

## Solution

- Slight change to `IntoChainSystem` signature and implementation.
- Remove some usages of `.system()` in the chaining example, to verify the implementation.

---

I swear, I'm not splitting these up on purpose, I just legit forgot about most of the things where `System` appears in public API, and my trait usage explorer mingles that with the gajillion internal uses.

In case you're wondering what happened to part 5, bevyengine#2446 ate it.
bors bot pushed a commit that referenced this pull request Jul 27, 2021
# Objective

- Remove all the `.system()` possible.
- Check for remaining missing cases.

## Solution

- Remove all `.system()`, fix compile errors
- 32 calls to `.system()` remains, mostly internals, the few others should be removed after #2446
Copy link
Contributor

@wilk10 wilk10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent and enables some nice use cases, thanks a lot!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review labels Oct 21, 2021
@cart cart removed this from the Bevy 0.6 milestone Dec 18, 2021
@cart
Copy link
Member

cart commented Dec 18, 2021

Removed from the 0.6 milestone in the interest of time. Pipes are pretty niche, so requiring .system() in those cases doesn't bother me.

@cart
Copy link
Member

cart commented Feb 2, 2022

Pulling in bevyengine/rfcs#45 participants: @alice-i-cecile @DJMcNab @inodentry @maniwani @hymm
I haven't done a full review of the stageless RFC yet: how does "multipiping" play into those plans?

@alice-i-cecile
Copy link
Member

I haven't done a full review of the stageless RFC yet: how does "multipiping" play into those plans?

The current plans there are that run criteria automatically compose with AND.

Run criteria can no longer loop, so this simple rule is sufficient (at least for a first attempt).

@cart
Copy link
Member

cart commented Feb 2, 2022

Testing @bevyengine/ecs-team pings

@maniwani
Copy link
Contributor

maniwani commented Feb 2, 2022

I haven't done a full review of the stageless RFC yet: how does "multipiping" play into those plans?

So in current Bevy, RC can be referenced by label (like any other system) and they're collectively evaluated at the beginning of each stage.

Both of those properties go away in the RFC. Instead, each system gets its own instances of any RC it's been assigned. And the new definition is that all of a system's RC must return true "just before" it runs. (RFC makes RC return bool and limits them to read-only access to make them easier to combine.)

Current plan is to have the executor evaluate all of a system's RC inside the main executor task, after that system becomes ready (none of its dependencies remain). If both system + all its RC can run, the RC is ran first. If all return true (this is the AND), only then do we spawn and signal the system task, otherwise we just mark the system as completed.

For toggling a group of systems, users can assign RC to labels (which conceptually map to sub-graphs in the overall system graph). When the executor encounters a ready system with "not-yet-seen" labels, it checks if system + all its RC + all not-yet-seen label RC can run. If so, the label RC is ran first. All systems with the labels whose RC returned false would be immediately marked as completed.

Edit: tl;dr direct RC piping goes away. That said, users can have a system writing some resource that later RC instances just read. We're hoping that the read-only access and lack of task overhead counterbalances the inability to share RC (except indirectly through labels).

@alice-i-cecile
Copy link
Member

I'm going to close this out: I think it's increasingly unlikely that we go with this particular design and #4391 solves the underlying problem in a much more elegant way.

And with iyes_loopless out, there's much less need for an urgent solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants