Auto insert sync points #9822
Auto insert sync points #9822
Conversation
hymm
force-pushed
the
auto-sync-points
branch
2 times, most recently
from
f69df42
to
810a6d2
Compare
|
This is breaking, as users might depend on the deferred behavior of commands. This is known to exists, for example: https://discord.com/channels/691052431525675048/1150574788906065931 |
nicopap
added
C-Enhancement
|
Also this is a regression. I'd love to be able to control whether or not a sync point gets added after my system. For example, by adding a |
Something like this shouldn't be hard to add, but what is the use case here? it's pretty hard to control when commands are applied since apply_deferred applies commands for any system that has run. |
|
I was thinking of a system that very rarely spawns entities or insert components. I wouldn't want to pay the overhead of a de-parallelization in such a situation. |
hymm
force-pushed
the
auto-sync-points
branch
from
53e181b
to
7aeac56
Compare
|
I think a system like My thinking is that we should have a different |
hymm
force-pushed
the
auto-sync-points
branch
2 times, most recently
from
b153620
to
30236fa
Compare
|
removed any manual sync points in the bevy plugins. Most of the schedules don't have any sync points. A few did or now do.
Render had sync points between each major set. These are now removed, which accounts for the difference there. Need to take a closer look at SpawnScene and see if the added sync point makes sense or not. |
hymm
force-pushed
the
auto-sync-points
branch
5 times, most recently
from
65cc87c
to
0a880ee
Compare
hymm
force-pushed
the
auto-sync-points
branch
from
b54a3c1
to
2c0f183
Compare
|
I feel quite torn on this. I've always wanted automatic sync points, but I never envisioned them in this form. I always thought of it being more explicit, where you would express a dependency as |
|
Can this be added as an alternative I mean the change is super trivial and very beneficial. Maybe we can add it to 0.12 as a non-default |
|
I currently have a schedule that needs to be deterministic, therefore I make sure all systems that depend on each other have a before-after relationship (with the help of the ambiguity option). But I still want to apply commands only at the end of the schedule to keep it easier to follow what happens. So in summary, I think this should be fine as long the |
Same. I always thought we'd express these constraints as new bevy/crates/bevy_ecs/src/schedule/graph_utils.rs Lines 41 to 48 in 0181d40 And then we could solve them with a sort and sweep (SAP). As an example, say there are 6 systems, labeled To resolve all "Sweeping" through this list, we'd learn that Visually, that process looks like this. Expand
(edit) |
This is how I think of it. After seeing how many issues users run into in discord with not seeing the effects of their commands in the next system, I think this is the right default. Beginner users shouldn't have to think about when their commands are applied. They just want to see the effects. Controlling sync points per system is just a micro-optimization. The problem with API's like I don't think it'd be hard to add a
No the changes happen before the executor, during schedule construction. We could change the default of |
| fn before<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfigs { | ||
| self.into_configs().before(set) | ||
| } | ||
|
|
||
| /// Run after all systems in `set`. | ||
| /// Runs before all systems in `set`. If `set` has any systems that produce [`Commands`](crate::system::Commands) |
There was a problem hiding this comment.
Should be "runs after"
# Objective Re this comment: #11141 (comment) Since #9822, Bevy automatically inserts `apply_deferred` between systems with dependencies where needed, so manually inserted `apply_deferred` doesn't to anything useful, and in current state this example does more harm than good. ## Solution The example can be modified with removal of automatic `apply_deferred` insertion, but that would immediately upgrade this example from beginner level, to upper intermediate. Most users don't need to disable automatic sync point insertion, and remaining few who do probably already know how it works. CC @hymm
# Objective - Example in error B0003 is not failing anymore after #9822 ## Solution - Update the example code so that is always fail - Also update logs and instructions on how to debug as it's easier now --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective - In #9822 I forgot to disable auto sync points on the Extract Schedule. We want to do this because the Commands on the Extract Schedule should be applied on the render thread.
# Objective - In bevyengine#9822 I forgot to disable auto sync points on the Extract Schedule. We want to do this because the Commands on the Extract Schedule should be applied on the render thread.
# Objective Since #9822, `SimpleExecutor` panics when an automatic sync point is inserted: ```rust let mut sched = Schedule::default(); sched.set_executor_kind(ExecutorKind::Simple); sched.add_systems((|_: Commands| (), || ()).chain()); sched.run(&mut World::new()); ``` ``` System's param_state was not found. Did you forget to initialize this system before running it? note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Encountered a panic in system `bevy_ecs::schedule::executor::apply_deferred`! ``` ## Solution Don't try to run the `apply_deferred` system.
# Objective Since bevyengine#9822, `SimpleExecutor` panics when an automatic sync point is inserted: ```rust let mut sched = Schedule::default(); sched.set_executor_kind(ExecutorKind::Simple); sched.add_systems((|_: Commands| (), || ()).chain()); sched.run(&mut World::new()); ``` ``` System's param_state was not found. Did you forget to initialize this system before running it? note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Encountered a panic in system `bevy_ecs::schedule::executor::apply_deferred`! ``` ## Solution Don't try to run the `apply_deferred` system.
# Objective Since bevyengine#9822, `SimpleExecutor` panics when an automatic sync point is inserted: ```rust let mut sched = Schedule::default(); sched.set_executor_kind(ExecutorKind::Simple); sched.add_systems((|_: Commands| (), || ()).chain()); sched.run(&mut World::new()); ``` ``` System's param_state was not found. Did you forget to initialize this system before running it? note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Encountered a panic in system `bevy_ecs::schedule::executor::apply_deferred`! ``` ## Solution Don't try to run the `apply_deferred` system.
# Objective Since #9822, `SimpleExecutor` panics when an automatic sync point is inserted: ```rust let mut sched = Schedule::default(); sched.set_executor_kind(ExecutorKind::Simple); sched.add_systems((|_: Commands| (), || ()).chain()); sched.run(&mut World::new()); ``` ``` System's param_state was not found. Did you forget to initialize this system before running it? note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Encountered a panic in system `bevy_ecs::schedule::executor::apply_deferred`! ``` ## Solution Don't try to run the `apply_deferred` system.
Objective
Solution
after_ignore_deferred,before_ignore_deferredandchain_ignore_deferred. Theauto_insert_apply_deferredboolean onScheduleBuildSettingscan be set to false to opt out for the whole schedule.Perf
This has a small negative effect on schedule build times.
Changelog
after_ignore_deferred,before_ignore_deferred,chain_no_deferredandauto_insert_apply_deferredAPIs to opt out of this behaviorMigration Guide
apply_deferredpoints are added automatically when there is ordering relationship with a system that has deferred parameters likeCommands. If you want to opt out of this you can switch fromafter,before, andchainto the correspondingignore_deferredAPI,after_ignore_deferred,before_ignore_deferredorchain_ignore_deferredfor your system/set ordering.ScheduleBuildSettings::auto_insert_sync_pointstofalseif you want to do it for the whole schedule. Note that in this mode you can still addapply_deferredpoints manually.apply_deferredyou should remove them as they cannot be merged with the automatically inserted points and might reduce parallelizability of the system graph.SystemParam, you will need to addsystem_meta.set_has_deferredif you useSystemParam::applyand want sync points auto inserted after use of yourSystemParam.TODO
decide if we should deprecate manually using apply_deferred.We'll still allow inserting manual sync points for now for whatever edge cases users might have.