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

Linux/OpenGL: Don't force vsync in the editor #82221

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

Riteo
Copy link
Contributor

@Riteo Riteo commented Sep 24, 2023

I couldn't tell whether this has an actual purpose and it feels more like a debug remnant.

We also need to be able to disable vsync in the editor for the WIP Wayland backend (in the EGL driver) as it does manual frame throttling.

CC @lawnjelly which seems the original author of this code.

Note that I haven't tested the X11 part and I've tested the EGL change only with the Wayland branch, so some further testing would be a good idea.

I couldn't tell whether this has an actual purpose and it feels more
like a debug remnant.

We also need to be able to disable vsync in the editor for the WIP
Wayland backend (in the EGL driver) as it does manual frame throttling.
@Riteo Riteo requested a review from a team as a code owner September 24, 2023 02:40
@darksylinc
Copy link
Contributor

darksylinc commented Sep 24, 2023

What is the purpose of this on X11? When I disable VSync on the editor (xfce + x11) my video card goes into full overdrive (110°C Junction Speed, which is expected but also overkill).

It should definitely be on. It is also a feature that can be toggled from the Editor so I don't understand why it should be hard-coded?

I can't speak for Wayland; but in principle Wayland also prefers VSync. I wouldn't be surprised though that it breaks VSync On + XWayland as Godot right now lacks a native Wayland backend.

(Edit: Re-reading what I wrote it sounds like it's an aggressive or condescending tone; that's not the intention)

@Riteo
Copy link
Contributor Author

Riteo commented Sep 24, 2023

@darksylinc

What is the purpose of this on X11? When I disable VSync on the editor (xfce + x11) my video card goes into full overdrive (110°C Junction Speed, which is expected but also overkill).

Just consistency tbh.

It should definitely be on. It is also a feature that can be toggled from the Editor so I don't understand why it should be hard-coded?

Yeah that's the thing. I'm not removing VSync (or at least, it shouldn't), I'm removing this weird override that doesn't allow us to do it in the first place.

I can't speak for Wayland; but in principle Wayland also prefers VSync.

Actually it might be surprising but we need to disable the "hard" vsync and switch to the equivalent of mailbox on OGL due to the fact that we're main thread bound and we have to throttle all frames manually based on when Wayland tells us that we can draw. Otherwise everything hard locks until the window is visible.

I know, this sounds weird but I couldn't find any better solution and it genuinely looks to me like it's supposed to be done.

See also: https://emersion.fr/blog/2018/wayland-rendering-loop/

I wouldn't be surprised though that it breaks VSync On + XWayland as Godot right now lacks a native Wayland backend.

I'm not exactly sure what you're talking about. If you're meaning that vsync might be broken on XWayland IIRC it should work as it uses the GLX driver. Not sure though.

(Edit: Re-reading what I wrote it sounds like it's an aggressive or condescending tone; that's not the intention)

No worries, text is ambiguous so I think that it's been a good idea to clarify :)


BTW does this actually disable vsync full stop in the editor? As I said I haven't tested this on anything other than wayland which depends on being able to override itself the vsync mode.

@darksylinc
Copy link
Contributor

Oh I completely misread your PR! I saw you were doing the opposite! (I thought you wanted to force-disable VSync)

Yeah, VSync should not be forced on; it should follow Editor settings.

@lawnjelly
Copy link
Member

lawnjelly commented Sep 24, 2023

I couldn't tell you whether I added that, or copy pasted in from an earlier section from e.g. 3.x. Either way it was from my very early GLES2 tests that went on to be used to bootstrap GLES3. I think it's fine to remove.

If it was me, it is likely the main danger I was trying to avoid at the time for debugging etc (particularly at that early stage of 4.x where there were lots of bugs) was with redraw requests. It is very easy to get into a situation where you are getting a bunch of redraw requests in the editor. At least with vsync on, you are limiting how much processing this can take, but with vsync off you could end up rendering e.g. 1000fps while moving your mouse.

But remember this was at a time of basically getting anything on the screen at all was a bonus, and expecting all this code to be gone over with fine toothed comb at a later date.

@Calinou
Copy link
Member

Calinou commented Sep 25, 2023

I suggest merging #48364 along the way 🙂

If you want low input lag in the editor and don't have a VRR display (or can't use VRR for Godot), disabling V-Sync is the only way to achieve low input lag. However, this is currently tied to the V-Sync project setting or the --disable-vsync command line argument. The linked PR moves this to an editor setting for the purposes of the editor itself, without affecting the running project.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Makes sense to remove this, but what controls whether the editor uses vsync currently?

If it's based on the edited project's settings, then it can indeed be problematic if users disable vsync for their project and that automatically disables it for the editor.

But I guess Calinou's PR decouples that?

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Sep 26, 2023
@akien-mga akien-mga changed the title Linux/OpenGL: don't force vsync in the editor Linux/OpenGL: Don't force vsync in the editor Sep 26, 2023
@akien-mga akien-mga merged commit 48bee5c into godotengine:master Sep 26, 2023
15 checks passed
@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.

6 participants