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

Webgpu support #8336

Merged
merged 20 commits into from
May 4, 2023
Merged

Webgpu support #8336

merged 20 commits into from
May 4, 2023

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Apr 9, 2023

Objective

Solution

For async renderer initialisation

  • Update the plugin lifecycle:
    • app builds the plugin
      • calls plugin.build
      • registers the plugin
    • app starts the event loop
    • event loop waits for ready of all registered plugins in the same order
      • returns true by default
    • then call all finish then all cleanup in the same order as registered
    • then execute the schedule

In the case of the renderer, to avoid anything async:

  • building the renderer plugin creates a detached task that will send back the initialised renderer through a mutex in a resource
  • ready will wait for the renderer to be present in the resource
  • finish will take that renderer and place it in the expected resources by other plugins
    • other plugins (that expect the renderer to be available) finish are called and they are able to set up their pipelines
  • cleanup is called, only custom one is still for pipeline rendering

For WebGPU support

  • update the build-wasm-example script to support passing --api webgpu that will build the example with WebGPU support
  • feature for webgl2 was always enabled when building for wasm. it's now in the default feature list and enabled on all platforms, so check for this feature must also check that the target_arch is wasm32

Migration Guide

  • Plugin::setup has been renamed Plugin::cleanup
  • Plugin::finish has been added, and plugins adding pipelines should do it in this function instead of Plugin::build
// Before
impl Plugin for MyPlugin {
    fn build(&self, app: &mut App) {
        app.insert_resource::<MyResource>
            .add_systems(Update, my_system);

        let render_app = match app.get_sub_app_mut(RenderApp) {
            Ok(render_app) => render_app,
            Err(_) => return,
        };

        render_app
            .init_resource::<RenderResourceNeedingDevice>()
            .init_resource::<OtherRenderResource>();
    }
}

// After
impl Plugin for MyPlugin {
    fn build(&self, app: &mut App) {
        app.insert_resource::<MyResource>
            .add_systems(Update, my_system);
    
        let render_app = match app.get_sub_app_mut(RenderApp) {
            Ok(render_app) => render_app,
            Err(_) => return,
        };
    
        render_app
            .init_resource::<OtherRenderResource>();
    }

    fn finish(&self, app: &mut App) {
        let render_app = match app.get_sub_app_mut(RenderApp) {
            Ok(render_app) => render_app,
            Err(_) => return,
        };
    
        render_app
            .init_resource::<RenderResourceNeedingDevice>();
    }
}

@mockersf mockersf added O-Web Specific to web (WASM) builds A-Rendering Drawing game state to the screen A-App Bevy apps and plugins labels Apr 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2023

Example no_renderer failed to run, please try running it locally and check the result.

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2023

Example no_renderer failed to run, please try running it locally and check the result.

@superdump
Copy link
Contributor

Yeeeeeeeeaaaaahhh! Much excite! :) I’ll take a look at the errors if no one else beats me to it. And you’re welcome to.

@hymm
Copy link
Contributor

hymm commented Apr 9, 2023

Rather than a finish lifecycle, would it make sense to have a renderer specific one to simplify the thinking around "where do I put what code".

impl RenderPlugin for MyPlugin {
  fn render_app(&self, mut render_app: &mut App) {
    // put code modifying render app here.
  }
  
  fn extract_app(&self mut extract_app: &mut App) {
    // put code modifying extract app here.
  }
}

you could not add the render and extract app until after the renderer is ready, to make it 100% clear to not modify the render app in the build function.

@mockersf
Copy link
Member Author

mockersf commented Apr 9, 2023

Rather than a finish lifecycle, would it make sense to have a renderer specific one to simplify the thinking around "where do I put what code".

Sounds good to me, to have a RenderPlugin "specialisation" of the general Plugin that would automatically plug the correct methods with the correct app at the correct step.

I'll wait for more feedback on the API which is definitely open to discussion

@cart
Copy link
Member

cart commented Apr 10, 2023

Yesssssss well done! This definitely seems like the better path. I'm stoked we don't need to async-ify the entire plugin api. I'll give the pipeline / render app init apis some thought, but I'm on board for the general direction.

We might even want to skip blocking on the "ideal render init api design" and do that in a follow up pr. It would be nice to get this in peoples' hands asap so we can iron out the kinks and capitalize on the "WebGPU release hype".

@superdump
Copy link
Contributor

Completely agree on not getting hung up too much on render app init api for this PR. It's a good discussion to have, and this PR is about making WebGPU go. :)

@mockersf
Copy link
Member Author

stencilLoadOp errors are now fixed on wgpu trunk

remaining:

Sample type TextureSampleType::Float for multisampled texture bindings was TextureSampleType::Float.
 - While validating entries[17]
 - While validating [BindGroupLayoutDescriptor "mesh_view_layout_multisampled"]
 - While calling [Device].CreateBindGroupLayout([BindGroupLayoutDescriptor "mesh_view_layout_multisampled"]).
[Invalid BindGroupLayout "mesh_view_layout_multisampled"] is invalid.
 - While validating [BindGroupDescriptor "mesh_view_bind_group"] against [Invalid BindGroupLayout "mesh_view_layout_multisampled"]
 - While calling [Device].CreateBindGroup([BindGroupDescriptor "mesh_view_bind_group"]).
[Invalid PipelineLayout] is invalid.
 - While calling [Device].CreateRenderPipeline([RenderPipelineDescriptor "gizmo_3d_pipeline"]).
[Invalid RenderPipeline "gizmo_3d_pipeline"] is invalid.
 - While encoding [RenderPassEncoder "main_opaque_pass_3d"].SetPipeline([Invalid RenderPipeline "gizmo_3d_pipeline"]).
[Invalid PipelineLayout] is invalid.
 - While calling [Device].CreateRenderPipeline([RenderPipelineDescriptor "pbr_opaque_mesh_pipeline"]).
[Invalid PipelineLayout] is invalid.
 - While calling [Device].CreateRenderPipeline([RenderPipelineDescriptor "pbr_alpha_blend_mesh_pipeline"]).

and a lot of unhelpful

[Invalid CommandBuffer] is invalid.
    at ValidateObject (..<URL>)
    at ValidateSubmit (..<URL>)

which I'm hoping will be fixed by fixing other issues

@mockersf
Copy link
Member Author

Fixed now, the flight helmet can be displayed 🎉

There is an issue with gamma correction, trying to find it

Screenshot 2023-04-12 at 19 41 43

@tim-blackbird
Copy link
Contributor

Incorrect colors should be because of the TODO left in the code here.
See this comment on the wgpu 0.15 update PR: #7356 (comment).

It should be the same surface format issue as with Nvidia + Wayland.

@mockersf
Copy link
Member Author

fixed too!
Screenshot 2023-04-14 at 02 45 47

@superdump
Copy link
Contributor

Awesome! Well done @mockersf ! :)

@tim-blackbird
Copy link
Contributor

Nice! I can confirm that fixes Nvidia + Wayland (and probably some other hardware/software combos) as well : )

@mockersf
Copy link
Member Author

mockersf commented Apr 14, 2023

Nice! I can confirm that fixes Nvidia + Wayland (and probably some other hardware/software combos) as well : )

Ho that's a nice side effect 😄 Do you know if there are issues opened?

@tim-blackbird
Copy link
Contributor

tim-blackbird commented Apr 14, 2023

#7318 for Nvidia + Wayland
#2374 and #5809 seem similar but for Intel iGPUs on either X11 or Wayland

@mockersf
Copy link
Member Author

thanks! let's wait for this to be merged so someone can check for the two others.

with gfx-rs/wgpu#3689 and a small workaround in Bevy, bloom also works 🎉
Screenshot 2023-04-14 at 12 13 34

@mockersf
Copy link
Member Author

every example I tried seems to work now, except compute_shader_game_of_life because it uses a read/write texture of format Rgba8Unorm which is not yet supported (gpuweb/gpuweb#1772 and gpuweb/gpuweb#3838)

@mockersf mockersf force-pushed the webgpu-support-not-async branch 2 times, most recently from f2a6d6e to aecabb8 Compare April 26, 2023 18:55
@mockersf
Copy link
Member Author

With #8446 merged and wgpu updated to 0.16, this is now ready 🎉

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

I don’t have too much in the way of opinions about plugin stuff. Others should definitely review that, please. It does feel like a shame that users implementing rendering stuff have to care about that and implement finish() but maybe that’s the only way to handle it.

The rendering bits generally look fine. It’s mostly just plugin adjustment. Just a few comments.

@@ -130,7 +130,8 @@ fn downsample_first(@location(0) output_uv: vec2<f32>) -> @location(0) vec4<f32>
// Lower bound of 0.0001 is to avoid propagating multiplying by 0.0 through the
// downscaling and upscaling which would result in black boxes.
// The upper bound is to prevent NaNs.
sample = clamp(sample, vec3<f32>(0.0001), vec3<f32>(3.40282347E+38));
// with f32::MAX (E+38) Chrome fails with ":value 340282346999999984391321947108527833088.0 cannot be represented as 'f32'"
sample = clamp(sample, vec3<f32>(0.0001), vec3<f32>(3.40282347E+37));
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to know what value should be used. But I guess this will do.

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/view/window.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/mesh2d/material.rs Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
crates/bevy_app/src/plugin.rs Show resolved Hide resolved
@mockersf
Copy link
Member Author

Should webgpu be the default? It's the easiest way to do it through rust features, and adding one to target webgl2 instead.

It would probably be possible to keep webgl2 the default, but that would mean needing to disable default features to target webgpu.

@cart
Copy link
Member

cart commented Apr 26, 2023

Should webgpu be the default? It's the easiest way to do it through rust features, and adding one to target webgl2 instead. It would probably be possible to keep webgl2 the default, but that would mean needing to disable default features to target webgpu.

I think WebGL2 should be the default until WebGPU is more widely supported. Making it harder to target WebGPU (by requiring disabling default features) while it is still rolling out seems very reasonable to me. The alternative (forcing users onto WebGPU by default before it is widely supported) seems worse to me.

@mockersf
Copy link
Member Author

I think WebGL2 should be the default until WebGPU is more widely supported. Making it harder to target WebGPU (by requiring disabling default features) while it is still rolling out seems very reasonable to me. The alternative (forcing users onto WebGPU by default before it is widely supported) seems worse to me.

Done!

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This is in a good spot now. Im stoked to finally have WebGPU support!
We should follow up with a "render plugin init" rethink.

@cart cart added this pull request to the merge queue May 4, 2023
Merged via the queue into bevyengine:main with commit 71842c5 May 4, 2023
mockersf added a commit that referenced this pull request May 16, 2023
# Objective

- I missed a few examples in #8336 
- fixes #8556 
- fixes #8620

## Solution

- Update them
@Selene-Amanita Selene-Amanita added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 10, 2023
github-merge-queue bot pushed a commit that referenced this pull request Jul 23, 2023
# Objective

Fixes #9121

Context:
- `ImageTextureLoader` depends on `RenderDevice` to work out which
compressed image formats it can support
- `RenderDevice` is initialised by `RenderPlugin`
- #8336 made `RenderPlugin`
initialisation async
- This caused `RenderDevice` to be missing at the time of
`ImageTextureLoader` initialisation, which in turn meant UASTC encoded
ktx2 textures were being converted to unsupported formats, and thus
caused panics

## Solution

- Delay `ImageTextureLoader` initialisation

---

## Changelog

- Moved `ImageTextureLoader` initialisation from `ImagePlugin::build()`
to `ImagePlugin::finish()`
- Default to `CompressedImageFormats::NONE` if `RenderDevice` resource
is missing

---------

Co-authored-by: 66OJ66 <hi0obxud@anonaddy.me>
cart pushed a commit that referenced this pull request Aug 10, 2023
# Objective

Fixes #9121

Context:
- `ImageTextureLoader` depends on `RenderDevice` to work out which
compressed image formats it can support
- `RenderDevice` is initialised by `RenderPlugin`
- #8336 made `RenderPlugin`
initialisation async
- This caused `RenderDevice` to be missing at the time of
`ImageTextureLoader` initialisation, which in turn meant UASTC encoded
ktx2 textures were being converted to unsupported formats, and thus
caused panics

## Solution

- Delay `ImageTextureLoader` initialisation

---

## Changelog

- Moved `ImageTextureLoader` initialisation from `ImagePlugin::build()`
to `ImagePlugin::finish()`
- Default to `CompressedImageFormats::NONE` if `RenderDevice` resource
is missing

---------

Co-authored-by: 66OJ66 <hi0obxud@anonaddy.me>
dekirisu pushed a commit to dekirisu/bevy_gltf_trait that referenced this pull request Jul 7, 2024
# Objective

Fixes #9121

Context:
- `ImageTextureLoader` depends on `RenderDevice` to work out which
compressed image formats it can support
- `RenderDevice` is initialised by `RenderPlugin`
- bevyengine/bevy#8336 made `RenderPlugin`
initialisation async
- This caused `RenderDevice` to be missing at the time of
`ImageTextureLoader` initialisation, which in turn meant UASTC encoded
ktx2 textures were being converted to unsupported formats, and thus
caused panics

## Solution

- Delay `ImageTextureLoader` initialisation

---

## Changelog

- Moved `ImageTextureLoader` initialisation from `ImagePlugin::build()`
to `ImagePlugin::finish()`
- Default to `CompressedImageFormats::NONE` if `RenderDevice` resource
is missing

---------

Co-authored-by: 66OJ66 <hi0obxud@anonaddy.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins A-Rendering Drawing game state to the screen C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide O-Web Specific to web (WASM) builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Official WebGPU Support Wayland windows' colors are too dark
6 participants