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

feat: Smoother render loop #2188

Merged

Conversation

fredizzimo
Copy link
Member

@fredizzimo fredizzimo commented Dec 18, 2023

What kind of change does this PR introduce?

This makes rendering a little bit smoother, at least on some platforms, and also cleans up and unifies some code.

  • Remove some redundant polling for new events after the first frame is rendered
  • Do the animation and shaping as early as possible in the frame, which leaves just rendering to the end. This makes the rendering part much more predictable, which plays nicer with how some Wayland compositors are made. For example on Gnome many frames were too late because of the unpredictability.
  • Got rid of the manual dt calculation, and tries to use the monitor refresh rate instead, with a fallback to neovide_refresh_rate.
  • The frames are generated using a real-time clock and the frame rate calculated above, duplicating frames when rendering too fast. But that duplication does not matter since the frames would not be visible without enabling tearing anyway. And when the vsync is disabled, the configured refresh rate is used as the base, so it does not matter in that case either.
  • If frames are skipped by the compositor, the animation step is still done, which reduces the visual artefacts. This actually happens maybe once every 10 second on the gnome system I was mostly profiling on.
  • The Windows and macOS vsync implementation has been simplified and emulates the the native winit vsync support, which also unifies the rest of the code. But the timer based fallback and X11 opengl based vsync still uses the old system, with the latter almost impossible to port.
  • The render thread is removed on Windows. Winit has done some optimisations and the event loop is just a tiny bit on the slower side now. Furthermore, both the vsync and the event driven update loop relies on it being fast. This also simplifies the code, and removes one of the platform specific code paths.
  • On Windows the frames start rendering in the middle of the Vsync interval. According to my tests this is as smooth as I think we can get on OpenGL, we need to switch to D3D and waitable swap chains to do better. Maybe it's placebo, but it's still clearly better IMO, but not as smooth as on Wayland for example.

NOTE: This is functionality complete and can be tested, but depends on, so it's best to not review before that is merged. That's also why it's marked as a draft.

I have also not tested this properly on all platforms, I have only really done testing on Wayland and Windows. So we need testing on macOS and X11 before merging this. I have also only tested the very latest commit on a remote desktop connection to Windows.

Did this PR introduce a breaking change?

A breaking change includes anything that breaks backwards compatibility either at compile or run time.

  • No

@fredizzimo
Copy link
Member Author

@9mm, could you test this at some point? There's no hurry, but I wonder if this makes the it possible for you to run with the default settings and vsync enabled, without the need for disabling it and specifying a high refresh rate.

@fredizzimo fredizzimo marked this pull request as draft December 18, 2023 19:14
@9mm
Copy link
Contributor

9mm commented Dec 18, 2023

Absolutely, will check tomorrow morning!

@9mm
Copy link
Contributor

9mm commented Dec 19, 2023

Ooooo, this definitely is much smoother, if not AS smooth as the other way I was doing it, and this even with animation set to 0.0 which makes it much more obvious when something lags.

If I use the profiler it looks like it hovers around 65-70 fps, which is much lower than the 120 hz of my screen, but I can say even despite that (somehow?). Also the max hovers around 13-15ms. That is a bit confusing to me though given how smooth everything is, I also checked scrolling.

Edit: I think I found why ^ .. it looks like scrolling window is closer to 120 but scrolling through a list in telescope is less than 120, so maybe just item scrolling through a telescope list is maxed out already and thats why it's just as fast visually despite lower frame rate. When scrolling through a long file it hovers around 8.3ms max

@fredizzimo
Copy link
Member Author

@9mm, that's very good news!

On Windows, the news is not as good though. On my work computer it still stutters a lot, even if it was almost smooth on my home computer. I don't think that can be solvable without moving to D3D.

@mati865
Copy link

mati865 commented Dec 19, 2023

On Wayland Gnome under VirtualBox VM it's even smoother than current main but more importantly for me this PR solves high CPU usage when unfocused like I already said in #2167 (comment)

I have noticed weird issue however, very rarely when I bring back Neovide to the front the window will shrink from maximized state. Profiling would be though as it has happened like three times during the whole workday.

@fredizzimo
Copy link
Member Author

@mati865, I think this issue might be related to the maximised state bug you are having

I have not properly checked what's going on, but I think it's a bug in winit rather than Neovide, since we are not really doing much special. It might also be that winit sends us some window size before the maximized one, and then we send that window size to Neovim, which in turn resizes the window back. But it needs more investigation to fully understand what's going on.

@mati865
Copy link

mati865 commented Dec 19, 2023

I do hit #2137 when launching Neovide as the window typically (but not always) starts as a small square-ish box.
The issue I described in the previous comment is a bit different: very rarely Neovide would shrink vertically (horizontal size doesn't seem to change) when focusing it.
I haven't observed it before switching to this branch but there are too many variables (and too rare reproduction) to pinpoint the problem, so it might be something entirely unrelated to this branch.

Copy link
Contributor

@MultisampledNight MultisampledNight left a comment

Choose a reason for hiding this comment

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

First: Thank you for the PR!

src/renderer/vsync/vsync_macos.rs Show resolved Hide resolved
src/renderer/vsync/vsync_macos.rs Show resolved Hide resolved
src/renderer/vsync/vsync_win.rs Outdated Show resolved Hide resolved
src/renderer/vsync/vsync_win.rs Show resolved Hide resolved
src/window/update_loop.rs Show resolved Hide resolved
@MultisampledNight MultisampledNight marked this pull request as ready for review December 23, 2023 20:11
@MultisampledNight
Copy link
Contributor

Thank you! Chooooooooooooooooooooooooooooooooooooo chooooooooo! 🚂

@MultisampledNight MultisampledNight merged commit 2a1074b into neovide:main Dec 23, 2023
2 checks passed
@9mm
Copy link
Contributor

9mm commented Dec 23, 2023

On a roll today 😎

@MultisampledNight
Copy link
Contributor

Yep, we're getting ready for release!

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

Successfully merging this pull request may close these issues.

None yet

4 participants