Restructure and reimplement v-sync options #48622

Geometror · 2021-05-10T19:17:08Z

This PR is a complete reorginization of the v-sync related code for DisplayServers with the changes discussed here and on IRC. So it's no longer a reimplementation of PR #36862, altough @giarve's work was very helpful.

Adds a v-sync mode setting which allows to choose between DISABLED, ON, ADAPTIVE and MAILBOX (fixes Vulkan: disabling V-Sync not reimplemented yet #36467, fixes Add support for adaptive V-Sync godot-proposals#1283)
-> can be modified via project settings or via code at runtime [for Windows, Linux and OSX]
-> fallback to VK_PRESENT_MODE_FIFO_KHR if the desired mode is not supported
-> No engine restart is required for a change of the vsync-mode setting
Removed the v-sync via compositor option as it was problematic (as suggested by @Calinou)

Calinou · 2021-05-10T19:35:08Z

I can confirm it works on Linux + NVIDIA:

Testing project: test_vsync_disable.zip

Geometror · 2021-05-23T15:43:20Z

Rebased and partly restructured,
vsync setting is now read in Main::setup2 + removed dead code in OS

What is the plan for vsync_via_compositor in 4.0? The code regarding vsync_via_compositor is currently dead in master.

Calinou · 2021-05-23T16:11:39Z

What is the plan for vsync_via_compositor in 4.0? The code regarding vsync_via_compositor is currently dead in master.

I think it would be better to remove it, as it doesn't work correctly for everyone. It causes the framerate to halve unexpectedly on some setups. Physics step interpolation, syncing frame delta after draw, fixing negative delta values and frame delta smoothing will likely be enough to provide a good experience regardless of potential compositor issues.

Geometror · 2021-05-24T11:49:12Z

Ok, I removed the V-Sync via Compositor option entirely, now its ready for review :)

clayjohn · 2021-05-25T04:16:11Z

Looks interesting, but is this not just a reimplementation of #36862? See this comment #36862 (comment) for reduz' response to that PR.

I echo reduz' concern and I am not certain that just grabbing the first available V-sync format is the proper way to expose vsync to users.

Can you elaborate why this is the best approach for exposing v-sync? Please pardon my ignorance if it is something obvious.

Geometror · 2021-05-25T10:59:46Z

It is a reimplementation of #36862, but moved to DisplayServer as requested by reduz. In addition I removed the broken vsync via compositor option.
Managing v-sync that way(choosing the first available v-sync present mode) could keep it simple and uniform between the Vulkan renderer and the upcoming GLES renderer. As giarve mentioned VK_PRESENT_MODE_FIFO_KHR in #36862 is the only mode required to be supported, so it makes sense to prefer it over the others as the driver may support it the best. However, I think providing the user with an option to choose the preferred "v-sync enabled" present mode would be a good idea. (mainly between MAILBOX[could reduce input lag] and FIFO[guaranteed supported], FIFO_RELAXED isn't that useful imo)
Do you think that would be a better way to expose v-sync to users?

clayjohn · 2021-05-25T16:24:31Z

I'm not certain what the best way to expose the vsync settings to the user is.

I don't think falling back to VK_PRESENT_MODE_FIFO_RELAXED_KHR makes sense when a user disables v-sync but does not support VK_PRESENT_MODE_IMMEDIATE_KHR.

I think one option may be to have a toggle (between on and off, if VK_PRESENT_MODE_IMMEDIATE_KHR is not available then try the selected v-sync mode) and a dropdown menu (for v-sync mode, defaulting to VK_PRESENT_MODE_FIFO_KHR and falling back to VK_PRESENT_MODE_FIFO_KHR if selected mode is unavailable).

That being said, I'm not sure that it makes sense to do it like that, for android it looks like VK_PRESENT_MODE_SHARED_DEMAND_REFRESH_KHR or VK_PRESENT_MODE_SHARED_CONTINUOUS_REFRESH_KHR may be more suitable when v-sync is disabled (because VK_PRESENT_MODE_IMMEDIATE_KHR is never available)

Geometror · 2021-05-25T18:13:56Z

Ok, I will look into that. By digging through the codebase I found out that it is more work than I initially thought, so I should change this PR to a draft as there are still some things to decide, restructure and implement. Maybe I should ask reduz if he has concrete plans for v-sync atm.

clayjohn · 2021-05-25T18:50:10Z

@Geometror that is a good idea. Reduz typically has some ideas for this kind of stuff, and it is important to work on soon as we are approaching alpha.

akien-mga

Looks pretty good!

doc/classes/DisplayServer.xml

