Add reference Web GPU Spinning Cube Demo and glMatrix by JusSn · Pull Request #209 · gpuweb/gpuweb

JusSn · 2019-02-11T01:26:08Z

This demo serves as a reference for a Web GPU implementation. It reflects the current API as well as approved pull requests.

Also included is the glMatrix matrix math library by Brandon Jones.

litherum · 2019-02-13T20:19:38Z

+
+    const verticesBufferDescriptor = {
+        size: vertexArray.byteLength,
+        usage: GPUBufferUsage.VERTEX | GPUBufferUsage.TRANSFER_DST


It's kind of unfortunate that setSubData requires TRANSFER_DST and nothing else. On UMA, setSubData might be able to write directly into the buffer (if the stars align correctly).

litherum · 2019-02-13T20:24:38Z

+        depthCompare: "less"
+    };
+
+    /* Render Pipeline */


I'd move this comment to above line 150.

litherum · 2019-02-13T20:26:07Z

+        clearColor: { r: 0.5, g: 1.0, b: 1.0, a: 1.0 }
+    };
+
+    // GPUExtent3D


I wish we could do something more type-safe here. It seems silly to attach a 3D size to a 2D texture.

Yeah, maybe we need GPUSize2D and GPUSize3D (I prefer both of them over "Extent")

litherum · 2019-02-13T20:26:39Z

+        mipLevelCount: 1,
+        sampleCount: 1,
+        dimension: "2d",
+        format: "d32-float-s8-uint",


At a glance, this looks like a UUID. 🤮

We should at least make them consistent with the other texture formats.

I think I missed this one when updating the names.

Oh, you forgot to include depth and stencil formats when you updated the list of texture formats in the IDL. I'll make a PR in a bit to add them.

litherum · 2019-02-13T20:28:00Z

+        clearDepth: 1.0
+    };
+
+    renderPassDescriptor = {


Instead of writing into globals, it's probably better style to return a collection of all the variables.

That sounds very functional indeed.

litherum · 2019-02-13T20:28:44Z

+    };
+}
+
+class MappedBufferBindGroup {


I think if we're going to add classes as helpers, we could do the same for the transform array matrix. e.g. a class that holds the mappedGroup and exposes the arrayBuffer.

litherum · 2019-02-13T20:31:53Z

+    queue.submit([commandEncoder.finish()]);
+
+    // Ready the current buffer for update after GPU is done with it.
+    mappedGroup.buffer.mapWriteAsync().then((arrayBuffer) => {


The right thing will happen here, but we might not wants apps to rely on the specific timeline that the mapWriteAsync promise resolves. It may be better to explicitly handle the dependency of "the mapping should happen after the command buffer finishes" by using fences. I'm not sure which behavior we want to promote.

If this does the right thing, it's probably more readable than barriers.

The thing that I think is a bit weird is that mappedGroups could grow infinitely (if some other part of the code appends to it outside this loop). It's a shame we can't attach the group to the queue or something.

I have this example working with my mapWriteAsync() prototype. I could use a class to control access to the mappedGroups queue, if that's what you mean?

Oh, I understand what you're saying now. Shouldn't mapWriteAsync only resolve after the GPU is done with the buffer?

Yes, but it means that we don't have any pipelining here based on how I understand it. We cannot upload the data for the second frame until the GPU is done with the data for the first. Our example should support double-buffering.

litherum · 2019-02-13T20:35:23Z

+    mappedGroup.buffer.unmap();
+
+    const commandEncoder = device.createCommandEncoder();
+    renderPassDescriptor.colorAttachments[0].attachment = swapChain.getTexture().createDefaultTextureView();


You've got a single render pass descriptor that every render pass uses. This implicitly requires that beginRenderPass copy the contents of the descriptor so that the next frame doesn't clobber the current frame's execution. This might be okay, but it might not be. If it isn't okay, the group should decide on a mechanism to make it obvious to authors what is going wrong when they write the same code you have here.

litherum · 2019-02-13T20:36:34Z

+
+const vertexSize = 4 * 8;
+const colorOffset = 4 * 4;
+const vertexArray = new Float32Array([


I have no idea whether this giant array is correct. Can't you compute it at runtime?

This is fairly standard in all spinning cube demos. I don't think it is a big deal. Computing it at runtime would probably be an equally confusing set of for loops.

Maybe the best thing to do would be to name the variables cubeVertices, and provide comments for the 4 * 8 and 4 * 4 values.

grorg · 2019-02-13T20:41:08Z

+<script src="gl-matrix-min.js"></script>
+</head>
+<body>
+<canvas height=1000 width=1000></canvas>


You set a 600px viewport above, so 1000x1000 would be mostly offscreen.

grorg · 2019-02-13T20:41:59Z

+
+vertex FragmentData vertex_main(float4 position : attribute(0),
+                                float4 color : attribute(1),
+                                constant float4x4* modelViewProjectionMatrix : register(b0)) {


Why is the input a pointer to a float4x4?

Do you mean as opposed to a reference or copy?

As opposed to the more normal HLSL mechanism of defining a cbuffer/ConstantBuffer with your input variables.

grorg · 2019-02-13T20:49:09Z

+
+    const depthTexture = device.createTexture(depthTextureDescriptor);
+
+    // GPURenderPassDepthStencilAttachmentDescriptor


I don't understand this comment.

grorg · 2019-02-13T20:53:24Z

+        this.bindGroup = bindGroup;
+    }
+}
+const mappedGroups = [];


This should probably go at the top.

grorg · 2019-02-13T20:56:54Z

+        const { buffer, arrayBuffer } = device.createMappedBuffer(transformBufferDescriptor);
+        const bindGroup = device.createBindGroup(createTransformBindGroupDescriptor(buffer));
+
+        mappedGroup = new MappedBufferBindGroup(buffer, arrayBuffer, bindGroup);


Since a MappedBufferBindGroup doesn't have anything other than a constructor, it's basically equivalent to an anonymous object. i.e. you could omit the class and just say:

mappedGroup = { buffer, arrayBuffer, bindGroup };

grorg · 2019-02-13T21:01:18Z

+init();
+</script>
+</body>
+</html>


Add a newline.

JusSn · 2019-02-14T20:49:25Z

+    } else
+        mappedGroup = mappedGroups.shift();
+
+    updateTransformArray(mappedGroup.arrayBuffer);


Leaving myself a note here to fix: updateTransformArray takes a Float32Array, not an ArrayBuffer.

kdashg

Can we link to an external source for the library?
https://cdnjs.com/libraries/gl-matrix

JusSn · 2019-07-01T19:51:19Z

I've updated the file to match our API changes and removed the library file in favor of an external source. It does not work in current Safari since WHLSL is not the default ingestion format yet.

Feel free to make a PR after merging to feature-detect and use SPIR-V shaders, or other API updates I may have missed @kainino0x @austinEng

* Add reference demo files * Add shading language notice * Addressed basic review issues * Small fixes * Add some clarifying comments * Update to IDL commit e137ea9 * Attempt to get WHLSL shaders working * whoops * Update reference for submission * Delete gl-matrix-min.js

Using npm slightly reduces unnecessary dependencies of the project.

JusSn mentioned this pull request Feb 11, 2019

Add WIP WebGPU spinning cube demo and glMatrix #187

Closed

kainino0x reviewed Feb 11, 2019

View reviewed changes

Comment thread samples/hello-cube.html Resolved

litherum approved these changes Feb 13, 2019

View reviewed changes

litherum reviewed Feb 13, 2019

View reviewed changes

grorg reviewed Feb 13, 2019

View reviewed changes

Comment thread samples/hello-cube.html Resolved

grorg reviewed Feb 13, 2019

View reviewed changes

JusSn commented Feb 14, 2019

View reviewed changes

Justin Fan added 6 commits February 25, 2019 11:53

Add reference demo files

9f828d6

Add shading language notice

5bc139f

Addressed basic review issues

6501f8d

Small fixes

b5238a0

Add some clarifying comments

949df91

Update to IDL commit e137ea9

de6bbf4

JusSn force-pushed the CubeDemoRef branch from 5684c4f to de6bbf4 Compare February 25, 2019 20:07

Justin Fan added 2 commits March 22, 2019 16:16

Attempt to get WHLSL shaders working

0e41a76

whoops

461166c

kdashg approved these changes Jul 1, 2019

View reviewed changes

Justin Fan added 2 commits July 1, 2019 12:46

Update reference for submission

6c7aee2

Delete gl-matrix-min.js

fdb5ebd

JusSn merged commit 627d096 into gpuweb:master Jul 1, 2019

ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022

Switch to npm instead of yarn, and update deps (gpuweb#209)

fd88cc6

Using npm slightly reduces unnecessary dependencies of the project.


		const depthTexture = device.createTexture(depthTextureDescriptor);

		// GPURenderPassDepthStencilAttachmentDescriptor

Conversation

JusSn commented Feb 11, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdashg left a comment

Choose a reason for hiding this comment

Uh oh!

JusSn commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JusSn commented Jul 1, 2019 •

edited

Loading