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

Keep processing Graphics if there are pending operations #100257

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

darksylinc
Copy link
Contributor

@darksylinc darksylinc commented Dec 10, 2024

Fixes #90017
Fixes #90030
Fixes #98044

This PR makes the following changes:

Force processing of GPU commands for frame_count frames

The variable frames_pending_resources_for_processing is added to track this.

The ticket #98044 suggested to use _flush_and_stall_for_all_frames() while minimized.

Technically this works and is a viable solution.

However I noticed that this issue was happening because Logic/Physics continue to work "business as usual" while minimized(*). Only Graphics was being deactivated (which caused commands to accumulate until window is restored).

To continue this behavior of "business as usual", I decided that GPU work should also "continue as usual" by buffering commands in a double or triple buffer scheme until all commands are done processing (if they ever stop coming). This is specially important if the app specifically intends to keep processing while minimized.

Calling _flush_and_stall_for_all_frames() would fix the leak, but it would make Godot's behavior different while minimized vs while the window is presenting.

(*) OS::add_frame_delay does consider being minimized, but it just throttles CPU usage. Some platforms such as Android completely disable processing because the higher level code stops being called when the app goes into background. But this seems like an implementation-detail that diverges from the rest of the platforms (e.g. Windows, Linux & macOS continue to process while minimized).

Rename p_swap_buffers for p_present

This is potentially a breaking change (if it actually breaks anything, I ignore. But I strongly suspect it doesn't break anything).

"Swap Buffers" is a concept carried from OpenGL, where a frame is "done" when glSwapBuffers() is called, which basically means "present to the screen".

However it also means that OpenGL internally swaps its internal buffers in a double/triple buffer scheme (in Vulkan, we do that ourselves and is tracked by RenderingDevice::frame).

Modern APIs like Vulkan differentiate between "submitting GPU work" and "presenting".

Before this PR, calling RendererCompositorRD::end_frame(false) would literally do nothing. This is often undesired and the cause of the leak. After this PR, calling RendererCompositorRD::end_frame(false) will now process commands, swap our internal buffers in a double/triple buffer scheme but avoid presenting to the screen.

Hence the rename of the variable from p_swap_buffers to p_present (which slightly alters its behavior).
If we want RendererCompositorRD::end_frame(false) to do nothing, then we should not call it at all.

This PR reflects such change: When we're minimized and has_pending_resources_for_processing() returns false, we don't call RendererCompositorRD::end_frame() at all.

But if has_pending_resources_for_processing() returns true, we will call it, but with p_present = false because we're minimized.

There's still the issue that Godot keeps processing work (logic, scripts, physics) while minimized, which we shouldn't do by default. But that's work for follow up PR.

@darksylinc darksylinc requested review from a team as code owners December 10, 2024 22:26
@clayjohn clayjohn added this to the 4.4 milestone Dec 11, 2024
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great!

There are two places where we call draw(false). Both are in editor classes (resource preview and onion skinning). In both cases we are just forcing the rendering server to redraw a viewport without needing to wait for vsync and draw to the main screen. So I don't think either one will regress with this change. In fact, this might be an improvement in both cases as it seems like they wouldn't properly end the frame previously.

Fixes godotengine#90017
Fixes godotengine#90030
Fixes godotengine#98044

This PR makes the following changes:

# Force processing of GPU commands for frame_count frames

The variable `frames_pending_resources_for_processing` is added to track
this.

The ticket godotengine#98044 suggested to use `_flush_and_stall_for_all_frames()`
while minimized.

Technically this works and is a viable solution.

However I noticed that this issue was happening because Logic/Physics
continue to work "business as usual" while minimized(\*). Only Graphics
was being deactivated (which caused commands to accumulate until window
is restored).

To continue this behavior of "business as usual", I decided that GPU
work should also "continue as usual" by buffering commands in a double
or triple buffer scheme until all commands are done processing (if they
ever stop coming). This is specially important if the app specifically
intends to keep processing while minimized.

Calling `_flush_and_stall_for_all_frames()` would fix the leak, but it
would make  Godot's behavior different while minimized vs while the
window is presenting.

\* `OS::add_frame_delay` _does_ consider being minimized, but it just
throttles CPU usage. Some platforms such as Android completely disable
processing because the higher level code stops being called when the app
goes into background. But this seems like an implementation-detail that
diverges from the rest of the platforms (e.g. Windows, Linux & macOS
continue to process while minimized).

# Rename p_swap_buffers for p_present

**This is potentially a breaking change** (if it actually breaks
anything, I ignore. But I strongly suspect it doesn't break anything).

"Swap Buffers" is a concept carried from OpenGL, where a frame is "done"
when `glSwapBuffers()` is called, which basically means "present to the
screen".

However it _also_ means that OpenGL internally swaps its internal
buffers in a double/triple buffer scheme (in Vulkan, we do that
ourselves and is tracked by `RenderingDevice::frame`).

Modern APIs like Vulkan differentiate between "submitting GPU work" and
"presenting".

Before this PR, calling `RendererCompositorRD::end_frame(false)` would
literally do nothing. This is often undesired and the cause of the leak.
After this PR, calling `RendererCompositorRD::end_frame(false)` will now
process commands, swap our internal buffers in a double/triple buffer
scheme **but avoid presenting to the screen**.

Hence the rename of the variable from `p_swap_buffers` to `p_present`
(which slightly alters its behavior).
If we want `RendererCompositorRD::end_frame(false)` to do nothing, then
we should not call it at all.

This PR reflects such change: When we're minimized **_and_**
`has_pending_resources_for_processing()` returns false, we don't call
`RendererCompositorRD::end_frame()` at all.

But if `has_pending_resources_for_processing()` returns true, we will
call it, but with `p_present = false` because we're minimized.

There's still the issue that Godot keeps processing work (logic,
scripts, physics) while minimized, which we shouldn't do by default. But
that's work for follow up PR.
@Repiteo Repiteo merged commit 23afda4 into godotengine:master Dec 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 11, 2024

Thanks!

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