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

Change integer texture parameters to unsigned - #2722

Merged
kdashg merged 1 commit into
gpuweb:mainfrom
alan-baker:texture-integer-sign
May 3, 2022
Merged

Change integer texture parameters to unsigned#2722
kdashg merged 1 commit into
gpuweb:mainfrom
alan-baker:texture-integer-sign

Conversation

@alan-baker

@alan-baker alan-baker commented Apr 1, 2022

Copy link
Copy Markdown
Contributor

Fixes #2570

  • Change all the integer parameters for texture built-in functions from
    signed to unsigned
  • With literal unification, unsuffixed literals will continue to be
    accepted in these functions

I plan to leave #2588 open for now, but this is a more reasonable state than the current one and likely easier to get consensus on.

@alan-baker alan-baker added the wgsl WebGPU Shading Language Issues label Apr 1, 2022
@github-actions

github-actions Bot commented Apr 1, 2022

Copy link
Copy Markdown
Contributor

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

litherum
litherum reviewed Apr 2, 2022
View reviewed changes
Comment thread wgsl/index.bs Outdated
textureSample(t: texture_2d<f32>, s: sampler, coords: vec2<f32>, offset: vec2<i32>) -> vec4<f32>
textureSample(t: texture_2d_array<f32>, s: sampler, coords: vec2<f32>, array_index: i32) -> vec4<f32>
textureSample(t: texture_2d_array<f32>, s: sampler, coords: vec2<f32>, array_index: i32, offset: vec2<i32>) -> vec4<f32>
textureSample(t: texture_2d<f32>, s: sampler, coords: vec2<f32>, offset: vec2<u32>) -> vec4<f32>

@litherum litherum Apr 2, 2022

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'm confused. Offsets can be negative.

@alan-baker alan-baker Apr 3, 2022

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.

Good catch, got a little overzealous with search and replace.

@github-actions

github-actions Bot commented Apr 4, 2022

Copy link
Copy Markdown
Contributor

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

@kdashg

kdashg commented Apr 13, 2022

Copy link
Copy Markdown
Contributor
WGSL meeting minutes 2022-04-12
  • KG: textureDimensions yield unsigned.
  • AB: Didn’t go as far as we had proposed at the F2F. This is the half-stage: if the value can’t be negative then change it to unsigned. Should be a reasonable first step.
  • MM: Think we should go farther. Using int’s is idiomatic in other shading languages. And would be unfortunate to require casts.
  • DN: Are we going to add the signed forms again?
  • Yes
  • DN: So let’s not remove them now.
  • AB: Ok, so add the variants. The whole cross product of cases.
  • KG: consensus to have both overloads.
  • KG: Suggest have all-signed forms, and all-unsigned forms. Rather than full cross product. Wait for feedback if people miss it.
  • AB: Ok, will update the PR that way.

@alan-baker
Update texture built-in functions' use of integers
daba2c7 
Fixes gpuweb#2570

* Returned values are always unsigned now
* Added overloads for texture functions that had i32 based parameters
  (other than `offset`) to have an equivalent u32 version
@alan-baker
alan-baker force-pushed the texture-integer-sign branch from 6017ee4 to daba2c7 Compare April 14, 2022 15:11
@github-actions

github-actions Bot commented Apr 14, 2022

Copy link
Copy Markdown
Contributor

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

@kdashg
kdashg merged commit 12aa43c into gpuweb:main May 3, 2022
github-actions Bot added a commit that referenced this pull request May 3, 2022
@alan-baker @github-actions
Update texture built-in functions' use of integers (#2722)
1a21609 
SHA: 12aa43c
Reason: push, by @kdashg

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 May 3, 2022
@alan-baker @github-actions
Update texture built-in functions' use of integers (#2722)
11660ed 
SHA: 12aa43c
Reason: push, by @kdashg

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 May 3, 2022
@alan-baker @github-actions
Update texture built-in functions' use of integers (#2722)
4a5b052 
SHA: 12aa43c
Reason: push, by @kdashg

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@kdashg

kdashg commented May 4, 2022

Copy link
Copy Markdown
Contributor
WGSL meeting minutes 2022-05-03
  • KG: Approved by me.
  • AB: Added extra set of unsigned overloads. 2 versions, signed and unsigned.
  • KG: anyone opposed? More time to think
  • AB: All queries are also unsigned returns. Dimensions, num layers, etc
  • MM: Adding option for signed as well?
  • KG: Will accept signed but queries are unsigned results. Asking about ability to accept signed or via extension to request signed?
  • MM: Using for loop with signed loop counter to iterate over a load command
  • AB: Adds new overloads in terms of params
  • MM: Sounds fine
  • KG: Does change for (i32 0 ; to max;) doesn’t typecheck now. Saved a bit because no-longer int. Don’t have int vs unsigned int verbosity. If have to write type will write i32 which is same number of types as u32. Gets better with ranged for loops as well. This will case previously valid for loops to no longer type check.
  • MM: Think that’s bad. Try to match two things together. Adding overloads shouldn’t break.
  • AB: New overloads of param types. But things that return can only change to unsigned
  • MM: RIght so, signed in 1->7 no problem; signed 1->TexSize may not work. Ok with that
  • BC: New overloads for texture load takes i32 for cords and level new ones take u32 for coords and level. Two variants for i32 and u32 but no way to mix u32 and i32.
  • AB: Agreed to that in previous meeting
  • BC: Thought we had different template per param
  • KG: Considered it but ended up just trying this way with more overloads later if needed
  • MM: You can also complain and suggest it.
  • KG: Will to add later, for now try to not add all the things.
  • KG: We’ll take this for now.

@teoxoy teoxoy mentioned this pull request Jun 15, 2022
Closed
@dneto0 dneto0 mentioned this pull request Jul 5, 2022
Closed
jdarpinian pushed a commit to jdarpinian/gpuweb that referenced this pull request Aug 12, 2022
@alan-baker @jdarpinian
Update texture built-in functions' use of integers (gpuweb#2722)
0ea8c2c 
Fixes gpuweb#2570

* Returned values are always unsigned now
* Added overloads for texture functions that had i32 based parameters
  (other than `offset`) to have an equivalent u32 version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@litherum litherum litherum left review comments
@kdashg kdashg kdashg approved these changes
@jimblandy jimblandy Awaiting requested review from jimblandy
@dneto0 dneto0 Awaiting requested review from dneto0

Assignees

No one assigned

Labels

wgsl WebGPU Shading Language Issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Integer signedness consistency

3 participants

@alan-baker @kdashg @litherum