Arc WinitWindow and extract to render world #12524
Conversation
james7132
added
C-Bug
There was a problem hiding this comment.
@james7132 I don't agree with feature-flagging this :)
It looks like it should always work, even when pipelined rendering is off, and maintaining compile-gated code like this is substantially harder.
That said, I don't feel strongly enough to block on this point: the cost is small and the fix is important.
I don't really feel either way, but i should add that this fix forcefully skips the last frame of window events since it doesn't have a Window component anymore. This is irrelevant in pipelined because it already does (needs it's own issue) either unintentionally or crashing. Forcing this fix upon non-pipelined also makes it so they skip this last frame of window events even though this bug can't happen. Personally, I would keep it feature-flagged until a definite fix can be found rather than adding a "bug" to non-pipelined. |
|
Maybe we can wrap the window in an Arc or something? |
|
My biggest concern here is that the extra frame that we're now running potentially has extra user or plugin logic running through it, which may run contrary to the expectations that closing the window will immediately send an AppExit event. With that said, this problem persists even with the cfg block, which comes with more complexity. I'm fine with this as is for now, but @JMS55's suggestion to internally wrap it in an Arc may be the right strategy here. |
# Conflicts: # crates/bevy_winit/src/lib.rs
|
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
|
Unsure what's up with CI and the bot but fix should be ready for review. |
s-puig
changed the title
| } else { | ||
| bevy_utils::tracing::warn!("Winit feature was enabled but couldn't detect WinitPlugin. Are you sure you loaded this after WinitPlugin?"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This potentially hard-couples bevy_render and bevy_winit. I'm not sure if we want to do this.
There was a problem hiding this comment.
I'm open to alternatives, but i don't see a way around it if we want to properly fix it. There is always the option of using a hack (delaying the winit window removal in app world).
There was a problem hiding this comment.
@s-puig Take a look at this: Friz64@0561754
It also works as a fix, and avoids the hard-coupling to bevy_winit. I'm not convinced that dynamic dispatch is the most elegant solution here.
… using pipelined rendering (#12978) # Objective A `RawWindowHandle` is only valid as long as the window it was retrieved from is alive. Extend the lifetime of the window, so that the `RawWindowHandle` doesn't outlive it, and bevy doesn't crash when closing a window a pipelined renderer is drawing to. - Fix #11236 - Fix #11150 - Fix #11734 - Alternative to / Closes #12524 ## Solution Introduce a `WindowWrapper` that takes ownership of the window. Require it to be used when constructing a `RawHandleWrapper`. This forces windowing backends to store their window in this wrapper. The `WindowWrapper` is implemented by storing the window in an `Arc<dyn Any + Send + Sync>`. We use dynamic dispatch here because we later want the `RawHandleWrapper` to be able dynamically hold a reference to any windowing backend's window. But alas, the `WindowWrapper` itself is still practically invisible to windowing backends, because it implements `Deref` to the underlying window, by storing its type in a `PhantomData`. --- ## Changelog ### Added - Added `WindowWrapper`, which windowing backends are now required to use to store their underlying window. ### Fixed - Fixed a safety problem which caused crashes when closing bevy windows when using pipelined rendering. ## Migration Guide - Windowing backends now need to store their window in the new `WindowWrapper`.
Objective
Fixes #11236
(likely)Fixes #11150(likely)Fixes #11734Problem
Window crashes when using pipelined rendering under GPU-bound scenarios.
It is not exclusive to XWayland and is based on a race condition with pipelined rendering.
Winit windows might be deleted on app world while the window is still alive in render world, if the render world window tries to access a winit resource (i.e. a surface or raw handle) it panics because the winit window no longer exists.
All those crashes go away by disabling pipelined rendering.
Solution
Arc<WinitWindow>and extract it when using pipelined rendering.Alternative solutions