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

Add systems to sets based on their world accesses #7857

Closed
JoJoJet opened this issue Mar 1, 2023 · 10 comments
Closed

Add systems to sets based on their world accesses #7857

JoJoJet opened this issue Mar 1, 2023 · 10 comments
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

Comments

@JoJoJet
Copy link
Member

JoJoJet commented Mar 1, 2023

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

Currently, it is difficult to order systems based only on their logical constraints. In cases where you want a system to run after (or before) other systems which access a resource or component, you need to have global knowledge of the schedule and which systems might access that data.

One example of this is for events: if you have a system that reads AppExit events, it must run after the system that sends those events -- otherwise the event will be missed forever. Ordering the systems correctly requires knowing which system sends the event and adding a dependency to it, which is difficult to discover. See #7067, which is motivated by this issue.

What solution would you like?

Make it possible to automatically assign systems to a set based on their world accesses. An event reader system with proper ordering could look like:

fn read_app_exit(mut events: EventReader<AppExit>) { ... }

app.add_system(read_app_exit.after(SendsEvents::<AppExit>::default());

When combined with #7838, transform propagation could be as simple as:

#[derive(Component)]
#[component(write_set = WriteTransform)] // any system with access to `&mut Transform` will be added to this set.
pub struct Transform { ... }

// Perform transform propagation after every system has modified `Transform`,
// and after their commands have been flushed.
app.add_system(transform_propagate_system.after_and_flush(WriteTransform));

There should be a way of manually opting out of access sets:

fn jump(q: Query<&mut Transform>) { ... }

// This system can run after `transform_propagate_system`.
app.add_system(jump.without_set(WritesTransform));

What alternative(s) have you considered?

Do nothing, and continue using global reasoning to order systems.

Additional context

A variant of this feature that is restricted to events was originally suggested in #4872.

@JoJoJet JoJoJet added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 1, 2023
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR and removed S-Needs-Triage This issue needs to be labelled labels Mar 1, 2023
@alice-i-cecile
Copy link
Member

I'm in favor of this, but we need to be careful not to explode our schedule graph. These sets should be built on an as-needed basis, rather than opportunistically.

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 1, 2023

I agree. These sets should only be added if the creator of the component or resources chooses to expose a public SystemSet for it. We don't need dozens of unused sets for every single world access.

@alice-i-cecile
Copy link
Member

Via something like app.define_set(Reads<Transform>), which only requires type access?

Or via like... flags on the derive macro or a trait somehow, in which case it would really only be up to the owner of the component type?

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 1, 2023

I was suggesting the latter, but I could see value in allowing outside configuration.

@alice-i-cecile
Copy link
Member

I think I like the former better: it's annoying as a plugin author to try and predict and respond to all of the needs of your users.

I also think that in this case it will be much simpler to implement :)

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 1, 2023

I also think that in this case it will be much simpler to implement :)

From my initial experiments, I think that the opposite is true. The derive macro approach can just build on top of System::default_system_sets, which already exists.

Making it externally configurable means creating a whole API based around access sets (need a better name for this concept), which Schedule must be aware of.

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 1, 2023

I stand corrected -- it's actually way simpler to do it dynamically after all. Here's what I've got so far: main...JoJoJet:bevy:access-system-sets

@maniwani
Copy link
Contributor

maniwani commented Mar 2, 2023

If these sets are automatically populated, they need same or nearly the same usage restrictions as SystemTypeSet.

Saying app.add_system(X.after(WritesT)) makes it more likely that things result in a DependencyCycle or SetsHaveOrderButIntersect error.

I also expect some users to mistakenly think this accounts for apply_system_buffers and mean "after all writers before next sync point" instead of "after all writers in the schedule."

Note that this implementation would need to change to use relations if/when we add them and move to systems being entities (no promises, but trying to do both for 0.11).

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 3, 2023

If these sets are automatically populated, they need same or nearly the same usage restrictions as SystemTypeSet.

Meaning, no configuration can be done on it? It's only used for ordering other things? Seems reasonable.

Saying app.add_system(X.after(WritesT)) makes it more likely that things result in a DependencyCycle or SetsHaveOrderButIntersect error.

Yeah, this is a powerful feature so we'll have to warn about common pitfalls and encourage users to be careful with how this is used.

@alice-i-cecile
Copy link
Member

Closing this since I suspect the use cases will be very limited in real applications. If someone else wants to pursue this, feel free, though it should probably go through the RFC process

@alice-i-cecile alice-i-cecile closed this as not planned Won't fix, can't repro, duplicate, stale Mar 24, 2024
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
3 participants