Closed kvark closed 3 years ago
So I rewrote the present
bit like this
if image.present_with_transaction {
command_buffer.commit();
command_buffer.wait_until_scheduled();
image.drawable.present();
} else {
command_buffer.present_drawable(&image.drawable);
command_buffer.commit();
}
Because the apple docs on presentWithTransaction says that you "commit, waitUntilScheduled, present" to ensure that the transaction is available.
This works, in that all frames I get are perfect, but after couple of back-to-back resize events, I get a second long stall which is subpar.
I wonder if I'm flooding it with frames somehow. Funny thing, with this setup, my fps counter goes to 120, disabling the feature and its back to 60.
Though I get "not annoying anymore, even if technically not perfect" solution was to put this at the start of Surface::new
(as mentioned in the Tristan Hume's blog post)
Perhaps winit could do that instead.
let NSViewLayerContentsPlacementTopLeft = 11;
if let Some(v) = view {
unsafe { let () = msg_send![v.as_ptr(), setLayerContentsPlacement:NSViewLayerContentsPlacementTopLeft]; }
}
@scoopr that seems like an interesting solution without any obvious costs. Want to make a PR for it?
For whatever reason, setting the layerContentsPlacement in winit side didn't work for me, I wonder if it somehow gets reset when the layer is added to the view.
There is few downsides I've seen so far.. One is a smaller thing, the resizing from left edge is can result in black bars to the right side etc., but its not like something like vscode handles that all that much better.
But a real issue is that the dpi changes (like dragging a window from hires screen to lores) leaves the layer to be completely wrong size. I guess it needs to watch for those notifications and set the backing scale factor. The default scaling behavior handles that fine.
Just to document my tests,
WindowEvent::Resized
, I'm supposing it is sending out more resized events than it is willing to present for the compositor and then runs out of drawables? needsDisplayOnBoundsChange
property to true would then give me Event::RedrawRequested
from winit, whenever the compositor feels like its time to redraw, but those event didn't come through, didn't yet investigate why. window.request_redraw()
from the Resized event works but is again too late and stretching would occur@scoopr I just realized I didn't push the changes to the right branch. The thing that works for me is close to what you describe in https://github.com/gfx-rs/gfx/pull/3627#issuecomment-819786578, and it's pushed now.
Am I reading this right that the only concern with the change was having a long stall, and this turned out to be a problem with how you handle Resize
, and not something we should be concerned about in gfx?
Also, do you understand why the original code here didn't work as expected? The one with the "scheduled handler", I'd expect it to do the same thing, just more asynchronously.
Well, if I don't render within the Resized event, then there is no resized frame to display, so the result is perfectly synchronized wrong frame.
Not sure why the scheduled way didn't work, perhaps there is some work done after the final user callback when properly waiting for it 🤷 Does metal call those callbacks straight from the NSRunLoop queue? I guess you could post a dispatch_async on the mainloop so it would run the persent just after the callback as opposed in the callback, dunno.
I think the layer placement should be done in any case, because that makes the error-behaviour to be the least annoying and works in all cases, just need to take care of the small details so the size is correctly tracked.
It would be nice to have the present_with_transaction flow to work as well, maybe by making one of the vsyncing PresentModes use it, or as an additional option. But need to be careful with it so that it wouldn't be a non-obvious perfomance footgun, or document well on the restrictions it might impose.
@scoopr ok, would you want to make a PR for layer placement then?
On closer inspection
layer:shouldInheritContentsScale:fromWindow:
implementation didn't handle the dpi change when I tested the layerPlacement .. but then I wonder, you set a GfxManagedMetalLayerDelegate
as the CAMetalLayer
delegate, but that is excepting a CALayerDelegate
, but the .. shouldInheritContentsScale..
is part of NSViewLayerContentScaleDelegate
. I wonder if that method should actually be part of the NSView
implementation? The docs are not super clear on this, but the protocol name perhaps suggests this.The present_to_transaction implementation in this pull request is I think fine, and I think the option should be exposed for those who want to utilize it. Might warrant some extra testing, as:
Are you going to put this in for 0.8? Its a (small) api change
@scoopr I thought this is internal API for gfx-backend-metal, not gfx-hal exposed. So we could add this API in a non-breaking way at any point. Anyway, I'm going to make this mergable now.
bors r=scoopr
Add a presentation path that forces CoreAnimation transaction.