Skip to content

[naga] Support textureSampleBaseClampToEdge() for texture2d #7709

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

Merged
merged 2 commits into from
Jun 17, 2025

Conversation

jamienicol
Copy link
Contributor

@jamienicol jamienicol commented May 21, 2025

Adds a new flag to the IR indicating when image sample coordinates are to be clamped. Adds wgsl-in support for parsing and lowering to IR. Validation ensures this flag is only used when sampling a 2D non-arrayed sampled texture, without offset, gather, or depth comparison. This matches the WGSL requirements, with the exception of supporting texture_external textures, which will follow in a later patch.

SPIRV, HLSL, and Metal backends are supported so far, with GLSL left for a follow up. (In GLSL the texture will simply be sampled without the coordinates being clamped.)

It may seem unfortunate to have to handle this separately for each backend, and indeed it would have been possible to implement this simply in the WGSL frontend. However, future patches will add support for using textureSampleBaseClampToEdge() with external textures, which will actually have to be handled by each backend. This patch is laying the groundwork for that.

Connections
Part of #4386
Fixes #7539
Depends on #7804 (for texturesample CTS tests to pass on Mac)

Testing
Added snapshot test

Makes these CTS tests pass

Squash or Rebase?

Rebase

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@jamienicol jamienicol force-pushed the texture-sample-base-clamp-to-edge branch from b25453c to 466e9b8 Compare May 27, 2025 08:34
@jamienicol jamienicol requested review from crowlKats and a team as code owners May 27, 2025 08:34
@jamienicol jamienicol force-pushed the texture-sample-base-clamp-to-edge branch 2 times, most recently from 133ad63 to b27265b Compare May 27, 2025 14:51
@jamienicol jamienicol force-pushed the texture-sample-base-clamp-to-edge branch 2 times, most recently from 6df8c7c to 9103208 Compare June 4, 2025 15:06
@jamienicol jamienicol force-pushed the texture-sample-base-clamp-to-edge branch from 9103208 to 05da8d1 Compare June 10, 2025 07:29
@cwfitzgerald cwfitzgerald changed the title [naga] Support textureSampleBaseClampToEdge() for texture2d [naga] Support textureSampleBaseClampToEdge() for texture2d Jun 11, 2025
@jimblandy jimblandy force-pushed the texture-sample-base-clamp-to-edge branch from 05da8d1 to b71a0e2 Compare June 11, 2025 20:09
@jimblandy
Copy link
Member

@jamienicol This is a bit dumb, but does b71a0e2 look okay to you? The MSL backend has no obligations here, since it's a validation error, but I just felt it would be nicer for the MSL backend to die instead of quietly misbehaving.

@jamienicol
Copy link
Contributor Author

@jimblandy I don't mind it at all. I wouldn't want to go hugely out of the way to repeat validation in the backends, but that is simple enough.

Personally I'd prefer an assertion to returning an error: if we hit that case there's a bug somewhere and I'd rather we know about it via crash reports than users just suffering in silence. But I might be in the minority of preferring crashes to errors.

@jamienicol
Copy link
Contributor Author

Any reason to limit that to MSL rather than doing it for all backends? or was that just as an example?

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

Looks like @jimblandy is ramping up on review now, so I won't drive a full review myself, but I'll comment on some things I noticed before I left on vacation and now.

ErichDonGubler

This comment was marked as duplicate.

@jimblandy
Copy link
Member

Any reason to limit that to MSL rather than doing it for all backends? or was that just as an example?

It'd make sense for all backends.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Thoughts so far:

@jamienicol
Copy link
Contributor Author

@jamienicol This is a bit dumb, but does b71a0e2 look okay to you? The MSL backend has no obligations here, since it's a validation error, but I just felt it would be nicer for the MSL backend to die instead of quietly misbehaving.

Did the same thing for HLSL in 2fb382c. Slightly different approach for SPIRV as the implementation is different. In theory it should be able to handle other sample flags, except for the level which must be zero. So I've added a check for that alone - I don't think the backends should be in the business of repeating validation in its entirety - rather they should make a reasonable effort to ensure their own constraints are satisfied.

@jimblandy
Copy link
Member

I don't think the backends should be in the business of repeating validation in its entirety - rather they should make a reasonable effort to ensure their own constraints are satisfied.

Yes, 100%. Backends are allowed to panic and misbehave when validation is buggy. My wanting to tighten up these matches was just opportunistic: it seemed like a small change might save someone some debugging time in he future.

Copy link
Collaborator

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

Deno part LGTM

Copy link
Member

@jimblandy jimblandy 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 to me.

@jimblandy jimblandy force-pushed the texture-sample-base-clamp-to-edge branch from 2fb382c to 77129a0 Compare June 17, 2025 18:51
@jimblandy
Copy link
Member

Oops, this needs #7804. Sorry for pushing my rebase prematurely.

Adds a new flag to the IR indicating when image sample coordinates are
to be clamped. Adds wgsl-in support for parsing and lowering to
IR. Validation ensures this flag is only used when sampling a 2D
non-arrayed sampled texture, without offset, gather, or depth
comparison. This matches the WGSL requirements, with the exception of
supporting `texture_external` textures, which will follow in a later
patch.

SPIRV, HLSL, and Metal backends are supported so far, with GLSL left
for a follow up. (In GLSL the texture will simply be sampled without
the coordinates being clamped.)

It may seem unfortunate to have to handle this separately for each
backend, and indeed it would have been possible to implement this simply
in the WGSL frontend. However, future patches will add support for using
textureSampleBaseClampToEdge() with external textures, which will
actually have to be handled by each backend. This patch is laying the
groundwork for that.
@jimblandy jimblandy force-pushed the texture-sample-base-clamp-to-edge branch from 77129a0 to 19e825f Compare June 17, 2025 19:27
@jimblandy jimblandy merged commit 111a95b into gfx-rs:trunk Jun 17, 2025
40 checks passed
@jamienicol jamienicol deleted the texture-sample-base-clamp-to-edge branch June 18, 2025 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wgsl-in] textureSampleBaseClampToEdge not present as a built-in
5 participants