move time update to after everything #4728

hymm · 2022-05-12T02:44:21Z

Objective

Move time update to end of frame instead of the First stage. First is not a good place to update time because it is actually after the input processing from winit. This leads to time.delta() reporting timings for input processing in frame 2 and vsync waiting time from frame 1. This is bad because a longer input processing section leads to a shorter vsync time. So we can potentially end up with a short input processing time with a short vsync time in one delta time or a long-long combination.
Fixes Res<Time> is unreliable / jittery #4669

Solution

Add a stage that is run after the main schedule and sub apps are run. This makes it so that the input processing and vsync wait for the same frame are paired together. so we end up with short-long pairings between vsync wait and the rest of the processing time.
time.delta() for before and after

Changelog

Make time more consistent
add stage that runs after app and render schedules.

Migration Guide

Reports zero delta time for one more frame than before

Why this might not be the right solution

this fix is not valid once we have pipelined rendering.
a little awkward adding a last last stage for the app world.

alice-i-cecile · 2022-05-12T02:45:26Z

Wow, way smoother!

alice-i-cecile · 2022-05-12T02:46:42Z

crates/bevy_app/src/app.rs

@@ -56,6 +56,8 @@ pub struct App {
    pub runner: Box<dyn Fn(App)>,
    /// A container of [`Stage`]s set to be run in a linear order.
    pub schedule: Schedule,
+    /// stage run after main schedule and sub app schedules
+    pub stage_very_last: SystemStage,


Maybe final, rather than very_last?

I'm down for that. very_last was my 🚲 🏠 name.

alice-i-cecile · 2022-05-12T02:49:08Z

crates/bevy_app/src/app.rs

@@ -326,6 +330,15 @@ impl App {
        self.add_system_to_stage(CoreStage::Update, system)
    }

+    /// adds a system to the very last stage


Suggested change

/// adds a system to the very last stage

/// Adds a system to the very last stage in the [`Schedule`]

///

/// [`Time`] is updated during [`CoreSystem::TIme`] in this stage: try to make sure any system that you add runs before it in the schedule to ensure the smoothest possible frame timing.

alice-i-cecile · 2022-05-12T02:55:07Z

Hard-coding the "final_final_ultimate_last_stage" is a little silly, but I think it's probably the right approach for robustness until #4173 lands. And since that's blocked on #4391, I'd prefer to merge this approach ASAP rather than waiting.

alice-i-cecile · 2022-05-12T03:08:39Z

Can you note that this also closes #4691 in the PR description?

hymm · 2022-05-12T03:29:19Z

Can you note that this also closes #4691 in the PR description?

This is more of a fix for when vsync is on than when vsync is off like in that issue.

cart · 2022-05-12T04:19:20Z

crates/bevy_app/src/app.rs

@@ -115,6 +118,7 @@ impl App {
        for sub_app in self.sub_apps.values_mut() {
            (sub_app.runner)(&mut self.world, &mut sub_app.app);
        }
+        self.stage_very_last.run(&mut self.world);


Maybe we should consider bumping time management as a concern into the "app runner"? It seems like what we really want is "very first" (as in pre-input event processing for a given update), not "very last".

Conceptually "starting the stopwatch at the beginning of a frame" feels easier to reason about and ensures we don't have multiple frames with zero elapsed time.

Although I'm not convinced winit will make that switch easy on us. Processing input after deciding to present seems desirable, but they abstract that out from us.

I'm not sure if we want "very first". Ideally we'd be setting the time as close to when the frame is presented as possible. Which means during the render stage. For example, there might be a future where people are inserting systems after the frame is presented like bevy_framepace is doing. (This positioning doesn't actual work for frame pacing in pipelined rendering. It actually does want to be before input processing. But it's late and I'm having trouble coming up with something that would actually be inserted after frame presentation.) This hypothetical system could cause the same timing issues that we're currently seeing with input processing.

I did consider adding the time update to after app.update in the event handler, but any other runners would need to know to add the call there as well.

When we do have pipelined rendering I suspect that we'll want to run the time update system right after the frame presentation in the render stage and then move the time into the app world during extract. This won't currently work very well because that would cause the time to be for the N-2 frame. But in pipelined rendering it would be for the N-1 frame, where N is the current frame. Ideally ideally we'd be getting the frame time from hardware, but that requires api support that isn't fully supported on all hardware/os's/wgpu yet.

I suppose we can move towards something more like the future with pipelines rendering by adding another extract stage at the end of the rendering schedule. This stage would logically be merged with the other one once we have pipelined rendering.

When we do have pipelined rendering I suspect that we'll want to run the time update system right after the frame presentation in the render stage and then move the time into the app world during extract.

This would also be my suggestion based on my experimenting with bevy_framepace. However, instead of using extract we could try using a globally shared Arc<RwLock>, or use a channel.

However, instead of using extract we could try using a globally shared Arc, or use a channel.

A channel sounds like a good idea. I'll try that.

hymm · 2022-05-14T04:15:39Z

closing in favor of #4744

@aevyrie

# Objective - The time update is currently done in the wrong part of the schedule. For a single frame the current order of things is update input, update time (First stage), other stages, render stage (frame presentation). So when we update the time it includes the input processing of the current frame and the frame presentation of the previous frame. This is a problem when vsync is on. When input processing takes a longer amount of time for a frame, the vsync wait time gets shorter. So when these are not paired correctly we can potentially have a long input processing time added to the normal vsync wait time in the previous frame. This leads to inaccurate frame time reporting and more variance of the time than actually exists. For more details of why this is an issue see the linked issue below. - Helps with #4669 - Supercedes #4728 and #4735. This PR should be less controversial than those because it doesn't add to the API surface. ## Solution - The most accurate frame time would come from hardware. We currently don't have access to that for multiple reasons, so the next best thing we can do is measure the frame time as close to frame presentation as possible. This PR gets the Instant::now() for the time immediately after frame presentation in the render system and then sends that time to the app world through a channel. - implements suggestion from @aevyrie from here #4728 (comment) ## Statistics ![image](https://user-images.githubusercontent.com/2180432/168410265-f249f66e-ea9d-45d1-b3d8-7207a7bc536c.png) --- ## Changelog - Make frame time reporting more accurate. ## Migration Guide `time.delta()` now reports zero for 2 frames on startup instead of 1 frame.

@aevyrie

# Objective - The time update is currently done in the wrong part of the schedule. For a single frame the current order of things is update input, update time (First stage), other stages, render stage (frame presentation). So when we update the time it includes the input processing of the current frame and the frame presentation of the previous frame. This is a problem when vsync is on. When input processing takes a longer amount of time for a frame, the vsync wait time gets shorter. So when these are not paired correctly we can potentially have a long input processing time added to the normal vsync wait time in the previous frame. This leads to inaccurate frame time reporting and more variance of the time than actually exists. For more details of why this is an issue see the linked issue below. - Helps with bevyengine#4669 - Supercedes bevyengine#4728 and bevyengine#4735. This PR should be less controversial than those because it doesn't add to the API surface. ## Solution - The most accurate frame time would come from hardware. We currently don't have access to that for multiple reasons, so the next best thing we can do is measure the frame time as close to frame presentation as possible. This PR gets the Instant::now() for the time immediately after frame presentation in the render system and then sends that time to the app world through a channel. - implements suggestion from @aevyrie from here bevyengine#4728 (comment) ## Statistics ![image](https://user-images.githubusercontent.com/2180432/168410265-f249f66e-ea9d-45d1-b3d8-7207a7bc536c.png) --- ## Changelog - Make frame time reporting more accurate. ## Migration Guide `time.delta()` now reports zero for 2 frames on startup instead of 1 frame.

@aevyrie

# Objective - The time update is currently done in the wrong part of the schedule. For a single frame the current order of things is update input, update time (First stage), other stages, render stage (frame presentation). So when we update the time it includes the input processing of the current frame and the frame presentation of the previous frame. This is a problem when vsync is on. When input processing takes a longer amount of time for a frame, the vsync wait time gets shorter. So when these are not paired correctly we can potentially have a long input processing time added to the normal vsync wait time in the previous frame. This leads to inaccurate frame time reporting and more variance of the time than actually exists. For more details of why this is an issue see the linked issue below. - Helps with bevyengine#4669 - Supercedes bevyengine#4728 and bevyengine#4735. This PR should be less controversial than those because it doesn't add to the API surface. ## Solution - The most accurate frame time would come from hardware. We currently don't have access to that for multiple reasons, so the next best thing we can do is measure the frame time as close to frame presentation as possible. This PR gets the Instant::now() for the time immediately after frame presentation in the render system and then sends that time to the app world through a channel. - implements suggestion from @aevyrie from here bevyengine#4728 (comment) ## Statistics ![image](https://user-images.githubusercontent.com/2180432/168410265-f249f66e-ea9d-45d1-b3d8-7207a7bc536c.png) --- ## Changelog - Make frame time reporting more accurate. ## Migration Guide `time.delta()` now reports zero for 2 frames on startup instead of 1 frame.

@aevyrie

# Objective - The time update is currently done in the wrong part of the schedule. For a single frame the current order of things is update input, update time (First stage), other stages, render stage (frame presentation). So when we update the time it includes the input processing of the current frame and the frame presentation of the previous frame. This is a problem when vsync is on. When input processing takes a longer amount of time for a frame, the vsync wait time gets shorter. So when these are not paired correctly we can potentially have a long input processing time added to the normal vsync wait time in the previous frame. This leads to inaccurate frame time reporting and more variance of the time than actually exists. For more details of why this is an issue see the linked issue below. - Helps with bevyengine#4669 - Supercedes bevyengine#4728 and bevyengine#4735. This PR should be less controversial than those because it doesn't add to the API surface. ## Solution - The most accurate frame time would come from hardware. We currently don't have access to that for multiple reasons, so the next best thing we can do is measure the frame time as close to frame presentation as possible. This PR gets the Instant::now() for the time immediately after frame presentation in the render system and then sends that time to the app world through a channel. - implements suggestion from @aevyrie from here bevyengine#4728 (comment) ## Statistics ![image](https://user-images.githubusercontent.com/2180432/168410265-f249f66e-ea9d-45d1-b3d8-7207a7bc536c.png) --- ## Changelog - Make frame time reporting more accurate. ## Migration Guide `time.delta()` now reports zero for 2 frames on startup instead of 1 frame.

alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Core Common functionality for all bevy apps labels May 12, 2022

alice-i-cecile reviewed May 12, 2022

View reviewed changes

alice-i-cecile mentioned this pull request May 12, 2022

As soon as possible and as late as possible system ordering #4173

Closed

alice-i-cecile requested a review from aevyrie May 12, 2022 03:00

alice-i-cecile mentioned this pull request May 12, 2022

Res<Time> is unreliable / jittery #4669

Open

cart reviewed May 12, 2022

View reviewed changes

move time update to after render schedule

2166c90

hymm force-pushed the time-at-end branch from 1d12cd5 to 2166c90 Compare May 12, 2022 04:34

alice-i-cecile added C-Bug An unexpected or incorrect behavior X-Controversial There is active debate or serious implications around merging this PR and removed C-Usability A targeted quality-of-life change that makes Bevy easier to use labels May 12, 2022

This was referenced May 12, 2022

update time in render stage #4735

Closed

[Merged by Bors] - Update time by sending frame instant through a channel #4744

Closed

hymm closed this May 14, 2022

hymm deleted the time-at-end branch October 5, 2023 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move time update to after everything #4728

move time update to after everything #4728

hymm commented May 12, 2022 •

edited

Loading

alice-i-cecile commented May 12, 2022

alice-i-cecile May 12, 2022

hymm May 12, 2022

alice-i-cecile May 12, 2022 •

edited

Loading

alice-i-cecile commented May 12, 2022

alice-i-cecile commented May 12, 2022

hymm commented May 12, 2022

cart May 12, 2022

hymm May 12, 2022

hymm May 12, 2022

aevyrie May 13, 2022

hymm May 13, 2022

hymm commented May 14, 2022

-    /// adds a system to the very last stage
+    /// Adds a system to the very last stage in the [`Schedule`]
+    ///
+    /// [`Time`] is updated during [`CoreSystem::TIme`] in this stage: try to make sure any system that you add runs before it in the schedule to ensure the smoothest possible frame timing.

move time update to after everything #4728

move time update to after everything #4728

Conversation

hymm commented May 12, 2022 • edited Loading

Objective

Solution

Changelog

Migration Guide

Why this might not be the right solution

alice-i-cecile commented May 12, 2022

alice-i-cecile May 12, 2022

Choose a reason for hiding this comment

hymm May 12, 2022

Choose a reason for hiding this comment

alice-i-cecile May 12, 2022 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile commented May 12, 2022

alice-i-cecile commented May 12, 2022

hymm commented May 12, 2022

cart May 12, 2022

Choose a reason for hiding this comment

hymm May 12, 2022

Choose a reason for hiding this comment

hymm May 12, 2022

Choose a reason for hiding this comment

aevyrie May 13, 2022

Choose a reason for hiding this comment

hymm May 13, 2022

Choose a reason for hiding this comment

hymm commented May 14, 2022

hymm commented May 12, 2022 •

edited

Loading

alice-i-cecile May 12, 2022 •

edited

Loading