-
Notifications
You must be signed in to change notification settings - Fork 919
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
[metal] Improve layer initialization and resizing #6107
Conversation
66eec32
to
37dba73
Compare
Another solution than this would be to make |
37dba73
to
817257c
Compare
I'm a bit short on time right now, but I'll try to have a deeper look later this week! Looks really well researched and documented, looking forward to it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! This documents what's going on and all the considerations going on so much better, with the old code (not having read it before) I had no clue what was actually going on.
I tested it on an M1 Sonoma 14.4.1 with a low-dpi screen attached and it performs as advertised in the PR description 👌
Can land as-is, but I think @cwfitzgerald wanted to have look as well (?)
Regarding the todos left on the PR description: I see no problem in catching up on this later. Unfortunately I also don't have any of these slightly more unusual cases ready for testing
e337fdb
to
ff9d88f
Compare
Overriding the `layer` on `NSView` makes the view "layer-hosting", see [wantsLayer], which disables drawing functionality on the view like `drawRect:`/`updateLayer`. This prevents crates like Winit from providing a robust rendering callback that integrates well with the rest of the system. Instead, if the layer is not CAMetalLayer, we create a new sublayer, and render to that instead. [wantsLayer]: https://developer.apple.com/documentation/appkit/nsview/1483695-wantslayer?language=objc
We do not need to use it, and the layer itself is already retained, so it won't be de-allocated from under our feet.
ff9d88f
to
9192c90
Compare
So, I've run into a macOS bug that has been driving me insane, because it took me so long to figure out that it's actually that, an OS bug, and not because I was doing something wrong. It happens when setting an I am fairly sure that what's happening is that AppKit's auto layout algorithm, when calculating the size on the containing This is a problem when using Wgpu inside more complicated layouts, since the bounds of the layer will slowly drift out of sync with reality the more you resize, as can be seen in this repo by using the broken-auto-layout.movI would argue this is not a blocker for this PR, as it still improves things so far, but it's something that I'll try to work on resolving in any case. |
Hm, it looks like I was less familiar with the AppKit view and layer management than I thought. By not making it layer-hosting, does that mean that the layer should be only updated in the I'm fairly sure I'm just paranoid but wanted to air out my concerns. |
No no, layers can be updated safely anywhere, from any thread, regardless of whether the view they're contained in is layer-hosting or just layer-backed. Only issue is that you will get tearing and such unless you use Wgpu's The difference between layer-hosting and layer-backed is only in terms of how the Does that make sense? |
I've toiled further away on this, and I I've come to the conclusion that the only proper way to fix the issue I talked about in #6107 (comment) is to use observers, that is, make the new layer that we create observe the You can see implementations of this idea rust-windowing/raw-window-metal#19 and rust-windowing/softbuffer#234, it's fairly tricky stuff since the Key-Value-Observing APIs are just very terrible outside of Objective-C. I then tried to actually implement this in Wgpu, but since it's still using the |
sounds very reasonable to me. Thanks again for documenting everything so well both in comments as well as the thought process here on the PR! |
@madsmtm Thank you so much for the comments in |
* [metal]: Create a new layer instead of overwriting the existing one Overriding the `layer` on `NSView` makes the view "layer-hosting", see [wantsLayer], which disables drawing functionality on the view like `drawRect:`/`updateLayer`. This prevents crates like Winit from providing a robust rendering callback that integrates well with the rest of the system. Instead, if the layer is not CAMetalLayer, we create a new sublayer, and render to that instead. [wantsLayer]: https://developer.apple.com/documentation/appkit/nsview/1483695-wantslayer?language=objc * [metal]: Fix double-free when re-using layer * doc: Document the behavior when mis-configuring width/height of Surface * [metal]: Use kCAGravityResize for smoother resizing * [metal] Do not keep the view around that the surface was created from We do not need to use it, and the layer itself is already retained, so it won't be de-allocated from under our feet. * Always set delegate on layers created by Wgpu * More docs on contentsGravity
* [metal]: Create a new layer instead of overwriting the existing one Overriding the `layer` on `NSView` makes the view "layer-hosting", see [wantsLayer], which disables drawing functionality on the view like `drawRect:`/`updateLayer`. This prevents crates like Winit from providing a robust rendering callback that integrates well with the rest of the system. Instead, if the layer is not CAMetalLayer, we create a new sublayer, and render to that instead. [wantsLayer]: https://developer.apple.com/documentation/appkit/nsview/1483695-wantslayer?language=objc * [metal]: Fix double-free when re-using layer * doc: Document the behavior when mis-configuring width/height of Surface * [metal]: Use kCAGravityResize for smoother resizing * [metal] Do not keep the view around that the surface was created from We do not need to use it, and the layer itself is already retained, so it won't be de-allocated from under our feet. * Always set delegate on layers created by Wgpu * More docs on contentsGravity
This reverts commit 54c801c.
Description
Overriding the
layer
onNSView
, as is currently done, makes the view "layer-hosting", seewantsLayer
.This prevents Winit from emitting the
RedrawRequested
event at the desired frame, because overwriting the layer implicitly disables thedrawRect:
/updateLayer
mechanisms that we need to use. Instead, Winit is currently forced to emit the event after all other events have been processed, which is hacky, and does not integrate well with redraws.In this PR, I've changed it so that
wgpu
, when it is given a view without aCAMetalLayer
, instead creates a new sublayer that it renders into (as is already done on iOS).Note that this might theoretically have a slight performance impact, since the system now has to do more state-tracking, but I suspect it's miniscule (wasn't able to measure it with
bunnymark
), and I'd argue that it's a small price to pay for correctness.I guess that another solution would be to properly implement the
CALayerDelegate
, and calldrawRect:
/updateLayer
in there ourselves - though that's a can of worms I'm not really prepared to open.Connections
Fixes #1168, mostly by using
kCAGravityResize
, but the situation will also be properly fixed in the future, since Winit can clean up its current redrawing hacks.Related similar issues: #249, gfx-rs/wgpu-rs#536, rust-windowing/glutin#1340, rust-windowing/winit#1901 and rust-windowing/winit#2640.
Also makes the situation better for #3756, if the user enables
present_with_transaction
, resizing becomes perfectly smooth.Finally, it removes our dependence on having access to
NSView
, paving the way for a future whereraw-window-handle
may only provideCALayer
.Testing
See this repo which contains roughly the "minimal" setup needed to get
wgpu
working on macOS and iOS without Winit. The crate renders a triangle whose top corner is at a fixed location from the right edge of the window, which is useful for testing that redrawing works as it should. It also has a few different features that you can try out to see how things look using some of the different methods for rendering that macOS/iOS provides. Use thepatch
key inCargo.toml
to test it with and without this PR.I've also tested this with Winit on macOS and Mac Catalyst, by resizing the
hello_triangle
window, and moving it between an external monitors.Current behaviour:
current.mp4
With this PR:
now.mp4
Checklist
cargo fmt
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
--target wasm32-unknown-emscripten
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.TODO (though can still be reviewed in the meantime)
raw-window-metal
, or by using .wgpu
.--features=mtkview
.sdl2
's example seems to work fine (they're using a custom view with aCAMetalLayer
as the root layer, so shouldn't be affected by most of this).