bevy_render: Pass DisplayHandle to wgpu for compositor-aware Instance creation#20358
Draft
MarijnS95 wants to merge 2 commits intobevyengine:mainfrom
Draft
bevy_render: Pass DisplayHandle to wgpu for compositor-aware Instance creation#20358MarijnS95 wants to merge 2 commits intobevyengine:mainfrom
DisplayHandle to wgpu for compositor-aware Instance creation#20358MarijnS95 wants to merge 2 commits intobevyengine:mainfrom
Conversation
MarijnS95
changed the title
DisplayHandle to wgpu for compositor-aware Instance creation
alice-i-cecile
added
A-Rendering
Author
|
@alice-i-cecile just curious why this PR, which is effectively a one-line change of code on top of dependent PR #20357 is marked
D-Complex
Edit: this likely needs
O-OpenGL
|
alice-i-cecile
added
D-Modest
Member
|
Very reasonable; I've amended the labels :) |
Long ago `raw-window-handle` and `winit` split out a `RawDisplayHandle` type and respective traits for dealing with the connection - or at the very least type - of a compositor, and typically implement this for a `Window` directly. `wgpu` and `bevy` seem to have caught on to the latter and folded the two traits/types together because `Window` provides it, but miss a critical goal here: that `(Raw)DisplayHandle` is important for initialization of certain graphics APIs. In the case of Wayland all resources are unique per connection, and in general for others it's important to distinguish between Wayland and X11 if `winit` chose one over the other, even if both are available; currently that's just guesswork inside `wgpu`. Newer APIs like Vulkan don't suffer from this, but older graphics APIs like OpenGL and specifically the EGL backend (or GLX for X11) set up their entire state based on the compositor connection (alternatives are available) even if it's ultimately "only" important for the surface that is going to be rendered into. Note that I haven't yet checked all the safety constraints carefully in this PR; some existing messages seem flawed but I need to perform a clean audit from scratch to denote what limitations should apply to the newly proposed `RawDisplayHandleWrapper` as well.
…tance` creation Multiple community members have asked me to look into persistent `BadDisplay` crashes on `wgpu` when using its EGL backend in conjunction with Wayland. Besides realizing that this backend is quite literally the definition of Undefined Behaviour itself, various hacks to patch the EGL context - which is compositor-specific - inside the `Instance`/`Adapter` with the actual `wl_display` handle as soon as a `surface` is created will not only void any previously created GL resources, only the `Instance` handles are patched leaving any previously queried `Adapter`s to reference destroyed EGL objects causing those `BadDisplay` errors. Solving that handle lifetime/ownership problem has yet to be done (and is why crates like `glutin` exist...), but at the very least we might as well create an `EGLDisplay` and `EGLContext` which is compatible with the current compositor from the get-go, which is what gfx-rs/wgpu#8012 allows callers to do. This is one of the reasons why `raw-display-handle` split out a `RawDisplayHandle` type and related enums: to know up-front what compositor is used (telling X11 and Wayland apart, if both could be used concurrently instead of letting `wgpu` "guess") and to use the same `wl_display` compositor connection as `winit`. There are alternatives available, which is why this and the related `wgpu` PR is a draft to shake out some feedback. Anything that's involving more complexity inside `wgpu`'s EGL backend should be attempted incredibly cautiously if it weren't for a full rewrite on top of a higher-level EGL abstraction.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on: #20357
Depends on: gfx-rs/wgpu#8012
Objective
BadDisplayafter winit 0.30 on Wayland OpenGL #13923Solution
Multiple community members have asked me to look into persistent
BadDisplaycrashes onwgpuwhen using its EGL backend in conjunction with Wayland. Besides realizing that this backend is quite literally the definition of Undefined Behaviour itself, various hacks to patch the EGL context - which is compositor-specific - inside theInstance/Adapterwith the actualwl_displayhandle as soon as asurfaceis created will not only void any previously created GL resources, only theInstancehandles are patched leaving any previously queriedAdapters to reference destroyed EGL objects causing thoseBadDisplayerrors.Solving that handle lifetime/ownership problem has yet to be done (and is why crates like
glutinexist...), but at the very least we might as well create anEGLDisplayandEGLContextwhich is compatible with the current compositor from the get-go, which is what gfx-rs/wgpu#8012 allows callers to do. This is one of the reasons whyraw-display-handlesplit out aRawDisplayHandletype and related enums: to know up-front what compositor is used (telling X11 and Wayland apart, if both could be used concurrently instead of lettingwgpu"guess") and to use the samewl_displaycompositor connection aswinit.There are alternatives available, which is why this and the related
wgpuPR is a draft to shake out some feedback. Anything that's involving more complexity insidewgpu's EGL backend should be attempted incredibly cautiously if it weren't for a full rewrite on top of a higher-level EGL abstraction.Testing
WGPU_BACKEND=gl cargo r --example 3d_bloom -Fwayland -Fbevy_render/decoupled_nagaon a Wayland compositor.