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.18 #10266

Merged
merged 14 commits into from
Dec 14, 2023
Merged

Update to wgpu 0.18 #10266

merged 14 commits into from
Dec 14, 2023

Conversation

Elabajaba
Copy link
Contributor

@Elabajaba Elabajaba commented Oct 25, 2023

Objective

Keep up to date with wgpu.

Solution

Update the wgpu version.

Currently blocked on naga_oil updating to naga 0.14 and releasing a new version.

3d scenes (or maybe any scene with lighting?) currently don't render anything due to

error: naga_oil bug, please file a report: composer failed to build a valid header: Type [2] '' is invalid
 = Capability Capabilities(CUBE_ARRAY_TEXTURES) is required

I'm not sure what should be passed in for wgpu::InstanceFlags, or if we want to make the gles3minorversion configurable (might be useful for debugging?)

Currently blocked on bevyengine/naga_oil#63, and gfx-rs/wgpu#4569 to be fixed upstream in wgpu first.

Known issues

Amd+windows+vulkan has issues with texture_binding_arrays (see the image here), but that'll be fixed in the next wgpu/naga version, and you can just use dx12 as a workaround for now (Amd+linux mesa+vulkan texture_binding_arrays are fixed though).


Changelog

Updated wgpu to 0.18, naga to 0.14.2, and naga_oil to 0.11.

  • Windows desktop GL should now be less painful as it no longer requires Angle.
  • You can now toggle shader validation and debug information for debug and release builds using WgpuSettings.instance_flags and InstanceFlags

Migration Guide

  • RenderPassDescriptor color_attachments (as well as RenderPassColorAttachment, and RenderPassDepthStencilAttachment) now use StoreOp::Store or StoreOp::Discard instead of a boolean to declare whether or not they should be stored.
  • RenderPassDescriptor now have timestamp_writes and occlusion_query_set fields. These can safely be set to None.
  • ComputePassDescriptor now have a timestamp_writes field. This can be set to None for now.
  • See the wgpu changelog for additional details

@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 S-Blocked This cannot move forward until something else changes labels Oct 25, 2023
@superdump
Copy link
Contributor

Cool, basically LGTM. :)

@robtfm
Copy link
Contributor

robtfm commented Nov 16, 2023

wgpu/naga have updated, if you get a chance please check/approve the naga_oil pr and i'll merge and release (unless i need to do something further for the capabilities?)

@Elabajaba
Copy link
Contributor Author

Rebased, fixed transmission, and it seems to work now.

@@ -197,6 +197,10 @@ impl ShaderCache {
}
}

// TODO: Check if this is supported, though I'm not sure if bevy works without this feature?
// We can't compile for native at least without it.
capabilities |= Capabilities::CUBE_ARRAY_TEXTURES;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this for native to compile, and webgl2 still works even with it enabled.

Ideally we feature gate it but I'm not sure how since it's a part of DownLevelFlags and doesn't seem to be exposed as a wgpu feature or limit.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we want to check, we could pass the RenderAdapter down through PipelineCache::new and use adapter.get_downlevel_capabilities().flags. but i think it's ok not to since we already assume support in point shadow bindings anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Ok. We can fix it if it’s a problem anywhere.

@robtfm
Copy link
Contributor

robtfm commented Nov 17, 2023

looks like we'll need at least one shader update too:

   ┌─ crates\bevy_pbr\src\render/utils.wgsl:35:135 │ ╭ fn octahedral_encode(v: vec3<f32>) -> vec2<f32> {
36 │ │     var n = v / (abs(v.x) + abs(v.y) + abs(v.z));
37 │ │     let octahedral_wrap = (1.0 - abs(n.yx)) * select(vec2(-1.0), vec2(1.0), n.xy > 0.0);
   │ │                                                                             ^^^^^^^^^^ naga::Expression [26]
38 │ │     let n_xy = select(octahedral_wrap, n.xy, n.z >= 0.0);
39 │ │     return n_xy * 0.5 + 0.5;
   │ ╰────────────────────────────^ naga::Function [62]
   │
   = Expression [26] is invalid
   = Operation Greater can't work with [24] and [25]

works replacing the 0.0 with vec2(0.0)

@Elabajaba
Copy link
Contributor Author

looks like we'll need at least one shader update too:

   ┌─ crates\bevy_pbr\src\render/utils.wgsl:35:135 │ ╭ fn octahedral_encode(v: vec3<f32>) -> vec2<f32> {
36 │ │     var n = v / (abs(v.x) + abs(v.y) + abs(v.z));
37 │ │     let octahedral_wrap = (1.0 - abs(n.yx)) * select(vec2(-1.0), vec2(1.0), n.xy > 0.0);
   │ │                                                                             ^^^^^^^^^^ naga::Expression [26]
38 │ │     let n_xy = select(octahedral_wrap, n.xy, n.z >= 0.0);
39 │ │     return n_xy * 0.5 + 0.5;
   │ ╰────────────────────────────^ naga::Function [62]
   │
   = Expression [26] is invalid
   = Operation Greater can't work with [24] and [25]

works replacing the 0.0 with vec2(0.0)

It works, but it spits out a vulkan validation error when I do that.

Also the texture_binding_array example is broken on vulkan. (works fine on dx12 though)

image

@superdump
Copy link
Contributor

I've seen mentions of the texture binding array bug on 0.12 I think? If so, it shouldn't block this PR as this PR does not cause the regression.

What is the vulkan validation error with robtfm's proposal?

@JMS55 JMS55 added this to the 0.13 milestone Nov 23, 2023
@Elabajaba
Copy link
Contributor Author

Elabajaba commented Nov 23, 2023

I've seen mentions of the texture binding array bug on 0.12 I think? If so, it shouldn't block this PR as this PR does not cause the regression.

It was an issue on AMD+Linux RADV drivers on 0.12. With wgpu 0.18 (this PR) it's fixed on AMD+Linux RADV, but broken on AMD+Windows (and apparently also broken on AMD+Linux amdvlk).

(amdvlk is AMD's official FOSS linux vulkan driver, but people generally use Mesa/RADV because it's better)

On Windows at least there's somewhat of a workaround because the issue doesn't happen on dx12.

What is the vulkan validation error with robtfm's proposal?

2023-11-23T20:19:56.968250Z ERROR log: VALIDATION [VUID-VkShaderModuleCreateInfo-pCode-01379 (0x2a1bf17f)]
        Validation Error: [ VUID-VkShaderModuleCreateInfo-pCode-01379 ] | MessageID = 0x2a1bf17f | SPIR-V module not valid: OpConstantComposite Constituent <id> count does not match Result Type <id> '5[%5]'s vector component count.
  %2173 = OpConstantComposite %v4float %329 %float_1
 The Vulkan spec states: If pCode is a pointer to GLSL code, it must be valid GLSL code written to the GL_KHR_vulkan_glsl GLSL extension specification (https://vulkan.lunarg.com/doc/view/1.3.261.1/windows/1.3-extensions/vkspec.html#VUID-VkShaderModuleCreateInfo-pCode-01379)

2023-11-23T20:19:58.192546Z ERROR log: VALIDATION [VUID-VkShaderModuleCreateInfo-pCode-01379 (0x2a1bf17f)]
        Validation Error: [ VUID-VkShaderModuleCreateInfo-pCode-01379 ] | MessageID = 0x2a1bf17f | SPIR-V module not valid: OpConstantComposite Constituent <id> count does not match Result Type <id> '3[%3]'s vector component count.
  %2814 = OpConstantComposite %v4float %717 %float_1
 The Vulkan spec states: If pCode is a pointer to GLSL code, it must be valid GLSL code written to the GL_KHR_vulkan_glsl GLSL extension specification (https://vulkan.lunarg.com/doc/view/1.3.261.1/windows/1.3-extensions/vkspec.html#VUID-VkShaderModuleCreateInfo-pCode-01379)

@mockersf
Copy link
Member

I think we should merge this now and try to investigate / fix after

@Elabajaba
Copy link
Contributor Author

There's an upstream patch that fixes the texture_binding_array example on windows+amd+vulkan.

I guess since deferred works despite the validation error, it's probably fine?

@Elabajaba Elabajaba marked this pull request as ready for review November 25, 2023 01:03
@robtfm
Copy link
Contributor

robtfm commented Nov 25, 2023

this fixes the validation error. seems like it shouldn't be needed but 🤷

diff --git a/crates/bevy_pbr/src/deferred/pbr_deferred_functions.wgsl b/crates/bevy_pbr/src/deferred/pbr_deferred_functions.wgsl
index d401805eb..f888c0f30 100644
--- a/crates/bevy_pbr/src/deferred/pbr_deferred_functions.wgsl
+++ b/crates/bevy_pbr/src/deferred/pbr_deferred_functions.wgsl
@@ -71,7 +71,7 @@ fn pbr_input_from_deferred_gbuffer(frag_coord: vec4<f32>, gbuffer: vec4<u32>) ->
     let emissive = rgb9e5::rgb9e5_to_vec3_(gbuffer.g);
     if ((pbr.material.flags & STANDARD_MATERIAL_FLAGS_UNLIT_BIT) != 0u) {
         pbr.material.base_color = vec4(emissive, 1.0);
-        pbr.material.emissive = vec4(vec3(0.0), 1.0);
+        pbr.material.emissive = vec4(0.0, 0.0, 0.0, 1.0);
     } else {
         pbr.material.base_color = vec4(pow(base_rough.rgb, vec3(2.2)), 1.0);
         pbr.material.emissive = vec4(emissive, 1.0);

@superdump
Copy link
Contributor

@robtfm so one can no longer construct a vecN from components up to vecM, M <= N?

@robtfm
Copy link
Contributor

robtfm commented Nov 29, 2023

@robtfm so one can no longer construct a vecN from components up to vecM, M <= N?

it seems like some kind of inference issue in the particular case. just below we have pbr.material.emissive = vec4(emissive, 1.0); which doesn't trigger the error.

@superdump
Copy link
Contributor

I’m curious if vec4(vec3(), ) works…

@robtfm
Copy link
Contributor

robtfm commented Nov 29, 2023

I’m curious if vec4(vec3(), ) works…
^^^^ type can't be inferred

also doesn't work:
vec4(vec3<f32>(), 1.0) -> same validation error
vec4<f32>(vec3<f32>(0.0), 1.0); -> same validation error
let zero: vec3<f32> = 0.0; ... vec4(zero, 1.0); -> same validation error

@Elabajaba
Copy link
Contributor Author

gfx-rs/wgpu#4695 is the upstream fix. Do we want this backported to avoid breaking downstream user's shaders? Does this actually break anything or is it just a validation error?

@Elabajaba
Copy link
Contributor Author

gfx-rs/wgpu#4695 is the upstream fix. Do we want this backported to avoid breaking downstream user's shaders? Does this actually break anything or is it just a validation error?

Backported and released in naga 0.14.2, I think this should be ready to be merged assuming CI passes?

Copy link
Contributor

@robtfm robtfm left a comment

Choose a reason for hiding this comment

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

looks good

@mockersf mockersf added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Blocked This cannot move forward until something else changes labels Dec 14, 2023
@superdump superdump added this pull request to the merge queue Dec 14, 2023
Merged via the queue into bevyengine:main with commit 70a592f Dec 14, 2023
27 checks passed
@eerii eerii mentioned this pull request Dec 21, 2023
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
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants