Skip to content

Conversation

@aloucks
Copy link
Contributor

@aloucks aloucks commented Mar 20, 2025

  • Tested on all platforms changed

Comment on lines +979 to +985
let size = self.window.surface_size();
if let (Some(width), Some(height)) =
(NonZeroU32::new(size.width), NonZeroU32::new(size.height))
{
self.surface.resize(width, height)?;
}

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a bug in some other place? The resize must be handled inside the resize, and if the window changes the size in draw but resize event is not delivered at this point, this means that the Resize should be done from within whatever triggered the draw?

Also, which platform resulted in this being required?

@aloucks
Copy link
Contributor Author

aloucks commented Mar 20, 2025

It panics on MacOS when indexing the buffer. There's no resize event logged before the panic.

025-03-20T12:32:08.926853Z  INFO application: Executing action: ToggleFullscreen

thread 'main' panicked at examples/application.rs:993:21:
index out of bounds: the len is 1920000 but the index is 1920000

buffer[index] = match self.theme {

@aloucks
Copy link
Contributor Author

aloucks commented Mar 20, 2025

I added some extra debug printlns and confirmed that the RedrawRequested event comes before the SurfaceResized after ToggleFullscreen is triggered on MacOS. ToggleSimpleFullscreen also panics in the same way.

I think you're right. This seems like a bug in winit and not in the example application.rs.

@aloucks
Copy link
Contributor Author

aloucks commented Mar 20, 2025

The windowDidResize event is firing before the redraw, but it does not emit a SurfaceResized event:

#[unsafe(method(windowDidResize:))]
fn window_did_resize(&self, _: Option<&AnyObject>) {
trace_scope!("windowDidResize:");
// NOTE: WindowEvent::SurfaceResized is reported using NSViewFrameDidChangeNotification.
self.emit_move_event();
}

The NSViewFrameDidChangeNotification is not triggered before the redraw request in this scenario. I can see in the trace logs that it does fire, but it's logged after the redraw request and panic.

@madsmtm You left the note here that SurfaceResized happens in NSViewFrameDidChangeNotification. I've tried to experiment with emitting the SurfaceResized event in the windowDidResize handler, but it still seems to get queued after RedrawRequested.

I think the issue comes down to the "source of truth" for the surface size (or any of these attributes). The value reported by Window::surface_size() and the value in the SurfaceResized events seems to be temporarily out of sync. The method is reporting the new value before the event is delivered to the application handler. I would expect the events to fire before the method reports the new value. This seems to be the case on Windows, but not on MacOS.

Windows

Windows also seems to fire two surface events. One that looks closer to the old value, but possibly including the window decorations?

2025-03-20T22:05:37.752082Z  INFO application: Executing action: ToggleFullscreen
SurfaceResized
2025-03-20T22:05:37.755868Z  INFO application: Surface resized to PhysicalSize { width: 816, height: 639 }
SurfaceResized
2025-03-20T22:05:37.756606Z  INFO application: Surface resized to PhysicalSize { width: 2560, height: 1440 }
RedrawRequested

MacOS

2025-03-20T22:01:11.088989Z  INFO application: Executing action: ToggleFullscreen
RedrawRequested

thread 'main' panicked at examples/application.rs:995:21:

@kchibisov
Copy link
Member

I think the issue comes down to the "source of truth" for the surface size (or any of these attributes). The value reported by Window::surface_size() and the value in the SurfaceResized events seems to be temporarily out of sync. The method is reporting the new value before the event is delivered to the application handler. I would expect the events to fire before the method reports the new value. This seems to be the case on Windows, but not on MacOS.

That's exactly the reason I've asked when it happens, since the size desync shouldn't happen.

@kchibisov kchibisov requested a review from madsmtm March 21, 2025 09:51
@aloucks aloucks force-pushed the fix-application-example branch from b2317b1 to e00689d Compare March 23, 2025 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants