Fix attempt to import offset as const. #121

tychedelia · 2025-06-09T22:44:14Z

For some reason wgpu-25 becomes very grumpy with this otherwise:

called `Result::unwrap()` on an `Err` value: BadHandle { kind: "naga::ir::Expression", index: 16 }

superdump · 2025-06-10T07:25:19Z

src/derive.rs

                coordinate: map_expr!(coordinate),
                array_index: map_expr_opt!(array_index),
-                offset: offset.map(|c| self.import_global_expression(c)),
+                offset: map_expr_opt!(offset),


What is this code doing and why does this change help?

You tell me! It resolves the bug. :) More specifically, it follows the pattern of most of the other items here, which is how I arrived at the solution. I haven't seen any other evidences of errors produced by this change.

Iirc, this happens when using expressions when sampling (i.e. not using constant uv coordinates). But obviously our code did that before and I have no idea what changed in the Naga that could have caused this to break. I reviewed their changelog and didn't find anything obvious.

As the person in the git blame, I have no idea. It looks like naga just renamed const_expressions to global_expressions on Modules in naga 0.20 so I just copied their renaming. I have to relearn the map_expr! stuff every time I update naga_oil.

IceSentry

I don't know naga_oil enough to tell if that's correct or not, but if all the test passed and things now work with wgpu 25 I don't see anything wrong with it.

robtfm · 2025-06-10T17:10:15Z

from the webgpu spec, The offset expression [must](https://www.w3.org/TR/WGSL/#shader-creation-error) be a [const-expression](https://www.w3.org/TR/WGSL/#const-expressions) (e.g. vec2<i32>(1, 2)). (https://www.w3.org/TR/WGSL/#texturesample) so i think this ought to work as is with all valid shaders.

can you share the code that's triggering the error?

edit: that said, the fix should work fine too, i'm just curious really.

tychedelia · 2025-06-10T19:43:46Z

can you share the code that's triggering the error?

Yes, for example, here: https://github.com/bevyengine/bevy/blob/27b64c3bf15882761ac914c72bfdad675bebe84a/crates/bevy_pbr/src/ssao/spatial_denoise.wgsl#L24-L33. which I may have mixed up in my head, these do appear to be constant.

robtfm · 2025-06-14T10:06:23Z

i'll wait to do a release until you're happy with the bevy wgpu bump

@group

# Objective Upgrade to `wgpu` version `25.0`. Depends on bevyengine/naga_oil#121 ## Solution ### Problem The biggest issue we face upgrading is the following requirement: > To facilitate this change, there was an additional validation rule put in place: if there is a binding array in a bind group, you may not use dynamic offset buffers or uniform buffers in that bind group. This requirement comes from vulkan rules on UpdateAfterBind descriptors. This is a major difficulty for us, as there are a number of binding arrays that are used in the view bind group. Note, this requirement does not affect merely uniform buffors that use dynamic offset but the use of *any* uniform in a bind group that also has a binding array. ### Attempted fixes The easiest fix would be to change uniforms to be storage buffers whenever binding arrays are in use: ```wgsl #ifdef BINDING_ARRAYS_ARE_USED @group(0) @binding(0) var<uniform> view: View; @group(0) @binding(1) var<uniform> lights: types::Lights; #else @group(0) @binding(0) var<storage> view: array<View>; @group(0) @binding(1) var<storage> lights: array<types::Lights>; #endif ``` This requires passing the view index to the shader so that we know where to index into the buffer: ```wgsl struct PushConstants { view_index: u32, } var<push_constant> push_constants: PushConstants; ``` Using push constants is no problem because binding arrays are only usable on native anyway. However, this greatly complicates the ability to access `view` in shaders. For example: ```wgsl #ifdef BINDING_ARRAYS_ARE_USED mesh_view_bindings::view.view_from_world[0].z #else mesh_view_bindings::view[mesh_view_bindings::view_index].view_from_world[0].z #endif ``` Using this approach would work but would have the effect of polluting our shaders with ifdef spam basically *everywhere*. Why not use a function? Unfortunately, the following is not valid wgsl as it returns a binding directly from a function in the uniform path. ```wgsl fn get_view() -> View { #if BINDING_ARRAYS_ARE_USED let view_index = push_constants.view_index; let view = views[view_index]; #endif return view; } ``` This also poses problems for things like lights where we want to return a ptr to the light data. Returning ptrs from wgsl functions isn't allowed even if both bindings were buffers. The next attempt was to simply use indexed buffers everywhere, in both the binding array and non binding array path. This would be viable if push constants were available everywhere to pass the view index, but unfortunately they are not available on webgpu. This means either passing the view index in a storage buffer (not ideal for such a small amount of state) or using push constants sometimes and uniform buffers only on webgpu. However, this kind of conditional layout infects absolutely everything. Even if we were to accept just using storage buffer for the view index, there's also the additional problem that some dynamic offsets aren't actually per-view but per-use of a setting on a camera, which would require passing that uniform data on *every* camera regardless of whether that rendering feature is being used, which is also gross. As such, although it's gross, the simplest solution just to bump binding arrays into `@group(1)` and all other bindings up one bind group. This should still bring us under the device limit of 4 for most users. ### Next steps / looking towards the future I'd like to avoid needing split our view bind group into multiple parts. In the future, if `wgpu` were to add `@builtin(draw_index)`, we could build a list of draw state in gpu processing and avoid the need for any kind of state change at all (see gfx-rs/wgpu#6823). This would also provide significantly more flexibility to handle things like offsets into other arrays that may not be per-view. ### Testing Tested a number of examples, there are probably more that are still broken. --------- Co-authored-by: François Mockers <[email protected]> Co-authored-by: Elabajaba <[email protected]>

@group

# Objective Upgrade to `wgpu` version `25.0`. Depends on bevyengine/naga_oil#121 ## Solution ### Problem The biggest issue we face upgrading is the following requirement: > To facilitate this change, there was an additional validation rule put in place: if there is a binding array in a bind group, you may not use dynamic offset buffers or uniform buffers in that bind group. This requirement comes from vulkan rules on UpdateAfterBind descriptors. This is a major difficulty for us, as there are a number of binding arrays that are used in the view bind group. Note, this requirement does not affect merely uniform buffors that use dynamic offset but the use of *any* uniform in a bind group that also has a binding array. ### Attempted fixes The easiest fix would be to change uniforms to be storage buffers whenever binding arrays are in use: ```wgsl #ifdef BINDING_ARRAYS_ARE_USED @group(0) @binding(0) var<uniform> view: View; @group(0) @binding(1) var<uniform> lights: types::Lights; #else @group(0) @binding(0) var<storage> view: array<View>; @group(0) @binding(1) var<storage> lights: array<types::Lights>; #endif ``` This requires passing the view index to the shader so that we know where to index into the buffer: ```wgsl struct PushConstants { view_index: u32, } var<push_constant> push_constants: PushConstants; ``` Using push constants is no problem because binding arrays are only usable on native anyway. However, this greatly complicates the ability to access `view` in shaders. For example: ```wgsl #ifdef BINDING_ARRAYS_ARE_USED mesh_view_bindings::view.view_from_world[0].z #else mesh_view_bindings::view[mesh_view_bindings::view_index].view_from_world[0].z #endif ``` Using this approach would work but would have the effect of polluting our shaders with ifdef spam basically *everywhere*. Why not use a function? Unfortunately, the following is not valid wgsl as it returns a binding directly from a function in the uniform path. ```wgsl fn get_view() -> View { #if BINDING_ARRAYS_ARE_USED let view_index = push_constants.view_index; let view = views[view_index]; #endif return view; } ``` This also poses problems for things like lights where we want to return a ptr to the light data. Returning ptrs from wgsl functions isn't allowed even if both bindings were buffers. The next attempt was to simply use indexed buffers everywhere, in both the binding array and non binding array path. This would be viable if push constants were available everywhere to pass the view index, but unfortunately they are not available on webgpu. This means either passing the view index in a storage buffer (not ideal for such a small amount of state) or using push constants sometimes and uniform buffers only on webgpu. However, this kind of conditional layout infects absolutely everything. Even if we were to accept just using storage buffer for the view index, there's also the additional problem that some dynamic offsets aren't actually per-view but per-use of a setting on a camera, which would require passing that uniform data on *every* camera regardless of whether that rendering feature is being used, which is also gross. As such, although it's gross, the simplest solution just to bump binding arrays into `@group(1)` and all other bindings up one bind group. This should still bring us under the device limit of 4 for most users. ### Next steps / looking towards the future I'd like to avoid needing split our view bind group into multiple parts. In the future, if `wgpu` were to add `@builtin(draw_index)`, we could build a list of draw state in gpu processing and avoid the need for any kind of state change at all (see gfx-rs/wgpu#6823). This would also provide significantly more flexibility to handle things like offsets into other arrays that may not be per-view. ### Testing Tested a number of examples, there are probably more that are still broken. --------- Co-authored-by: François Mockers <[email protected]> Co-authored-by: Elabajaba <[email protected]>

Fix attempt to import offset as const.

8e242af

tychedelia mentioned this pull request Jun 9, 2025

Ugrade to wgpu version 25.0 bevyengine/bevy#19563

Merged

superdump reviewed Jun 10, 2025

View reviewed changes

IceSentry approved these changes Jun 10, 2025

View reviewed changes

JMS55 approved these changes Jun 13, 2025

View reviewed changes

Elabajaba approved these changes Jun 13, 2025

View reviewed changes

robtfm merged commit 6eee1e6 into bevyengine:master Jun 14, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix attempt to import offset as const. #121

Fix attempt to import offset as const. #121

Uh oh!

tychedelia commented Jun 9, 2025

Uh oh!

superdump Jun 10, 2025

Uh oh!

tychedelia Jun 10, 2025 •

edited

Loading

Uh oh!

Elabajaba Jun 10, 2025 •

edited

Loading

Uh oh!

IceSentry left a comment

Uh oh!

robtfm commented Jun 10, 2025 •

edited

Loading

Uh oh!

tychedelia commented Jun 10, 2025

Uh oh!

Uh oh!

robtfm commented Jun 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix attempt to import offset as const. #121

Fix attempt to import offset as const. #121

Uh oh!

Conversation

tychedelia commented Jun 9, 2025

Uh oh!

superdump Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

tychedelia Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Elabajaba Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IceSentry left a comment

Choose a reason for hiding this comment

Uh oh!

robtfm commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tychedelia commented Jun 10, 2025

Uh oh!

Uh oh!

robtfm commented Jun 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tychedelia Jun 10, 2025 •

edited

Loading

Elabajaba Jun 10, 2025 •

edited

Loading

robtfm commented Jun 10, 2025 •

edited

Loading