Bug 192611 – [WebGPU] Vertex buffers and WebGPUInputState

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Justin Fan]

[WebGPU] Vertex buffers and WebGPUInputState

[Justin Fan]

<rdar://problem/46608344>

[Justin Fan]

Created attachment 357100 [details]
Patch

[Justin Fan]

Created attachment 357102 [details]
Patch

[Justin Fan]

Created attachment 357104 [details]
Patch

[Dean Jackson]

Comment on attachment 357104 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357104&action=review

> Source/WebCore/ChangeLog:15
> +        Add symbols and files for WebGPUIndexFormat, WebGPUInputStateDescriptor, WebGPUInputStepMode, 
> +        WebGPUVertexAttributeDescriptor, WebGPUVertexFormat, WebGPUVertexInputDescriptor:

I know it's annoying, but I suggest for the future that each of these could be a different patch. While this means you'll have to stage them and get multiple reviews, it's better to have small changes than big ones. Basically, if you're adding more than one class in a patch, there should be a good reason.

Don't do it for this patch now, but in the future.

> Source/WebCore/Modules/webgpu/WebGPUIndexFormat.idl:37
> +] interface WebGPUIndexFormat {
> +    const u32 UINT16 = 0;
> +    const u32 UINT32 = 1;
> +};

I'm going to file a bug on the spec asking why this isn't an enum.

> Source/WebCore/Modules/webgpu/WebGPUInputStepMode.idl:31
> +    DoNotCheckConstants,

Did we decide if we need this?

> Source/WebCore/Modules/webgpu/WebGPUInputStepMode.idl:37
> +] interface WebGPUInputStepMode {
> +    const u32 VERTEX = 0;
> +    const u32 INSTANCE = 1;
> +};

And this one.

> Source/WebCore/platform/graphics/gpu/GPUVertexAttributeDescriptor.h:44
> +class GPUVertexFormat : public RefCounted<GPUVertexFormat> {
> +public:
> +    enum Enum : GPUVertexFormatEnum {
> +        FloatR32G32B32A32 = 0,
> +        FloatR32G32B32 = 1,
> +        FloatR32G32 = 2,
> +        FloatR32 = 3
> +    };
> +};

And if we made these enums in the IDL, we wouldn't need an instance.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:90
> +    // FIXME: Only worry about the first buffer for now, and treat startSlot as the index.
> +    [m_platformRenderPassEncoder setVertexBuffer:buffers[0]->platformBuffer() offset:offsets[0] atIndex:index];

Please check for empty vectors.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:118
> +    // FIXME: What happens here when mtlVertexDescriptor RetainPtr is destroyed?
> +    mtlDescriptor.vertexDescriptor = mtlVertexDescriptor.get();

I assume that the MTLVertexDescriptor retains the MTLRenderPipelineDescriptor.

> LayoutTests/webgpu/vertex-buffer-triangle-strip.html:57
> +    let arrayBuffer = buffer.mapping;
> +    let floatArray = new Float32Array(arrayBuffer);
> +
> +    floatArray[0] = -1;
> +    floatArray[1] = 1;
> +    floatArray[2] = 0;
> +    floatArray[3] = 1;
> +
> +    floatArray[4] = -1;
> +    floatArray[5] = -1;
> +    floatArray[6] = 0;
> +    floatArray[7] = 1;
> +
> +    floatArray[8] = 1;
> +    floatArray[9] = 1;
> +    floatArray[10] = 0;
> +    floatArray[11] = 1;
> +
> +    floatArray[12] = 1;
> +    floatArray[13] = -1;
> +    floatArray[14] = 0;
> +    floatArray[15] = 1;
> +

I don't know how this works without mapping it to the vertex buffer.

[Justin Fan]

Created attachment 357160 [details]
Patch

[Justin Fan]

Created attachment 357164 [details]
Patch

[Justin Fan]

Created attachment 357179 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 357179 [details]
Patch

Clearing flags on attachment: 357179

Committed r239139: <https://trac.webkit.org/changeset/239139>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.