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 filtered texture and sampler binding types #1076

Closed
wants to merge 2 commits into from
Closed

Conversation

kvark
Copy link
Contributor

@kvark kvark commented Sep 15, 2020

Fixes #1034
Given the discussion on the issue, we can't have even the Float component type linearly sampled if the format doesn't support this. The only way to catch that early is having these new binding types.
Names are up to bikeshedding!


Preview | Diff

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
: {{GPUBindingType/"comparison-sampler"}}
:: the |binding| is a comparison sampler
: {{GPUBindingType/"sampled-texture"}}
:: the |binding| is a sampled texture with the component type of |entry|.{{GPUBindGroupLayoutEntry/textureComponentType}}
:: The |binding| is a sampled texture with the component type of |entry|.{{GPUBindGroupLayoutEntry/textureComponentType}},
and it has to have a sample count of 1.
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, what sample count does this refer to? This is talking about the binding type inside the shader, right? So rather than sample count I think it should say it's non-multisampled (or more precisely refer to the actual type from WGSL).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed! Not directly related to the PR though

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@kvark
Copy link
Contributor Author

kvark commented Oct 5, 2020

Names are updated, the PR is rebased (and mostly reimplemented given the amount of conflicts...). Please take another look!

spec/index.bs Outdated Show resolved Hide resolved
@@ -3381,12 +3410,20 @@ A {{GPUProgrammableStageDescriptor}} describes the entry point in the user-provi
1. If |entry|.{{GPUBindGroupLayoutEntry/type}} is:
<dl class=switch>
: {{GPUBindingType/"sampler"}}
:: the |binding| is a non-comparison sampler
:: the |binding| is a non-comparison sampler, and all the textures it samples
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ideally things in this switch should be more specific, like compareEnable is false instead of "non-comparison", but probably not worth fixing this right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, upon further consideration, not sure if we'd be able to use that language here.
This spec text talks about the bindings reflected from the shader. So it's sort of an abstract thing, it generally has less information than the bindings on the API side. We could base this off the implicit bind group layout logic, but I think that would be a very wrong thing to do :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, for some reason I thought the "is a non-comparison sampler" was talking about the sampler itself and not the type of the binding variable in the shader. If the latter, we should say "sampler (not sampler_comparison)" here and "sampler_comparison or sampler" there, or something.

Copy link
Contributor

@Kangz Kangz left a comment

Choose a reason for hiding this comment

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

No concerns, and the names are good enough that I don't want to bikeshed to avoid noise before the rest of the group takes a look.

spec/index.bs Outdated Show resolved Hide resolved
@kvark
Copy link
Contributor Author

kvark commented Oct 6, 2020

Alright, thank you for the quick reviews!
@litherum and @RafaelCintron please check this out when you have time!

@austinEng
Copy link
Contributor

austinEng commented Oct 21, 2020

We only need to care if about filtering for float formats that don't support it since it's disallowed on D3D12 for all integer formats anyway. Maybe to reduce the members in GPUBindGroupLayoutEntry we can merge the filtering concept into the GPUTextureComponentType "filtered-float" ?

@kvark
Copy link
Contributor Author

kvark commented Nov 26, 2020

This became a part of #1223

@kvark kvark closed this Nov 26, 2020
bors bot added a commit to gfx-rs/wgpu that referenced this pull request Nov 30, 2020
1047: Update bind group layout API to match upstream r=cwfitzgerald a=kvark

**Connections**
Follows gpuweb/gpuweb#1076, gpuweb/gpuweb#1223 (gpuweb/gpuweb#1164), gpuweb/gpuweb#1255, and gpuweb/gpuweb#1256

**Description**
Aligns our API closer to the latest changes in WebGPU upstream. We technically don't have to do this, but I believe in the end it would be best if our API gets close to upstream.

Note: this is a sensitive change for the users, everybody will get their code broken. So please take a look at the API and see if something is missing or needs improvement, so that we don't have to go through the changes again afterwards.

**Testing**
Doesn't really need testing. Partially covered by the existing playtest.

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
* wgsl: Add validation tests for tokenization

Covers the following cases:
- Empty module
- Null characters
- Whitespace (delimiters)
- Comments
- Identifiers
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.

Separate linear sampler binding type?
4 participants