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

Pass the actual draw delta to the RenderingServer #82222

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Riteo
Copy link
Contributor

@Riteo Riteo commented Sep 24, 2023

Fixes #87963.

When the engine is unable to draw (e.g. a minimized window), it skips the drawing step and keeps ticking at a rate depending on the low CPU usage mode's time and the max fps setting.

Once we can draw again the rendering server will get passed this usually low delta as a frame step instead of the actual delta from the last draw invocation, messing up all time-dependent shaders.

This commit fixes the issue by calculating, when drawing, the ticks passed since the last draw invocation and passing that to the rendering server.

Context

Since Wayland wants the compositor to control the draw loop, #57025 accomplishes this by marking the window as able to get drawn only for small instants, coinciding with the ones we are told to draw.

This means that for most of the time, the window is marked as hidden, so we depend a lot on this "undrawn" ticking.

One user started reporting that their shaders were going weirdly slow and I stumbled on this weird behavior where the drawing loop had the same delta as the process loop despite being effectively uncoupled from it.

This is PoC-ish as I'm not sure what would be the proper way to implement this (for example, should we expose this as a static member akin to last_ticks?) and whether this change to the main loop logic would be acceptable at all.

I've also tested this only on the Wayland PR so if this makes sense in the first place further testing is needed.

@darksylinc
Copy link
Contributor

darksylinc commented Sep 24, 2023

Yeah, having both physics and graphics use the same tickDelta when they're decoupled is wrong.

Small suggestion: Graphics' delta should have a cap. e.g.

double scaled_step = (draw_ticks - last_draw_ticks) / 1000000.0 * time_scale;
scaled_step = MIN( 1.0, scaled_step ); // scaled_step is no larger than 1 second

The reason being is that large deltas can cause multiple issues:

  1. It can cause infs / nan.
  2. It can cause certain things (like particle FXs) to overshoot (think of a water tap that for a very brief moment lets out a burst of water at full pressure splashing everywhere).
  3. It can potentially cause a positive feedback loop (e.g. larger deltas cause a subsystem to process more, which takes more time, thus the next iteration the delta is even bigger, which takes even more CPU time... and so on)

Out of experience I find that 1 second is a good compromise between simulating (rendering-side) a large delta and protecting the engine from undesired behaviors.

If the rendering engine was disabled for more than 1 second then that sounds more like intended behavior (e.g. the app was suspended or minimized) or things went horribly wrong (e.g. CPU/GPU got stuck processing for more than a second) in which case a large uncapped delta will probably make things worse.

@Riteo
Copy link
Contributor Author

Riteo commented Sep 24, 2023

@darksylinc

Small suggestion: Graphics' delta should have a cap. e.g. [snip]

Uh yeah that makes sense. 1 FPS is a pretty unlikely thing to see anyways I suppose.

There, I pushed this change along with a prettier comment.

When the engine is unable to draw (e.g. a minimized window), it skips
the drawing step and keeps ticking at a rate depending on the low CPU
usage mode's time and the max fps setting.

Once we can draw again the rendering server will get passed this usually
low delta as a frame step instead of the actual delta from the last
draw invocation, messing up all time-dependent shaders.

This commit fixes the issue by calculating, when drawing, the ticks
passed since the last draw invocation and passing that to the rendering
server.
@Calinou
Copy link
Member

Calinou commented Sep 26, 2023

Remember to double-check behavior when using --fixed-fps 🙂

Small suggestion: Graphics' delta should have a cap. e.g.

Should the delta cap also apply to very high FPS values (e.g. above 10,000 FPS)? I believe we have some checks in place for this already, but the engine doesn't have any FPS cap by default and there is no hardcoded cap elsewhere.

@Riteo
Copy link
Contributor Author

Riteo commented Sep 26, 2023

You know what, I'm removing the draft status as there seems to be some approval.

Edit: Testing would still be wise.

@Riteo Riteo marked this pull request as ready for review September 26, 2023 08:50
@Riteo Riteo requested a review from a team as a code owner September 26, 2023 08:50
@akien-mga akien-mga requested a review from a team September 26, 2023 09:13
@TuxTheAstronaut
Copy link

TuxTheAstronaut commented Sep 29, 2023

I added this code and then compiled it with the Wayland WIP PR, the issue I reported here with having shader animated effects slowing down or pausing with a uncapped framerate so far appears to be working. My games shader effects no longer randomly slow down or pause. 👍

@LunarTides
Copy link

Is this still required? The linked issue was closed

@Riteo
Copy link
Contributor Author

Riteo commented May 5, 2024

@LunarTides yes. There's still a correctness issue and presumably it might also help with other mysterious issues like #84137. The linked ticket is closed as I found about this quirk while debugging a problem in the Wayland backend, which I fixed differently (I should've probably filed two tickets, sorry).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wayland: Shader TIME is unreliable (slow shaders) due to frame pacing method
6 participants