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

Remove inputSlot member from WebGPUVertexInputDescriptor - #151

Closed
litherum wants to merge 1 commit into
gpuweb:masterfrom
litherum:vertexdescriptor
Closed

litherum wants to merge 1 commit into
gpuweb:masterfrom
litherum:vertexdescriptor

Conversation

@litherum

Copy link
Copy Markdown
Contributor

Part 1 of #146

Comment thread design/sketch.webidl
void setIndexBuffer(WebGPUBuffer buffer, u32 offset);
void setVertexBuffers(u32 startSlot, sequence<WebGPUBuffer> buffers, sequence<u32> offsets);
// "startSlot" refers to an index into the "inputs" sequence in the currently bound pipeline's WebGPUInputStateDescriptor.
// "buffers" and "offsets" are parallel.

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.

We can enforce that they're parallel by making a new object which holds both items.

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.

If we did this, it would probably make sense to use it in setIndexBuffer, copyBufferToBuffer and possibly BufferCopyView and BufferBinding too.

Comment thread design/sketch.webidl
void setVertexBuffers(u32 startSlot, sequence<WebGPUBuffer> buffers, sequence<u32> offsets);
// "startSlot" refers to an index into the "inputs" sequence in the currently bound pipeline's WebGPUInputStateDescriptor.
// "buffers" and "offsets" are parallel.
void setVertexBuffers(u32 startSlot, sequence<WebGPUBuffer?> buffers, sequence<u32?> offsets);

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.

What's the reasoning for allowing holes in setVertexBuffers? It seems that it could be trivially replaced by multiple calls that don't have holes.

@magcius magcius Dec 18, 2018 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we figured out whether holes in setVertexBuffers unbind the buffer resource, or leave the old resource bound? If we leave the old resource bound, if I destroy the buffer, will the validation layer complain? How can I explicitly unbind the resource?

My recommendation, which is a bit bizarre, is that a hole/undefined means "leave existing resource", explicit null means "undefined", and we remove the startSlot parameter from setVertexBuffers, meaning that someone can use undefined for the same purpose. undefined is more flexible.

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.

separating undefined and null seems like a recipe for troubles in WASM land

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.

In principle I like being able to distinguish "keep" and "clear" but distinguishing null and undef/hole is just too unexpected IMO. In the meeting I think we decided for now that null/undef/hole would clear, and if you don't want to clear you just have to do multiple setVertexBuffers calls.

We could also disallow undef/hole and allow only null, if that would be less confusing.

@kainino0x kainino0x Dec 18, 2018 •

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.

Or I suppose we could do:

enum WebGPUSetVertexBuffersEntry { "unset", "keep" };
partial interface WebGPURenderPassEncoder {
    void setVertexBuffers(u32 startSlot,
        sequence<WebGPUBuffer | WebGPUSetVertexBuffersEntry> buffers,
        sequence<u32?> offsets);
};

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.

Maybe in that case it would be better startSlot entirely and expect the full range to be passed? Otherwise we could accept ranges for keep/clear to reduce the amount repeated values.

@litherum

Copy link
Copy Markdown
Contributor Author

This same logic should be applied to resource bind points as well, not just vertex arrays.

@litherum

litherum commented Jan 25, 2019 •

Copy link
Copy Markdown
Contributor Author

I noticed that GPUVertexAttributeDescriptor has u32 shaderLocation which is supposed to match what's in the shader source code, but GPUAttachmentDescriptor doesn't. It looks like fragment shaders would use MRT by referencing the index into the sequence<GPUAttachmentDescriptor> colorAttachments inside the GPUAttachmentsStateDescriptor.

@litherum

Copy link
Copy Markdown
Contributor Author

This same logic should be applied to resource bind points as well, not just vertex arrays.

#188 does this.

@grorg

grorg commented Mar 4, 2019

Copy link
Copy Markdown
Contributor

Discussed in the 4 March 2019 meeting

@kainino0x

kainino0x commented Mar 27, 2019 •

Copy link
Copy Markdown
Contributor

I think this is obsoleted by #186 and #249.

Bind group/resource bindings and {buffer,offset} pair tbd.

@kainino0x kainino0x closed this Mar 27, 2019
@kainino0x kainino0x mentioned this pull request Oct 30, 2019
13 tasks done
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants