Vertex input: description, mental model, naming by kainino0x · Pull Request #469 · gpuweb/gpuweb

kainino0x · 2019-10-10T02:42:14Z

I've been struggling with coming up with a mental model for vertex input
ever since we rephrased it in terms of the "push" model. Because we were
talking about vertex pulling recently, in which things can really be
phrased as arrays-of-structures, I realized that was the basis I needed
to make the mental model finally make sense.

The key naming change is that "vertex buffers" no longer contain
"attributes". Instead, they are arrays which contain "structures", and
each structure is defined by a sequence of "members". Those members
secondarily specify their shader input location.

I also wrote some text to describe this mental model.

There is also one functional change aside from naming: the attribute
byte offset is no longer optional. I think it's crucial to the mental
modeling that the offset be front-and-center in the vertex structure
definition.

Preview | Diff

I've been struggling with coming up with a mental model for vertex input ever since we rephrased it in terms of the "push" model. Because we were talking about vertex pulling recently, in which things can really be phrased as arrays-of-structures, I realized that was the basis I needed to make the mental model finally make sense. The key naming change is that "vertex buffers" no longer contain "attributes". Instead, they are arrays which contain "structures", and each structure is defined by a sequence of "members". Those members *secondarily* specify their shader input location. I also wrote some text to describe this mental model. There is also one functional change aside from naming: the attribute byte offset is no longer optional. I think it's crucial to the mental modeling that the offset be front-and-center in the vertex structure definition.

kvark

I appreciate non-default offset :)

kvark · 2019-10-10T15:33:51Z

+    required GPUBufferSize elementStride;
    GPUInputStepMode stepMode = "vertex";
-    required sequence<GPUVertexAttributeDescriptor> attributeSet;
+    required sequence<GPUVertexStructureMemberDescriptor> structure;


There might be a bit of a clash between "element" and "structure". Perhaps, we can use one of those more consistently, e.g. call the other thing structureStride, or rename this one to sequence<GPUVertexElementDescriptor> elements?

I was going to comment on this, but it got a bit too close to bikeshedding, but I think this exposes the problem.

Structures aren't the same as elements. The mental model here is:

struct { float3 Position; float2 TexCoord; float4 Color; } VertexArray[];

VertexArray[i] is an "element", VertexArray[i].Position is a "structure member".

Ultimately, I don't like the name "element" because it could also mean "structure element" (as in structure member), or "primitive element" (e.g. one triangle, as used by glDrawElements)

This also ignores the admittedly rare case that vertex arrays might have extra data used by the draw, and that's OK. That is:

struct { float4 Position; char pad[29]; } VertexArray[];

is perfectly allowed just by setting the buffer stride to a greater value, and using a single element at offset +0x00. Imagine if you could explicitly size a struct with __attribute__((size=0x3f)) struct Foo or something in GCC.

Could you distill that to concrete wording and names you think we should be using? Or are you arguing that the PR as it stands is clear enough?

I would vote to keep existing language. Quick survey:

D3D12

GPUVertexInputDescriptor maps to an "input layout" (described by D3D12_INPUT_LAYOUT_DESC).

GPUVertexBufferDescriptor is the odd one out, since it's not required at input layout creation time. Instead, it's part of D3D12_VERTEX_BUFFER_VIEW, which also includes the buffer handle for the data.

GPUVertexAttributeDescriptor maps to D3D12_INPUT_ELEMENT_DESC (another mismatch for "element", being used in the "structure element" case as above). Instead of it being hierarchical, a number-based binding scheme is used.

Vulkan

GPUVertexInputDescriptor maps to VkPipelineVertexInputStateCreateInfo

GPUVertexBufferDescriptor maps to VkVertexInputBindingDescription, again, though, a number-based binding is used like D3D11/D3D12 rather than the hierarchy.

GPUVertexAttributeDescriptor maps to VkVertexInputAttributeDescription

Jasper's Personal Taste

I'm not entirely happy with "GPUVertexBufferDescriptor", because it's not yet attached to a vertex buffer with data, it's simply a skeleton for what might fit. However, I'm having a hard time coming up with something better, especially if we don't have the number-based binding approach and mandate strides at input layout creation time. "VertexInputBinding" is way worse.

GPUVertexInputDescriptor and GPUVertexAttributeDescriptor are fine.

got a bit too close to bikeshedding

I think this is 100% ok. After all, this PR is pretty much just renaming things.

I will catch up on this thread when I have a chance.

@magcius thank you for the mini-investigation!
I think the links you provided support the idea of not using the "element" as this PR suggests: D3D12 "element" is a member of the struct (so, our "struct member"). I don't think we should introduce semantics that will confuse D3D12 users, if we can avoid this.

How about elementStride -> structureStride and structure -> structureMembers?

That sounds pretty good to me. Maybe arrayStride and structureMembers? structMembers?

I'm not entirely happy with "GPUVertexBufferDescriptor"

Perhaps we should steal from Vulkan: GPUVertexBindingDescriptor? That said, I'm also fine with GPUVertexBufferDescriptor.

Updated with arrayStride and structMembers

Kangz

Looks very good!

Kangz · 2019-10-15T15:09:00Z

PTAL @RafaelCintron @JusSn

kdashg · 2019-10-30T23:24:43Z

-    GPUBufferSize offset = 0;
-    required GPUVertexFormat format;
-    required unsigned long shaderLocation;
+dictionary GPUVertexInputDescriptor {


I'm happy to propose this elsewhere, but I think GPUVertexInputsDescriptor (InputS) would make it easier to keep straight vs Vertex[Buffer,Attribute]Desc.

Right after I pushed the previous commit I decided to rename this too. WDYT?

"State" is a little vague, but I think it's an improvement.
I think adding "Layout" is more verbose than we need, but it's also fine.
I'm happy with this overall, thanks!

…outDescriptor

kainino0x · 2019-10-30T23:30:06Z

@magcius PTAL as well

magcius · 2019-10-30T23:31:39Z

I like these names.

Spec changes: gpuweb/gpuweb#469

Specification change: gpuweb/gpuweb#469 Dawn roll: https://dawn.googlesource.com/dawn/+log/c3284fa40ec6b12731ed66c2f2a9256ae3fa692e..ae1f25fee85ebf2773b24a0a4f39c160839b1dbb CTS roll: gpuweb/cts@3dc37c8...e114192 TBR: haraken@chromium.org Bug: dawn:22 Change-Id: Ib427760b9fba4b2cc4290de046c4a31d77e0b67a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1900807 Reviewed-by: Austin Eng <enga@chromium.org> Reviewed-by: Yunchao He <yunchao.he@intel.com> Reviewed-by: Corentin Wallez <cwallez@chromium.org> Commit-Queue: Kai Ninomiya <kainino@chromium.org> Cr-Commit-Position: refs/heads/master@{#713682}

1.Add Vulkan validation layers on Windows https://dawn-review.googlesource.com/c/dawn/+/11120 Vulkan validation layer is defaultly enabled on Windows, to disable Vulkan validation layer, pass args "dawn_enable_vulkan_validation_layers=false" 2.Update naming for vertex state gpuweb/gpuweb#469 https://dawn-review.googlesource.com/c/dawn/+/13100

Automatic update from web-platform-tests Update naming for vertex state Specification change: gpuweb/gpuweb#469 Dawn roll: https://dawn.googlesource.com/dawn/+log/c3284fa40ec6b12731ed66c2f2a9256ae3fa692e..ae1f25fee85ebf2773b24a0a4f39c160839b1dbb CTS roll: gpuweb/cts@3dc37c8...e114192 TBR: haraken@chromium.org Bug: dawn:22 Change-Id: Ib427760b9fba4b2cc4290de046c4a31d77e0b67a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1900807 Reviewed-by: Austin Eng <enga@chromium.org> Reviewed-by: Yunchao He <yunchao.he@intel.com> Reviewed-by: Corentin Wallez <cwallez@chromium.org> Commit-Queue: Kai Ninomiya <kainino@chromium.org> Cr-Commit-Position: refs/heads/master@{#713682} -- wpt-commits: 4831b37a33f002e67ecc5f9f5b41145721e0ab60 wpt-pr: 20186

Automatic update from web-platform-tests Update naming for vertex state Specification change: gpuweb/gpuweb#469 Dawn roll: https://dawn.googlesource.com/dawn/+log/c3284fa40ec6b12731ed66c2f2a9256ae3fa692e..ae1f25fee85ebf2773b24a0a4f39c160839b1dbb CTS roll: gpuweb/cts@3dc37c8...e114192 TBR: harakenchromium.org Bug: dawn:22 Change-Id: Ib427760b9fba4b2cc4290de046c4a31d77e0b67a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1900807 Reviewed-by: Austin Eng <engachromium.org> Reviewed-by: Yunchao He <yunchao.heintel.com> Reviewed-by: Corentin Wallez <cwallezchromium.org> Commit-Queue: Kai Ninomiya <kaininochromium.org> Cr-Commit-Position: refs/heads/master{#713682} -- wpt-commits: 4831b37a33f002e67ecc5f9f5b41145721e0ab60 wpt-pr: 20186 UltraBlame original commit: 9839ef573c437b6f4f2550b8514fd36964f525fe

* Add validation tests for Timestamp Query - Test writeTimestamp in all possible encoders. - Test resolveQuerySet for all types of queries. - Test destoryed query set in writeTimestamp and resolveQuerySet. * Refactor to use cases, subcases, createEncoder, createQuerySetWithState, createBufferWithState * Fix typo * Add control case in resolveQuerySet,invalid_queryset_and_destination_buffer * Add comments for control cases * Add missed condition in expectValidationError

Automatic update from web-platform-tests Update naming for vertex state Specification change: gpuweb/gpuweb#469 Dawn roll: https://dawn.googlesource.com/dawn/+log/c3284fa40ec6b12731ed66c2f2a9256ae3fa692e..ae1f25fee85ebf2773b24a0a4f39c160839b1dbb CTS roll: gpuweb/cts@3dc37c8...e114192 TBR: haraken@chromium.org Bug: dawn:22 Change-Id: Ib427760b9fba4b2cc4290de046c4a31d77e0b67a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1900807 Reviewed-by: Austin Eng <enga@chromium.org> Reviewed-by: Yunchao He <yunchao.he@intel.com> Reviewed-by: Corentin Wallez <cwallez@chromium.org> Commit-Queue: Kai Ninomiya <kainino@chromium.org> Cr-Commit-Position: refs/heads/master@{#713682} -- wpt-commits: 4831b37a33f002e67ecc5f9f5b41145721e0ab60 wpt-pr: 20186

kvark approved these changes Oct 10, 2019

View reviewed changes

renames

35d81c8

kvark approved these changes Oct 11, 2019

View reviewed changes

Kangz approved these changes Oct 15, 2019

View reviewed changes

Comment thread spec/index.bs Outdated

kainino0x added 2 commits October 30, 2019 16:03

Merge branch 'master' into vertex-inputs-again

025cb6c

StructureMember -> Attribute

5de73d3

kdashg approved these changes Oct 30, 2019

View reviewed changes

VertexInput -> VertexState, VertexBufferDescriptor -> VertexBufferLay…

4020d46

…outDescriptor

kainino0x requested a review from kvark October 30, 2019 23:29

kainino0x requested a review from Kangz October 30, 2019 23:31

magcius approved these changes Oct 30, 2019

View reviewed changes

kainino0x mentioned this pull request Oct 30, 2019

[meta] Vertex Input #484

Closed

13 tasks done

kvark approved these changes Oct 31, 2019

View reviewed changes

JusSn approved these changes Nov 4, 2019

View reviewed changes

Merge branch 'master' into vertex-inputs-again

6b24523

kainino0x force-pushed the vertex-inputs-again branch from ee3f11a to 6b24523 Compare November 4, 2019 23:10

kainino0x merged commit aa266bd into gpuweb:master Nov 4, 2019

kainino0x deleted the vertex-inputs-again branch November 4, 2019 23:13

kainino0x added a commit to kainino0x/cts that referenced this pull request Nov 6, 2019

Update for vertex input renames

4b1647a

Spec changes: gpuweb/gpuweb#469

kainino0x mentioned this pull request Nov 6, 2019

Update for vertex input renames gpuweb/cts#89

Merged

kainino0x added a commit to gpuweb/cts that referenced this pull request Nov 7, 2019

Update for vertex input renames

e114192

Spec changes: gpuweb/gpuweb#469

chromium-wpt-export-bot mentioned this pull request Nov 8, 2019

Update naming for vertex state web-platform-tests/wpt#20186

Merged

JiangYizhou mentioned this pull request Nov 11, 2019

Rebase Dawn webatintel/aquarium#44

Merged

Conversation

kainino0x commented Oct 10, 2019 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kvark Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

magcius Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

magcius Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

D3D12

Vulkan

Jasper's Personal Taste

Uh oh!

kainino0x Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

kvark Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kainino0x Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kainino0x Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

Kangz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kangz commented Oct 15, 2019

Uh oh!

kdashg Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

kainino0x Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

kdashg Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Oct 30, 2019

Uh oh!

magcius commented Oct 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kainino0x commented Oct 10, 2019 •

edited by pr-preview Bot

Loading

magcius Oct 10, 2019 •

edited

Loading

magcius Oct 10, 2019 •

edited

Loading

kvark Oct 10, 2019 •

edited

Loading

kainino0x Oct 11, 2019 •

edited

Loading