-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Recursive run_system #18076
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
Recursive run_system #18076
Conversation
|
Oh hey, I remember being annoyed by this and not being able to figure out how to fix it when I was working on the MVP one-shot systems. Definitely a bug. |
| /// Runs the system with the given input in the world. | ||
| /// | ||
| /// [`run_readonly`]: ReadOnlySystem::run_readonly | ||
| fn run_without_applying_deferred( |
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 is a useful API to expose! The single-threaded executor has some unsafe code and a special case for exclusive systems because it wants exactly this behavior:
| if system.is_exclusive() { |
It might be worth doing a follow-up to use run_without_applying_deferred there.
| } | ||
|
|
||
| // Run any commands enqueued by the system | ||
| self.flush(); |
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 thought commands were only run at sync points? Not after any system has been run?
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.
We are at a sync point, in the sense that we have an exclusive reference to the world. Before this PR, commands queued by the system were executed here via System::run.
There is a difference in behavior though: Previously, only the commands queued by the system were applied (which might include running other systems and their commands); now, this includes commands that were already queued before the system was run. I've documented it, but if we think the previous behavior is less surprising, I'll add a method to CommandQueue to only apply part of it and use it here. I'd do that in a followup PR though because it also involves unsafe and I want to keep it easy to review.
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.
All good I mostly asked for my own understanding.
Carter0
left a comment
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.
Minor question, but I think its good
# Objective Fix panic in `run_system` when running an exclusive system wrapped in a `PipeSystem` or `AdapterSystem`. #18076 introduced a `System::run_without_applying_deferred` method. It normally calls `System::run_unsafe`, but `ExclusiveFunctionSystem::run_unsafe` panics, so it was overridden for that type. Unfortunately, `PipeSystem::run_without_applying_deferred` still calls `PipeSystem::run_unsafe`, which can then call `ExclusiveFunctionSystem::run_unsafe` and panic. ## Solution Make `ExclusiveFunctionSystem::run_unsafe` work instead of panicking. Clarify the safety requirements that make this sound. The alternative is to override `run_without_applying_deferred` in `PipeSystem`, `CombinatorSystem`, `AdapterSystem`, `InfallibleSystemWrapper`, and `InfallibleObserverWrapper`. That seems like a lot of extra code just to preserve a confusing special case! Remove some implementations of `System::run` that are no longer necessary with this change. This slightly changes the behavior of `PipeSystem` and `CombinatorSystem`: Currently `run` will call `apply_deferred` on the first system before running the second, but after this change it will only call it after *both* systems have run. The new behavior is consistent with `run_unsafe` and `run_without_applying_deferred`, and restores the behavior prior to #11823. The panic was originally necessary because [`run_unsafe` took `&World`](https://github.com/bevyengine/bevy/pull/6083/files#diff-708dfc60ec5eef432b20a6f471357a7ea9bfb254dc2f918d5ed4a66deb0e85baR90). Now that it takes `UnsafeWorldCell`, it is possible to make it work. See also Cart's concerns at #4166 (comment), although those also predate `UnsafeWorldCell`. And see #6698 for a previous bug caused by this panic.
# Objective Fix panic in `run_system` when running an exclusive system wrapped in a `PipeSystem` or `AdapterSystem`. #18076 introduced a `System::run_without_applying_deferred` method. It normally calls `System::run_unsafe`, but `ExclusiveFunctionSystem::run_unsafe` panics, so it was overridden for that type. Unfortunately, `PipeSystem::run_without_applying_deferred` still calls `PipeSystem::run_unsafe`, which can then call `ExclusiveFunctionSystem::run_unsafe` and panic. ## Solution Make `ExclusiveFunctionSystem::run_unsafe` work instead of panicking. Clarify the safety requirements that make this sound. The alternative is to override `run_without_applying_deferred` in `PipeSystem`, `CombinatorSystem`, `AdapterSystem`, `InfallibleSystemWrapper`, and `InfallibleObserverWrapper`. That seems like a lot of extra code just to preserve a confusing special case! Remove some implementations of `System::run` that are no longer necessary with this change. This slightly changes the behavior of `PipeSystem` and `CombinatorSystem`: Currently `run` will call `apply_deferred` on the first system before running the second, but after this change it will only call it after *both* systems have run. The new behavior is consistent with `run_unsafe` and `run_without_applying_deferred`, and restores the behavior prior to #11823. The panic was originally necessary because [`run_unsafe` took `&World`](https://github.com/bevyengine/bevy/pull/6083/files#diff-708dfc60ec5eef432b20a6f471357a7ea9bfb254dc2f918d5ed4a66deb0e85baR90). Now that it takes `UnsafeWorldCell`, it is possible to make it work. See also Cart's concerns at #4166 (comment), although those also predate `UnsafeWorldCell`. And see #6698 for a previous bug caused by this panic.
…utor` (#18684) # Objective Simplify code in the `SingleThreadedExecutor` by removing a special case for exclusive systems. The `SingleThreadedExecutor` runs systems without immediately applying deferred buffers. That required calling `run_unsafe()` instead of `run()`, but that would `panic` for exclusive systems, so the code also needed a special case for those. Following #18076 and #18406, we have a `run_without_applying_deferred` method that has the exact behavior we want and works on exclusive systems. ## Solution Replace the code in `SingleThreadedExecutor` that runs systems with a single call to `run_without_applying_deferred()`. Also add this as a wrapper in the `__rust_begin_short_backtrace` module to preserve the special behavior for backtraces.
…utor` (bevyengine#18684) # Objective Simplify code in the `SingleThreadedExecutor` by removing a special case for exclusive systems. The `SingleThreadedExecutor` runs systems without immediately applying deferred buffers. That required calling `run_unsafe()` instead of `run()`, but that would `panic` for exclusive systems, so the code also needed a special case for those. Following bevyengine#18076 and bevyengine#18406, we have a `run_without_applying_deferred` method that has the exact behavior we want and works on exclusive systems. ## Solution Replace the code in `SingleThreadedExecutor` that runs systems with a single call to `run_without_applying_deferred()`. Also add this as a wrapper in the `__rust_begin_short_backtrace` module to preserve the special behavior for backtraces.
Objective
Fixes #18030
Solution
When running a one-shot system, requeue the system's command queue onto the world's command queue, then execute the later.
If running the entire command queue of the world is undesired, I could add a new method to
RawCommandQueueto only apply part of it.Testing
See the new test.
Showcase