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

Clean up WebGPUVertexInputDescriptor #146

Closed
wants to merge 1 commit into from

Conversation

litherum
Copy link
Contributor

@litherum litherum commented Dec 7, 2018

Three changes:

  • As described in Relationship between setVertexBuffers() and setBindGroup() is unclear #145, inputSlot in the
    WebGPUVertexInputDescriptor should instead be implicitly derived from its
    position in the inputs field.
  • The format of each attribute is already present in the shader. We should remove
    the redundant information.
  • "Input" is not a very descriptive name. Instead, rename it to be "VertexBuffer".

Three changes:
- As described in gpuweb#145, inputSlot in the
      WebGPUVertexInputDescriptor should instead be implicitly derived from its
      position in the inputs field.
- The format of each attribute is already present in the shader. We should remove
      the redundant information.
- "Input" is not a very descriptive name. Instead, rename it to be "VertexBuffer".
u32 offset;
WebGPUVertexFormatEnum format;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to keep the format because for example a shader taking vec2 as an input can have data stored at in many different ways:

  • As two 8/16/32 bit integers that are converted to a float with the same value.
  • As two 8/16/32 bit integers that are "normalized" to be turned in a float in the [0, 1] range.
  • As two float.

};

dictionary WebGPUVertexInputDescriptor {
u32 inputSlot;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of keeping this, so that applications can define "holes" in their vertex buffers, which would allow minimizing state setting between two pipeline using very similar but different vertex buffers (they could match on VB 0 to 5 and one of them having VB 6 and the other VB 7).

Actually having holes in interfaces like this is something I also wanted to bring up for the pipeline layout and color attachments as well. We already had at least on project prototyping on Dawn ask if we could have holes in the pipeline layout.

Copy link
Contributor

Choose a reason for hiding this comment

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

JavaScript arrays are sparse and heterogeneous, so they don't prevent us from having holes. With the change at [1], we could probably (if the IDL accepts it) have:

{ vertexBuffers: [buffer0, null, buffer2] }

or

const vertexBuffers = [];
vertexBuffers[0] = buffer0;
vertexBuffers[2] = buffer2;
{ vertexBuffers }

@magcius
Copy link

magcius commented Dec 7, 2018

The name change is OK (I would bikeshed for "VertexInputBuffer" but no strong feelings on that). Strong 👎 on the other two changes for rationale discussed as before.

Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

I think I like VertexInputBuffer or VertexInputIndex (or VertexInputSlot because I don't think it hurts to still make a distinction on the word "index"), because it matches with VertexInputDescriptor.

@@ -383,7 +381,7 @@ dictionary WebGPUInputStateDescriptor {
WebGPUIndexFormatEnum indexFormat;

sequence<WebGPUVertexAttributeDescriptor> attributes;
sequence<WebGPUVertexInputDescriptor> inputs;
sequence<WebGPUVertexBufferDescriptor> vertexBuffers;
Copy link
Contributor

Choose a reason for hiding this comment

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

[1] This would probably have to be sequence<WebGPUVertexBufferDescriptor?>

};

dictionary WebGPUVertexInputDescriptor {
u32 inputSlot;
Copy link
Contributor

Choose a reason for hiding this comment

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

JavaScript arrays are sparse and heterogeneous, so they don't prevent us from having holes. With the change at [1], we could probably (if the IDL accepts it) have:

{ vertexBuffers: [buffer0, null, buffer2] }

or

const vertexBuffers = [];
vertexBuffers[0] = buffer0;
vertexBuffers[2] = buffer2;
{ vertexBuffers }

@grorg
Copy link
Contributor

grorg commented Dec 10, 2018

This was discussed in the 10 Dec 2018 meeting

@grorg
Copy link
Contributor

grorg commented Dec 10, 2018

Resolution at the meeting: @litherum will split this into multiple PRs.

@litherum
Copy link
Contributor Author

Moved to #151

@litherum litherum closed this Dec 13, 2018
@kainino0x kainino0x mentioned this pull request Oct 30, 2019
13 tasks
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

5 participants