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

System & frame stepping support #8063

Closed
wants to merge 20 commits into from
Closed

Conversation

dmlary
Copy link
Contributor

@dmlary dmlary commented Mar 12, 2023

Objective

  • Add support to interactively step through systems & frames within Schedules
    • system step runs the next system in the schedule on the next frame
    • frame step runs all remaining systems in the schedule on the next frame
  • Used with crates like bevy-inspector-egui to inspect & modify components between system executions

Solution

  • Add support to Schedule, and SystemExecutor to implement system & frame stepping
  • Add plumbing necessary to control Schedule stepping
  • Add support for systems to be exempt from stepping
  • Mark all bevy systems (such as input, render, ui) as exempt from stepping

Systems will permit stepping by default. This is more work within bevy, but allows crate users the most benefit by having it enabled by default.

Note: In the critical path (SystemExecutor::run()), when stepping is enabled, this PR only introduces one call, conditional check, and return.

Limitations

As this is the initial implementation for this capability, and a need to limit scope of these changes, there are some limitations to system stepping.
Due to architectural constraints in bevy, there are some cases systems stepping will not function as a user would expect.

Event-driven systems

Stepping does not support systems that are driven by Events as events are flushed after 1-2 frames. Although game systems are not running while stepping, ignored systems are still running every frame, so events will be flushed.

This presents to the user as stepping the event-driven system never executes the system. It does execute, but the events have already been flushed.

This can be resolved by changing event handling to use a buffer for events, and only dropping an event once all readers have read it.

The work-around to allow these systems to properly execute during stepping is to have them ignore stepping: app.add_systems(event_driven_system.ignore_stepping()). This was done in the breakout example to ensure sound played even while stepping.

Conditional Systems

When a system is stepped, it is given an opportunity to run. If the conditions of the system say it should not run, it will not.

Similar to Event-driven systems, if a system is conditional, and that condition is only true for a very small time window, then stepping the system may not execute the system. This includes depending on any sort of external clock.

This exhibits to the user as the system not always running when it is stepped.

A solution to this limitation is to ensure any conditions are consistent while stepping is enabled. For example, all systems that modify any state the condition uses should also enable stepping.

State-transition Systems

Stepping is controlled on the per-Schedule level, requiring the user to have a ScheduleLabel.

To support state-transition systems, bevy generates needed schedules dynamically. Currently it’s very difficult (if not impossible, I haven’t verified) for the user to get the labels for these schedules.

Without ready access to the dynamically generated schedules, and a resolution for the Event lifetime, stepping of the state-transition systems is not supported

Demo

Updated demonstration of stepping with a UI in breakout example

Screen.Recording.2023-03-19.at.3.00.13.PM.mov

The grave key is used to enable stepping mode, S steps forward a single system, Space steps forward a full frame. The video demonstrates walking through the systems as a collision occurs in breakout.

A much better UI will be possible with the use of external crates such as egui.

Performance Details

As this change leverages the existing completed_systems list in the SystemExecutors, the addition of stepping has no discernible impact on performance.

Measurement Methodology

I tweaked FrameTimeDiagnosticsPlugin to keep 8000 fps samples, then ran cargo run --example many_foxes --release twice, letting many_foxes run for 20 seconds. I ran this once with the hooks for stepping present in both MultiThreadedExecutor and SingleThreadExecutor. For the second run, I commented out the hooks in both executors. Stepping was never enabled during either test run.

The average FPS for the two runs after 20 seconds were within 1 fps of each other, around 126fps.

With a longer run (60 seconds), both binaries leveled out around 125fps.


Changelog

The key changes were made to SystemSchedule to track stepping state, and return a list of systems to be skipped in SystemExecutor::run(). All system executors were updated to apply the skip list from SystemSchedule.

A new event type, ScheduleEvent, was added to control stepping behavior from within systems. This was needed as the currently running schedule is not accessible in the Schedules resource, and we don't want stepping changes to apply mid-frame. App::update() calls Schedule::handle_event() for each event after all schedules have been run.

Added example/games/stepping.rs, a new plugin to control stepping and display a UI for example games. It has been integrated into both breakout and alien_cake_addict.

Added a number of helper functions to Schedule to expose needed data for the example UI.

Marked every existing system in bevy with ignore_stepping() in their add_systems() calls. This should ensure bevy is still fully functional (input/ui/render) when stepping is enabled.

Migration Guide

If a new system is added to one of bevy's plugins, ensure .ignore_stepping() is added. Example:

app.add_system(shiny_new_input_system.ignore_stepping())

This is very much a short-cut proof-of-concept.  Breakout happens to all
of the gameplay in the `FixedUpdate` schedule, so I didn't have to tag
every input & render system in bevy as `ignore_stepping()`.
@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 12, 2023
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible 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 Mar 12, 2023
@@ -40,6 +40,7 @@ pub struct SystemConfig {
pub(super) system: BoxedSystem,
pub(super) graph_info: GraphInfo,
pub(super) conditions: Vec<BoxedCondition>,
pub(super) ignore_stepping: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would probably be clearer as a SteppingBehavior enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rust idiom here?

enum SteppingBehavior {
   Permit,
   Ignore,
}

// or
enum SteppingBehavior {
    PermitStepping,
    IgnoreStepping,
}

The actual code has comments and defaults, but I'm not sure about naming convention here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning towards the second to permit:

        use config::SteppingBehavior::*;
        let stepping = match stepping_behavior {
            PermitStepping => true,
            IgnoreStepping => false,
        };

Copy link
Contributor Author

@dmlary dmlary Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also looking for better names here.

I rejected Enable/Disable because they conflict with enabling/disabling on the Schedule. I want to avoid "oh, you have to enable stepping both on the system and the schedule".

Observe/Ignore is an option, but not much better than Permit/Ignore.

Observe/Disregard?

Allow/Deny -- Deny feels too much like it could stop stepping from working at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

enum Stepping {
   Incremental,
   Continual,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just came across Exempt, but at the same time, that's a real weird english word, and doesn't have a clear antonym. Perhaps Observe/Exempt?

@github-actions
Copy link
Contributor

Example breakout failed to run, please try running it locally and check the result.

Moved the stepping control & UI code out into a module, and loading it
back into breakout as a plugin.  The code should work fine for any of
the other game examples.  Will get to that eventually.

I had to add a bunch of helper methods to `Schedule` to get the
information needed for the UI.  Need to clean those up still.

Also, there's a bug in the mapping from
`SystemConfig::stepping_behavior`, through
`ScheduleGraph::system_stepping_enabled`, into
`SystemsSchedule::systems_with_stepping_enabled`.
When I started marking bevy systems as `ignore_stepping()`, different
systems were ignoring stepping in the UI.  It may also be a bug in my
quickly thrown together helpers in `Schedule`.
I wasn't using the correct index when transferring the stepping enabled
systems from `ScheduleGraph` to `SystemSchedule`.  That's fixed.
I went through every `impl Plugin` looking for calls to `add_systems()`,
and added `ignore_stepping()` to all of them.  It feels a little dirty,
so any suggestions of a better way to disperse this would be
appreciated.  I just don't know of a better approach than scattering
these throughout the code.

I also broke these changes into a single commit so it's easy to see all
of them at once.
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@@ -78,7 +81,8 @@ impl Plugin for Core3dPlugin {
sort_phase_system::<Opaque3d>.in_set(RenderSet::PhaseSort),
sort_phase_system::<AlphaMask3d>.in_set(RenderSet::PhaseSort),
sort_phase_system::<Transparent3d>.in_set(RenderSet::PhaseSort),
),
)
.ignore_stepping(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand why rustfmt is indenting this, but it happens everywhere I added ignore_stepping() to a tuple.

crates/bevy_ecs/src/schedule/executor/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/schedule.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/schedule.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/schedule.rs Outdated Show resolved Hide resolved
examples/games/stepping.rs Outdated Show resolved Hide resolved
examples/games/stepping.rs Show resolved Hide resolved
@dmlary dmlary changed the title WIP: system & frame stepping System & frame stepping support Mar 19, 2023
@dmlary dmlary marked this pull request as ready for review March 19, 2023 20:33
@cart
Copy link
Member

cart commented Mar 21, 2023

First: this is really cool and something I'd like to include in Bevy. I haven't done a full review yet, but after a first pass my biggest concern is the prevalence of ignore_stepping. This design forces pretty much every system implementer (bevy internals, 3rd party plugins, even user code in some cases) to be aware of stepping and make "the right call" about stepping configuration in order to preserve the integrity of the stepping system. I don't feel comfortable thrusting that concern on every system implementer / I consider solving that problem a hard blocker. We should consider ways to abstract this out wherever possible and solve foundational problems (such as event buffering).

@dmlary
Copy link
Contributor Author

dmlary commented Mar 22, 2023

TL;DR: To implement stepping, there must be some complexity added. For the
broadest benefit to users, that complexity should be on the system-implementers
for render, input, and windowing, otherwise stepping won't be widely used. The
complexity of the decision of whether to use ignore_stepping() with a system
comes down to is the application responsive to input, and able to display
frames without that system. That said, there are ways we can make this easier,
and ensure nobody accidentally breaks stepping (see Possible Paths Forward).

my biggest concern is the prevalence of ignore_stepping. This design forces pretty much every system implementer (bevy internals, 3rd party plugins, even user code in some cases) to be aware of stepping and make "the right call" about stepping configuration in order to preserve the integrity of the stepping system.

@cart Some complexity is inherent in the nature of system-based stepping, but
I'll argue that it's not as far reaching as you're seeing it right now.

First off, I went nuts with .ignore_stepping() in the PR; probably half of them aren't needed. I marked every system in bevy with it because I wasn't confident stepping would be accepted. I didn't want to dig into every system to see if it was critical to a responsive application while stepping without more guidance.

Critical Systems for a Responsive Application

For stepping to be a usable feature, there exists a subset of systems
within bevy that must always run to provide the user a responsive application.
If render systems (including UI) don't run, we can't see anything. If input
systems don't run, we can't step to the next system, much less exit stepping
mode. For a smooth experience, we can probably also throw the window
management systems in this group to be safe.

Edit I managed to forget about a bevy pattern where systems run Schedules, such as apply_state_transitions(). These systems must also be .ignore_system.

There must be some way to track which systems are critical to a responsive
application. For this PR, I've implemented it as .ignore_system(), but there
may be alternative approaches for determining this subset.

Outside of those three four groups, everything else should be "safe" to step.

What is "safe"? And making "the right call" for your system

(Note: This section is written assuming that at some point we make the change
to bevy where events are buffered until systems consume them. Yes, I know this
is cheating, but it's not impossible to implement. Joy has an idea about
this; I saw it.)

The vast majority of systems will never have to be considered in the context of
stepping. They're game systems that don't handle rendering or input. If
you're not pushing pixels, or reading input, most likely stepping will work
perfectly without any changes to your system.

For rendering systems, there's no complexity here as render will always ignore
stepping. Stepping render systems would require support directly in the render
pipeline to do some sort of draw-on-demand, layering each system as you move
forward in time. Sure, not all systems are required to draw a functional
application, but there's no real benefit to stripping some out (except maybe
performance testing, but that should be system enable/disable support).

For input systems, there is a little complexity; Is this system required for
keypress/mouse clicks to get to whatever stepping-control system is running.
This just ends up in the same category as render; Input will always ignore
stepping.

Edit If a system calls World::run_schedule(), or Schedule::run(), it must
also ignore stepping. I think this is the one that makes things complex, but it's a
reflection of the complexity of nesting Schedules within Systems. If we were
able to add a Schedule to another Schedule to be run exclusively, then
this would not be needed. The exclusive system gluing the two together isn't
necessary long-term.

ignore_stepping() Complexity by System-Implementer Category

For the specific system implementer group, the burden added to them can be
pinned down.

One key thing to keep in mind while going through these groups. This decision
only needs to be made once per system created. It's going to be rare for a
system to switch from drawing on the screen to updating entities, so the
decision won't need to be re-evaluated each time the system is updated.

Bevy Internals

These implementers will have to suffer the worst of it. Render, UI, input,
window handling, and schedule executing systems must ignore stepping.
But that's all, they just need to remember to put .ignore_stepping() in
those areas. I've already added it to the tuples they use in calls to
.add_systems(), and the individual systems.

The risk here is a new system gets added, in its own .add_system() call, and
doesn't duplicate the .ignore_stepping() all around it.

There are some things we can do to reduce the burden here further:

  • Edit Replace exclusive systems for running schedules with support for
    adding a Schedule to a parent Schedule for exclusive execution
  • Automated test that verifies .ignore_stepping() is used in these places
    • It would be complex, but should be possible to implement a stepping test
      that uses input & render
    • Another option is dynamic checking of systems; see Mitigations to
      Complexity
  • Independent Schedule for these; schedule marked to ignore_stepping
    • See Mitigations to Complexity below

I think a lot of the complexity concern here is because I threw
ignore_stepping() all over the place in the PR.

Crate Authors

Most crate authors won't have to worry about ignore_stepping(), but it does
put some burden on the authors of render, UI, and input handling crates. The
complexity comes out the the same as for bevy internals. They can probably
get away with just marking all of their systems as ignore_stepping().

As an alternative, there's the independent Schedule approach, which is
probably more ergonomic for crates.

Finally for this category, it would be nice if we had some mechanism for crate
authors to verify that their crate doesn't interfere with stepping.
That said, I have no solid idea of how to a) implement this, b) share it to the crate
community as part of bevy. Absolutely open to suggestions here.

Bevy Users

This category is easier to discuss. As an end-user, if they're not using
stepping, they don't ever need to worry about ignore_stepping().

If they do use stepping, there's two approaches:

  • ignore_stepping() for all render & input systems
  • Try it, see which system it hangs on when you step, add ignore_stepping()

The impact of an incorrect choice for ignore_stepping() is simply stepping
doesn't move forward, or the screen freezes.

I feel that the complexity burden with this group balances out as they're the
target for this feature.

The Trade-Off

What do we get in exchange for this increased complexity for systems
implementers?

Bevy users gain the ability to pause their entire game at any moment in
gameplay, and step through each one to diagnose strange behavior. This in
combination with an entity inspector & editor gives bevy a built-in runtime
debugger.

It is hard to understate how powerful this is. I've used this
capability in the past (other platforms) to diagnose why physics was going
sideways when I was having trouble diagnosing it via lldb. Being able to see &
interact with the application while debugging is ... addictive.

Alternatives

Stepping Opt-In

One alternate approach would be to have bevy users opt their systems into
stepping. I don't like this approach for the user-experience.
If systems must opt-in to stepping, users will experience two pain points:

  • external crates not enabling stepping on the systems they need to step through
    • this becomes a maintenance problem for most crates & users
  • users will likely want to step through the majority of their systems, not minority

Honestly, if we add barriers to getting stepping (or any debugging tool)
working, it won't be used. "If I just add a few more println!s, I know I can
figure this out!"

External Crate

Right now, there's no way to implement stepping as an external crate. This is
because the SystemExecutor and SystemSchedule are buried quite deeply in
Schedule, and offer no entry-points.

Even with an alternate API, this would end up shifting the burden of
ignore_stepping() only off of Bevy Internal, not off crate makers. It would
likely shift more burden to stepping users to allow/disallow some systems
manually, reducing the adoption of stepping.

Sidebar on what's needed to do this externally

I do see one path to implementing stepping as an external crate, but it
requires at least the following changes:

  • Per-System enable/disable support in Schedule
    • This is very easy to implement, and can be based off this PR
  • Better visibility into Schedule and System
    • Right now it's very difficult to discover every schedule & system that
      should be run
      • Dynamically generated Schedules for State change
      • Schedules that are executed from exclusive systems
    • There needs to be some consistent way to iterate all Schedules
  • Buffered Events
    • Because if I'm gonna write a wishlist, why not include all the parts

Dynamically applying ignore_stepping()

There's probably a dynamic way to determine a system should not be stepped.
We do have System::name(), and can easily pattern match bevy::* to ignore
stepping.

This does solve the burden for bevy, but doesn't really help crate authors.
Only a small number of crates need to consider stepping though.

