Skip to content

RFC: Barriers with Before/After markers #10618

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

Closed
wants to merge 1 commit into from

Conversation

stepancheg
Copy link
Contributor

@stepancheg stepancheg commented Nov 17, 2023

Why

Explicit ordering of systems is hard: during refactoring you forgot one edge, and your program befomes non-deterministic.

How

This draft PR proposes implementing ordering based on function signature params (SystemParam).

Basically, the idea is this. We introduce two new system params:

Before<T>
After<T>

where T must be a SystemSet + Default.

And it can be used like this:

#[derive(SystemSet)]
struct MyBarrier;

fn first(_before: Before<MyBarrier>) {}
fn second(_after: After<MyBarrier>) {}

Now if first and second are added to the same schedule, they will be ordered like if first.before(after) was done explicitly.

Working example for this is in this commit as test_before_after_from_param.

Composable

SystemParam are composable, so it is possible to build higher level constructs on top of it. For example, we may want to schedule all event writers before all event writers:

#[derive(SystemParam)]
struct MyOrderedEventWriter<T> {
    writer: EventWriter<T>,
    after: After<MyOrderedEventBarrier<T>>,
}

#[derive(SystemParam)]
struct MyOrderedEventReader<T> {
    reader: EventReader<T>,
    after: Before<MyOrderedEventBarrier<T>>,
}

Feedback?

Want to get early feedback before spending several more hours on it.

WDYT?

@stepancheg stepancheg changed the title RFC: Before/After markers RFC: Barriers with Before/After markers Nov 17, 2023
@ItsDoot ItsDoot added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Nov 17, 2023
@alice-i-cecile alice-i-cecile marked this pull request as draft November 17, 2023 23:20
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Nov 17, 2023
@alice-i-cecile
Copy link
Member

I'm not a fan of adding this to the system signature. Systems should describe what they do, their configuration should determine when and how they're run. When we blur the barrier, we increase cognitive and implementation complexity, and reduce reusability.

I'd much prefer #8595 + system ambiguity detection by default as a way to avoid these problems.

@stepancheg
Copy link
Contributor Author

stepancheg commented Nov 17, 2023

Systems should describe what they do, their configuration should determine when and how they're run.

I think this is terminology issue.

We may call this thing "configured system" of something like that instead of just system.

Practically current setup is very inconvenient and my largest usability issue with bevy. I'm not not confident any time that my system setup is configured correctly, and I'm getting less confidence after any change. And to have good night sleep I just group the systems into larger systems. Which is not how bevy is supposed to be used.

BTW I watched your talk recently, you mentioned dependency injection, and this PR is it! It allows not just linking systems to world objects, but also systems to each other. (Also this can be viewed as an alternative to commands.run_system).

I'd much prefer #8595

  • It has the same issue as current setup: system configuration and system itself are too far apart
  • and again there's no typechecker enforcement that systems are used properly: if one accidentally adds .after(ComponentAccessSet::Writes::<AccidentallyWrongComponent>), there's no protection against it
  • also not everything can be modelled as dependency between components (e.g. event reader/writer pairing)
  • and that PR is orthogonal. The same .after<ComponentAccessSet::Writes::<Transform>> could be configured like _after: After<ComponentAccessSet::Writes::<Transform>>

Explicit ordering of systems is hard: during refactoring you forgot one
edge, and your program befomes non-deterministic.

This draft PR proposes implementing ordering based on function signature
params (`SystemParam`).

Basically, the idea is this. We introduce two new system params:

```
Before<T>
After<T>
```

where `T` must be a `SystemSet + Default`.

And it can be used like this:

```
#[derive(SystemSet)]
struct MyBarrier;

fn first(_before: Before<MyBarrier>) {}
fn second(_after: After<MyBarrier>) {}
```

Now if `first` and `second` are added to the same schedule, they
will be ordered like if `first.before(after)` was done explicitly.

`SystemParam` are composable, so it is possible to build higher level
constructs on top of it. For example, we may want to schedule all event
writers before all event writers:

```
#[derive(SystemParam)]
struct MyOrderedEventWriter<T> {
    writer: EventWriter<T>,
    after: After<MyOrderedEventWriterBarrier<T>>,
}

#[derive(SystemParam)]
struct MyOrderedEventWriter<T> {
    reader: EventReader<T>,
    after: Before<MyOrderedEventWriterBarrier<T>>,
}
```

Want to get early feedback before spending several more hours on it.

WDYT?
@B-Reif
Copy link
Contributor

B-Reif commented Nov 18, 2023

Strongly against this proposal.

Re #8595

system configuration and system itself are too far apart

This separation is intentional. Systems are intended to be flexibly re-used in multiple contexts.

there's no typechecker enforcement

This proposed API encodes user ordering logic in the type system, which should not be mistaken for an API where the type system actually helps you order systems correctly.

You can just as easily put _after in the wrong function signature as you can invoke .after incorrectly in the existing API. This proposal doesn’t use the type checker helpfully, so this is not an advantage at all.

By contrast, #8595 uses the type system to make meaningful configuration changes based on the actual data the systems are accessing, and not an empty marker value.

not everything can be modelled as dependency between components (e.g. event reader/writer pairing)

#8595 is based on world access, not component access, and event sending is literally the first example the author mentioned.

I agree that ordering systems in Bevy could be meaningfully improved. The place to improve it is in the graph that does the ordering, not in the function signatures of the systems themselves.

@maniwani
Copy link
Contributor

maniwani commented Nov 18, 2023

This proposal seems like it would need to extend the System trait so that our scheduler could extract this embedded information. It also seems like it'd just shake up our scheduling API, more than I'm really comfortable with.

It seems like your biggest goal is not having to repeat yourself, and for that, I just don't see a practical advantage of doing it like this over writing a plugin with a method to handle any schedule configuration.

pub struct ExamplePlugin;

impl ExamplePlugin {
    // you only have to get this function right once
    pub fn schedule_my_systems(schedule: &mut Schedule) {
        schedule.add_systems(
            (
                foo.in_set(X),
                bar.before(Y),
                baz.after(Z),
            )
            .chain()
            .run_if(something_happens)
        );
        /* the rest of the code */
    }
}

impl Plugin for ExamplePlugin {
    fn build(&self, app: &mut App) {
        app.edit_schedule(SomeSchedule, |schedule| {
            self.schedule_my_systems(schedule);
        });
        /* the rest of the code */
    }
}

Since scheduling happens at runtime and what you propose expresses the same before and after ordering constraints as before, "being confident that you scheduled things correctly" would be just as difficult as before. (There is no place where we could statically verify that a schedule is correct.)

I will also admit that, aesthetically, I'm quite biased against "hard-coding" schedule details in each system's fn signature. If my schedule_my_systems example needed a rewrite, at least all of that configuration exists in one place.

@stepancheg
Copy link
Contributor Author

stepancheg commented Nov 18, 2023

It seems like 4 against 1, and I have zero credibility in this project, so feel free to ignore and close as won't fix or whatever.

Regardless of that, I feel obligated to answer the comments above. I suspect my proposal is misunderstood given commenters:

@B-Reif

This separation is intentional. Systems are intended to be flexibly re-used in multiple contexts.

I'm fine with being able to configure them additionally to annotations on functions itself. This RFC proposes safe shortcut for simplest cases, which are majority.

You can just as easily put _after in the wrong function signature as you can invoke .after incorrectly in the existing API. This proposal doesn’t use the type checker helpfully, so this is not an advantage at all.

Yes, it does.

First, it it much easier to not lose the type annotations rather than .before/.after calls when:

  • refactoring code (e.g. splitting systems into smaller systems)
  • resolving merge conflicts
  • updating to new bevy version

Second, as with any SystemParam these can be composable into higher level primitives like OrderedEventWriter in the example above. Please check it.

By contrast, #8595 uses the type system to make meaningful configuration changes based on the actual data the systems are accessing, and not an empty marker value.

Sorry for repeating it again:

  • no it does not prevent you from using it incorrectly. You can too easily say .after(ComponentAccessSet::Writes::<AccidentallyWrongComponent>) (or for example, capture component which is also updated in another entity not where you expect, but somewhere in the end of the schedule killing performance of the schedule, for example, by updating Transform component not used in rendering)
  • second, Before/After can be composed into higher level utilities like OrderedInitRes<T>/OrderedRes<T>, which checks that OrderedRes<T> is accessed only after OrderedInitRes<T> during startup

event sending is literally the first example the author mentioned.

As I said before, that setup does not prevent using it incorrectly, e.g.

  • accidentally put wrong event type
  • or forget to put .after to one of several event readers

By contrast, OrderedEventReader I describes here, strictly guarantees safe ordering of event read/write systems.

I agree that ordering systems in Bevy could be meaningfully improved.

At least something we agree upon.

The place to improve it is in the graph that does the ordering, not in the function signatures of the systems themselves.

Function signatures describe the graph node inputs and outputs. So using function signature to describe dependencies in one way or another might be reasonable.

(This can be done for queries too by the way. Just add something like OrderedQuery<T> and OrderedQueryMut<T> wrapper for Query<T> which would schedule one after another. But I don't see any way how it can be done magically by the bevy schedule.)

@maniwani

I just don't see a practical advantage of doing it like this over writing a plugin with a method to handle any schedule configuration

For example, I can copy-paste system, or move it to another file, and I'm certain it is going to work properly.

The function is the unit here. Being a unit, code block makes it safer for changes.

Additionally (not sure why people ignore that argument) higher level safe utilities can be built on top of before/after. Like OrderedEventReader/OrderedEventWriter.

If my schedule_my_systems example needed a rewrite, at least all of that configuration exists in one place

Which is not an advantage from my point of view. Because "system configuration" is split in two places. And during refactoring, two places needs to be changed.

I will also admit that, aesthetically, I'm quite biased against "hard-coding" schedule details in each system's fn signature.

Initially I wanted to propose proc macro like

#[after(ControlsProcessed)]
fn my() {}

but this only solves the smaller problem and does not allow building higher level utilities on top of it (like event queues, InitRes etc).

@nicopap
Copy link
Contributor

nicopap commented Nov 18, 2023

This proposal is still more powerful than #8595, in that, for example, you could have several "stages" on a specific data, and order systems based on those stages. But it's true that #8595 occupies the same niche.

I have some concerns:

  • It would create "useless" system params, that only exist to describe properties of the system. Right now all system params (apart the odd NonSendResource<()> in bevy_winit) only exist to provide additional data to a system, not describe how the system should be scheduled.
  • It will create two different APIs for scheduling. I like the "only one way" philosophy. IMO, a per-system API seems more robust than the current system, but replacing entirely the current API seems like a difficult sell right now.

While I'm skeptical about this approach, I have been much more skeptical about other APIs that are now part of the project.

@nicopap
Copy link
Contributor

nicopap commented Nov 18, 2023

I think it should be possible to implement the API as a third party crate. @stepancheg I would be open to collaborate on a proof of concept.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Nov 18, 2023

My fundamental objections are to blending system scheduling information into the system params. Any proposal that does this, regardless of other merits, will cause serious architectural issues with how Bevy handles scheduling. To lay it out explicitly:

  1. It would add another way to handle system scheduling. Even the distinction between schedules and system sets is frustrating, and results in meaningful new user confusion.
  2. Systems that use this API are less reusable and configurable.
  3. Systems that are schedule-configured using this API cannot be easily interacted with using set-based abstractions.
  4. Duplicate system parameters would need to be created, greatly increasing API surface for learning and maintenance.
  5. System definition is now tightly coupled to scheduling abstractions, making it harder for Bevy or third-parties to use an alternative scheduling implementation.
  6. When used as part of alternative system scheduling implementations like one-shot systems, system-param-based scheduling is silently ignored (or maybe panics?), while set-based scheduling simply cannot be expressed.
  7. Rather than being able to inspect the plugin initialization (as a human) or system sets (as a machine) to determine system ordering, we also need to drill down into the system parameters of all of our systems.
  8. Plugin authors can change the system signatures of their publicly exported systems, leading to silent and unfixable changes in the scheduling downstream.

Generating system sets based on system-param-scheduling would mitigate 3, 7 and perhaps 8, but in exchange would lead to an exposed API that could be misused in the same way you're concerned about for the API in #8595.

It seems impossible for a system param based approach to cover all of your ordering needs, and so issue 1 cannot be resolved.

The composability is neat! This is something that can be done using system sets as well, but the type-level composition is genuinely appealing. I'm sorry for not commenting on it earlier: engaging with other elements of the design when severe architectural concerns are present felt disrespectful and misleading.

@alice-i-cecile
Copy link
Member

From what I understand, the driving concerns here are:

  1. System scheduling is hard to understand at a glance.
  2. System scheduling is stored separately from the system definition.
  3. It is possible to generate nonsensical or clearly incorrect system scheduling constraints, but hard to detect them.
  4. Creating abstractions for system scheduling is hard.
  5. System set configuration is not shared across schedules.

Taken together, this makes refactoring and working with complex schedules painful.

Is that correct? From my perspective:

  1. Absolutely true. I think that interactive visualization tools are the best solution here.
  2. This is a deliberate trade-off, as laid out above. I can see the other side though.
  3. Absolutely agree: I would like better tools to warn about sketchy scheduling.
  4. I think we do a poor job explaining how to use system sets in practice at scale, in order to create reusable abstractions.
  5. I classify this as a bug, and would like to see it fixed (likely via systems-as-entities).

@ickk
Copy link
Contributor

ickk commented Nov 18, 2023

I think we all agree there are problems with scheduling in bevy as it currently exists, especially for large projects. (I know of some larger projects that have given up and more or less just use one big linear .chain)

There's been a lot of conjecture in this thread, but before making decisions for or against this rfc, or for or against other competing developments to scheduling in bevy, I think we need answers to the following questions — preferably weighing the perspective of some of the few large projects that currently exist

  • "what do large schedules actually look like in the wild?"
  • "what are the challenges with scheduling in the wild?"
  • "how often does each type of challenge present itself"
  • "what scheduling features are heavily used in the wild?"
  • "what scheduling features are rarely used in the wild?"
  • "what types of scheduling bugs are most commonly found?"
  • "what types of scheduling bugs are the most painful to track down?"
  • "when you are editing the constraints of a system, do you find yourself referring to the system's data dependencies or implementation?"
  • "when you are editing constraints of a system, do you find yourself referring to the adjacent scheduling constraints in the plugin?"
  • "do you find it easy to visualise & reason about the schedule by reading the scheduling logic in plugins?"
  • "do you rely on graph visualisation tools to view & reason about your schedule?"
  • "do you regularly need to specify constraints that straddle the boundaries of many plugins?"
  • "do you have trouble reasoning about constraints that straddle the boundaries of many plugins?"
  • "how often do you reuse systems?"
  • "when reusing systems, in what ways do the constraints change between usages?"

There are lots of tradeoffs to be made (many pointed out in this thread), and good decisions are difficult to make without knowing the value of what is being traded

@maniwani
Copy link
Contributor

maniwani commented Nov 18, 2023

I see the merit of embedding scheduling info in the fn itself—the motivation for doing so is ultimately the the same as #9792—but I just can't get over how it looks.

It's a clever idea, but style-wise, it's a tough sell in my opinion. It's not so bad when it's just one or two markers, but if there are more constraints, it'll look bad unless they're combined into a new, custom param.

(I'm not sure if such nested params are immune to the same kinds of issues that nested bundles and nested states have.)

It's kinda interesting that you could take that to an extreme, advise writing a unique param per fn, and it would vaguely resemble a trait impl. I wish it could be an actual trait impl, then it wouldn't clutter the signature, but I know we can't implement a trait on an individual fn directly.

My other issue is this would be another huge breaking change to the scheduling API.

The suggested API couldn't just coexist with the existing one (not that anyone was suggesting we mix and match them). UX-wise, we shouldn't have two distinct APIs controlling the same thing. This one is being pitched as a "just better" way to handle before and after anyway. (I'm not sure if that extends to in_set as well.)

Such a drastic change would result in another large effort to refactor everything like 0.10 (but with way less payoff).

@maniwani
Copy link
Contributor

maniwani commented Nov 18, 2023

Something I'm curious to know is what these params would even write to SystemMeta for the scheduler to see? i.e. What would the plumbing to extract this embedded info look like?

(Also, if this is going to turn into a true RFC, I think the discussion should move to a GitHub discussion or to https://github.com/bevyengine/rfcs.)

@Diddykonga
Copy link

A clear example of why this wouldn't work, at least on its own, is what Ordering Params would apply_deferred use?
It is a very generic system that you may want to have .after or .before in many different scenarios, ultimately this creates Rigidity which is bad In my opinion.

@B-Reif
Copy link
Contributor

B-Reif commented Nov 18, 2023

A clear example of why this wouldn't work, at least on its own, is what Ordering Params would apply_deferred use?

If I understand correctly, I think there would be some kind of composed type which would express each of the different places to instantiate the system.

@stepancheg how would the API order multiple instances of the same system?

@B-Reif
Copy link
Contributor

B-Reif commented Nov 18, 2023

Also I want to apologize for being judgmental too quickly; I’m not necessarily sold but I’d be happy to see a full RFC explaining the concept in fuller detail.

@stepancheg
Copy link
Contributor Author

@nicopap

I think it should be possible to implement the API as a third party crate. @stepancheg I would be open to collaborate on a proof of concept.

I don't understand how. This implementation requires adding members to SystemParam trait.

Alternatively

  • SystemParam trait can be extended as another trait
  • then extended trait can be implemented for all possible SystemParam implementations
  • then custom derive needs to be implemented
  • then custom App wrapper needs to be used

This would be duplication of large part of bevy_ecs.

@stepancheg
Copy link
Contributor Author

@B-Reif

@stepancheg how would the API order multiple instances of the same system?

Hard to answer without an example of problem to solve. I don't even know why would we need multiple instances of the same system.

Universal answer might be:

  • use different type annotations (e.g. fn reusable_system<StageMarker>(events: OrderedEventReader<MyEvent, StageMarker>))
  • do not use type annotations for ordering, keep using .after and .before

Generally I'm not 100% sure that ordering with types can replace ordering with method calls completely. I know it would solve all my problems, but my setup is perhaps not complex enough.

@stepancheg
Copy link
Contributor Author

@nicopap

OK, I gave an idea how it can be done, and perhaps bevy maintainers might be OK with it.

We implement dynamic provide-like API in bevy.

So bevy would require these changes:

trait SystemParam {
  // Request user metadata from param. Note this is dynamically typed like Rust provide API
  fn provide_user_meta(/* no self */ request: &mut Request) {}
}

trait System {
  // Request user metadata from all the parameters
  fn provide_param_user_meta(&self, request: &mut Request) { ... }
}

impl dyn System {
  // Easier to use wrapper for `provide_param_user_meta`
  fn user_meta<T>(&self) -> Vec<T> { ... }
}

Then a user (or third-party library) could:

  • create custom Before<T>/After<T> types which would just provide before/after information as metadata. The metadata would be the lists of InternedSystemSet
  • ordering won't happen automatically by bevy, high level API would need to be used

instead of

app.add_systems(Update, my_system)

this function should be used:

add_systems_with_scheduling_from_user_meta(app, Update, my_system)

@stepancheg
Copy link
Contributor Author

Draft RFC for parameter metadata #10678 which can be used instead of scheduling instructions built into systems.

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-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants