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

Remove inputSlot member from WebGPUVertexInputDescriptor #151

Closed
wants to merge 1 commit into from

Conversation

litherum
Copy link
Contributor

Part 1 of #146

@@ -505,7 +504,9 @@ interface WebGPURenderPassEncoder : WebGPUProgrammablePassEncoder {
void setScissorRect(u32 x, u32 y, u32 width, u32 height);

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
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
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.

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
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.

Copy link

@magcius magcius Dec 18, 2018

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
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
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.

Copy link
Contributor

@kainino0x kainino0x Dec 18, 2018

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
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
Contributor Author

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

@litherum
Copy link
Contributor Author

litherum commented Jan 25, 2019

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
Contributor Author

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

#188 does this.

@grorg
Copy link
Contributor

grorg commented Mar 4, 2019

Discussed in the 4 March 2019 meeting

@kainino0x
Copy link
Contributor

kainino0x commented Mar 27, 2019

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
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.

None yet

6 participants