Add optional feature `clip-distances` #4588

Jiawei-Shao · 2024-04-18T08:47:18Z

This patch adds the optional feature clip-distances to WebGPU API and WGSL SPEC.

Add WebGPU API and WGSL optional feature clip-distances
Add new vertex stage output builtin variable clip_distances
Update the primitive clipping algorithm with user-defined clip distances
Add new validation rules to the validation algorithm on inter-stage interfaces when clip-distances is used in the vertex stage
Add new device limit maxClipDistancesSize and update the related vertex stage validation algorithm

Fixed: #390

This patch adds the optional feature `clip-distance to WebGPU API and WGSL SPEC. - Add WebGPU API and WGSL optional feature `clip-distance` - Add new vertex stage output builtin variable `clip-distance` - Update the primitive clipping algorithm with user-defined clip distances - Add new validation rules to the validation algorithm on inter-stage interfaces when `clip-distance` is used in the vertex stage - Add new device limit `maxClipDistances` and update the related vertex stage validation algorithm

beaufortfrancois

I believe it would help developers if WGSL builtin value was renamed to clip_distances and WebGPU feature to "clip-distances".

spec/index.bs

github-actions · 2024-04-19T08:39:14Z

Previews, as seen when this build job started (466c36f):
WebGPU _{webgpu.idl | Explainer | Correspondence Reference}
WGSL _{grammar.js | wgsl.lalr.txt}

Jiawei-Shao · 2024-04-19T08:45:09Z

I believe it would help developers if WGSL builtin value was renamed to clip_distances and WebGPU feature to "clip-distances".

Done @beaufortfrancois

beaufortfrancois · 2024-04-19T08:46:53Z

I believe it would help developers if WGSL builtin value was renamed to clip_distances and WebGPU feature to "clip-distances".

Done @beaufortfrancois

I'm still seeing singular "clip distance", not plural "clip distances".

Jiawei-Shao · 2024-04-19T08:53:24Z

I believe it would help developers if WGSL builtin value was renamed to clip_distances and WebGPU feature to "clip-distances".

Done @beaufortfrancois

I'm still seeing singular "clip distance", not plural "clip distances".

Sorry @beaufortfrancois , now there should be no clip distance.

wgsl/index.bs

beaufortfrancois · 2024-04-19T09:02:28Z

I believe it would help developers if WGSL builtin value was renamed to clip_distances and WebGPU feature to "clip-distances".

Done @beaufortfrancois

I'm still seeing singular "clip distance", not plural "clip distances".

Sorry @beaufortfrancois , now there should be no clip distance.

You may want to edit #4588 (comment) and PR title as well.

spec/index.bs

Jiawei-Shao · 2024-04-22T01:32:30Z

I believe it would help developers if WGSL builtin value was renamed to clip_distances and WebGPU feature to "clip-distances".

Done @beaufortfrancois

I'm still seeing singular "clip distance", not plural "clip distances".

Sorry @beaufortfrancois , now there should be no clip distance.

You may want to edit #4588 (comment) and PR title as well.

Done

spec/index.bs

alan-baker · 2024-05-13T14:34:07Z

wgsl/index.bs

+  <tr><td>[=built-in values/clip_distances=]
+      <td>vertex
+      <td>output
+      <td>array&lt;f32, N&gt;


From what I can tell, the API requires N to be 8. Is there a reason not to restrict WGSL to the same value?

I've added the restriction to WGSL. PTAL, thanks!

wgsl/index.bs

Kangz · 2024-05-14T22:13:34Z

GPU Web WG 2024-05-08 Pacific-time

JS: One question is whether we should make the limit for number to 8, since that’s what Metal and D3D has as hardcoded.
KN: I think we could add a limit later if we wanted to. I think the downside is that if we ship this with the hardcoded limit, that might cause minor issues, but it’s very unlikely, because of the reasons you mentioned.
KG: Sounds reasonable to me
MW: Believe it’s indeed limited to 8 on Metal.
JS: I really want someone to review the WGSL side
KN: Everyone happy to land this feature, once details are ironed out?
KG: sounds like yes, and OK with hardcoding to 8.
Consensus: Hardcode max to 8, and we’ll review the WGSL side before landing.

spec/index.bs

Jiawei-Shao · 2024-05-17T01:15:16Z

Hi @kainino0x @alan-baker could you take a look at this PR?

kainino0x

Specs LGTM. I haven't dug into the backend investigations but it seems like the mapping is fairly straightforward. Still, what is the status of prototyping and testing? It would be helpful to know that it's behaving consistently across backends.

spec/index.bs

Jiawei-Shao · 2024-05-20T05:41:47Z

Hi @kainino0x ,

I haven't started prototyping and testing yet but according to previous investigations the behaviors of clip distances are quite clear and should be same among D3D12, Metal and Vulkan. Do you think now we should have more experiments and prototyping on this feature?

kainino0x · 2024-05-21T03:10:25Z

IMO for nontrivial changes, at least one prototype and some basic tests should work before we land changes to the spec (and then land the tests).

Generally I'd like the spec to be "evergreen", by which I mean, it should always be a valid target for implementations, and we need implementation experience to be sure of that. We could reevaluate that once we've published a Candidate Recommendation, but right now I think it's necessary.

kainino0x · 2024-05-21T03:11:59Z

We can discuss at the WG meeting though, maybe people think the investigation already meets the bar needed to land the spec.

Jiawei-Shao · 2024-05-29T01:20:27Z

Note that in SPIRV-Cross the SPIRV ClipDistance is directly translated into [[clip_distance]] in MSL when it is an vertex output and SV_ClipDistance0 and SV_Clip_Distance1 in HLSL.

beaufortfrancois reviewed Apr 18, 2024

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

Jiawei-Shao added 3 commits April 19, 2024 14:33

Fix build errors

70fbac5

Fix more build errors

2da3005

Fix

47fef8b

Jiawei-Shao requested review from Kangz, dneto0 and kdashg April 19, 2024 08:45

Rename clip-distance to clip-distances

01fa158

beaufortfrancois reviewed Apr 19, 2024

View reviewed changes

wgsl/index.bs Outdated Show resolved Hide resolved

beaufortfrancois reviewed Apr 19, 2024

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

Address reviewer's comments

87938ca

Jiawei-Shao changed the title ~~Add optional feature clip-distance~~ Add optional feature clip-distances Apr 22, 2024

beaufortfrancois self-requested a review April 22, 2024 07:26

beaufortfrancois approved these changes Apr 22, 2024

View reviewed changes

Jiawei-Shao requested a review from kainino0x April 23, 2024 05:55

kainino0x reviewed Apr 24, 2024

View reviewed changes

kainino0x added the for webgpu editors meeting label Apr 24, 2024

Jiawei-Shao added 3 commits April 24, 2024 13:59

Merge branch 'main' into add-clip-distance

f548cd5

Address more reviewer's comments

4286539

Small fix

0c4e2a0

kainino0x mentioned this pull request Apr 30, 2024

Define conventions/rules for features that add new limits #4613

Draft

Jiawei-Shao added 2 commits May 7, 2024 09:45

Merge branch 'main' into add-clip-distance

cae90d3

Specify the maximum array size of clip_distances to be 8

a21ffcb

Merge branch 'main' into add-clip-distance

2ec5a07

kainino0x reviewed May 8, 2024

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

kainino0x requested a review from alan-baker May 8, 2024 23:15

alan-baker reviewed May 13, 2024

View reviewed changes

Jiawei-Shao added 4 commits May 14, 2024 13:20

Merge branch 'main' into add-clip-distance

a602152

Define maximum array length of clip_distances in WGSL SPEC

29ae38a

Fix typo in validating inter-stage interfaces

4e01f76

Small fix

0dd9ffc

alan-baker reviewed May 14, 2024

View reviewed changes

wgsl/index.bs Outdated Show resolved Hide resolved

Jiawei-Shao added 2 commits May 15, 2024 07:17

Merge branch 'main' into add-clip-distance

6f8cd93

Fix typo

23a46cc

kainino0x reviewed May 15, 2024

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

Jiawei-Shao added 2 commits May 16, 2024 09:26

Merge branch 'main' into add-clip-distance

cd80608

Small fix

9428928

Merge branch 'main' into add-clip-distance

b245209

kainino0x approved these changes May 18, 2024

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

Jiawei-Shao added 2 commits May 20, 2024 13:21

Merge branch 'main' into add-clip-distance

c59dffe

Fix indent

466c36f

kainino0x added wgsl WebGPU Shading Language Issues api WebGPU API labels May 21, 2024

kdashg merged commit adb07b1 into gpuweb:main May 29, 2024
4 checks passed

dneto0 mentioned this pull request May 30, 2024

Test clip-distances feature gpuweb/cts#3773

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional feature `clip-distances` #4588

Add optional feature `clip-distances` #4588

Jiawei-Shao commented Apr 18, 2024 •

edited

beaufortfrancois left a comment

github-actions bot commented Apr 19, 2024 •

edited

Jiawei-Shao commented Apr 19, 2024

beaufortfrancois commented Apr 19, 2024

Jiawei-Shao commented Apr 19, 2024

beaufortfrancois commented Apr 19, 2024

Jiawei-Shao commented Apr 22, 2024

alan-baker May 13, 2024

Jiawei-Shao May 14, 2024

Kangz commented May 14, 2024

Jiawei-Shao commented May 17, 2024

kainino0x left a comment

Jiawei-Shao commented May 20, 2024

kainino0x commented May 21, 2024

kainino0x commented May 21, 2024

Jiawei-Shao commented May 29, 2024

Add optional feature clip-distances #4588

Add optional feature clip-distances #4588

Conversation

Jiawei-Shao commented Apr 18, 2024 • edited

beaufortfrancois left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 19, 2024 • edited

Jiawei-Shao commented Apr 19, 2024

beaufortfrancois commented Apr 19, 2024

Jiawei-Shao commented Apr 19, 2024

beaufortfrancois commented Apr 19, 2024

Jiawei-Shao commented Apr 22, 2024

alan-baker May 13, 2024

Choose a reason for hiding this comment

Jiawei-Shao May 14, 2024

Choose a reason for hiding this comment

Kangz commented May 14, 2024

Jiawei-Shao commented May 17, 2024

kainino0x left a comment

Choose a reason for hiding this comment

Jiawei-Shao commented May 20, 2024

kainino0x commented May 21, 2024

kainino0x commented May 21, 2024

Jiawei-Shao commented May 29, 2024

Add optional feature `clip-distances` #4588

Add optional feature `clip-distances` #4588

Jiawei-Shao commented Apr 18, 2024 •

edited

github-actions bot commented Apr 19, 2024 •

edited