-
Notifications
You must be signed in to change notification settings - Fork 66
Encoding Side Effect Lifecycles as Subgraphs #36
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
base: main
Are you sure you want to change the base?
Conversation
8df1bfc
to
6eaaee5
Compare
```rs | ||
fn main() { | ||
App::new() | ||
.add_system(apply_commands_buffer.consumes("commands")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to use strongly typed labels here when possible, especially for standard concepts like Commands.
|
||
// if we need to schedule a system after commands are applied | ||
.add_system(create_commands_A.label("system A").produces("commands")) | ||
.add_system(after_commands_A.after("system A").after("commands")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this will give the desired behavior: it will run after all of the available systems with the "commands" label are complete, when we want "at least one".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be particularly nasty if you want to chain several of these things back-to-back within a frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I was trying to replicate the current behavior to show how the API is related to how things currently work. I think this API should work just fine for chaining things together. I added an example where you apply the command buffers separately.
.label(SideEffectLabel) | ||
.add_system_set(SystemSet::new() | ||
.label(produces(SideEffectLabel)) | ||
.before(reads(SideEffectLabel)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this really wants the before_if_needed
API to reduce pointless blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think before if needed makes sense here, since if you're reading the side effect, it doesn't exist until after the producing systems run.
|
||
## Alternatives Considered | ||
|
||
* The example above for expressing the relationships between side effects using just labeling and system sets has one major problem. There is no current api for adding systems to system sets after the system set has been created. If such an api did exist we could potentially just add systems to the correct system set. This didn't seem better than the proposed API and has some implementation complexity since system sets are mostly just api sugar for labeling multiple systems and does not exist at runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also steal the "label configuration" idea from #33 (https://github.com/alice-i-cecile/rfcs/blob/apps-own-scheduling/rfcs/33-apps_own_scheduling.md#configuring-systems-by-their-label).
* This is part of a bigger redesign of the ECS trying to solve problems with scheduling around hard sync points and data consistency between states. This RFC by itself doesn't solve those problems, but it does encode information about dependencies that other RFCs might need to actually solve the data consistency problems. | ||
* Side effects are effectively typed. Build an API to pass and send side effects and enforce the typing between producers and consumers. This could either be with data stored in the executor or a Resource that manages access through access tokens (what Boxy is working on). | ||
* How to specify an order to how commands are applied? | ||
* This API doesn't really directly address data consistency problems due to changes to run criteria. Ideally we'd figure out a way to enforce that the consumer needs to run if side effects have been produced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to chew on this in my pre-draft RFC here: https://github.com/alice-i-cecile/rfcs/blob/system-graph-flavors/rfcs/34-system-graph-flavors.md#causal-ties-systems-that-must-run-together
|
||
* The example above for expressing the relationships between side effects using just labeling and system sets has one major problem. There is no current api for adding systems to system sets after the system set has been created. If such an api did exist we could potentially just add systems to the correct system set. This didn't seem better than the proposed API and has some implementation complexity since system sets are mostly just api sugar for labeling multiple systems and does not exist at runtime. | ||
|
||
## Future Work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indexing and relations are two other highly visible use cases that are enabled by solving the data consistency problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this. It's concise, useful and simple enough to actually explain to users and reason about.
-combined reads and writes into one function -rewrote motivation section -added an example for a possible way to order command buffers -changed filename to include PR#
This is an alternative to the scheduling api from #34 that covers more types of side effects.
Rendered