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

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

Merged
merged 1 commit into from
May 17, 2023

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Sep 1, 2021

Frame deltas are currently measured by querying the OS timer each frame. This is subject to random error. Frame delta smoothing instead filters the delta read from the OS by replacing it with the refresh rate delta wherever possible.

This PR also contains code to estimate the refresh rate based on the input deltas, without reading the refresh rate from the host OS.

The delta_smooth_enabled setting can also be modified at runtime through OS::, and there is also now a command line setting to override the project setting.

Notes

  • This is pretty much a straight port of Add frame delta smoothing option #48390 from Godot 3.4.
  • vsync_enabled is read slightly differently, from DisplayServer on recommendation from @Geometror
  • At the moment the smoothing is only attempted when vsync mode is set to enabled. I'm not sure the current method makes sense vsync is set to the newer modes MAILBOX or ADAPTIVE. We may at a later time introduce an additional simple rolling average or similar for these other modes and when vsync is off.

Extra Info

As before the majority of the code is the refresh rate estimator, which may take a bit of time to get your head around, however the important part that does the business is quite simple:

	// accumulate the time we have available to use
	_leftover_time += p_delta;

	// how many vsyncs units can we fit?
	int64_t units = _leftover_time / _vsync_delta;

	// a delta must include minimum 1 vsync
	// (if it is less than that, it is either random error or we are no longer running at the vsync rate,
	// in which case we should switch off delta smoothing, or re-estimate the refresh rate)
	units = MAX(units, 1);
	_leftover_time -= units * _vsync_delta;
	return units * _vsync_delta;

Essentially the idea is that we are just incrementing the delta by whole frame intervals. This works great around the refresh rate, because it takes care of small fluctuations, and if the frame rate is deviating too much it simply turns smoothing off again until it re-stabilizes.

@lawnjelly lawnjelly requested review from a team as code owners September 1, 2021 15:33
@Calinou Calinou added this to the 4.0 milestone Sep 1, 2021
@Calinou
Copy link
Member

Calinou commented Sep 1, 2021

The main difference is that there is no vsync_enabled to read from Engine in 4.x, the vsync status seems mostly stored downstream in the servers, so presumably wouldn't be good to read each frame, so I'm simply passing on the value window_vsync_mode currently stored as a static in main.cpp

Remember that V-Sync status can be changed at run-time, so this should ideally be changed. It's not critical as toggling V-Sync remains possible, but people playing a game after toggling V-Sync (and without restarting the game) will get a subpar experience.

At the moment the smoothing is only attempted when vsync mode is set to enabled. I'm not sure the current method makes sense vsync is set to the newer modes MAILBOX or ADAPTIVE. We may at a later time introduce an additional simple rolling average or similar for these other modes and when vsync is off.

My guess is that delta smoothing improves the situation with adaptive V-Sync if you can keep up with the monitor refresh rate. In situations where this is not the case, it will likely worsen things as V-Sync is automatically disabled when the CPU/GPU can't keep up with the monitor refresh rate.

@lawnjelly
Copy link
Member Author

lawnjelly commented Sep 1, 2021

Remember that V-Sync status can be changed at run-time, so this should ideally be changed. It's not critical as toggling V-Sync remains possible, but people playing a game after toggling V-Sync (and without restarting the game) will get a subpar experience.

Yes I'm sure we can get this working, I'm hoping to get some ideas from reviewers who are familiar with vsync in 4.

EDIT: Actually having a look, as far as I can see, the DisplayServer::create is only called at startup (and the window_vsync_mode is correct there). I don't know if it even is currently possibly to change vsync mode without a restart. In which case the current version would be fine (unless this changed in the future).

My guess is that delta smoothing improves the situation with adaptive V-Sync if you can keep up with the monitor refresh rate. In situations where this is not the case, it will likely worsen things as V-Sync is automatically disabled when the CPU/GPU can't keep up with the monitor refresh rate.

It is possible it could improve things as is particular in ADAPTIVE mode but I'd like to get this thoroughly tested before enabling it in those modes, just in case it doesn't work well. I don't think this is necessary anyway to start with, it's fairly easy to turn on for the other modes if we decide, and we don't want this PR to hold up further work on the mainloop.

@Geometror
Copy link
Member

Setting the VSync-mode at runtime is currently possible. (via DisplayServer::window_set_vsync_mode) If I remember correctly, it should even be possible to set the mode from another thread (however, don't pin me down on this). It will result in a recreation of the swapchain similar to when the window is resized, DisplayServer::create just creates the swapchain for the first time. (looking at #48622 may provide more information, but I didn't update the initial comment frequently so there might be additional changes done later)
So the current VSync mode of the main window can be obtained with DisplayServer::window_get_vsync_mode with MAIN_WINDOW_ID as the window id parameter. DisplayServer::window_get_vsync_mode just calls VulkanContext::set_vsync_mode (depending on the platform-specific implementation, but currently it is the same for all DisplayServers), where a check whether the window exists is performed and if so, the value is returned. So calling it every frame should be fine.
I think this feature should be enabled with adaptive VSync from the start but you are right, proper testing is needed.

@lawnjelly
Copy link
Member Author

lawnjelly commented Sep 14, 2021

This is now updated with reading the vsync from DisplayServer each frame, so should work if the user changes vsync in a game settings for example.

@lawnjelly
Copy link
Member Author

lawnjelly commented Sep 8, 2022

Note: Approved in PR meeting today, just needs a rebase.

Rebased. Should be fine now, just tested.

@Calinou
Copy link
Member

Calinou commented Oct 21, 2022

Note: Approved in PR meeting today, just needs a rebase.

Rebased. Should be fine now, just tested.

This needs a small rebase again, but we should try to merge this PR before RC. We should also enable the option by default, and consider doing so in new projects in 3.6.

Also, now that there's DisplayServer.screen_get_refresh_rate(), can we use it to set the initial estimation automatically? The dynamic estimator will still be useful for platforms where that method isn't available.

@Maran23
Copy link
Contributor

Maran23 commented Dec 21, 2022

Needs another rebase.
Since this is already in 3.x and working nicely (I always keep it turned on), there should not be a high risk that this cause any problems, right?

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 10, 2023
@clayjohn
Copy link
Member

@lawnjelly I don't know how we missed this in the final PR review for 4.0. But I agree with Maran23, this looks like a straight port of a well-loved feature in 3.x. We may need to do something with the other vsync modes eventually, but starting with a straight port seems pretty safe and desirable to me.

Do you know of any reason why we should delay merging this? If not, go ahead and rebase and we can merge soon.

@lawnjelly
Copy link
Member Author

Yes it should be fine, I'll rebase, and poke someone to merge. 👍

@lawnjelly
Copy link
Member Author

@clayjohn @akien-mga Rebased and ready.

@Maran23
Copy link
Contributor

Maran23 commented Apr 10, 2023

Friendly ping from me as well, just so we don't forget this PR again. 😄

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 good to me. It appears to work the same as in 3.x and I am not aware of any reason why the same approach wouldn't work here. That being said, I am not an expert on main loop/timing stuff, so take my review for what it is

Would be nice to have @RandomShaper's review as well in case he is aware of something that I am not.

main/main.cpp Outdated Show resolved Hide resolved
Frame deltas are currently measured by querying the OS timer each frame. This is subject to random error. Frame delta smoothing instead filters the delta read from the OS by replacing it with the refresh rate delta wherever possible.

This PR also contains code to estimate the refresh rate based on the input deltas, without reading the refresh rate from the host OS.

The delta_smooth_enabled setting can also be modified at runtime through OS::, and there is also now a command line setting to override the project setting.
@lawnjelly lawnjelly force-pushed the four_delta_smooth branch from 4658cdb to 7925670 Compare May 16, 2023 12:57
@akien-mga akien-mga merged commit 411b6a9 into godotengine:master May 17, 2023
@akien-mga
Copy link
Member

Thanks!

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.

9 participants