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

[metal] Improve layer initialization and resizing #6107

Merged
merged 7 commits into from
Sep 8, 2024

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Aug 12, 2024

Description

Overriding the layer on NSView, as is currently done, makes the view "layer-hosting", see wantsLayer.

This prevents Winit from emitting the RedrawRequested event at the desired frame, because overwriting the layer implicitly disables the drawRect:/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 a CAMetalLayer, 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 call drawRect:/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 where raw-window-handle may only provide CALayer.

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 the patch key in Cargo.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

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

TODO (though can still be reviewed in the meantime)

  • Test drawing in non-fullwindow views (and maybe scaled / transformed / rotated / weird views?).
  • Test passing a MTKView to wgpu.
  • Test with SDL and other such "window providers" in the Rust ecosystem? I'll need some help with this!
    • sdl2's example seems to work fine (they're using a custom view with a CAMetalLayer as the root layer, so shouldn't be affected by most of this).

@madsmtm
Copy link
Contributor Author

madsmtm commented Aug 12, 2024

Another solution than this would be to make wgpu handle when to render internally (e.g. provide a "render" callback). Not sure how that'd work on other platforms though, which is why I've gone this route instead.

@Wumpf
Copy link
Member

Wumpf commented Aug 13, 2024

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 :)

@Wumpf Wumpf self-requested a review August 13, 2024 21:08
Copy link
Member

@Wumpf Wumpf left a 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

wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-hal/src/metal/surface.rs Show resolved Hide resolved
wgpu-hal/src/metal/surface.rs Show resolved Hide resolved
@madsmtm madsmtm force-pushed the improve-metal-resizing branch 3 times, most recently from e337fdb to ff9d88f Compare August 25, 2024 15:50
@madsmtm madsmtm mentioned this pull request Aug 25, 2024
4 tasks
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.
@madsmtm
Copy link
Contributor Author

madsmtm commented Aug 30, 2024

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 autoresizingMask to something that auto-sizes (like we do with kCALayerWidthSizable | kCALayerHeightSizable), using a contentsGravity that auto-resizes (like we do with kCAGravityResize), and using auto layout on the owning view. I believe that it is an OS bug because I can reproduce it in a virtual machine on macOS 14.6 and on macOS 15 beta 3, but not on macOS 13.5.

I am fairly sure that what's happening is that AppKit's auto layout algorithm, when calculating the size on the containing NSView, sets the same size on the inner CALayer, and this size happens to contain a fractional value on high DPI screens (on macOS 13 and below, the auto layout algorithm did not create fractional view sizes, so it is not a problem there). Layers do not like fractional values in their width/height (documented here, though unknown why this is the case), and so the auto-resize mechanism ends up rounding these values to not be fractional. This, in turn, causes there to be a difference between the layer's bounds, and the superlayer's bounds, and kCAGravityResize interprets it as-if we we want it to maintain this difference, and then things blow up.

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 "two-triangles" Cargo feature, or in the video below:

broken-auto-layout.mov

I 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.

@scoopr
Copy link
Contributor

scoopr commented Sep 3, 2024

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 drawRect:/updateLayer?
I'm just slightly concerned that there is no undue restrictions when doing it like that. I'm thinking of maybe a case of using another thread to do nextDrawable in a custom loop, to do some fancy framepacing tricks (or maybe slow rendering without interfering other UI). Its just a bit unclear from the docs that what is required or allowed for layer-backed vs layer-hosted in the particular case of CAMetalLayer, though I bet it skips most of AppKit requirements.

I'm fairly sure I'm just paranoid but wanted to air out my concerns.

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 4, 2024

By not making it layer-hosting, does that mean that the layer should be only updated in the drawRect:/updateLayer?

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 present_with_transaction, or restrict your drawing inside drawRect:/updateLayer.

The difference between layer-hosting and layer-backed is only in terms of how the NSView talks to its CALayer - and with layer-hosting views, NSView functionality that Winit uses (i.e. drawRect:/updateLayer) is disabled, which is the primary motivation for this PR's changes.

Does that make sense?

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 4, 2024

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 contentsScale and bounds of the super layer, and update it's properties when those change.

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 objc crate, I had to resort to all sorts of hacks to get it to work. In the end, I threw the code away; it's doable, but it's not pretty, and I would rather have Wgpu depend on raw-window-metal so that we can maintain this logic in a central place. That's a discussion for a separate PR though, I think this PR is good to go now.

@Wumpf
Copy link
Member

Wumpf commented Sep 8, 2024

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!

@Wumpf Wumpf merged commit fb0cb1e into gfx-rs:trunk Sep 8, 2024
25 checks passed
@madsmtm madsmtm deleted the improve-metal-resizing branch September 8, 2024 14:45
@jimblandy
Copy link
Member

@madsmtm Thank you so much for the comments in wgpu_hal::metal::Surface::get_metal_layer! They're very helpful.

ArthurBrussee pushed a commit to ArthurBrussee/wgpu that referenced this pull request Sep 15, 2024
* [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
ArthurBrussee pushed a commit to ArthurBrussee/wgpu that referenced this pull request Sep 16, 2024
* [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
ArthurBrussee added a commit to ArthurBrussee/wgpu that referenced this pull request Sep 16, 2024
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.

Resizing on macOS is cursed again
5 participants