Sitelet https://github.com/gpuweb/gpuweb/pull/1978
Skip to content

Stencil reads are S,X,X,X. (but expect S,S,S,S or S,0,0,1) - #1978

Merged
kdashg merged 1 commit into
gpuweb:mainfrom
kdashg:stencil-reads
Aug 4, 2021
Merged

Stencil reads are S,X,X,X. (but expect S,S,S,S or S,0,0,1)#1978
kdashg merged 1 commit into
gpuweb:mainfrom
kdashg:stencil-reads

Conversation

@kdashg

@kdashg kdashg commented Jul 21, 2021 •
edited by pr-preview Bot
Loading

Copy link
Copy Markdown
Contributor

Closes #1198.

Preview | Diff

kvark
kvark reviewed Jul 21, 2021
View reviewed changes

@kvark kvark left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks fine, assuming that we can indeed rely on these two possibilities.

Comment thread spec/index.bs Outdated
<td>1 &minus; 4
<td>stencil
<td>{{GPUTextureSampleType/"uint"}}
<td>`vec4<u32>(S, S, S, S)` or `vec4<u32>(S, 0, 0, 1)`

@kvark kvark Jul 21, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can also write vec4<u32>(S)

@kdashg kdashg Aug 2, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This needs to be S,X,X,X

@litherum litherum Aug 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this matches the prose below.

Comment thread spec/index.bs Outdated
@kainino0x

kainino0x commented Jul 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Resolution: Make this non-normative because Metal doesn't actually guarantee this.
(but S being the first component IS normative.)

@kdashg

kdashg commented Jul 26, 2021

Copy link
Copy Markdown
Contributor Author

Resolution: Make this non-normative because Metal doesn't actually guarantee this.

More specifically, our consensus is that it's not worth it to try to make this normative. (not just because Metal doesn't guarantee it per se) Making it normative can exert pressure to keep just these two possibilities, so that no one ends up adding a third. (like S,0,0,0?) Probably not a big concern.
Myles additionally noted that making this normative probably doesn't help authors, and that authors are our primary audience.

@kdashg
kdashg requested review from kainino0x, kvark and litherum July 26, 2021 20:21
@kdashg kdashg changed the title Stencil reads are S,S,S,S or S,0,0,1. Stencil reads are S,X,X,X. (but expect S,S,S,S or S,0,0,1) Jul 26, 2021
@github-actions

github-actions Bot commented Jul 26, 2021

Copy link
Copy Markdown
Contributor

Previews, as seen when this build job started (4bb067a):
WebGPU | IDL
WGSL
Explainer

kainino0x
kainino0x approved these changes Jul 26, 2021
View reviewed changes

@kainino0x kainino0x left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I think we also needed to do something like this for binding depth textures as non-depth textures? But we can follow up with it.

@kvark

kvark commented Jul 27, 2021

Copy link
Copy Markdown
Contributor

LGTM. I think we also needed to do something like this for binding depth textures as non-depth textures? But we can follow up with it.

I thought the response from Apple was that it's just UB and we aren't supposed to do this at all. (i.e. it's not just the swizzling problem).

@kainino0x

kainino0x commented Jul 27, 2021

Copy link
Copy Markdown
Contributor

Oh yeah, I think you're right. Can't keep all this stuff in my head

kainino0x
kainino0x reviewed Aug 2, 2021
View reviewed changes
Comment thread spec/index.bs Outdated
litherum
litherum reviewed Aug 2, 2021
View reviewed changes
Comment thread spec/index.bs Outdated
<td>1 &minus; 4
<td>stencil
<td>{{GPUTextureSampleType/"uint"}}
<td>`vec4<u32>(S, S, S, S)` or `vec4<u32>(S, 0, 0, 1)`

@litherum litherum Aug 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this matches the prose below.

Comment thread spec/index.bs Outdated
@kdashg
Stencil reads are S,X,X,X.
7307ee0 
But probably S,S,S,S or S,0,0,1.
@kdashg
kdashg merged commit b7ed248 into gpuweb:main Aug 4, 2021
github-actions Bot added a commit that referenced this pull request Aug 4, 2021
@kdashg @github-actions
Stencil reads are S,X,X,X. (#1978)
77d62de 
SHA: b7ed248
Reason: push, by @jdashg

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions Bot added a commit that referenced this pull request Aug 4, 2021
@kdashg @github-actions
Stencil reads are S,X,X,X. (#1978)
99cd7de 
SHA: b7ed248
Reason: push, by @jdashg

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions Bot added a commit that referenced this pull request Aug 4, 2021
@kdashg @github-actions
Stencil reads are S,X,X,X. (#1978)
188e92d 
SHA: b7ed248
Reason: push, by @jdashg

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kainino0x kainino0x kainino0x approved these changes
@litherum litherum litherum left review comments
@kvark kvark Awaiting requested review from kvark

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2nd, 3rd, and 4th components of sampled depth/stencil textures are undefined in Metal

4 participants

@kdashg @kainino0x @kvark @litherum