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

Use Windows compositor for vsync when appropriate. #33127

Closed
wants to merge 3 commits into from
Closed

Use Windows compositor for vsync when appropriate. #33127

wants to merge 3 commits into from

Conversation

TerminalJack
Copy link
Contributor

Some users are reporting bad jitter when running in windowed mode on the
Windows OS. This change will cause the OS's compositor to be used for
vsync when it is appropriate. This is a strategy that is used by other
projects such as Chromium and glfw.

fixes #19783
fixes #27211

@clayjohn
Copy link
Member

You'll also want to squash your three commits together. :)

If you don't know how, there is a great reference in the docs http://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#mastering-the-pr-workflow-the-rebase

@akien-mga akien-mga added this to the 3.2 milestone Oct 28, 2019
@akien-mga akien-mga requested a review from reduz October 28, 2019 07:14
@akien-mga
Copy link
Member

akien-mga commented Oct 28, 2019

To test this PR, I reworked the test project from #19783 (comment) to allow toggling various export variables on the main script to control things which can influence stuttering.

StutterTest.zip

image

You can also toggle Fullscreen with the Return key.

To avoid interference from the remote debugger, I suggest running the project directly (not from the editor). Also best tested with a target=release template or at least target=release_debug editor build.

@akien-mga
Copy link
Member

akien-mga commented Oct 28, 2019

TL;DR: No impact from this PR on my system that I can notice.
Still get no stutter on Intel, and stutter at irregular intervals on Radeon.

I ran some tests on my system using the above project. So far I don't see much difference before/after the PR, but I'm probably one of the users who is not too affected by the stutter issue (which seems mostly to affect NVidia users).

Windows 10 Build 18362, laptop HP Spectre x360.
4K display but configured as 1080p, 125% scaling including Windows' "Fix scaling for apps" option.
Monitor refresh rate: unsure, Windows says 59 Hz in one settings window and 60 Hz in the more advanced one shrug.

Builds of commit 5014baf, respectiely without (Before PR) and with (After PR) the current https://github.com/godotengine/godot/pull/33127.patch
scons p=windows tools=yes target=release_debug

Applications running: explorer.exe, Firefox Beta, git-bash.exe, Notepad++

Intel HD Graphics 630, drivers 26.20.100.6912.

Before PR:

  • VSync ON
    • Process
      • Windowed: No stutter
      • Fullscreen: No stutter
    • Physics Process
      • Windowed: No stutter
      • Fullscreen: No stutter
  • VSync OFF
    • Process
      • Windowed: No stutter
      • Fullscreen: No stutter
    • Physics Process
      • Windowed: No stutter
      • Fullscreen: No stutter

After PR:

  • VSync ON
    • Process
      • Windowed: No stutter
      • Fullscreen: No stutter
    • Physics Process
      • Windowed: No stutter
      • Fullscreen: No stutter
  • VSync OFF
    • Process
      • Windowed: No stutter
      • Fullscreen: No stutter
    • Physics Process
      • Windowed: No stutter
      • Fullscreen: No stutter

Radeon RX Vega M GL, drivers 25.20.15002.58.

Before PR:

  • VSync ON
    • Process
      • Windowed: Occasional stutter (irregular, every 3-15 sec)
      • Fullscreen: Occasional stutter (irregular, every 3-15 sec)
    • Physics Process
      • Windowed: Occasional stutter (irregular, every 3-15 sec)
      • Fullscreen: Occasional stutter (irregular, every 3-15 sec)
  • VSync OFF
    • Process
      • Windowed: Occasional micro stutter
      • Fullscreen: Occasional micro stutter
    • Physics Process
      • Windowed: Occasional stutter (irregular, every 3-15 sec)
      • Fullscreen: Occasional stutter (irregular, every 3-15 sec)

After PR:

  • VSync ON
    • Process
      • Windowed: Occasional stutter (irregular, every 3-15 sec)
      • Fullscreen: Occasional stutter (irregular, every 3-15 sec)
    • Physics Process
      • Windowed: Occasional stutter (irregular, every 3-15 sec)
      • Fullscreen: Occasional stutter (irregular, every 3-15 sec)
  • VSync OFF
    • Process
      • Windowed: Occasional micro stutter
      • Fullscreen: Occasional micro stutter
    • Physics Process
      • Windowed: Occasional stutter (irregular, every 3-15 sec)
      • Fullscreen: Occasional stutter (irregular, every 3-15 sec)

@starry-abyss
Copy link
Contributor

starry-abyss commented Oct 28, 2019

To avoid interference from the remote debugger, I suggest running the project directly (not from the editor)

Note that the compositor issue is more likely to happen when there are other windows on the screen with activity in them (e.g. the editor).

@@ -33,6 +33,9 @@
// Author: Juan Linietsky <reduzio@gmail.com>, (C) 2008

#include "context_gl_windows.h"
#include <dwmapi.h>

#pragma comment(lib, "dwmapi.lib")
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? If you want to link dwmapi.lib, it should be added to the LIBS array in platform/windows/detect.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been fixed.

@@ -33,6 +33,9 @@
// Author: Juan Linietsky <reduzio@gmail.com>, (C) 2008

#include "context_gl_windows.h"
#include <dwmapi.h>
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been fixed.


if (OS::get_singleton()->is_window_fullscreen()) {
return false;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

No need for the else and extra indentation since we're just returning in the previous branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been fixed.

SwapBuffers(hDC);
}

void ContextGL_Windows::set_use_vsync(bool p_use) {

if (p_use && should_vsync_via_compositor()) {
swap_interval = 0; // vsync will be done via DwmFlush().
Copy link
Member

@akien-mga akien-mga Oct 28, 2019

Choose a reason for hiding this comment

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

It seems hacky to rely on the swap interval to define whether vsync should be handled by the compositor. I'd use a boolean like vsync_via_compositor to make things explicit. When swapping buffers you can then check if the value for the cached bool would change, and do logic appropriately.

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 reworked it as you suggested.

@TerminalJack
Copy link
Contributor Author

I screwed things up during a rebase so I'm going to close this PR and try submitting it again later today.

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.

Very bad FPS stability. Heavy stuttering issue in simple 2D game [Windows 10, Nvidia]
5 participants