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

Inferred command flushes (hard sync points) and at-least once system separation #7838

Closed
alice-i-cecile opened this issue Feb 28, 2023 · 3 comments
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 28, 2023

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

When working with command flushes, you rarely care that two systems are separated by a particular copy of apply_system_buffers.

Instead, you care that it is "separated by at least one copy". This pattern occurs commonly: with command buffers, state transitions, transform propagation and pretty much anything you might use the Deferred type (#6817) for.

Trying to manage this in a single app can be challenging, as it requires a fairly global view of system scheduling that's easy to accidentally break.
The problem gets much worse when third-party plugins are involved though: each plugin must add its own cleanup systems to uphold its invariants, and is unaware of what other plugins may be doing, even if they could trivially share these cleanup systems.

What solution would you like?

Add a new form of system ordering, which requires commands to be applied at least once between them.

This could look like:

// Special casing
app.add_system(a.before_and_flush(b));

// Generalized chain-like syntax on system sets
app.add_systems((a,b).separated_by(apply_system_buffers));
app.configure_set(MySet.seperated_by(apply_system_buffers));

// Generalized two argument syntax
app.add_system(a.before_and_seperated_by(b, apply_system_buffers);

The scheduler should then attempt to ensure that at least one system of the appropriate type is run between a and b
Of course, figuring out how to do so efficiently is far from trivial and an MVP should probably be quite naive (perhaps only checking, rather than automatically inserting!).

Interestingly, I think that this should only work for system type sets, not arbitrary system sets. It's far too easy to get very surprising behavior if the exact system separating your system can change in non-local ways.

What alternative(s) have you considered?

Better graph visualization would help, but doesn't solve the problem of third-party plugins that have no way of being aware of each other.

This could be generalized to arbitrary systems which would be very useful for cache-flushing patterns like #4513 or state transitions. It's unclear if that will be more or less complex than simply doing so for apply_state_transitions.

Additional context

Previously discussed in bevyengine/rfcs#45, but cut due to scope concerns.

This would be a powerful tool to enable better configurability of plugins (#2160), as plugin authors can be much less strict about the provided configuration.

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-ECS Entities, components, systems, and events D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR labels Feb 28, 2023
@alice-i-cecile
Copy link
Member Author

After doing this writeup, I'm personally convinced that we should aim for a general system-type agnostic solution. The pattern is very useful in a ton of places, and I think the syntax is significantly more explicit (and it makes it much easier to search for apply_system_buffers).

@alice-i-cecile
Copy link
Member Author

Useful discussion on approaches [here] (https://discord.com/channels/691052431525675048/749335865876021248/1079960842583408681).

We can either use a sweep line approach or a backtracking algorithm.

@alice-i-cecile
Copy link
Member Author

#9822 effectively fixed this. Closing out.

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 D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

No branches or pull requests

1 participant