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

Use attributeCount=0 as sentinel instead of VertexBufferNotUsed? #432

Closed
kainino0x opened this issue Nov 23, 2024 · 6 comments · Fixed by #439
Closed

Use attributeCount=0 as sentinel instead of VertexBufferNotUsed? #432

kainino0x opened this issue Nov 23, 2024 · 6 comments · Fixed by #439

Comments

@kainino0x
Copy link
Collaborator

See discussion here: #424 (comment)

  • to answer question: No, don't need Undefined
  • KN to investigate using attributeCount (what does the JS API do - and could it change to treat [] as a hole?)

But if we do remove VertexBufferNotUsed=0 then there will still be a reserved 0 for Undefined and then we should use it for defaulting since that's what we do everywhere else

@kainino0x kainino0x added the needs exploration Needs offline exploration (e.g. in an implementation) label Nov 23, 2024
@kainino0x
Copy link
Collaborator Author

kainino0x commented Nov 23, 2024

Here is the relevant spec discussion: gpuweb/gpuweb#2864

@kainino0x
Copy link
Collaborator Author

kainino0x commented Nov 23, 2024

tl;dr: This was actually how webgpu.h in Dawn worked 2.5 years ago, but then we found that there was a mismatch between C and JS so we added VertexBufferNotUsed. We (the WebGPU editors/WG) just didn't think it was necessary to change the JS spec.

Honestly though, the only reason not to change it in JS is that the otherwise-meaningless validation (still validates arrayStride, and draw calls still validate there's a vertex buffer bound even though it's not really used) might catch some weird programming error.

If we do use attributeCount=0 as the sentinel, but JS doesn't change, then:

  • C-on-JS implementations will simply translate WGPUVertexBufferLayouts with attributeCount=0 as null.
  • JS-on-C implementations will have to inject an error (to createRenderPipeline) when they get attributes=[]. (This problem goes away if JS changes to treat it the same as a hole.)

@kainino0x
Copy link
Collaborator Author

Nov 25 meeting:

  • KN: Inclined to change C back, and then try to change JS to match (loosen the validation there so it doesn't check you have a vertex buffer bound if you're actually not using it)
  • CF: Agree, this seems silly
  • KN to attempt a webgpu.h PR

@kainino0x
Copy link
Collaborator Author

  • JS-on-C implementations will have to inject an error (to createRenderPipeline) when they get attributes=[]. (This problem goes away if JS changes to treat it the same as a hole.)

Sorry, this was totally incorrect, don't know why I said it. JS does not treat this as an error. Instead it requires there be a vertex buffer bound at that slot, even though it will not be used. I do not think there is a reasonable way we can use attributeCount=0 as the sentinel unless either the JS API changes (gpuweb/gpuweb#4999) or browsers communicate the distinction through some side-channel (extension, or some possibly-temporary sentinel value like whether the step mode is Undefined or not).

@kainino0x
Copy link
Collaborator Author

kainino0x commented Nov 26, 2024

some possibly-temporary sentinel value like whether the step mode is Undefined or not

OK, here's a proposal for how we can do this in C without VertexBufferNotUsed (and while being flexible to whatever JS does): #439

@kainino0x kainino0x added !discuss Needs discussion (at meeting or online) and removed needs exploration Needs offline exploration (e.g. in an implementation) labels Nov 27, 2024
@kainino0x
Copy link
Collaborator Author

kainino0x commented Dec 9, 2024

Dec 9 meeting:

  • KN: Turned out JS needs to distinguish null vs { attributes: [] }. I suggested changing this in the WG, but they preferred not to introduce extra steps to the spec. Regardless of whether JS ever changes, came up with a proposal for how to do this in C: the sentinel value is stepMode=Undefined AND attributeCount=0.
  • Remove WGPUVertexStepMode_VertexBufferNotUsed #439
  • CF: Sounds good
  • (LK/KN discussion of migration in Dawn)

@kainino0x kainino0x removed the !discuss Needs discussion (at meeting or online) label Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant