maxBindGroupsPlusVertexBuffers and unbinding #3938

kainino0x · 2023-03-08T03:34:56Z

My suggestion to use overloads with null didn't work (there's no null type in WebIDL), so I just did the simplest thing and we can bikeshed from here. Alternatives:

Name as "bind" and some other type like GPUBindGroup?/GPUBuffer? in the overloads and explicitly throw a TypeError if not null (weird)
Name as "bind" and write the overload so it just doesn't have a second parameter (why not rename it then?)
Name as "unbind" (straightforward)
Instead of an overload, just make the parameter ~~optional~~nullable (weird because extra parameters are thrown away; do we validate them?)

Fixes #3787
Fixes #2749
Closes #3788 (this PR includes that commit)

Fixes gpuweb#2749 On an implementation on top of Metal that chooses to use argument buffers, bind groups and vertex buffers use the same metal resource (vertex buffer slots). This limit allows the site to trade-off bind groups and vertex buffers themselves, as long as the total sum of them don't exceed a limit. This will be a bit tricky to implement on Metal if it's possible to unbind vertex buffers or bind groups (see gpuweb#3787) but I think it's possible and worth the tradeoff to not expose the complexity to the web, but to instead just handle it in the browser.

greggman · 2023-03-08T19:26:29Z

WebGL uses Type? and accepts anything that coerses to null

https://jsgist.org/?src=8d13f4e798ca408ed0111f40d34d0bb5

kainino0x · 2023-03-08T23:02:14Z

sorry, right, I meant "nullable", not "optional". That's the 4th option above, it just doesn't work as well as WebGL because of the dynamicOffsets and offset/size parameters that come along with these calls.

kainino0x · 2023-03-09T01:54:31Z

That 4th option was also the suggestion we resolved was OK at the F2F, so we could just go ahead and do it.

kainino0x · 2023-03-14T02:40:17Z

[Editors] agreed to make the parameters optional. For validation, treat null as if it's an empty buffer or empty bind group, such that only the defaults (, 0, undefined or , 0, 0 for setVertexBuffer, , [] for setBindGroup) will be valid anyway.

I'll revise soon.

Kangz · 2023-03-14T09:17:15Z

Wasn't the consensus from the F2F that there would be no validation for these optional arguments if the buffer of bindgroup is null?

kainino0x · 2023-03-14T17:25:31Z

Wasn't the consensus from the F2F that there would be no validation for these optional arguments if the buffer of bindgroup is null?

Technically yes but we didn't really talk about it. Editors had some discomfort with it (ambiguity of how many sanity checks there would be on the extra parameters, [EDIT] and accepting clearly-nonsense input from the user), so this was what we decided to draft.

toji

LGTM

spec/index.bs

kainino0x · 2023-03-17T23:07:33Z

Editors meeting again for:

validation of offset/size/dynamicOffsets
when the validation occurs (set time vs draw time)

spec/index.bs

kainino0x · 2023-03-20T21:56:49Z

validation of offset/size/dynamicOffsets

Editors still prefer to keep this validation so we can tell the user when they did something that clearly doesn't make sense. Plan to tacit-resolution-queue it when this PR is ready, but @Kangz please comment anytime if you think we shouldn't do it this way.

Kangz · 2023-03-21T15:36:27Z

That sounds good, and we can always relax validation later if we find it is a problem, so better to start this way.

jimblandy · 2023-03-27T21:26:23Z

That sounds good, and we can always relax validation later if we find it is a problem, so better to start this way.

I agree with this.

github-actions · 2023-03-28T06:40:32Z

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

spec/index.bs

kainino0x · 2023-04-03T20:54:07Z

EDIT: comment moved to correct thread: #3938 (comment)

toji

LGTM with one question.

toji · 2023-04-17T23:10:46Z

spec/index.bs

        optional sequence<GPUBufferDynamicOffset> dynamicOffsets = []);

-    undefined setBindGroup(GPUIndex32 index, GPUBindGroup bindGroup,
+    undefined setBindGroup(GPUIndex32 index, GPUBindGroup? bindGroup,


Does this overload need the bind group to be nullable? I remember we talked about how to interpret these values earlier, and I forget where we landed, but it seems like we only really need one variant of the function (the one with the default value for dynamic offsets) to accept null here.

Strictly speaking we only need one, but it makes just as much sense in both: both can specify zero dynamicOffsets. So for example WASM bindings could exclusively use the Uint32Array overload instead of having to call the other when bindGroup is null.

Thanks for keeping an eye out for Wasm. This indeed makes it easier to implement.

Do WebGPU implementations care here if user calls encoder.setBindGroup(i, undefined, ...); versus encoder.setBindGroup(i, null, ...); ? I find that it would be simpler from marshalling perspective to call encoder.setBindGroup(i, undefined, ...); to unbind a bind group, but is that guaranteed to work all the same as passing null?

It's guaranteed to accept both null and undefined. Per WebIDL spec for conversions from ECMAScript values into a nullable type T?:

2. Otherwise, if V is undefined, and T includes undefined, return the unique undefined value.
3. Otherwise, if V is null or undefined, then return the IDL nullable type T? value null.

T here is GPUBindGroup, which doesn't include undefined.

kainino0x · 2023-04-19T00:15:16Z

I'll land this but leave it on the for webgpu editors meeting label to touch on the question above.

…uweb/gpuweb#3938

…indGroup() and setVertexBuffer(). gpuweb/gpuweb#3938

litherum and others added 3 commits March 7, 2023 19:05

Convert "must" language to make the next bit easier

6647ab1

Add new validation and unbinding calls

fa0b321

kainino0x added tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet labels Mar 8, 2023

kainino0x added this to the V1.0 milestone Mar 8, 2023

kainino0x requested review from litherum and toji March 8, 2023 03:34

kainino0x mentioned this pull request Mar 13, 2023

Add maxBindGroupsPlusVertexBuffers limit #3788

Closed

kainino0x added the for webgpu editors meeting label Mar 13, 2023

Nullable instead of overload

5fe4d62

toji approved these changes Mar 15, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

kainino0x added for webgpu editors meeting and removed for webgpu editors meeting labels Mar 17, 2023

wip throwaway

c0953c6

kainino0x marked this pull request as draft March 20, 2023 20:45

kainino0x commented Mar 20, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

kainino0x added 3 commits March 27, 2023 15:07

Make the default limit 12

b4f3874

Merge branch 'main' into maxBindGroupsPlusVertexBuffers-with-unbind

4110d2f

Add guarantees, some fixmes

5e778d9

kainino0x commented Mar 28, 2023

View reviewed changes

spec/index.bs Show resolved Hide resolved

kainino0x added 2 commits April 4, 2023 11:50

Move validation to draw time

e4a516e

Fix limit description to include draw calls

cd8ad7f

kainino0x force-pushed the maxBindGroupsPlusVertexBuffers-with-unbind branch from ee3c3db to cd8ad7f Compare April 4, 2023 19:02

nit

fa5d5f8

kainino0x requested a review from toji April 4, 2023 19:15

kainino0x added copyediting Pure editorial stuff (copyediting, *.bs file syntax, etc.) and removed for webgpu editors meeting tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue labels Apr 17, 2023

Lift limit to 24

12c085a

kainino0x marked this pull request as ready for review April 17, 2023 20:48

toji approved these changes Apr 17, 2023

View reviewed changes

kainino0x added 2 commits April 17, 2023 20:45

Merge branch 'main' into maxBindGroupsPlusVertexBuffers-with-unbind

b92c26e

typo

22ff782

kainino0x added the for webgpu editors meeting label Apr 18, 2023

kainino0x merged commit 1abedb5 into gpuweb:main Apr 19, 2023

kainino0x deleted the maxBindGroupsPlusVertexBuffers-with-unbind branch April 19, 2023 00:15

kainino0x removed the for webgpu editors meeting label Apr 24, 2023

juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 4, 2023

IDL: Implement GPUSupportedLimits..maxBindGroupsPlusVertexBuffers. gp…

b9b6b88

…uweb/gpuweb#3938

juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 4, 2023

IDL: Add support for unbinding bindgroup and vertex buffers with setB…

e30b1d4

…indGroup() and setVertexBuffer(). gpuweb/gpuweb#3938

teoxoy mentioned this pull request Aug 18, 2023

Add missing limits to correspondence doc #4273

Closed

3 tasks

teoxoy mentioned this pull request Dec 15, 2023

Bind group and layout entries should be optional gfx-rs/wgpu#4738

Open

ErichDonGubler mentioned this pull request Sep 27, 2024

GPU{ComputePass,RenderPass,RenderBundle}Encoder.setBindGroup is now nullable mdn/content#36078

Closed

caugner mentioned this pull request Nov 5, 2024

Unset bind group by passing null to setBindGroup() mdn/browser-compat-data#24936

Merged

maxBindGroupsPlusVertexBuffers and unbinding #3938

maxBindGroupsPlusVertexBuffers and unbinding #3938

Uh oh!

Conversation

kainino0x commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greggman commented Mar 8, 2023

Uh oh!

kainino0x commented Mar 8, 2023

Uh oh!

kainino0x commented Mar 9, 2023

Uh oh!

kainino0x commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kangz commented Mar 14, 2023

Uh oh!

kainino0x commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kainino0x commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kainino0x commented Mar 20, 2023

Uh oh!

Kangz commented Mar 21, 2023

Uh oh!

jimblandy commented Mar 27, 2023

Uh oh!

github-actions bot commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kainino0x commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toji left a comment

Choose a reason for hiding this comment

Uh oh!

toji Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

kainino0x Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

juj Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

kainino0x Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Apr 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kainino0x commented Mar 8, 2023 •

edited

Loading

kainino0x commented Mar 14, 2023 •

edited

Loading

kainino0x commented Mar 14, 2023 •

edited

Loading

kainino0x commented Mar 17, 2023 •

edited

Loading

github-actions bot commented Mar 28, 2023 •

edited

Loading

kainino0x commented Apr 3, 2023 •

edited

Loading

kainino0x Aug 4, 2023 •

edited

Loading