doc/classes/ProjectSettings.xml

drivers/vulkan/vulkan_context.cpp

Geometror · 2021-06-19T15:57:03Z

Rebased.
@Calinou Do you think I should include the editor VSync option from your 4.0 PR (#48364) as part of the whole VSync options reimplementation? Because when/if this is merged, that PR must be redone anyway, so I could save you some work.

clayjohn · 2021-06-19T17:06:57Z

@Geometror I think it is best for each PR to do one thing. My preference is to merge this PR first and then look at Calinou's PR after.

-Add a v-sync mode setting which allows to choose between DISABLED, ON, ADAPTIVE and MAILBOX -Removed the V-Sync via Compositor option

Calinou

Tested locally with this PR rebased against the latest master branch. All options seem to work as expected on Linux + NVIDIA.

Edit: Tested on a Windows 10 + Intel IGP setup, it works as expected there too.

Calinou · 2021-07-09T23:02:10Z

Thanks!

Calinou · 2021-07-10T14:32:23Z

core/config/project_settings.cpp

@@ -1114,6 +1114,7 @@ ProjectSettings::ProjectSettings() {

 	// Keep the enum values in sync with the `DisplayServer::ScreenOrientation` enum.
 	custom_prop_info["display/window/handheld/orientation"] = PropertyInfo(Variant::INT, "display/window/handheld/orientation", PROPERTY_HINT_ENUM, "Landscape,Portrait,Reverse Landscape,Reverse Portrait,Sensor Landscape,Sensor Portrait,Sensor");
+	custom_prop_info["display/window/vsync/vsync_mode"] = PropertyInfo(Variant::STRING, "display/window/vsync/vsync_mode", PROPERTY_HINT_ENUM, "Disabled,Enabled,Adaptive,Mailbox");


I just noticed this is a string enum rather than an integer enum. I think using an integer enum would be better as it's more Godot-y overall. (We're trying to move away from string usage when possible in 4.0 onwards.)

Also, in places where we use a string enum (such as the stretch mode project setting), we use snake_case strings instead of PascalCase.

I presume I just copied how it was done for the screen orientation (which was before you changed it to an integer enum). But you are right, an integer enum is cleaner as it wouldn't need that ugly if-elseif construct in main.cpp. I will open a PR soon to change that.

vnen · 2021-07-13T14:58:25Z

platform/linuxbsd/display_server_x11.h

+	virtual void window_set_vsync_mode(DisplayServer::VSyncMode p_vsync_mode, WindowID p_window = MAIN_WINDOW_ID) override;
+	virtual DisplayServer::VSyncMode window_get_vsync_mode(WindowID p_vsync_mode) const override;


This class didn't have override before this, so now Clang complain about the others that are missing.

Is the use of override desired? If so, I could add it where it should be (only display_server_iphone and display_server_osx use it currently), otherwise I could open a PR to remove the keyword from all display_server classes except osx and iphone.

Is the use of override desired?

Yes, it is. I'm not sure why it wasn't used in those classes before.

I opened a PR for this: #50513

Geometror requested review from a team as code owners May 10, 2021 19:17

Calinou added enhancement topic:rendering labels May 10, 2021

Calinou added this to the 4.0 milestone May 10, 2021

Geometror force-pushed the reimplement-disableable-vsync branch 2 times, most recently from c5c3224 to e896526 Compare May 10, 2021 20:11

Geometror force-pushed the reimplement-disableable-vsync branch 2 times, most recently from c229cac to 4aa4a33 Compare May 23, 2021 15:37

Geometror force-pushed the reimplement-disableable-vsync branch from 4aa4a33 to 079f65f Compare May 24, 2021 01:34

Geometror requested review from a team as code owners May 24, 2021 01:34

Geometror changed the title ~~Reimplement disableable V-Sync~~ Reimplement disableable V-Sync and restructure V-Sync code May 24, 2021

Geometror force-pushed the reimplement-disableable-vsync branch from 079f65f to e6ca83b Compare May 24, 2021 01:43

Geometror marked this pull request as draft May 25, 2021 18:14

Geometror force-pushed the reimplement-disableable-vsync branch 4 times, most recently from 5f41303 to bea4958 Compare May 30, 2021 14:58

Geometror reopened this Jun 2, 2021

clayjohn approved these changes Jun 6, 2021

View reviewed changes

akien-mga reviewed Jun 7, 2021

View reviewed changes

akien-mga requested review from Calinou and reduz June 7, 2021 07:23

Geometror force-pushed the reimplement-disableable-vsync branch 4 times, most recently from e5f2cba to c635d3d Compare June 10, 2021 01:36

Geometror force-pushed the reimplement-disableable-vsync branch 2 times, most recently from ea9b5f5 to 3c8bae7 Compare June 19, 2021 15:45

Restructure and reimplement vsync options

043ae91

-Add a v-sync mode setting which allows to choose between DISABLED, ON, ADAPTIVE and MAILBOX -Removed the V-Sync via Compositor option

Geometror force-pushed the reimplement-disableable-vsync branch from 3c8bae7 to 043ae91 Compare July 6, 2021 14:39

Calinou approved these changes Jul 7, 2021

View reviewed changes

reduz approved these changes Jul 9, 2021

View reviewed changes

Calinou merged commit a2d5f19 into godotengine:master Jul 9, 2021

Chaosus mentioned this pull request Jul 10, 2021

Add a V-Sync editor setting #48364

Merged

Calinou reviewed Jul 10, 2021

View reviewed changes

Geometror deleted the reimplement-disableable-vsync branch July 11, 2021 12:25

Calinou mentioned this pull request Jul 12, 2021

Change VSync mode project setting enum type from string to integer #50385

Merged

vnen reviewed Jul 13, 2021

View reviewed changes

Geometror mentioned this pull request Jul 16, 2021

Modernize display_server_... headers and make style consistent (use override keyword) #50513

Merged

Geometror mentioned this pull request Sep 2, 2021

Add frame delta smoothing option (4.x) #52314

Merged

Calinou mentioned this pull request Dec 20, 2021

Vulkan: Spurious "Failed to open JSON file" printed due to invalid Vulkan ICD files #56089

Closed

Calinou mentioned this pull request Feb 21, 2022

Expose double-buffered V-Sync as an option godotengine/godot-proposals#4065

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure and reimplement v-sync options #48622

Restructure and reimplement v-sync options #48622

Geometror commented May 10, 2021 •

edited by Calinou

Loading

Calinou commented May 10, 2021 •

edited

Loading

Geometror commented May 23, 2021

Calinou commented May 23, 2021 •

edited

Loading

Geometror commented May 24, 2021

clayjohn commented May 25, 2021

Geometror commented May 25, 2021 •

edited

Loading

clayjohn commented May 25, 2021

Geometror commented May 25, 2021

clayjohn commented May 25, 2021

akien-mga left a comment

Geometror commented Jun 19, 2021

clayjohn commented Jun 19, 2021

Calinou left a comment •

edited

Loading

Calinou commented Jul 9, 2021

Calinou Jul 10, 2021 •

edited

Loading

Geometror Jul 11, 2021

vnen Jul 13, 2021

Geometror Jul 13, 2021

vnen Jul 13, 2021

Geometror Jul 16, 2021

		virtual void window_set_vsync_mode(DisplayServer::VSyncMode p_vsync_mode, WindowID p_window = MAIN_WINDOW_ID) override;
		virtual DisplayServer::VSyncMode window_get_vsync_mode(WindowID p_vsync_mode) const override;

Restructure and reimplement v-sync options #48622

Restructure and reimplement v-sync options #48622

Conversation

Geometror commented May 10, 2021 • edited by Calinou Loading

Calinou commented May 10, 2021 • edited Loading

Geometror commented May 23, 2021

Calinou commented May 23, 2021 • edited Loading

Geometror commented May 24, 2021

clayjohn commented May 25, 2021

Geometror commented May 25, 2021 • edited Loading

clayjohn commented May 25, 2021

Geometror commented May 25, 2021

clayjohn commented May 25, 2021

akien-mga left a comment

Choose a reason for hiding this comment

Geometror commented Jun 19, 2021

clayjohn commented Jun 19, 2021

Calinou left a comment • edited Loading

Choose a reason for hiding this comment

Calinou commented Jul 9, 2021

Calinou Jul 10, 2021 • edited Loading

Choose a reason for hiding this comment

Geometror Jul 11, 2021

Choose a reason for hiding this comment

vnen Jul 13, 2021

Choose a reason for hiding this comment

Geometror Jul 13, 2021

Choose a reason for hiding this comment

vnen Jul 13, 2021

Choose a reason for hiding this comment

Geometror Jul 16, 2021

Choose a reason for hiding this comment

Geometror commented May 10, 2021 •

edited by Calinou

Loading

Calinou commented May 10, 2021 •

edited

Loading

Calinou commented May 23, 2021 •

edited

Loading

Geometror commented May 25, 2021 •

edited

Loading

Calinou left a comment •

edited

Loading

Calinou Jul 10, 2021 •

edited

Loading