-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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
Added support for vertical syncing via the Windows OS compositor (DWM.) #33414
Conversation
This PR makes this feature an option that will be disabled by default. It can be enabled or disabled from the command-line, a project setting, or from the API. Command-line options:
The command-line options will override the project setting. Project setting:
The default value is New API property and methods:
|
This project... ...can be used to test this feature. Note that the stutter/jitter problem seems to be specific to Nvidia GPUs and only occurs while in windowed mode. Sometimes a game will stutter/jitter constantly and other times it will behave fairly well. On my system (the one affected), the problem can be reproduced by...
This is a Windows 10 system with an Nvidia GeForce GTX 1070 GPU. |
I can't get this to build on MinGW on Fedora 31 for some reason:
I'll use MSVC to provide builds for testing, but I'd like to know how to build it with MinGW still 🙂 |
MinGW have its own, separate library list in In addition to the line 224, |
Here are two Windows 64-bit editor builds compiled with MSVC 2019 and
(Those links will expire around July 2020.) |
Tried the Calinou's PR build, works good for me with the new option enabled (and jitters with it disabled). |
I've attached a couple of screenshots from Microsoft's GpuView utility (available in the Windows ADK.) This is more or less a profiler that focuses on the GPU rather than the CPU. It allows you to see the workflow of GPU-related work from beginning to end. These screenshots are for a Godot game running on two systems and show this feature both disabled and enabled. There are a couple of things of interest in the screenshots. This screenshot is from a system that doesn't seem to be affected by the problem... The first three frames are with the feature turned off. (The blue vertical bars denote the vertical blank period.) The last three frames are with the feature turned on. AC.EXE is the Godot game. The things of interest to note here are...
The 'Monitored Fence' packet is likely being used to synchronize with the compositor. It isn't known why one of the threads would require 100% CPU utilization when the This screenshot is from a system that is affected by this problem... Once again, the first three frames are with the feature turned off and the last three are with the feature turned on. The things to note here are:
|
Ok, i tested that and.... you are the man!!!!. Test (Bad computer, bad card):Windows 7 64 bit, 2 screen fullHD + 1 HUION tablet in a GeForce GTX 660/PCIe Full Screen:
Windowed:
Well. This can be partially the solution to the Godot stutter problem, at least in windows, and that is too important!!!.
Well, CC @akien-mga , sorry to ping, but this is not a new feature and is a very important bug fix to: #19783 (comment) and to #25162 and all the recurrent issues about start using godot and seing the stutter in the first simple sprite movement... I think that this option should be "ON" by default on windows computers, to avoid more issues about the stutter (though can be some issues by windows 7 users about the tearing in full screen). Thanks a lot for this!!!! (I will say that there are other things that can do godot stutter and flicker, but the big problem of stutter in the "empty project" is gone in my case) |
@Ranoller This new option comes into play only when vsync is enabled. From what you describe, it sounds like vsync was disabled when you ran your tests. This would account for the tearing in full screen mode. |
Yes. I´m testing more and see that. A project to test: For the merge valoration: This feature is risk 0 (optional and bug fix oriented), and soo useful in the developer side. I vote for press merge button until it breaks. (I see this commit tagged 4.0, but we don´t know if vulkan will battle with windows the same as gles2-3, so, if this will be merged, maybe should be merged in the 3.x branch, only an idea) |
@Ranoller Note if you disable v-sync, the issue may disappear too. So when testing this, one shouldn't disable v-sync. |
In my demo? There is not force fps, is that: Button -> Godot Vsync (Internal): Button -> Windows Vsync (compositor): Button -> No Vsync: Button -> Toggle Screen Mode: With buttons "Godot Vsync" and "Windows Vsync" you should get 60, 120 fps or whatever report your screen. Difference consists in this commit, vsync with or without compositor, to test the differences (This commit merged is needed) If you dissable vsync the issues disappear, of course, but tearing can happend too, but maybe is good to test all the options, no-vsync included, is valid option for players that configure manually their vertical sincronization, i think. Sorry if terminology (internal, compositor, etc...)in the demo is confusing. I´m not expert in that. If some definition is bad, demo can be changed and re-uploaded. (The demo is a re-do of other demo made with a car sprites to test some years ago that i can´t find now, was for godot 2.1 i think) |
Yes, was confused with the name, since regular monitor v-sync is not a Godot's internal thing. %) |
Just tested @Calinou's build on my small games, and the stuttering is gone! I've been worrying about this for a while and was even thinking of switching engines... (my thinking was if it lags on small tests, why spend time on making a game on godot?) Just a question though, Are there any performance hits with turning this on? (ie should we turn it off when its full screen mode or deployed on android?) |
@keenfoong This setting currently only affects (non-UWP) Windows targets so it won't matter whether it is enabled or not in the case of android or other platforms. On Windows, there won't be any performance hits when running in full screen mode, regardless of whether this setting is enabled or not. When the game is in full screen mode the vertical sync will be done via OpenGL and this feature won't come into play. If you are deploying a Windows game and it runs in windowed mode and you are concerned about performance then you may want to consider giving the end-user an option via the UI to disable this feature. If the end-user's system isn't affected by the problem then the game may perform better with this option disabled. Even if you don't provide a UI option to control this feature then the end-user can still enable or disable it via one of the following command line options when launching the game...
This way if you ship with the feature disabled by default and you get reports about stuttering then you can instruct the affected end-users to enable this feature via |
Sorry I missed this (must have been away that week, and there have been several PRs).. But just to say myself I'm fine to trial this now the command line switch is available. For something like this I was seeing the main risk when first introducing it is whether it gives a worse experience for some users, but making it switchable solves that. 👍 The graphs were interesting! They beg the question, I wonder if there are any power saving settings that affect this (in the GPU or the OS)? e.g. |
@@ -890,6 +890,9 @@ | |||
<member name="vsync_enabled" type="bool" setter="set_use_vsync" getter="is_vsync_enabled" default="true"> | |||
If [code]true[/code], vertical synchronization (Vsync) is enabled. | |||
</member> | |||
<member name="vsync_via_compositor" type="bool" setter="set_vsync_via_compositor" getter="is_vsync_via_compositor_enabled" default="false"> | |||
If [code]true[/code] and [code]vsync_enabled[/code] is true, the operating system's window compositor will be used for vsync when the compositor is enabled and the game is in windowed mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add:
[b]Note:[/b] This property is implemented on Windows.
following what was done in #34087?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do this fixup in a follow-up commit with other unrelated changes.
@@ -221,7 +221,8 @@ def configure_msvc(env, manual_msvc_config): | |||
|
|||
LIBS = ['winmm', 'opengl32', 'dsound', 'kernel32', 'ole32', 'oleaut32', | |||
'user32', 'gdi32', 'IPHLPAPI', 'Shlwapi', 'wsock32', 'Ws2_32', | |||
'shell32', 'advapi32', 'dinput8', 'dxguid', 'imm32', 'bcrypt','Avrt'] | |||
'shell32', 'advapi32', 'dinput8', 'dxguid', 'imm32', 'bcrypt','Avrt', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a space before 'Avrt',
. It's not from your commit but it would be worth fixing nevertheless (maybe also put it on the new line with 'dwmapi'
to keep a consistency line wrapping).
Given the good feedback and the relatively low risk in this feature's implementation, let's merge :) It should provide a good stopgap solution for 3.2, at least until we can investigate further and better understand what is involved exactly. |
Thanks for the great work! :) |
I had missed that the PR had 3 commits, including a merge commit, which makes the Git history quite hard to follow (you can't tell at a glance which of the 3 commits actually implements the patch since they all have the same diff...). I force pushed an update with a squashed commit: e1dda51 |
This is great, following some of the other threads about stuttering this has been plaguing people for many years already. People are gonna be happy to hear that this will fix that problem. =) Thanks for the great work |
Yes, this fix one of the stutter problems... not all of them. But that first impression that some users had from godot in their first sessions, that a simple sprite moving in screen will stutters a lot, was very injurious to the capabilities perception of the engine. So, good news here. |
@Ranoller The rest of the stuttering issues could likely be fixed by adding semi-fixed timestep or fixed step interpolation. |
Why is this not enabled by default? |
I believe the reasoning for that should be in the duplicate PR (the first attempt). |
Presumably because of the risk of it being detrimental on some systems. With a rollout of this type, it makes sense to initially have it as an option. After testing with a wider audience doesn't show negative effects, it is trivial to do another PR to set it as default. Note that game authors can set it as the default for the game they release currently. |
@neikeq We could consider enabling this by default in a 3.2 point release or 4.0. As @lawnjelly pointed out, we need further testing before we can enable it on all systems. |
This feature was added in godotengine#33414 but it was disabled by default. Now that it got some testing, it's probably safe to enable it by default.
@Ranoller @TerminalJack I've found that the stuttering only occurs when vsync is set to "always off" in graphics card settings. With vsync_via_compositor disabled, does the stuttering disappear after making this change? My findings: Effect: Stuttering Effect: Stuttering Effect: Half refresh rate |
Some users are reporting bad jitter when running in windowed mode on the
Windows OS. This change will allow the OS' compositor (DWM) to be used for
vsync. The end-user can enable/disable this feature through a project setting,
command-line option, and/or through the API.
fixes #19783
fixes #27211
Bugsquad edit: See this comment for compiled builds incorporating this pull request.