Skip to content

Recursive run_system#18076

Merged
alice-i-cecile merged 1 commit intobevyengine:mainfrom
SpecificProtagonist:recursive-run_system
Mar 10, 2025
Merged

Recursive run_system#18076
alice-i-cecile merged 1 commit intobevyengine:mainfrom
SpecificProtagonist:recursive-run_system

Conversation

@SpecificProtagonist
Copy link
Copy Markdown
Contributor

@SpecificProtagonist SpecificProtagonist commented Feb 27, 2025 •
edited
Loading

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 RawCommandQueue to only apply part of it.

Testing

See the new test.

Showcase

#[derive(Resource)]
pub struct Test {
    id: SystemId,
    counter: u32,
}

let mut world = World::new();
let id = world.register_system(|mut commands: Commands, mut test: ResMut<Test>| {
    print!("{:?} ", test.counter);
    test.counter -= 1;
    if test.counter > 0 {
        commands.run_system(test.id);
    }
});
world.insert_resource(Test { id, counter: 5 });
world.run_system(id).unwrap();
5 4 3 2 1

@SpecificProtagonist SpecificProtagonist added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 27, 2025
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Feb 27, 2025

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.

chescock
chescock approved these changes Feb 28, 2025
View reviewed changes
crates/bevy_ecs/src/system/system.rs
/// Runs the system with the given input in the world.
///
/// [`run_readonly`]: ReadOnlySystem::run_readonly
fn run_without_applying_deferred(
Copy link
Copy Markdown
Contributor

@chescock chescock Feb 28, 2025

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:

bevy/crates/bevy_ecs/src/schedule/executor/single_threaded.rs

Line 116 in b73811d

if system.is_exclusive() {

It might be worth doing a follow-up to use run_without_applying_deferred there.

Carter0
Carter0 reviewed Mar 5, 2025
View reviewed changes
crates/bevy_ecs/src/system/system_registry.rs
}

// Run any commands enqueued by the system
self.flush();
Copy link
Copy Markdown
Contributor

@Carter0 Carter0 Mar 5, 2025

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?

Copy link
Copy Markdown
Contributor Author

@SpecificProtagonist SpecificProtagonist Mar 5, 2025 •
edited
Loading

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.

Copy link
Copy Markdown
Contributor

@Carter0 Carter0 Mar 5, 2025

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
Carter0 approved these changes Mar 5, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@Carter0 Carter0 left a 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

@SpecificProtagonist SpecificProtagonist added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 5, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 10, 2025
Merged via the queue into bevyengine:main with commit 54247bc Mar 10, 2025
35 checks passed
@chescock chescock mentioned this pull request Mar 19, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Mar 26, 2025
@chescock
Fix run_system for adapter systems wrapping exclusive systems (#18406)
5d1fe16 
# 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.
mockersf pushed a commit that referenced this pull request Mar 26, 2025
@chescock @mockersf
Fix run_system for adapter systems wrapping exclusive systems (#18406)
4bc4f93 
# 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.
@chescock chescock mentioned this pull request Apr 2, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request May 6, 2025
@chescock
Use new run_without_applying_deferred method in `SingleThreadedExec…
3442e25 
…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.
andrewzhurov pushed a commit to andrewzhurov/bevy that referenced this pull request May 17, 2025
@chescock @andrewzhurov
Use new run_without_applying_deferred method in `SingleThreadedExec…
ea943d8 
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Carter0 Carter0 Carter0 approved these changes

@chescock chescock chescock approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

One shot system cannot recursively call itself

4 participants

@SpecificProtagonist @alice-i-cecile @chescock @Carter0