Moved duplicated utils to separate file. #354
Conversation
Test Merge
Merge main into my fork
Merge Main
Merge main into my fork.
Merge branches
There was a problem hiding this comment.
I've left a bunch of thoughts but apologies, I don't think I can spend a lot more time on reviewing this. If we're going to have shared sample utils I'd like to hold them to a high standard of general usefulness, but these utils are rather ad hoc for these particular samples. Any utils will need a design that enhances readability/teachability overall, which means it needs to overcome the (IMO) extra readability overhead that comes with trying to abstract out certain helpers. I don't think these helpers are likely to do that as they mostly hide WebGPU complexity (which is valuable to teach) behind util complexity (which is much less valuable to teach).
| import type { GUI } from 'dat.gui'; | ||
| import fullscreenTexturedQuad from '../shaders/fullscreenTexturedQuad.wgsl'; | ||
|
|
||
| // A union type representing all posibble GPUBindingLayout types |
There was a problem hiding this comment.
| // A union type representing all posibble GPUBindingLayout types | |
| /** A union type representing all possible GPUBindingLayout types */ |
| bindGroupLayout: GPUBindGroupLayout; | ||
| }; | ||
|
|
||
| // A union type representing all possible binding members assignable to a GPUBindGroupLayoutEntry |
There was a problem hiding this comment.
| // A union type representing all possible binding members assignable to a GPUBindGroupLayoutEntry | |
| /** A union type representing all possible binding members assignable to a GPUBindGroupLayoutEntry */ |
|
|
||
| export type SampleInitParams = { | ||
| canvas: HTMLCanvasElement; | ||
| pageState: { active: boolean }; |
There was a problem hiding this comment.
all mentions of pageState are obsolete
| */ | ||
| export const createBindGroupCluster = ( | ||
| args: BindGroupClusterFunctionArgs | ||
| ): BindGroupCluster => { |
There was a problem hiding this comment.
This is a handy helper, but I'm a bit hesitant to hide away details of WebGPU API usage behind helpers because it adds some load to reading the samples - even if you know WebGPU, and you want to see how this technique works, you end up having to learn what the utility functions do (and this utility function is not very easy to read). Generally the style practices I'd apply to samples are a little different than normal: more focus on teaching, less on conciseness and refactorability.
I couldn't find any instances of this helper even being used with more than one bind group per cluster, though it seems on the latest revision some of the samples are broken / have broken imports*, so I might be wrong about that. But at least in that case the amount of code needed to use the API directly is barely more than using the helper and it diminishes the value of the automatic labeling and numbering.
* the build really shouldn't be passing, I'm getting lots of errors when I run npm run build on this PR, but it exits with return code 0. cc @greggman
| return init; | ||
| }; | ||
|
|
||
| // A class that provides a set of utilities for creating a basic fullscreen shader that renders |
There was a problem hiding this comment.
doc comments for all these comments too
| export abstract class BaseFullscreenShaderClass { | ||
| // The intended implementation of switchBindGroup is to facilitate switching between bind groups that conform | ||
| // to the same bind group layout used within the fullscreen shader | ||
| abstract switchBindGroup(name: string): void; |
|
|
||
| // A class that provides a set of utilities for creating a basic fullscreen shader that renders | ||
| // a programatically generated 2D image to the screen | ||
| export abstract class BaseFullscreenShaderClass { |
There was a problem hiding this comment.
broadly speaking I think a fullscreen quad helper could make more sense for samples, depending on the implementation; in principle it's pretty straightforward to a reader what it does just from the name, and it also encapsulates more stuff.
But as written its interface seems quite complicated, especially using inheritance where I don't think it really helps much. Nothing here even uses this so it doesn't seem like it's using the fact that it's a class. I have left a few additional thoughts below but let me know what you think.
| executeRun( | ||
| commandEncoder: GPUCommandEncoder, | ||
| renderPassDescriptor: GPURenderPassDescriptor, | ||
| pipeline: GPURenderPipeline, | ||
| bindGroups: GPUBindGroup[] | ||
| ) { | ||
| const passEncoder = commandEncoder.beginRenderPass(renderPassDescriptor); | ||
| passEncoder.setPipeline(pipeline); | ||
| for (let i = 0; i < bindGroups.length; i++) { | ||
| passEncoder.setBindGroup(i, bindGroups[i]); | ||
| } | ||
| passEncoder.draw(6, 1, 0, 0); | ||
| passEncoder.end(); | ||
| } |
There was a problem hiding this comment.
IMO could be a standalone or static function or more likely just don't need a helper for this.
| setUniformArguments<T, K extends readonly string[]>( | ||
| device: GPUDevice, | ||
| uniformBuffer: GPUBuffer, | ||
| instance: T, | ||
| keys: K | ||
| ) { | ||
| for (let i = 0; i < keys.length; i++) { | ||
| device.queue.writeBuffer( | ||
| uniformBuffer, | ||
| i * 4, | ||
| new Float32Array([instance[keys[i]]]) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Doesn't seem at all specific to rendering a quad. Unclear how the instance/keys thing helps, I can't find any examples of it being used.
Also this does a lot of writeBuffer calls where just one would be much more ideal.
Moved createBindGroupCluster and other util functions to common utils file to prevent code duplication. Re-documented code to make the functions more understandable/extensible to other use cases.