-
Notifications
You must be signed in to change notification settings - Fork 885
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
Skip releasing displays that are unavailable #3733
Skip releasing displays that are unavailable #3733
Conversation
While investigating bevyengine/bevy#13669 (comment) I saw that there was chance that this method is called with a display handle that relates to a display (monitor) that was disconnected. This led to a panic in the example added in the bevy PR. |
2f1fdf6
to
ae63c12
Compare
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.
Noting something that got me confused at first, it's been a while since I read this code: "release" in this context means "release a previously captured display", not "release the resources associated with this pointer" like it usually does.
What is the exact assertion error that you get? I'm asking because I'd like to know the error type that you're hitting as the result of CGDisplayRelease
?
In any case, it sound like the proper fix instead would be to remove the assertion? And instead error or warning log the result code.
Nvmd, I just saw your logs in the linked Bevy issue, it sounds like you're hitting |
Thanks for looking at this! Yeah that's the error code I saw. Here is the full log with a few 2024-06-17T08:13:55.528582Z INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "MacOS 14.4.1 ", kernel: "23.4.0", cpu: "Apple M1 Pro", core_count: "10", memory: "32.0 GiB" }
2024-06-17T08:13:55.626080Z INFO bevy_render::renderer: AdapterInfo { name: "Apple M1 Pro", vendor: 0, device: 0, device_type: IntegratedGpu, driver: "", driver_info: "", backend: Metal }
2024-06-17T08:13:56.242942Z INFO bevy_winit::system: Creating new window "Monitor #41039" (Entity { index: 2, generation: 1 })
2024-06-17T08:13:56.726636Z INFO bevy_winit::system: Creating new window "Monitor #16901" (Entity { index: 5, generation: 1 })
2024-06-17T08:13:57.045025Z WARN bevy_time: time_system did not receive the time from the render world! Calculations depending on the time may be incorrect.
2024-06-17T08:14:01.719370Z INFO bevy_winit::system: Monitor removed Entity { index: 0, generation: 1 }
2024-06-17T08:14:01.749790Z INFO bevy_winit::system: Closing window Entity { index: 5, generation: 1 }
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1397:17] video_mode = VideoModeHandle {
size: PhysicalSize {
width: 5120,
height: 2160,
},
bit_depth: 32,
refresh_rate_millihertz: 60000,
monitor: MonitorHandle {
name: Some(
"Monitor #16901",
),
native_identifier: 2,
size: PhysicalSize {
width: 1,
height: 1,
},
position: PhysicalPosition {
x: 0,
y: 0,
},
scale_factor: 1.0,
refresh_rate_millihertz: Some(
60000,
),
..
},
}
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1398:17] video_mode.monitor() = MonitorHandle {
name: Some(
"Monitor #16901",
),
native_identifier: 2,
size: PhysicalSize {
width: 1,
height: 1,
},
position: PhysicalPosition {
x: 0,
y: 0,
},
scale_factor: 1.0,
refresh_rate_millihertz: Some(
60000,
),
..
}
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1399:17] video_mode.monitor().native_identifier() = 2
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1400:17] video_mode.monitor() = MonitorHandle {
name: Some(
"Monitor #16901",
),
native_identifier: 2,
size: PhysicalSize {
width: 1,
height: 1,
},
position: PhysicalPosition {
x: 0,
y: 0,
},
scale_factor: 1.0,
refresh_rate_millihertz: Some(
60000,
),
..
}
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1401:17] video_mode.native_mode.0 = 0x00000001268378f0
thread 'main' panicked at /Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1407:25:
assertion `left == right` failed
left: 1001
right: 0
stack backtrace:
0: rust_begin_unwind
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:645:5
1: core::panicking::panic_fmt
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:72:14
2: core::panicking::assert_failed_inner
3: core::panicking::assert_failed
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:298:5
4: winit::platform_impl::platform::window_delegate::WindowDelegate::set_fullscreen
at /Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1407:25
5: winit::window::Window::set_fullscreen::{{closure}}
at /Users/pascal/code/rust-windowing/winit/src/window.rs:1138:50
6: winit::platform_impl::platform::window::Window::maybe_wait_on_main::{{closure}}
at /Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window.rs:49:46
7: objc2_foundation::thread::MainThreadBound<T>::get_on_main::{{closure}}
at /Users/pascal/.cargo/registry/src/index.crates.io-6f17d22bba15001f/objc2-foundation-0.2.2/src/thread.rs:440:27
8: objc2_foundation::thread::run_on_main
at /Users/pascal/.cargo/registry/src/index.crates.io-6f17d22bba15001f/objc2-foundation-0.2.2/src/thread.rs:113:9
9: objc2_foundation::thread::MainThreadBound<T>::get_on_main
at /Users/pascal/.cargo/registry/src/index.crates.io-6f17d22bba15001f/objc2-foundation-0.2.2/src/thread.rs:440:9
10: winit::platform_impl::platform::window::Window::maybe_wait_on_main
at /Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window.rs:49:9
11: winit::platform_impl::platform::window::Window::maybe_queue_on_main
at /Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window.rs:42:9
12: winit::window::Window::set_fullscreen
at /Users/pascal/code/rust-windowing/winit/src/window.rs:1138:9
13: bevy_winit::system::despawn_windows
at ./crates/bevy_winit/src/system.rs:215:17
14: core::ops::function::FnMut::call_mut
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/ops/function.rs:166:5
15: core::ops::function::impls::<impl core::ops::function::FnMut<A> for &mut F>::call_mut
at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/ops/function.rs:294:13
16: <Func as bevy_ecs::system::function_system::SystemParamFunction<fn(F0,F1,F2,F3,F4,F5,F6,F7) .> Out>>::run::call_inner
at ./crates/bevy_ecs/src/system/function_system.rs:704:21
17: <Func as bevy_ecs::system::function_system::SystemParamFunction<fn(F0,F1,F2,F3,F4,F5,F6,F7) .> Out>>::run
at ./crates/bevy_ecs/src/system/function_system.rs:707:17
18: <bevy_ecs::system::function_system::FunctionSystem<Marker,F> as bevy_ecs::system::system::System>::run_unsafe
at ./crates/bevy_ecs/src/system/function_system.rs:534:19
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Encountered a panic in system `bevy_winit::system::despawn_windows`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`! |
Thanks, seems like the Could you try running Will probably take a closer look myself in a few days when I get the time and have access to an external monitor (especially if reminded). In any case, I still currently think the correct solution is to log the error instead of asserting it. |
Sure! I did this now: + dbg!(video_mode);
+ dbg!(video_mode.monitor());
+ dbg!(video_mode.monitor().native_identifier());
+ dbg!(video_mode.monitor());
+ dbg!(video_mode.native_mode.0);
+ let available_monitors = monitor::available_monitors();
+ dbg!(available_monitors);
+
+ let monitor = &video_mode.monitor();
+ dbg!(unsafe { CGDisplayIsCaptured(monitor.native_identifier()) });
unsafe {
ffi::CGRestorePermanentDisplayConfiguration();
assert_eq!(
- ffi::CGDisplayRelease(video_mode.monitor().native_identifier()),
+ ffi::CGDisplayRelease(monitor.native_identifier()),
ffi::kCGErrorSuccess
);
}; and it yields 2024-06-17T08:52:20.221625Z INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "MacOS 14.4.1 ", kernel: "23.4.0", cpu: "Apple M1 Pro", core_count: "10", memory: "32.0 GiB" }
2024-06-17T08:52:20.345355Z INFO bevy_render::renderer: AdapterInfo { name: "Apple M1 Pro", vendor: 0, device: 0, device_type: IntegratedGpu, driver: "", driver_info: "", backend: Metal }
2024-06-17T08:52:20.935595Z INFO bevy_winit::system: Creating new window "Monitor #41039" (Entity { index: 2, generation: 1 })
2024-06-17T08:52:21.383400Z INFO bevy_winit::system: Creating new window "Monitor #16901" (Entity { index: 5, generation: 1 })
2024-06-17T08:52:21.767103Z WARN bevy_time: time_system did not receive the time from the render world! Calculations depending on the time may be incorrect.
2024-06-17T08:52:26.558842Z INFO bevy_winit::system: Monitor removed Entity { index: 0, generation: 1 }
2024-06-17T08:52:26.587391Z INFO bevy_winit::system: Closing window Entity { index: 5, generation: 1 }
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1398:17] video_mode = VideoModeHandle {
size: PhysicalSize {
width: 5120,
height: 2160,
},
bit_depth: 32,
refresh_rate_millihertz: 60000,
monitor: MonitorHandle {
name: Some(
"Monitor #16901",
),
native_identifier: 2,
size: PhysicalSize {
width: 1,
height: 1,
},
position: PhysicalPosition {
x: 0,
y: 0,
},
scale_factor: 1.0,
refresh_rate_millihertz: Some(
60000,
),
..
},
}
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1399:17] video_mode.monitor() = MonitorHandle {
name: Some(
"Monitor #16901",
),
native_identifier: 2,
size: PhysicalSize {
width: 1,
height: 1,
},
position: PhysicalPosition {
x: 0,
y: 0,
},
scale_factor: 1.0,
refresh_rate_millihertz: Some(
60000,
),
..
}
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1400:17] video_mode.monitor().native_identifier() = 2
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1401:17] video_mode.monitor() = MonitorHandle {
name: Some(
"Monitor #16901",
),
native_identifier: 2,
size: PhysicalSize {
width: 1,
height: 1,
},
position: PhysicalPosition {
x: 0,
y: 0,
},
scale_factor: 1.0,
refresh_rate_millihertz: Some(
60000,
),
..
}
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1402:17] video_mode.native_mode.0 = 0x000000014194fd30
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1404:17] available_monitors = [
MonitorHandle {
name: Some(
"Monitor #41039",
),
native_identifier: 1,
size: PhysicalSize {
width: 3024,
height: 1964,
},
position: PhysicalPosition {
x: 0,
y: 0,
},
scale_factor: 2.0,
refresh_rate_millihertz: Some(
120000,
),
..
},
]
[/Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1407:17] unsafe { CGDisplayIsCaptured(monitor.native_identifier()) } = 0
thread 'main' panicked at /Users/pascal/code/rust-windowing/winit/src/platform_impl/macos/window_delegate.rs:1410:21:
assertion `left == right` failed
left: 1001
right: 0
stack backtrace: … Curious how |
ae63c12
to
6e9a1c9
Compare
Rebase now. @madsmtm did you have time to look at this? Is there anything else I should check? |
This is obviously outside the scope of this PR, but ideally we'd have a way to react to the missing monitor in user code, i.e. by returning an error. But the log is definitely an upgrade over just panicking, which feels like it's almost never the right move. |
Just thought about this again. If it makes sense, I could change it so that instead of the |
Is there anything blocking this fix making it into |
Prevent assertion error when trying to close a fullscreen window that was on a display that got disconnected.
6e9a1c9
to
768f954
Compare
Prevent assertion error when trying to close a fullscreen window that was on a display that got disconnected.
changelog
module if knowledge of this change could be valuable to users