Schedule::ignore_stepping()

Keep in mind that stepping is per-Schedule. The example implementation
requires the user to specify which schedules to step. To reduce the burden for
both Bevy Internal and Crate Authors, we can implement
Schedule::ignore_stepping(), and ignore all stepping requests (panic!() or
warn!();return).

This allows a larger granularity for disabling stepping, and simplifies at
least the render systems in bevy. I remember seeing talk on the discord about
Crate Authors shifting to per-crate schedules to avoid interference from user
systems. This may be a good path.

Possible Paths Forward

Ok, after all those words (if you read them all, thank you), here are
paths I can see for moving forward:

Option A: Move Forward with Stepping Now

This is of course my preference; I'd like to use this functionality now, and
believe the earlier debugging tools are integrated into a system, the better.

I don't feel comfortable thrusting that concern ["the right thing"
per-system] on every system implementer

At the highest level the question boils down to: Add ignore_stepping() if
this system is rendering or handling input.

This can be baked into bevy for bevy contributors by either adding "dynamic"
ignore_stepping() based on System::name(), or creating dedicated Schedules
that ignore stepping.

For crate developers, really all we can do to reduce the load is allow them to
mark their custom Schedules to ignore stepping. This does require them to
switch to custom Schedules, so this may be a complexity wash.

We should consider ways to abstract this out wherever possible and solve
foundational problems (such as event buffering).

Agreed, especially event buffering. I will point out that even with the
existing limitations regarding events, stepping is useful as it is right now.
In my first run of it working in breakout, I found a bug in the collision
system. I didn't fix it, but there's a video of the ball destroying a block
and not changing course.

The best we can do for abstraction right now is probably the dynamic
application of ignore_stepping() based on bevy crate name. This doesn't
benefit the crate authors, but maybe dedicated Schedules are their solution?

How to make sure stepping isn't broken by contributions

Stepping is at risk of being broken by someone introducing a critical system
for render/input without adding ignore_stepping(). I don't have a good idea
of what's possible in bevy test cases, but it would be great if I could add a
testcase to verify that input handling & rendering are still working with
stepping enabled.

I think just adding this as a CI test would mitigate a lot of concerns from the
Bevy Internal side.

Any suggestions on how to implement this would be greatly appreciated. It
requires simulating input to be read by the input system, and reading the
rendered frame. I have no idea how to do either of those from a testcase.

Option B: Delay Stepping Until Supporting Infrastructure Exists

Delay stepping until the following bits are implemented:

  • Buffered Events
  • Enable/Disable Systems
    • Easier system addressing/labeling
      • To a user, the system is a function. To the Schedule it's a node
        with a NodeId.
      • There is no user-friendly mapping between the two right now
        • How do I get from fn my_system(...) to the NodeId on the
          Schedule?
  • Global Visibility to all Schedules

The last two could be easily covered if we got Systems implemented as Entities.

Everything on this list is at least as complex as this PR. I actually went
down the rabbit

There could be an intermediate step here with a crate, but long-term I feel
bevy benefits greatly from having this functionality built-in.

Option C: Something Else

I'm open to any idea that gets stepping capabilities into Bevy. That's really
all I'm looking for here.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 22, 2023

After implementing pause functionality in my game, it's struck me that, by and large, I want to group this stepping (and pause) behavior based on the data that the components access.

If we had automatically lazily generated system sets on the basis of access (#7857), I think that maintaining this distinction might be much less onerous. For example, everything that writes to a Window or Input should probably be ignored by default.

Not a complete fix, but perhaps a useful direction. We could also configure a default on a per-schedule basis?

@dmlary
Copy link
Contributor Author

dmlary commented Mar 22, 2023

After implementing pause functionality in my game, it's struck me that, by and large, I want to group this stepping (and pause) behavior based on the data that the components access.

This is interesting, but I'm curious if it's just moving the ignore_stepping() metadata from the system to components & resources.

I see two categories here: what system should always be run (!ignore_stepping()), and which subset of steppable systems should be stepped for debugging the current problem. The idea of component based selection for stepping makes a lot of sense for the second group.

BUT! Oh, does this mean there's a way to tell from the System object that a system reads some events? If so, we could automatically handle the ignore_stepping() for evented systems right now.

@alice-i-cecile
Copy link
Member

BUT! Oh, does this mean there's a way to tell from the System object that a system reads some events? If so, we could automatically handle the ignore_stepping() for evented systems right now.

Yes, that should be possible on the basis of the Access. #5388 by @JoJoJet may give you some helpful clues.

@cart
Copy link
Member

cart commented Mar 22, 2023

To help illustrate a direction I think we should be headed in, I put together a simple draft PR: #8168.

@dmlary
Copy link
Contributor Author

dmlary commented Apr 1, 2023

BUT! Oh, does this mean there's a way to tell from the System object that a system reads some events? If so, we could automatically handle the ignore_stepping() for evented systems right now.

Yes, that should be possible on the basis of the Access. #5388 by @JoJoJet may give you some helpful clues.

@alice-i-cecile I'm not seeing how to make this work. I've set up the following test to print the Access values for an event-based system, and both Access methods contain nothing:

    struct TestEvent;
    fn event_system(mut reader: EventReader<TestEvent>) {
        for _ in reader.iter() {}
    }

    #[test]
    fn detect_event_system() {
        let system = IntoSystem::into_system(event_system);
        println!("system.component_access: {:#?}", system.component_access());
        println!(
            "system.archetype_component_access: {:#?}",
            system.archetype_component_access()
        );

        assert!(false);
    }

The output shows empty Access structs for both methods:

system.component_access: Access {
    read_and_writes: [],
    writes: [],
    reads_all: false,
}
system.archetype_component_access: Access {
    read_and_writes: [],
    writes: [],
    reads_all: false,
}

Is there some other mechanism to detect a system takes an EventReader argument?

@JoJoJet
Copy link
Member

JoJoJet commented Apr 2, 2023

I've set up the following test to print the Access values for an event-based system, and both Access methods contain nothing:

let system = IntoSystem::into_system(event_system);
println!("system.component_access: {:#?}", system.component_access());

The issue is that you aren't initializing the system. A system's access sets will be empty until you call initialize() on it.

@dmlary
Copy link
Contributor Author

dmlary commented Apr 2, 2023

I've set up the following test to print the Access values for an event-based system, and both Access methods contain nothing:

let system = IntoSystem::into_system(event_system);
println!("system.component_access: {:#?}", system.component_access());

The issue is that you aren't initializing the system. A system's access sets will be empty until you call initialize() on it.

@JoJoJet Thank you! That makes more sense.

@dmlary
Copy link
Contributor Author

dmlary commented Apr 21, 2023

Followup on the detecting event reader systems; I got it working, but it requires World. Dropping the code here so I don't lose it:

/// helper function to determine if a system reads events
#[allow(dead_code)]
fn system_reads_events(
    system: &dyn System<In = (), Out = ()>,
    world: &crate::world::World,
) -> bool {
    for id in system.component_access().reads() {
        if world
            .components()
            .get_name(id)
            .unwrap()
            .starts_with("bevy_ecs::event::Events<")
        {
            return true;
        }
    }
    false
}

struct TestEvent;
fn read_event_system(mut reader: EventReader<TestEvent>) {
    for _ in reader.iter() {}
}

fn write_event_system(mut writer: EventWriter<TestEvent>) {
    writer.send(TestEvent);
}

#[test]
fn verify_system_reads_events() {
    let mut world = World::new();
    let mut reader = IntoSystem::into_system(read_event_system);
    reader.initialize(&mut world);
    let mut writer = IntoSystem::into_system(write_event_system);
    writer.initialize(&mut world);
    assert!(system_reads_events(&reader, &world));
    assert!(!system_reads_events(&writer, &world));
}

@alice-i-cecile
Copy link
Member

Closing in favor of #8453.

@alice-i-cecile alice-i-cecile removed this from the 0.11 milestone Jun 21, 2023
github-merge-queue bot pushed a commit that referenced this pull request Feb 3, 2024
# Objective

Add interactive system debugging capabilities to bevy, providing
step/break/continue style capabilities to running system schedules.

* Original implementation: #8063
    - `ignore_stepping()` everywhere was too much complexity
* Schedule-config & Resource discussion: #8168
    - Decided on selective adding of Schedules & Resource-based control

## Solution
Created `Stepping` Resource. This resource can be used to enable
stepping on a per-schedule basis. Systems within schedules can be
individually configured to:
* AlwaysRun: Ignore any stepping state and run every frame
* NeverRun: Never run while stepping is enabled
    - this allows for disabling of systems while debugging
* Break: If we're running the full frame, stop before this system is run

Stepping provides two modes of execution that reflect traditional
debuggers:
* Step-based: Only execute one system at a time
* Continue/Break: Run all systems, but stop before running a system
marked as Break

### Demo

https://user-images.githubusercontent.com/857742/233630981-99f3bbda-9ca6-4cc4-a00f-171c4946dc47.mov

Breakout has been modified to use Stepping. The game runs normally for a
couple of seconds, then stepping is enabled and the game appears to
pause. A list of Schedules & Systems appears with a cursor at the first
System in the list. The demo then steps forward full frames using the
spacebar until the ball is about to hit a brick. Then we step system by
system as the ball impacts a brick, showing the cursor moving through
the individual systems. Finally the demo switches back to frame stepping
as the ball changes course.


### Limitations
Due to architectural constraints in bevy, there are some cases systems
stepping will not function as a user would expect.

#### Event-driven systems
Stepping does not support systems that are driven by `Event`s as events
are flushed after 1-2 frames. Although game systems are not running
while stepping, ignored systems are still running every frame, so events
will be flushed.

This presents to the user as stepping the event-driven system never
executes the system. It does execute, but the events have already been
flushed.

This can be resolved by changing event handling to use a buffer for
events, and only dropping an event once all readers have read it.

The work-around to allow these systems to properly execute during
stepping is to have them ignore stepping:
`app.add_systems(event_driven_system.ignore_stepping())`. This was done
in the breakout example to ensure sound played even while stepping.

#### Conditional Systems
When a system is stepped, it is given an opportunity to run. If the
conditions of the system say it should not run, it will not.

Similar to Event-driven systems, if a system is conditional, and that
condition is only true for a very small time window, then stepping the
system may not execute the system. This includes depending on any sort
of external clock.

This exhibits to the user as the system not always running when it is
stepped.

A solution to this limitation is to ensure any conditions are consistent
while stepping is enabled. For example, all systems that modify any
state the condition uses should also enable stepping.

#### State-transition Systems
Stepping is configured on the per-`Schedule` level, requiring the user
to have a `ScheduleLabel`.

To support state-transition systems, bevy generates needed schedules
dynamically. Currently it’s very difficult (if not impossible, I haven’t
verified) for the user to get the labels for these schedules.

Without ready access to the dynamically generated schedules, and a
resolution for the `Event` lifetime, **stepping of the state-transition
systems is not supported**

---

## Changelog
- `Schedule::run()` updated to consult `Stepping` Resource to determine
which Systems to run each frame
- Added `Schedule.label` as a `BoxedSystemLabel`, along with supporting
`Schedule::set_label()` and `Schedule::label()` methods
- `Stepping` needed to know which `Schedule` was running, and prior to
this PR, `Schedule` didn't track its own label
- Would have preferred to add `Schedule::with_label()` and remove
`Schedule::new()`, but this PR touches enough already
- Added calls to `Schedule.set_label()` to `App` and `World` as needed
- Added `Stepping` resource
- Added `Stepping::begin_frame()` system to `MainSchedulePlugin`
    - Run before `Main::run_main()`
    - Notifies any `Stepping` Resource a new render frame is starting
    
## Migration Guide
- Add a call to `Schedule::set_label()` for any custom `Schedule`
    - This is only required if the `Schedule` will be stepped

---------

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

Add interactive system debugging capabilities to bevy, providing
step/break/continue style capabilities to running system schedules.

* Original implementation: bevyengine#8063
    - `ignore_stepping()` everywhere was too much complexity
* Schedule-config & Resource discussion: bevyengine#8168
    - Decided on selective adding of Schedules & Resource-based control

## Solution
Created `Stepping` Resource. This resource can be used to enable
stepping on a per-schedule basis. Systems within schedules can be
individually configured to:
* AlwaysRun: Ignore any stepping state and run every frame
* NeverRun: Never run while stepping is enabled
    - this allows for disabling of systems while debugging
* Break: If we're running the full frame, stop before this system is run

Stepping provides two modes of execution that reflect traditional
debuggers:
* Step-based: Only execute one system at a time
* Continue/Break: Run all systems, but stop before running a system
marked as Break

### Demo

https://user-images.githubusercontent.com/857742/233630981-99f3bbda-9ca6-4cc4-a00f-171c4946dc47.mov

Breakout has been modified to use Stepping. The game runs normally for a
couple of seconds, then stepping is enabled and the game appears to
pause. A list of Schedules & Systems appears with a cursor at the first
System in the list. The demo then steps forward full frames using the
spacebar until the ball is about to hit a brick. Then we step system by
system as the ball impacts a brick, showing the cursor moving through
the individual systems. Finally the demo switches back to frame stepping
as the ball changes course.


### Limitations
Due to architectural constraints in bevy, there are some cases systems
stepping will not function as a user would expect.

#### Event-driven systems
Stepping does not support systems that are driven by `Event`s as events
are flushed after 1-2 frames. Although game systems are not running
while stepping, ignored systems are still running every frame, so events
will be flushed.

This presents to the user as stepping the event-driven system never
executes the system. It does execute, but the events have already been
flushed.

This can be resolved by changing event handling to use a buffer for
events, and only dropping an event once all readers have read it.

The work-around to allow these systems to properly execute during
stepping is to have them ignore stepping:
`app.add_systems(event_driven_system.ignore_stepping())`. This was done
in the breakout example to ensure sound played even while stepping.

#### Conditional Systems
When a system is stepped, it is given an opportunity to run. If the
conditions of the system say it should not run, it will not.

Similar to Event-driven systems, if a system is conditional, and that
condition is only true for a very small time window, then stepping the
system may not execute the system. This includes depending on any sort
of external clock.

This exhibits to the user as the system not always running when it is
stepped.

A solution to this limitation is to ensure any conditions are consistent
while stepping is enabled. For example, all systems that modify any
state the condition uses should also enable stepping.

#### State-transition Systems
Stepping is configured on the per-`Schedule` level, requiring the user
to have a `ScheduleLabel`.

To support state-transition systems, bevy generates needed schedules
dynamically. Currently it’s very difficult (if not impossible, I haven’t
verified) for the user to get the labels for these schedules.

Without ready access to the dynamically generated schedules, and a
resolution for the `Event` lifetime, **stepping of the state-transition
systems is not supported**

---

## Changelog
- `Schedule::run()` updated to consult `Stepping` Resource to determine
which Systems to run each frame
- Added `Schedule.label` as a `BoxedSystemLabel`, along with supporting
`Schedule::set_label()` and `Schedule::label()` methods
- `Stepping` needed to know which `Schedule` was running, and prior to
this PR, `Schedule` didn't track its own label
- Would have preferred to add `Schedule::with_label()` and remove
`Schedule::new()`, but this PR touches enough already
- Added calls to `Schedule.set_label()` to `App` and `World` as needed
- Added `Stepping` resource
- Added `Stepping::begin_frame()` system to `MainSchedulePlugin`
    - Run before `Main::run_main()`
    - Notifies any `Stepping` Resource a new render frame is starting
    
## Migration Guide
- Add a call to `Schedule::set_label()` for any custom `Schedule`
    - This is only required if the `Schedule` will be stepped

---------

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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 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

Successfully merging this pull request may close these issues.

5 participants