googlecodelabs / feedback

Provide feedback to our codelabs by filing an issue here
18 stars 22 forks source link

[your-first-webgpu-app] Mistake in Bind Group Layout #1365

Closed stevenr4 closed 8 months ago

stevenr4 commented 8 months ago

In Step 1 for the section "Use Bind Group and Pipeline Layouts", this code example exists:

// Create the bind group layout and pipeline layout.
const bindGroupLayout = device.createBindGroupLayout({
  label: "Cell Bind Group Layout",
  entries: [{
    binding: 0,
    visibility: GPUShaderStage.VERTEX | GPUShaderStage.COMPUTE,
    buffer: {} // Grid uniform buffer
  }, {
    binding: 1,
    visibility: GPUShaderStage.VERTEX | GPUShaderStage.COMPUTE,
    buffer: { type: "read-only-storage"} // Cell state input buffer
  }, {
    binding: 2,
    visibility: GPUShaderStage.COMPUTE,
    buffer: { type: "storage"} // Cell state output buffer
  }]
});

I think this bug is in binding 0 on this line: visibility: GPUShaderStage.VERTEX | GPUShaderStage.COMPUTE,. When following the tutorial, the FRAGMENT stage also reads the grid value in order to determine the color.

This is my first introduction to GPU programming and shaders, and it took me a while to figure out what was wrong here. I'm not 100% sure if I messed something up elsewhere or if there is a better fix for this, but I was able to fix this issue by adding the fragment flag to this line, changing this line to visibility: GPUShaderStage.VERTEX | GPUShaderStage.COMPUTE | GPUShaderStage.FRAGMENT,

beaufortfrancois commented 8 months ago

There's a note for this in https://codelabs.developers.google.com/your-first-webgpu-app?hl=en#7:

image
stevenr4 commented 8 months ago

There's a note for this in https://codelabs.developers.google.com/your-first-webgpu-app?hl=en#7: image

Thank you for clearing this up! Yes, I completely missed this.

If there is reason to have the code not match the examples or the previous steps, I would love to know why that is. Like I mentioned previously, I'm new to shaders, and missing the note you mentioned resulted in an hour of confusion. I am worried that other people similar to me might run into the same issue.

Is there a reason for the fragment visibility to be mentioned in a note instead of included in the code?

If it is necessary to leave the visibility as it is, then could I suggest a compromise like this?

     // Add GPUShaderStage.FRAGMENT here if you are using the `grid` uniform in the fragment shader
    visibility: GPUShaderStage.VERTEX | GPUShaderStage.COMPUTE,
beaufortfrancois commented 8 months ago

The "Extra credit: make it more colorful!" section is optional. See https://codelabs.developers.google.com/your-first-webgpu-app?hl=en#5

We didn't want to force developers to add colors.

If it is necessary to leave the visibility as it is, then could I suggest a compromise like this?

👍 I'll update the codelab with your suggestion.

beaufortfrancois commented 8 months ago

I've updated the codelab. See https://codelabs.developers.google.com/your-first-webgpu-app?hl=en#7