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

Feature: allow a toggle to enable/disable vsync #8997

Merged
merged 2 commits into from Apr 11, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Apr 10, 2021

Related: #8963 , waiting on user response if this would fix the issue

Motivation / Problem

We have some reports about bad graphics, where if the user could toggle vsync on, the problem would not exist. So, instead of having it hidden away in a driver option, make it visible.

As we only implement this for OpenGL currently, the toggle only works when hardware acceleration is enabled.

Description

Vsync should be off by default, as for most players it will be
better to play without vsync. Exception exist, mainly people who
use fullscreen.

The second commit reworks the GUI a bit to look nicer, but has nothing to do with this change other than looking nicer.

Limitations

  • MacOS doesn't have current code to enable/disable vsync. So it is ignored on that platform. Not sure if I should disable to button on MacOS, or leave it as it is. (placebo effect can also be very strong :P)

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@michicc
Copy link
Member

michicc commented Apr 10, 2021

As far as I can tell, there's no way to influence vsync on macOS for any kind of non-exclusive screen access (and OTTD is non-exclusive).

There's not even a way to draw directly to screen, even the OpenGL stuff is forced to render to framebuffer by the Quartz compositor.

@TrueBrain
Copy link
Member Author

As far as I can tell, there's no way to influence vsync on macOS for any kind of non-exclusive screen access (and OTTD is non-exclusive).

There's not even a way to draw directly to screen, even the OpenGL stuff is forced to render to framebuffer by the Quartz compositor.

So I guess we should hide the setting completely for macOS?

@michicc
Copy link
Member

michicc commented Apr 10, 2021

Oh, and while I don't know about SDL, changing vsync state on Windows can be done any time and becomes effective at next present.

@michicc
Copy link
Member

michicc commented Apr 10, 2021

So I guess we should hide the setting completely for macOS?

Hide or at least disable I think.

@TrueBrain TrueBrain force-pushed the vsync-button branch 2 times, most recently from 1acc5dc to ad785b6 Compare April 10, 2021 13:30
@TrueBrain
Copy link
Member Author

Oh, and while I don't know about SDL, changing vsync state on Windows can be done any time and becomes effective at next present.

Currently I made that you have to restart, mainly as doing it runtime seems a lot of work, one that is not really worth doing. But that is jut my lazy opinion :)

Other than that, on macOS the option is now not shown \o/

@PeterN
Copy link
Member

PeterN commented Apr 10, 2021

Switching to fullscreen and back makes the change take effect too.

@TrueBrain
Copy link
Member Author

Switching to fullscreen and back makes the change take effect too.

But only for Windows, I think. But yeah, that is something to address ..

Changing SDL to do the same as Windows is easy .. just not sure how easy it is to create a next OpenGL context from that window. Maybe @michicc has some code-wise ideas how to make it so you can change vsync without restarting?

@TrueBrain TrueBrain added the backport requested This PR should be backport to current release (RC / stable) label Apr 10, 2021
@PeterN
Copy link
Member

PeterN commented Apr 11, 2021

Can't you just call _wglSwapIntervalEXT() / SDL_GL_SetSwapInterval() at any time?

@michicc
Copy link
Member

michicc commented Apr 11, 2021

Indeed, at last on Windows platforms, _wglSwapIntervalEXT can be called any time with a valid active context. This does mean though that the call has to be marshalled to the main thread and can not be done from the game thread.

The Linux/SDL GLX call should behave similarly, but I'm not totally sure on that.

Vsync should be off by default, as for most players it will be
better to play without vsync. Exception exist, mainly people who
play in fullscreen mode.
"Hardware acceleration" was not aligned with its checkbox. So instead
of drawing the labels left and the options right, now draw settings
one by one with a spacer between label and option to get the right
spacing.

Also, use SetPIP instead of repeating a SetPadding for all but
last element.
@TrueBrain
Copy link
Member Author

For some reason did not think of that, tnx guys!

SDL is fine too, as it calls the same OpenGL function in the end.

The only "issue" now is, that if you start without hardware acceleration, click that you want it, you can now click enable/disable vsync. This does absolutely nothing, but there is no feedback that you still have to restart the game. But I guess that is not a problem for this PR honestly.

@TrueBrain TrueBrain merged commit d50b934 into OpenTTD:master Apr 11, 2021
@TrueBrain TrueBrain deleted the vsync-button branch April 11, 2021 12:26
@LordAro LordAro added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants