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

[Windows] Disable G-SYNC in windowed mode #93737

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

aitorciki
Copy link
Contributor

@aitorciki aitorciki commented Jun 29, 2024

In response to #93155

G-SYNC (NVIDIA's VRR) is known to be buggy on windowed mode in Windows. While the driver only enables G-SYNC for full screen mode by default, users can toggle it on for windowed mode too, resulting in unstable refresh rates during Editor usage.

This patch extends Godot's NVIDIA profile to force the default full screen mode only G-SYNC with Godot.

Production edit:

G-SYNC (NVIDIA's VRR) is known to be buggy on windowed mode in Windows.
While the driver only enables G-SYNC for full screen mode by default,
users can toggle it on for windowed mode too, resulting in unstable
refresh rates during Editor usage.
This patch extends Godot's NVIDIA profile to force the default full
screen mode only G-SYNC with Godot.
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I am affected by the VRR problem on GSync, can test.

If this is equivalent to the Nvidia profile to disable GSync it would be good to me.

Hardware:
https://www.dell.com/en-ca/shop/alienware-34-curved-qd-oled-gaming-monitor-aw3423dw/apd/210-bcye/monitors-monitor-accessories

@aitorciki
Copy link
Contributor Author

@fire this modifies the NVIDIA profile to only enable G-SYNC in full screen mode (if G-SYNC is enabled globally), not to always disable it. Since the editor runs in windowed mode it won't trigger G-SYNC, but games entering full screen mode will still use G-SYNC if it's enabled in the NVIDIA Control Panel.

Worth noting: this affects all Godot-based applications, not only the editor. Games won't be able to enter G-SYNC mode while windowed after this change. Given windowed G-SYNC is known to misbehave in Windows I'd say this is probably a sensible default, but I wanted to clarify this before moving forward.

@ckaiser
Copy link
Contributor

ckaiser commented Jun 29, 2024

If this is going to affect non-editor builds there should be an option to disable it, at least at compile time IMO

@Calinou
Copy link
Member

Calinou commented Jun 29, 2024

If this is going to affect non-editor builds there should be an option to disable it, at least at compile time IMO

This should be toggleable with a project setting if possible, but I don't know if this can be done given how the NVIDIA app profile is created. Is it shared among all Godot processes, or is it split into its own profile when you export a project? (I'd prefer that to happen if possible.)

@aitorciki
Copy link
Contributor Author

NVIDIA profiles are identified by their name, and executables (not full paths, just the file name) are linked to them. In Godot the project name (application/config/name) is used as that identifier, and the file name of the executable launching the Godot application is linked to that name.

For example, in the case of the editor, Godot Engine is the profile name, and in the current 4.3-beta2 Godot_v4.3-beta2_win64.exe is linked to that profile. It's important to note many executables will use the same profile if the project name is shared. In the editor case, any Godot editor version is linked to the shared Godot Engine profile.

If Godot Engine is a default project name when none is given (I'm not sure about that, should double-check), I'd propose using Godot Editor as the profile name for the editor (probably using something like (Engine::get_singleton()->is_editor_hint()?) to be extra sure the editor will run its own profile.

On having a per-project toggle, I've never added new settings to Godot, but can look into that. What are we looking at exposing here?

  • Something like display/window/vsync/gsync_mode (adding it under v-sync as a related topic, not sure that's the right category though) that can take off, fullscreen or fullscreen and windowed values and defaulting to fullscreen mimicking the NVIDIA setting?
  • Or just a true/false toggle for a gsync_allow_windowed setting?

@Calinou
Copy link
Member

Calinou commented Jun 30, 2024

  • Or just a true/false toggle for a gsync_allow_windowed setting?

I think this would be preferable, considering I can't see any use case for disabling G-Sync in both fullscreen and windowed. NVIDIA has defined a few profile overrides for this, but they all seem to be related to engine-specific issues, not quirks that are specific to a project. The setting should default to true, as it should generally only be set to false for non-game applications.

@aitorciki
Copy link
Contributor Author

aitorciki commented Jun 30, 2024

Things can never be too easy 😅

Checking if exported games with no project name default to Godot Engine, I've realized the NVIDIA profile is created inside the GL manager for Windows, thus not being created for applications using other renderers. This makes sense since the original goal of the profile was to disable OpenGL threaded optimizations.

I didn't figure this out earlier because the editor is always covered: the project manager always renders using OpenGL, thus always creates the profile. And while the editor itself can be using a different renderer, its binary name has already been linked to the profile by the project manager.

Since VRR applies to all renderers, the NVIDIA profile creation should be moved elsewhere, maybe into the Windows display server which is the immediate predecessor to the GL manager?

Also important to note is that once a NVIDIA profile is linked to a binary, to the NVIDIA driver this is now a game and will be targeted by the GeForce overlay if it's enabled. This is alright for games, but can result in undesired overlays contaminating application UIs (#85111 is an example on how it's impacting the editor).

There is no officially supported way to signal to the NVIDIA driver an application is not a game and shouldn't be targeted by overlays. There is an unsupported setting, but it requires admin privileges to be added to the profile. Some tools (e.g. Unity, Unreal) come with that setting pre-applied by the NVIDIA driver, but even if we wanted to follow that path it wouldn't be easy: Godot's binary doesn't have a fixed name (e.g. godot.exe) as version numbers are included in the official distributed binaries, making them unfit for pre-defined targeting at the driver level.

In short, if we want to offer the toggle for windowed VRR, we need to move NVIDIA profile creation to a spot where it applies to all renderers, and by doing so we are exposing more applications to the potential "overlay invasion" bug. How do we want to proceed?

@Calinou
Copy link
Member

Calinou commented Jun 30, 2024

Godot's binary doesn't have a fixed name (e.g. godot.exe) as version numbers are included in the official distributed binaries, making them unfit for pre-defined targeting at the driver level.

Maybe we should start doing that? Most people only have a single executable installed or use a version manager like Godots, which could put each version in its own folder. (In fact, you must do that for C# builds.)

@aitorciki
Copy link
Contributor Author

As I was discussing in #85111 (comment), there might be a simpler path forward after all.

NVIDIA has started distributing its new beta NVIDIA App. This new app aims at replacing GeForce Experience and NVIDIA Control Panel with a single tool. What makes this very interesting for Godot is how the overlay indicators are implemented: unlike GeForce Experience that adds the indicators to every window (in Godot's case, that is every menu, tooltip, etc), NVIDIA App shows a single indicator for the whole screen, solving our issue without any change on Godot's side.

NVIDIA App will replace GeForce Experience when it leaves beta, and they will coexist until then. Since the app is already available in beta, we can maybe document its usage as the preferred workaround for now?

@Calinou
Copy link
Member

Calinou commented Jun 30, 2024

NVIDIA App will replace GeForce Experience when it leaves beta, and they will coexist until then. Since the app is already available in beta, we can maybe document its usage as the preferred workaround for now?

Sounds good to me. I've already been maining the NVIDIA app since it debuted beta and it works very well for me 🙂

@aitorciki
Copy link
Contributor Author

With NVIDIA App as the proposed workaround, should I go ahead and work on enabling the profile for all renderers to solve the VRR issue?

An alternative is to only disable windowed VRR for the editor for now (creating a separate profile for the editor, which I think makes sense in any case), and revisit extending to any application when NVIDIA App has become the default and we are confident we aren't triggering the indicators-everywhere issue for non-game apps.

@Calinou
Copy link
Member

Calinou commented Jul 2, 2024

With NVIDIA App as the proposed workaround, should I go ahead and work on enabling the profile for all renderers to solve the VRR issue?

Would this disable windowed VRR in all Godot projects, including exported ones? I'd prefer we avoid doing that. Windowed VRR is usually fine for games that always redraw.

@aitorciki
Copy link
Contributor Author

Would this disable windowed VRR in all Godot projects, including exported ones?

I propose starting only with the editor to reduce risk:

  • Create a dedicated NVIDIA profile (e.g. Godot Editor, will require doing some profile cleanup since a binary can't be added to multiple profiles) and only disable windowed VRR on that profile first. Having a dedicated profile will prove useful in the future, as we keep finding situations where we want to rollout settings specific to the editor without affecting all exported apps.
  • In a separate later step we can move NVIDIA profile creation to the display server initialization to apply to all renderers + make the VRR setting togglable.

Thoughts on this approach?

@Calinou
Copy link
Member

Calinou commented Jul 2, 2024

Thoughts on this approach?

Seems good to me, although it might be better to wait for others' opinion as well.

@akien-mga
Copy link
Member

Thoughts on this approach?

Seems good to me, although it might be better to wait for others' opinion as well.

Sounds good to me too.

@Calinou
Copy link
Member

Calinou commented Jul 18, 2024

  • Create a dedicated NVIDIA profile (e.g. Godot Editor, will require doing some profile cleanup since a binary can't be added to multiple profiles)

Is this really needed? Exported projects with no name use Unnamed Project as a title, so they shouldn't reuse the existing Godot Engine profile either way.

With this PR in its current state, the only quirk is that projects run from the editor will no longer have VRR enabled when windowed, but I don't think this is a dealbreaker. Exported projects that use a RenderingDevice-based rendering method will also not define the profile at all (since the OpenGL context manager will never be called), but this is usually fine as most exported projects that use RenderingDevice-based rendering methods redraw continuously anyway.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally on Windows 11 23H2 and NVIDIA 555.85, it works as expected.

image

Using my LG C2's VRR display information (press the green button 7 times on the remote). G-Sync is enabled system-wide on both videos, on both fullscreen and windowed apps. The profile successfully prevents Godot from using VRR in windowed mode, leading to smoother mouse cursor movement:

Before

before_vrr.mp4

After

after_no_vrr.mp4

This may be worth considering for a 4.2 cherrypick, although there's some risk of unforeseen regressions. We might want to wait for 4.3 to be released to get some feedback before cherry-picking this.

@akien-mga akien-mga merged commit 18da250 into godotengine:master Jul 19, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@aitorciki
Copy link
Contributor Author

Is this really needed? Exported projects with no name use Unnamed Project as a title, so they shouldn't reuse the existing Godot Engine profile either way.

Are we sure that is the case? Based on

// We need a name anyways, so let's use the engine name if an application name is not available
// (this is used mostly by the Project Manager)
if (app_profile_name.is_empty()) {
app_profile_name = VERSION_NAME;
}
I thought all projects not having a name would default to Godot Engine, which would bundle them with the editor for Nvidia profile purposes.

I can't test it until later this weekend (away from any Windows + Nvidia machine).

Also, just out of curiosity: why doesn't the editor set a project name (I guess Godot Editor)? That should simplify targeting it specifically in the future without extra logic in the profiles management.

@Calinou
Copy link
Member

Calinou commented Jul 19, 2024

Are we sure that is the case? Based on

I see. We have a lot of logic scattered around in various places as a fallback for projects with an empty name string. Some places use [unnamed project], others use Unnamed Project or Untitled Project. We should figure out a way to harmonize this. For the NVIDIA profile, I'd use Unnamed Godot Engine Project probably.

Also, just out of curiosity: why doesn't the editor set a project name (I guess Godot Editor)? That should simplify targeting it specifically in the future without extra logic in the profiles management.

I don't remember why exactly, but it makes sense to set its name to Godot Editor directly.

@aitorciki
Copy link
Contributor Author

I've finally been able to test what NVIDIA profile name is used for unnamed projects.

I've exported an anonymous Compatibility project (since only OpenGL projects are hitting the NVIDIA profile code path today), and as I expected the NVIDIA profile is created as Godot Engine. That means with this PR merged, any unnamed OpenGL project will apply the disable-VRR-in-windowed-mode fix.

I'm not sure how big a blast radius that might be, but it's definitely worth considering in case we want to roll the change back before the next release.

@aitorciki aitorciki deleted the disable-windowed-gsync branch July 21, 2024 17:00
@patwork
Copy link
Contributor

patwork commented Jul 24, 2024

@aitorciki Godot stopped compiling in dev_build=true dev_mode=true after this commit:

platform/windows/gl_manager_windows_native.cpp: In member function 'void GLManagerNative_Windows::_nvapi_setup_profile()':
platform/windows/gl_manager_windows_native.cpp:246:56: error: missing initializer for member '_NVDRS_SETTING_V1::settingName' [-Werror=missing-field-initializers]
  246 |         NVDRS_SETTING ogl_thread_control_setting = { 0 };
      |                                                        ^
platform/windows/gl_manager_windows_native.cpp:246:56: error: missing initializer for member '_NVDRS_SETTING_V1::settingId' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:246:56: error: missing initializer for member '_NVDRS_SETTING_V1::settingType' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:246:56: error: missing initializer for member '_NVDRS_SETTING_V1::settingLocation' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:246:56: error: missing initializer for member '_NVDRS_SETTING_V1::isCurrentPredefined' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:246:56: error: missing initializer for member '_NVDRS_SETTING_V1::isPredefinedValid' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:246:56: error: missing initializer for member '_NVDRS_SETTING_V1::<anonymous>' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:246:56: error: missing initializer for member '_NVDRS_SETTING_V1::<anonymous>' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:262:46: error: missing initializer for member '_NVDRS_SETTING_V1::settingName' [-Werror=missing-field-initializers]
  262 |         NVDRS_SETTING vrr_mode_setting = { 0 };
      |                                              ^
platform/windows/gl_manager_windows_native.cpp:262:46: error: missing initializer for member '_NVDRS_SETTING_V1::settingId' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:262:46: error: missing initializer for member '_NVDRS_SETTING_V1::settingType' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:262:46: error: missing initializer for member '_NVDRS_SETTING_V1::settingLocation' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:262:46: error: missing initializer for member '_NVDRS_SETTING_V1::isCurrentPredefined' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:262:46: error: missing initializer for member '_NVDRS_SETTING_V1::isPredefinedValid' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:262:46: error: missing initializer for member '_NVDRS_SETTING_V1::<anonymous>' [-Werror=missing-field-initializers]
platform/windows/gl_manager_windows_native.cpp:262:46: error: missing initializer for member '_NVDRS_SETTING_V1::<anonymous>' [-Werror=missing-field-initializers]
cc1plus: all warnings being treated as errors
scons: *** [platform/windows/gl_manager_windows_native.windows.editor.dev.x86_64.o] Error 1

@aitorciki
Copy link
Contributor Author

@patwork I can reproduce the issue.

Removing the initialization results in a correct compilation, I'll create a PR tomorrow to fix it after checking it's not affecting the NVIDIA functionality.

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