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

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

Closed
wants to merge 14 commits into from

Conversation

hymm
Copy link
Contributor

@hymm hymm commented May 14, 2022

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 Res<Time> is unreliable / jittery #4669
  • Supercedes move time update to after everything #4728 and update time in render stage #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 move time update to after everything #4728 (comment)

Statistics

image


Changelog

  • Make frame time reporting more accurate.

Migration Guide

time.delta() now reports zero for 2 frames on startup instead of 1 frame.

@hymm
Copy link
Contributor Author

hymm commented May 14, 2022

please ignore that my stats have a higher standard deviation than in #4735. I ran the other branches too and they also had a higher standard deviation. Seems to just be weirdness of testing on different days.

@superdump
Copy link
Contributor

We’ve been discussing ways of feeding back data from the render world to the main world, especially when pipelining is fully working, and keeping the latency low as rendering of frame n will happen in parallel with simulation of frame n+1.

I proposed use of channels for this where sometimes you just want to get the value back at some point, asynchronously, and other times you want to make simulation of the next frame wait on the current frame so you could have a system that blocks on receiving something over a channel. The latter serialises the pipelining again somewhat, but maybe some use cases require it.

For this case, not blocking is the right choice as time measurement is such a fundamental piece, it happens after present, and if we blocked then it would basically make pipelining useless.

I don’t remember how Unity solved time measurement with as pipelined renderer, I need to reread the article, but the odd thing in my mind is that if we use the presentation time for a previous frame, what does that tell us about the difference in presentation times for the previous and current frame? We’re assuming that the frame rate will be consistent. Maybe that’s the best we can do?

And when we have pipelining, the time in the channel would likely be a measurement from two frames ago because we’re simulating n+1, n is being rendered and hasn’t been presented yet, and n-1 has been presented. What impact fires that have?

I think this PR is probably a great step, just noting some thoughts on the end goal, and maybe they’ve already been discussed…

@superdump superdump requested a review from cart May 14, 2022 06:14
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Core Common functionality for all bevy apps labels May 14, 2022
@hymm
Copy link
Contributor Author

hymm commented May 14, 2022

We’re assuming that the frame rate will be consistent. Maybe that’s the best we can do?

I think this is right. The ideal would be to say that the current frame is going to be presented in x milliseconds and use that time, but needs new apis to do. KhronosGroup/Vulkan-Docs#1364 is this api. We could potentially experiment with android as there is an api available on some devices that that new api is based on. https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VK_GOOGLE_display_timing.html

We can also potentially get the actual presentation time from the graphics card, but this is only currently available on windows.
https://docs.microsoft.com/en-gb/windows/win32/api/dcomp/nf-dcomp-idcompositiondevice-getframestatistics

@inodentry
Copy link
Contributor

While this is indeed a significant and welcome improvement to Bevy's timing consistency, I don't think issue #4669 should be closed by it. The problem is not "resolved" and there is still room for improvement.

crates/bevy_core/Cargo.toml Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/Cargo.toml Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
@hymm hymm force-pushed the time-through-channel branch 2 times, most recently from 09c0335 to f008662 Compare May 26, 2022 18:05
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I can't comment on the impact of the change, but the code looks correct to me.

@hymm hymm force-pushed the time-through-channel branch 2 times, most recently from a59e2b0 to f8d8fa9 Compare June 6, 2022 16:33
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 6, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Some documentation nits, but I'm in favor of this approach. The arguments are solid, and the improvements look good.

@ciwolsey
Copy link

I used to experience a lot of stutters and timing issues but since I've been using this things have been much smoother. It's all very subjective (I have no statistics to back this up) but I'm eager to see this merged. Great work.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 21, 2022
@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Jun 21, 2022
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Mostly looks ok and so I only have minor requests below.

I'm still not convinced by using just a sampled point in time as the reference time for simulating and rendering the next frame which will be presented at a later point in time, but that is not a regression introduced by this PR and this PR brings other significant benefits regarding consistency.

crates/bevy_time/Cargo.toml Outdated Show resolved Hide resolved
crates/bevy_time/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_time/src/lib.rs Outdated Show resolved Hide resolved
hymm and others added 3 commits July 6, 2022 12:46
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'm sure this will change in the future as we add pipelining and framepacing, but thats a conversation for another day. Great work!

@cart
Copy link
Member

cart commented Jul 11, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jul 11, 2022
# 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.
@bors bors bot changed the title Update time by sending frame instant through a channel [Merged by Bors] - Update time by sending frame instant through a channel Jul 11, 2022
@bors bors bot closed this Jul 11, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# 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.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# 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.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# 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.
@hymm hymm deleted the time-through-channel branch October 5, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

8 participants