[naga wgsl-in] Add support for unsigned types when calling textureLoad with the level parameter. #7058

ygdrasil-io · 2025-02-05T01:42:31Z

Connections
Fix #5433

Description
In WSGL, the Naga frontend returns an error when using a u32 as the level parameter in textureLoad, although this is acceptable according to the specification at https://www.w3.org/TR/WGSL/#textureload.

Testing
This issue was tested by updating the image.wsgl file, which is executed as part of the unit test suite. The test includes a call to textureLoad with a u32 parameter.

Checklist

Run cargo fmt.
Run taplo format.
Run cargo clippy. If applicable, add:
- --target wasm32-unknown-unknown
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.

ygdrasil-io · 2025-02-05T01:55:40Z

FYI To test I used cargo test --all-features --workspace because cargo xtask test return

cargo xtask test
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
     Running `xtask/target/debug/xtask test`
[INFO  xtask::test] Generating .gpuconfig file based on gpus on the system
   Compiling ...
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 6.85s
     Running `target/debug/wgpu-info --json -o .gpuconfig`
[INFO  xtask::test] Found 1 gpu
[INFO  xtask::test] Running cargo tests
error: no such command: `nextest`

        Did you mean `test`?

        View all installed commands with `cargo --list`
        Find a package to install `nextest` with `cargo search cargo-nextest`
Error: Tests failed

Caused by:
    command exited with non-zero code `cargo nextest run --benches --tests --no-fail-fast --all-features --retries 0`: 101

ErichDonGubler · 2025-02-05T02:58:08Z

@ygdrasil-io: Sounds like you don't have cargo-nextest installed; you can find instructions to do so here. Our diagnostic ought to be better, though.

ygdrasil-io · 2025-02-06T22:18:38Z

The texelFetch function in GLSL does not support unsigned integers. Should we cast the value to an integer?
https://registry.khronos.org/OpenGL-Refpages/gl4/html/texelFetch.xhtml

ygdrasil-io · 2025-02-07T00:27:20Z

@cwfitzgerald suggested doing casting on matrix, so I implemented it with commit 546497b.

cwfitzgerald

Nice, looks generally good, a few comments

naga/tests/out/wgsl/image.wgsl

cwfitzgerald · 2025-02-11T00:55:58Z

naga/tests/out/hlsl/image.hlsl

@@ -37,6 +37,7 @@ void main(uint3 local_id : SV_GroupThreadID)
    uint2 dim = NagaRWDimensions2D(image_storage_src);
    int2 itc = (int2((dim * local_id.xy)) % int2(10, 20));
    uint4 value1_ = image_mipmapped_src.Load(int3(itc, int(local_id.z)));
+    uint4 value1_2_ = image_mipmapped_src.Load(int3(itc, uint(local_id.z)));


I feel like there's an implicit conversion here, and we should also wrap the argument in int as well, like we do glsl.

This is done. I also did some refactoring on wgpu/naga/src/back/hlsl/writer.rs to break up the monolithic function write_expr.

naga/src/back/hlsl/help.rs

cwfitzgerald

I think we're g2g after jimb's comment is addressed.

naga/tests/in/image.wgsl

ygdrasil-io · 2025-02-24T19:16:17Z

I think we're g2g after jimb's comment is addressed.

this is fixed

cwfitzgerald

Thanks!

cwfitzgerald · 2025-02-24T20:52:41Z

Got a conflict on image.spvasm - if you merge trunk then regenerate the files, this should resolve the conflict. Then ping me

Improve the validation logic for image level indices by matching against specific scalar types (Sint or Uint). This change ensures accurate error handling for invalid index types.

Moved HLSL image load logic into a dedicated `write_image_load` function to improve modularity and readability. Additionally, added explicit type casting for certain scalar values to ensure correctness in generated HLSL code. Updated tests to reflect these changes and added related utility updates.

Update parameter types in `write_expr` for consistency and add `#[allow(clippy::too_many_arguments)]` to suppress linter warnings. This improves readability and maintains compatibility with existing code conventions.

ygdrasil-io changed the title ~~Refactor image level index type validation logic.~~ [naga wgsl-in] Refactor image level type validation logic when calling textureLoad. Feb 5, 2025

ygdrasil-io changed the title ~~[naga wgsl-in] Refactor image level type validation logic when calling textureLoad.~~ [naga wgsl-in] Add support for unsigned types when calling textureLoad with the level parameter. Feb 5, 2025

ygdrasil-io marked this pull request as ready for review February 5, 2025 01:58

cwfitzgerald assigned teoxoy Feb 5, 2025

cwfitzgerald requested changes Feb 11, 2025

View reviewed changes

cwfitzgerald assigned cwfitzgerald and unassigned teoxoy Feb 12, 2025

ygdrasil-io force-pushed the fix-wgsl-textureLoad-level-parameter-checking branch from eb51db5 to dcc3a54 Compare February 16, 2025 15:22

jimblandy reviewed Feb 19, 2025

View reviewed changes

naga/src/back/hlsl/help.rs Outdated Show resolved Hide resolved

cwfitzgerald requested changes Feb 21, 2025

View reviewed changes

naga/tests/in/image.wgsl Show resolved Hide resolved

cwfitzgerald approved these changes Feb 24, 2025

View reviewed changes

ygdrasil-io added 12 commits February 24, 2025 21:54

Refactor image level index type validation logic.

c4c357a

Improve the validation logic for image level indices by matching against specific scalar types (Sint or Uint). This change ensures accurate error handling for invalid index types.

Run cargo fmt

3d350d6

Update CHANGELOG with new capabilities from RP

03dbf80

Handle Uint to SInt casting for GLSL texture operations.

7bb42ba

Add comment about u32 support of textureLoad on WSGL test file

91e642d

Refactor HLSL writer function parameters.

58bf409

Update parameter types in `write_expr` for consistency and add `#[allow(clippy::too_many_arguments)]` to suppress linter warnings. This improves readability and maintains compatibility with existing code conventions.

erratum

0c3e7ba

fix clippy

26aa6be

cargo fmt

c84a971

remove TypeInner import on help.rs

4d92439

regenerate image.spvasm

00dbdf8

ygdrasil-io force-pushed the fix-wgsl-textureLoad-level-parameter-checking branch from c94b7b4 to 00dbdf8 Compare February 24, 2025 21:00

cwfitzgerald enabled auto-merge (squash) February 24, 2025 21:01

cwfitzgerald merged commit 92a1702 into gfx-rs:trunk Feb 24, 2025
34 checks passed

ygdrasil-io deleted the fix-wgsl-textureLoad-level-parameter-checking branch February 24, 2025 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[naga wgsl-in] Add support for unsigned types when calling textureLoad with the level parameter. #7058

[naga wgsl-in] Add support for unsigned types when calling textureLoad with the level parameter. #7058

ygdrasil-io commented Feb 5, 2025 •

edited

Loading

ygdrasil-io commented Feb 5, 2025

ErichDonGubler commented Feb 5, 2025 •

edited

Loading

ygdrasil-io commented Feb 6, 2025

ygdrasil-io commented Feb 7, 2025

cwfitzgerald left a comment

cwfitzgerald Feb 11, 2025

ygdrasil-io Feb 16, 2025

cwfitzgerald left a comment

ygdrasil-io commented Feb 24, 2025

cwfitzgerald left a comment

cwfitzgerald commented Feb 24, 2025 •

edited

Loading

[naga wgsl-in] Add support for unsigned types when calling textureLoad with the level parameter. #7058

[naga wgsl-in] Add support for unsigned types when calling textureLoad with the level parameter. #7058

Conversation

ygdrasil-io commented Feb 5, 2025 • edited Loading

ygdrasil-io commented Feb 5, 2025

ErichDonGubler commented Feb 5, 2025 • edited Loading

ygdrasil-io commented Feb 6, 2025

ygdrasil-io commented Feb 7, 2025

cwfitzgerald left a comment

Choose a reason for hiding this comment

cwfitzgerald Feb 11, 2025

Choose a reason for hiding this comment

ygdrasil-io Feb 16, 2025

Choose a reason for hiding this comment

cwfitzgerald left a comment

Choose a reason for hiding this comment

ygdrasil-io commented Feb 24, 2025

cwfitzgerald left a comment

Choose a reason for hiding this comment

cwfitzgerald commented Feb 24, 2025 • edited Loading

ygdrasil-io commented Feb 5, 2025 •

edited

Loading

ErichDonGubler commented Feb 5, 2025 •

edited

Loading

cwfitzgerald commented Feb 24, 2025 •

edited

Loading