Skip to content
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

Update to wgpu 0.19 and raw-window-handle 0.6 #11280

Merged
merged 23 commits into from
Jan 26, 2024

Conversation

Elabajaba
Copy link
Contributor

@Elabajaba Elabajaba commented Jan 9, 2024

Objective

Keep core dependencies up to date.

Solution

Update the dependencies.

wgpu 0.19 only supports raw-window-handle (rwh) 0.6, so bumping that was included in this.

The rwh 0.6 version bump is just the simplest way of doing it. There might be a way we can take advantage of wgpu's new safe surface creation api, but I'm not familiar enough with bevy's window management to untangle it and my attempt ended up being a mess of lifetimes and rustc complaining about missing trait impls (that were implemented). Thanks to @MiniaczQ for the (much simpler) rwh 0.6 version bump code.

Unblocks #9172 and #10812

This might be blocked on cpal and oboe updating their ndk versions to 0.8, as they both currently target ndk 0.7 which uses rwh 0.5.2 Tested on android, and everything seems to work correctly (audio properly stops when minimized, and plays when re-focusing the app).


Changelog

  • wgpu has been updated to 0.19! The long awaited arcanization has been merged (for more info, see https://gfx-rs.github.io/2023/11/24/arcanization.html), and Vulkan should now be working again on Intel GPUs.
  • Targeting WebGPU now requires that you add the new webgpu feature (setting the RUSTFLAGS environment variable to --cfg=web_sys_unstable_apis is still required). This feature currently overrides the webgl2 feature if you have both enabled (the webgl2 feature is enabled by default), so it is not recommended to add it as a default feature to libraries without putting it behind a flag that allows library users to opt out of it! In the future we plan on supporting wasm binaries that can target both webgl2 and webgpu now that wgpu added support for doing so (see Support both WebGPU and WebGL2 in the same wasm binary #11505).
  • raw-window-handle has been updated to version 0.6.

Migration Guide

  • bevy_render::instance_index::get_instance_index() has been removed as the webgl2 workaround is no longer required as it was fixed upstream in wgpu. The BASE_INSTANCE_WORKAROUND shaderdef has also been removed.
  • WebGPU now requires the new webgpu feature to be enabled. The webgpu feature currently overrides the webgl2 feature so you no longer need to disable all default features and re-add them all when targeting webgpu, but binaries built with both the webgpu and webgl2 features will only target the webgpu backend, and will only work on browsers that support WebGPU.
    • Places where you conditionally compiled things for webgl2 need to be updated because of this change, eg:
      • #[cfg(any(not(feature = "webgl"), not(target_arch = "wasm32")))] becomes #[cfg(any(not(feature = "webgl") ,not(target_arch = "wasm32"), feature = "webgpu"))]
      • #[cfg(all(feature = "webgl", target_arch = "wasm32"))] becomes #[cfg(all(feature = "webgl", target_arch = "wasm32", not(feature = "webgpu")))]
      • if cfg!(all(feature = "webgl", target_arch = "wasm32")) becomes if cfg!(all(feature = "webgl", target_arch = "wasm32", not(feature = "webgpu")))
  • create_texture_with_data now also takes a TextureDataOrder. You can probably just set this to TextureDataOrder::default()
  • TextureFormat's block_size has been renamed to block_copy_size
  • See the wgpu changelog for anything I might've missed: https://github.com/gfx-rs/wgpu/blob/trunk/CHANGELOG.md
  • wgpu now surfaces errors at instance creation time, which may have you run into this error (we've also seen it with nsight instead of EOSOverlay):
2024-01-27T02:11:58.491767Z ERROR wgpu_hal::vulkan::instance: GENERAL [Loader Message (0x0)]
        loader_get_json: Failed to open JSON file C:\Program Files (x86)\Epic Games\Launcher\Portal\Extras\Overlay\EOSOverlayVkLayer-Win32.json
2024-01-27T02:11:58.492046Z ERROR wgpu_hal::vulkan::instance:   objects: (type: INSTANCE, hndl: 0x1fbe55dc070, name: ?)    
2024-01-27T02:11:58.492282Z ERROR wgpu_hal::vulkan::instance: GENERAL [Loader Message (0x0)]
        loader_get_json: Failed to open JSON file C:\Program Files (x86)\Epic Games\Launcher\Portal\Extras\Overlay\EOSOverlayVkLayer-Win64.json
2024-01-27T02:11:58.492525Z ERROR wgpu_hal::vulkan::instance:   objects: (type: INSTANCE, hndl: 0x1fbe55dc070, name: ?)

It just means that the program didn't properly cleanup their registry keys on an update/uninstall, and vulkan uses those keys to load validation layers. The fix is to backup your registry, then remove the offending keys in "Computer\HKEY_LOCAL_MACHINE\SOFTWARE\Khronos\Vulkan\ImplicitLayers".

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on labels Jan 9, 2024
@@ -831,6 +831,8 @@ impl RenderAsset for Image {
let texture = render_device.create_texture_with_data(
render_queue,
&self.texture_descriptor,
// TODO: Is this correct? Do we need to use `MipMajor` if it's a ktx2 file?
wgpu::util::TextureDataOrder::LayerMajor,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a new argument, I provided it with the default value so it should work the same way it did before. :)

We can explore forwarding this configuration to user API in further PRs.

self.0.window_handle
impl HasWindowHandle for ThreadLockedRawWindowHandleWrapper {
fn window_handle(&self) -> Result<WindowHandle, HandleError> {
// TODO: Unsure if this is the same safety as before
Copy link
Contributor

@MiniaczQ MiniaczQ Jan 10, 2024

Choose a reason for hiding this comment

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

I'll get this thread started since it's the biggest change here.

I think we still need to use raw handles. I believe you cannot store objects with lifetimes in the world, at least not ones with a shorter lifetime. We may be able to transmute lifetimes to outlive the World, but that would give false safety.

Copy link
Contributor

@daxpedda daxpedda Jan 18, 2024

Choose a reason for hiding this comment

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

If you wrap winit::window::Window in an Arc<Window>, you can share it with the surface while getting a 'static lifetime without any unsafe involved.

@mockersf
Copy link
Member

gfx-rs/wgpu#5044 may also need some work on the wasm side

@Elabajaba
Copy link
Contributor Author

Elabajaba commented Jan 17, 2024

gfx-rs/wgpu#5044 may also need some work on the wasm side

Yup, I was just thinking about that earlier. I guess I need to audit every cfg block in bevy_pbr/render/core_pipeline/sprite...

edit: Current status of web: webgpu seems to work, webgl2 doesn't (probably need to remove the webgpu feature from wgpu for webgl2 builds for now?)

@Elabajaba
Copy link
Contributor Author

Blocked on gfx-rs/wgpu#5088 and getting webgl2 working.

@mockersf
Copy link
Member

mockersf commented Jan 18, 2024

This might be blocked on cpal and oboe updating their ndk versions to 0.8, as they both currently target ndk 0.7 which uses rwh 0.5.2

RustAudio/cpal#820 and katyo/oboe-rs#57

and Elabajaba#2 to build this PR on Android

Copy link
Contributor

You added a new feature but didn't add a description for it. Please update the root Cargo.toml file.

@Elabajaba
Copy link
Contributor Author

I've added a (temporary) webgpu feature that's mutually exclusive with webgl2 for now (compile time error) to get them building. (I know it's incorrect, but it's needed for testing).

Currently running into an issue where there's a ton of "error[E0412]: cannot find type Gpu*** in crate web_sys" in wgpu-0.19.0\src\backend\webgpu.rs when compiling with both webgl2 and webgpu features enabled.

Copy link
Contributor

You added a new feature but didn't add a description for it. Please update the root Cargo.toml file.

@mockersf
Copy link
Member

mockersf commented Jan 19, 2024

I've added a (temporary) webgpu feature that's mutually exclusive with webgl2 for now (compile time error) to get them building. (I know it's incorrect, but it's needed for testing).

I think it's OK for now, we can work once merged to get them working together

Currently running into an issue where there's a ton of "error[E0412]: cannot find type Gpu*** in crate web_sys" in wgpu-0.19.0\src\backend\webgpu.rs when compiling with both webgl2 and webgpu features enabled.

webgpu still needs RUSTFLAGS=--cfg=web_sys_unstable_apis

@Elabajaba Elabajaba changed the title Update to wgpu trunk/0.19 and raw-window-handle 0.6 Update to wgpu 0.19 and raw-window-handle 0.6 Jan 22, 2024
Copy link
Contributor

You added a new feature but didn't add a description for it. Please update the root Cargo.toml file.

1 similar comment
Copy link
Contributor

You added a new feature but didn't add a description for it. Please update the root Cargo.toml file.

@Elabajaba
Copy link
Contributor Author

Reworked this so that instead of making webgpu and webgl mutually exclusive features and a compile error, enabling the webgpu feature now overrides the webgl feature as people seemed to maybe prefer that approach when I asked on discord (https://discord.com/channels/691052431525675048/692572690833473578/1199092028164821082)

Currently some examples are broken on webgl2 (load_gltf works fine, but the flight helmet explodes in deferred_rendering and anti_aliasing. many_foxes and shader_prepass straight up don't work on webgl2). I haven't really tested webgpu much yet other than load_gltf and deferred_rendering work.

Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@JMS55 JMS55 added this to the 0.13 milestone Jan 24, 2024
@Elabajaba Elabajaba marked this pull request as ready for review January 24, 2024 05:51
Copy link
Contributor

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@Vrixyz Vrixyz mentioned this pull request Jan 24, 2024
23 tasks
@Elabajaba
Copy link
Contributor Author

Tested this on my phone and android appears to work fine (audio played correctly, paused when minimized, plays when re-focused).

@@ -831,6 +831,8 @@ impl RenderAsset for Image {
let texture = render_device.create_texture_with_data(
render_queue,
&self.texture_descriptor,
// TODO: Is this correct? Do we need to use `MipMajor` if it's a ktx2 file?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we do, according to the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

::default() is equivalent to what we were doing before, and I can't visually see a difference with my ktx2 bistro if I change it to mipmajor, but I'm bad at spotting texture bugs.

@@ -193,7 +195,8 @@ fn extract_windows(
}

struct SurfaceData {
surface: wgpu::Surface,
// TODO: what lifetime should this be?
Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving a note that we have to figure this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think 'static is what we want for now until we rewrite bevy_winit to use safe winit APIs and surface creation.

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

LGTM

I tried a bunch of examples and I didn't find any issues (well, none that weren't already on main) but I only tested on my windows 10 desktop with my 4080. I also only tested vulkan and DX12.

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Jan 26, 2024

Transparent window is broken on Windows 11, but I don't think it's this PRs fault, I'll update this in a sec.
Yeah, just Win11 things, this PR does not make it better/worse.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 26, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 26, 2024
Merged via the queue into bevyengine:main with commit 35ac1b1 Jan 26, 2024
26 of 27 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

Keep core dependencies up to date.

## Solution

Update the dependencies.

wgpu 0.19 only supports raw-window-handle (rwh) 0.6, so bumping that was
included in this.

The rwh 0.6 version bump is just the simplest way of doing it. There
might be a way we can take advantage of wgpu's new safe surface creation
api, but I'm not familiar enough with bevy's window management to
untangle it and my attempt ended up being a mess of lifetimes and rustc
complaining about missing trait impls (that were implemented). Thanks to
@MiniaczQ for the (much simpler) rwh 0.6 version bump code.

Unblocks bevyengine#9172 and
bevyengine#10812

~~This might be blocked on cpal and oboe updating their ndk versions to
0.8, as they both currently target ndk 0.7 which uses rwh 0.5.2~~ Tested
on android, and everything seems to work correctly (audio properly stops
when minimized, and plays when re-focusing the app).

---

## Changelog

- `wgpu` has been updated to 0.19! The long awaited arcanization has
been merged (for more info, see
https://gfx-rs.github.io/2023/11/24/arcanization.html), and Vulkan
should now be working again on Intel GPUs.
- Targeting WebGPU now requires that you add the new `webgpu` feature
(setting the `RUSTFLAGS` environment variable to
`--cfg=web_sys_unstable_apis` is still required). This feature currently
overrides the `webgl2` feature if you have both enabled (the `webgl2`
feature is enabled by default), so it is not recommended to add it as a
default feature to libraries without putting it behind a flag that
allows library users to opt out of it! In the future we plan on
supporting wasm binaries that can target both webgl2 and webgpu now that
wgpu added support for doing so (see
bevyengine#11505).
- `raw-window-handle` has been updated to version 0.6.

## Migration Guide

- `bevy_render::instance_index::get_instance_index()` has been removed
as the webgl2 workaround is no longer required as it was fixed upstream
in wgpu. The `BASE_INSTANCE_WORKAROUND` shaderdef has also been removed.
- WebGPU now requires the new `webgpu` feature to be enabled. The
`webgpu` feature currently overrides the `webgl2` feature so you no
longer need to disable all default features and re-add them all when
targeting `webgpu`, but binaries built with both the `webgpu` and
`webgl2` features will only target the webgpu backend, and will only
work on browsers that support WebGPU.
- Places where you conditionally compiled things for webgl2 need to be
updated because of this change, eg:
- `#[cfg(any(not(feature = "webgl"), not(target_arch = "wasm32")))]`
becomes `#[cfg(any(not(feature = "webgl") ,not(target_arch = "wasm32"),
feature = "webgpu"))]`
- `#[cfg(all(feature = "webgl", target_arch = "wasm32"))]` becomes
`#[cfg(all(feature = "webgl", target_arch = "wasm32", not(feature =
"webgpu")))]`
- `if cfg!(all(feature = "webgl", target_arch = "wasm32"))` becomes `if
cfg!(all(feature = "webgl", target_arch = "wasm32", not(feature =
"webgpu")))`
- `create_texture_with_data` now also takes a `TextureDataOrder`. You
can probably just set this to `TextureDataOrder::default()`
- `TextureFormat`'s `block_size` has been renamed to `block_copy_size`
- See the `wgpu` changelog for anything I might've missed:
https://github.com/gfx-rs/wgpu/blob/trunk/CHANGELOG.md

---------

Co-authored-by: François <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Apr 23, 2024
The shader code was removed in #11280, but we never cleaned up the rust
code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants