-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix jittering during window resize on MacOS for WGPU/Metal #7641
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
base: main
Are you sure you want to change the base?
Conversation
…rame integration.
Preview is being built... Preview will be available at https://egui-pr-preview.github.io/pr/7641-osx_metal_resize_jitter_fix View snapshot changes at kitdiff |
@Wumpf would love to hear your feedback on this =) |
thanks for setting this up! will have a more thorough look and think in the coming days
You should be able to poke the wgpu adapter/device to ask it at runtime whether it's a metal device 🤔 Edit: Ah sorry, you already have that check via cast. The problem is that you don't know whether the types are available. That's nasty 🤔 |
This looks very promising! Thanks for working on this ❤️ |
Thanks for investigating this deeper and coming up with a ! Played around a bit with it and noticed that egui's fps counter goes up during resizing. In fact if I always enable present with transaction, it renders 240fps instead of the expected 120fps in continuous mode. That kinda explains how this can work: the screen (and presumably resize events) happen at 120fps and we're practically running into a sampling problem of the currently correct screen size. Fascinating! Unfortunately the jitter is still there a liiiitttle bit on external monitors. But on the builtin one it's gone completely. Interestingly, it's the entire window jittering which never happens on the internal monitor - notice how the top left of the windows moves as well (running via usbc @ 120hz): occasional.jitter.on.external.monitor.downsized.mov
Reading through the material I come to the same conclusion as well so far and agree that the proposed solution is the way to go forward. Let's make sure to have everything documented in the code! I can explore a bit how we can expose this in wgpu directly more easily, but the next time we can land semvar breaking changes is end of december, would be nice to figure this out without that :) |
Looking around a bit, doing platform specific features on Either way, we're probably better off either..
|
Overview
As mentioned in #903 the window resizing on macos in egui was hit by something heavy some long time ago due to, probably, changes in winit 0.29->0.30. I have not been able to narrow down to the root cause of that back then. But even though the issues were reported for glow initially, they are present with winit/wgpu/metal triplet as well today.
A.mp4
But it appears for the metal the solution is well known. (thx @mattia-marini #903 (comment))
The CAMetalLayer - one that wgpu draws to - has to draw the frames in sync with CoreAnimation, framework that drives the whole GUI rendering pipeline on apple platforms. The desynchronization between the two pipelines lead to the visual jitter. CA is unaware that a new frame is coming and tries to stretch the existing window content.
This behavior is controlled by presentsWithTransaction flag. The support for it has been implemented in wgpu many years ago, 2021 it seems, including the needed drawing logic alterations, and is exposed to the metal backend users as a pub field in the metal surface struct. Note it was not escalated to higher levels of abstraction as this is a backend-specific feature.
Yet enabling it comes with side-effects, best described in Zed GPUI Article and potential latencies (@Wumpf #903 (comment), gfx-rs/wgpu#8109). The metal layer is desynced with CA for most performance and control, but that limits the ability to play well with native UI elements, including window itself.
So the proposed solution is to enable the
present_with_transaction
only during the resizing - same solution as Zed did. I've made a prototype and.. Well. It works as expected.B.mp4
I have made changes to
egui-wgpu
: madePainter
responsible for toggling the surface properties in response to window resize begin & end; and toeframe
wgpu integration: detection of resize start and end w/ pokes to the painter.Things to address
Accessing the field
The field is currently accessed through a pointer cast. The API is specific for Metal and thus is not exposed to upper levels, so if one has no typed handle to the metal surface struct one has to downcast.
I think it won't change on the wgpu side in the future. It's a stable API for 4 years already :)
So having this abomination around might need an explicit approval. Personally I am completely fine with it.
Conditional compilation
The
wgpu::hal::api::Metal
andwgpu::hal::metal::Surface
are undermetal
flag in wgpu crate. Currently egui does not import wgpu backend by default or has feature flags for that. That's what defaulting to wgpu in eframe & #7615 recently hit into.This PR also needs backend feature flags in egui for it to work as it depends on those metal types. The
#[cfg(all(target_os = "macos"))]
is not enough and code won't compile if
wgpu/metal
is not enabled. But it's impossible to cfg('wgpu/metal') afaik. Any solutions? Should it wait @emilk for the metal flag?Window lifecycle events
Winit does not provide events for window resizing other than just a plain
Resized(new_size)
. In context of the issue at hand it's required to be aware that resize is in progress across frames. It seems that during the window resize winit fires an exclusive stream ofResized
events. I have assumed that only one window viewport can be resized at a single moment of time and added tracking of it's id. When an event arrives and it's not a resize - well, resize has ended it seems.It is safe for a random event to occur during the resizing and trigger the end prematurely. That will only lead to a possibility of a visual jitter occurrence at that frame.
Is it fine?
PS: So happy this thing stopped lagging on resize, it's a miracle. ^_^
PSS: The current draft will not build in CI because of the feature flag thingy.