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

Add textureSampleBaseClampToEdge #2534

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

evahop
Copy link
Contributor

@evahop evahop commented Oct 4, 2023

From gfx-rs/wgpu#4402

For everything excluding spv:
It creates a local variable containing the half-texel computed from each backend's respective image query function.
It then uses that half-texel to clamp the image coordinates to [half-texel, 1 - half-texel].

For spv:
Does the exact same thing only it's self contained.

The spec says at "base level" which I've interpreted to mean at level 0 everywhere.

The spv snapshot is a little noisy so the relevant bit begins on line 584.

@evahop evahop changed the title Add SampleBaseClampToEdge Add textureSampleBaseClampToEdge Oct 4, 2023
@evahop evahop force-pushed the base-clamp branch 2 times, most recently from 2966ec0 to 0e1dd25 Compare October 4, 2023 19:16
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

I haven't yet looked in detail at the PR but wanted to ask if it would be possible to do the translation in the wgsl frontend instead given that none of the other shading languages have this function.

wgsl -> wgsl translation of this fn with a texture_external wouldn't work but I don't see the usefulness of wgsl -> wgsl translation in general.

@jimblandy what do you think?

src/lib.rs Outdated Show resolved Hide resolved
@evahop
Copy link
Contributor Author

evahop commented Oct 16, 2023

Removed Base on SampleLevel in favor of clamp_to_edge. I can update this to be frontend only once decided.
Not sure what triggered the review request, that wasn't intentional.

@teoxoy
Copy link
Member

teoxoy commented Oct 17, 2023

Thanks!

Not sure what triggered the review request, that wasn't intentional.

We now have a codeowners file with the naga team in it which causes github to automatically request reviews.

@jimblandy
Copy link
Member

jimblandy commented Oct 17, 2023

Hmm. I actually think it's probably fine to put it in the IR, and make the backends decide how to deal with it. Generally we want the IR to follow WGSL pretty closely. I also think we might get slightly better output this way.

I think adding the Base variant to SampleLevel is actually better than an additional clamp_to_edge field, because the latter raises the question of whether backends should respect clamp_to_edge when SampleLevel is something other than Zero. For example, the change to the GLSL backend in a152303 means that the backend is just ignoring the bool, instead of flagging a problem with the validator.

I would call the variant BaseClampToEdge, though. Again, the IR should generally, roughly, follow WGSL.

Another approach with the same advantages would be to change the Zero variant to Zero { clamp_to_edge: bool }.

@jimblandy
Copy link
Member

@evahop Thanks for implementing this!

If you'd like to post work-in-progress PRs (which we're happy to see), consider marking them as "Draft" when you create the PR. That prevents reviews from being requested.

@teoxoy
Copy link
Member

teoxoy commented Oct 17, 2023

textureSampleBaseClampToEdge is sugar for:

let half_texel = vec2(0.5) / vec2<f32>(textureDimensions(t));
textureSampleBase(t, s, clamp(coords, half_texel, 1 - half_texel));

We also translate i++ in the frontend to i += 1 IR.

I think frontends should deal with sugar features like these since there is no benefit to have those in the IR.

You could actually make an argument i++ belongs in the IR given the wide availability in the backends but textureSampleBaseClampToEdge has no equivalent in any of the other shading languages.

@evahop evahop marked this pull request as draft October 17, 2023 19:04
@teoxoy
Copy link
Member

teoxoy commented Oct 24, 2023

@jimblandy and I talked about this and we are unsure where this translation belongs yet due to us not supporting texture_external yet (gfx-rs/wgpu#4386). textureSampleBaseClampToEdge can be used with a texture_2d but its main use-case is with a texture_external.

The translation of texture_external & textureSampleBaseClampToEdge will most likely have to happen in the same place (either in the front-end or the back-ends).

I think we should ideally add support for texture_external first or at the same time with textureSampleBaseClampToEdge.

@cwfitzgerald
Copy link
Member

Hello, thank you for your PR against Naga!

As part of gfx-rs/wgpu#4231, we have moved development of Naga into the wgpu repository in the Naga subfolder. We have transferred all issues, but we are unable to automatically transfer PRs.

As such, please recreate your PR against the wgpu repository. We apologize for the inconvenience this causes, but will make contributing to both projects more streamlined going forward.

We are leaving PRs open, but once they are transferred, please close the original Naga PR.

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.

4 participants