Add GPUPipelineBase.getBindGroupLayout. - #543
Conversation
kainino0x
left a comment
There was a problem hiding this comment.
no further comments, but easy to read and lgtm
aee44b1 to
e96bd3e
Compare
|
I redid this completely and this was stalled for a long time, so I resolved all comments and let's start the review anew! |
kvark
left a comment
There was a problem hiding this comment.
Looks better now! A few more concerns/suggestions here.
| <dl dfn-type=attribute dfn-for="GPUPipelineLayout"> | ||
| : <dfn>\[[bindGroupLayout]]</dfn> of type `sequence<GPUBindGroupLayout>`. | ||
| :: | ||
| The layout of {{GPUBindGroup}}s that {{GPUPipelineBase}} using `this` will be compatible with. |
There was a problem hiding this comment.
can we say something that BGL at sequence index i is expected at binding slot i, referring to set_bind_group from here to avoid confusion?
There was a problem hiding this comment.
Definitely thanks for looking into it in #724!
|
|
||
| 1. Return a new error {{GPUBindGroupLayout}}. | ||
|
|
||
| 1. Return a new {{GPUBindGroupLayout}} wrapping |
There was a problem hiding this comment.
the |this|.{{GPUPipelineBase/[[layout]]}}.{{GPUPipelineLayout/[[bindGroupLayout]]}}[|index|] already has a type of GPUBindGroupLayout. What is the "wrapping" here?
There was a problem hiding this comment.
I wanted to say that a new JS object is created so that pipeline.getBindGroupLayout(i) always provide the same JS object but at the same time have it be unrelated to GPUBindGroupLayout objects passed in GPUPipelineLayoutDescriptor.bindGroupLayouts.
This is to avoid having to deal with what happens if you call getBindGroupLayout on valid vs. invalid pipeline that was created with an explicit pipeline layout. Since the error state is on the device timeline, we don't know it in the call to getBindGroupLayout that's in the content timeline, and can't know if we should return the same GPUBindGroupLayout that was passed explicitly via that GPUPipelineLayout (with its expando properties) or a new JS object that represents an error GPUBindGroupLayout.
There was a problem hiding this comment.
Ok, that makes sense.
Sounds like some clarification is needed in the spec itself. Maybe shorter, but just saying it's a "wrap" is not clear.
There was a problem hiding this comment.
I'm not 100% sure how to phrase that, I wanted to write a new {{GPUBindGroupLayout}} that references the same [=bind group layout=] as ... but [=object=] only exists for device and adapter. Is the plan to do it for all objects? +CC @kainino0x
There was a problem hiding this comment.
We can leave this as is and then fix it to say "new {{GPUBindGroupLayout}} that is [=group-equivalent=] to ..." after #724 lands
There was a problem hiding this comment.
Is the plan to do it for all objects? +CC @kainino0x
Yes (see "define texture (internal object)", "define buffer (internal object)")
kvark
left a comment
There was a problem hiding this comment.
Almost there, from my side :)
|
|
||
| 1. Return a new error {{GPUBindGroupLayout}}. | ||
|
|
||
| 1. Return a new {{GPUBindGroupLayout}} wrapping |
There was a problem hiding this comment.
Ok, that makes sense.
Sounds like some clarification is needed in the spec itself. Maybe shorter, but just saying it's a "wrap" is not clear.
|
|
||
| 1. Return a new error {{GPUBindGroupLayout}}. | ||
|
|
||
| 1. Return a new {{GPUBindGroupLayout}} wrapping |
There was a problem hiding this comment.
Is the plan to do it for all objects? +CC @kainino0x
Yes (see "define texture (internal object)", "define buffer (internal object)")
|
@JusSn PTAL! |
* Add GPUPipelineBase.getBindGroupLayout. * Add an issue
* Add GPUPipelineBase.getBindGroupLayout. * Add an issue
WGSL now uses entry point function parameters and return values for entry point IO, instead of module-scope variables.
PTAL! I tried to make the algorithm for
getBindGroupLayoutas formal as possible but there's some hand-waviness because the spec doesn't haveGPUShaderModulereflection yet.Preview | Diff