Update samples to work with TypeScript strict mode (part 2) #285
Conversation
nash1111
changed the title
nash1111
changed the title
There was a problem hiding this comment.
Overall this is adding too much lines-of-code overhead to make TypeScript happy, which isn't good for readability. Some overhead is needed, but I think we can reduce it.
I've commented on a few things that should significantly improve the readability. Most of my suggestions apply to many places later in the PR - please apply them in all similar situations. (Or update one or two and ping me, if you'd like an incremental review of particular changes before you go through and do all of them.)
| @@ -198,7 +199,7 @@ const init: SampleInit = async ({ canvas, pageState }) => { | |||
| const renderPassDescriptor: GPURenderPassDescriptor = { | |||
| colorAttachments: [ | |||
| { | |||
| view: undefined, // Assigned later | |||
| view: undefined as any, // Assigned later | |||
There was a problem hiding this comment.
(In response to the GitHub Actions check warning)
It would be nice to make the build warning-free. In this case it can probably be view: undefined!,
There was a problem hiding this comment.
@kainino0x
I wanted to use view: undefined! too, but it was inferred as the never type and didn't work out. How about using view: undefined as unknown as GPUTextureView ? Using as twice isn't intuitive, but since we can't directly cast undefined to GPUTextureView, it seems unavoidable. If there's a better suggestion, I'd love to hear it.
src/sample/cornell/main.ts
Outdated
| gui.add(params, 'renderer', ['rasterizer', 'raytracer']); | ||
| gui.add(params, 'rotateCamera', true); | ||
| gui?.add(params, 'renderer', ['rasterizer', 'raytracer']); | ||
| gui?.add(params, 'rotateCamera', true); |
There was a problem hiding this comment.
Since this can't actually happen I think I'd prefer gui! over gui?
If it's wrong it'll just throw a TypeError which is fine for a sample.
Or just assert(gui).
There was a problem hiding this comment.
Thank you for the review. I used assert because non-null assertions are discouraged in some lint settings
| @@ -245,6 +246,7 @@ const init: SampleInit = async ({ canvas, pageState }) => { | |||
| function frame() { | |||
| // Sample is no longer the active page. | |||
| if (!pageState.active) return; | |||
| assert(device, 'device is null'); | |||
There was a problem hiding this comment.
Similarly shouldn't be necessary, though I'm less certain about this since it's inside a function. OK to keep if needed.
|
|
||
| assert(attachment, 'attachment is null'); | ||
|
|
||
| attachment.view = context.getCurrentTexture().createView(); |
There was a problem hiding this comment.
This is a complicated way to get colorAttachments[0]. How about storing colorAttachment0 into its own variable?
const colorAttachment0: GPURenderPassColorAttachment = {
view: undefined as any, // Assigned later
};
const renderPassDescriptor: GPURenderPassDescriptor = {
colorAttachments: [colorAttachment0],colorAttachment0.view = context.getCurrentTexture().createView();
|
Thank you for the review & I apologize for the late response. I will incorporate your feedback. |
|
Thanks, no rush, thank you for the contribution! |
…1/webgpu-samples into chore/strict_true_part_2
@kainino0x
I set strict:true and made sure the
npm run buildwent throughThis PR makes all the remaining examples work in TypeScript strict mode. Could you please take a look when you have a moment? Thank you in advance.
Ref: #278