webgpu / webgpufundamentals

https://webgpufundamentals.org
BSD 3-Clause "New" or "Revised" License
598 stars 83 forks source link

Vertex buffer with index buffer winding issue #115

Open Applequist opened 2 months ago

Applequist commented 2 months ago

Hi, I am working on the tutorial using rust and wgpu. While working on the vertex buffer with index buffer example, I ran into a little issue: nothing showed up. It turns out the winding of the triangle as generated by the create_circle_vertices is clockwise (CW) whereas the render pipeline primitive front face winding rule is using counter-clockwise winding (CCW).

A simple fix was to change the primitive front face winding rule from CCW to CW. But this is not mentioned in the tutorial.

Not sure if it is only an issue with rust and wgpu though (shouldn't be, right?).

Thanks for the awesome tutorial!

greggman commented 2 months ago

Did you turn on culling? The default is culling is off (unless there's a bug in WGPU)

These tutorials don't use culling (which defaults to off) until they get to 3d (where it's actually needed).

Applequist commented 2 months ago

yes, backface culling was on:

            primitive: wgpu::PrimitiveState {
                topology: wgpu::PrimitiveTopology::TriangleList,
                strip_index_format: None,
                front_face: wgpu::FrontFace::Ccw,
                cull_mode: Some(wgpu::Face::Back),
                polygon_mode: wgpu::PolygonMode::Fill,
                unclipped_depth: false,
                conservative: false,
            },

It's really a winding issue.

Before using the index buffer, the circle vertices are generated like this (per subdivision). That is counter-clockwise.

     // first triangle
     //          1
     //        /    \  
     //     /          \
     //  2  -----------0 
    addVertex(c1 * radius, s1 * radius, ...outerColor);   // 0
    addVertex(c2 * radius, s2 * radius, ...outerColor);   // 1
    addVertex(c1 * innerRadius, s1 * innerRadius, ...innerColor); // 2

    // second triangle
    addVertex(c1 * innerRadius, s1 * innerRadius, ...innerColor);   // 3 (=2)
    addVertex(c2 * radius, s2 * radius, ...outerColor);                        // 4 (=1)
    addVertex(c2 * innerRadius, s2 * innerRadius, ...innerColor);   // 5
  }

When using the index buffer, the circle vertices are generated like this:

  // 2 triangles per subdivision
  //
  // 0  2  4  6  8 ...
  //
  // 1  3  5  7  9 ...
  for (let i = 0; i <= numSubdivisions; ++i) {
    const angle = startAngle + (i + 0) * (endAngle - startAngle) / numSubdivisions;

    const c1 = Math.cos(angle);
    const s1 = Math.sin(angle);

    addVertex(c1 * radius, s1 * radius, ...outerColor);
    addVertex(c1 * innerRadius, s1 * innerRadius, ...innerColor);
  }

So outer-ring, inner-ring, outer, inner.... But the comment is actually misleading and with the generated indices (0, 1, 2, 2, 1, 3...) the winding change to clockwise.

greggman commented 2 months ago

I see your point and I'll fix the code. My question was, "why do you have culling on for drawing a 2d circle"?

For example, Skia is one of the most used 2D libraries there is. It doesn't use face culling at all

https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/gpu/ganesh/gl/GrGLGpu.cpp;l=591?q=CULL_FACE&sq=&ss=chromium%2Fchromium%2Fsrc:third_party%2Fskia%2F

greggman commented 2 months ago

Actually, I'm not seeing an issue.

The non-index code is clockwise. The index code is counter clockwise. The comments aren't wrong

non-indexded

  // 2 triangles per subdivision
  //
  // 0--1 4
  // | / /|
  // |/ / |
  // 2 3--5

which is the order of what's actually put in the buffer

  for (let i = 0; i < numSubdivisions; ++i) {
    const angle1 = startAngle + (i + 0) * (endAngle - startAngle) / numSubdivisions;
    const angle2 = startAngle + (i + 1) * (endAngle - startAngle) / numSubdivisions;

    const c1 = Math.cos(angle1);
    const s1 = Math.sin(angle1);
    const c2 = Math.cos(angle2);
    const s2 = Math.sin(angle2);

    // first triangle
+    addVertex(c1 * radius, s1 * radius, ...outerColor);
+    addVertex(c2 * radius, s2 * radius, ...outerColor);
+    addVertex(c1 * innerRadius, s1 * innerRadius, ...innerColor);

+    addVertex(c1 * innerRadius, s1 * innerRadius, ...innerColor);
+    addVertex(c2 * radius, s2 * radius, ...outerColor);
+    addVertex(c2 * innerRadius, s2 * innerRadius, ...innerColor);
  }

Indexded

  // 2 vertices per subdivision
  //
  // 0  2  4  6  8 ...
  //
  // 1  3  5  7  9 ...

which is the indices of the vertices of what's actually put in the buffer

  for (let i = 0; i <= numSubdivisions; ++i) {
    const angle = startAngle + (i + 0) * (endAngle - startAngle) / numSubdivisions;

    const c1 = Math.cos(angle);
    const s1 = Math.sin(angle);

    addVertex(c1 * radius, s1 * radius, ...outerColor);
    addVertex(c1 * innerRadius, s1 * innerRadius, ...innerColor);
  }

followed by

  // 0---2---4---...
  // | //| //|
  // |// |// |//
  // 1---3-- 5---...

These are it indices of the vertices of each triangle. This says nothing about whether the winding is clockwise or counter clockwise (there are no arrows). It only shows that the first triangle is made from vertices 0, 1, and 2 (in some undefined order) and that 2nd triangle is made from vertices 2, 3, and 4 (in some undefined order)

The code itself, which is super simple, shows the order (0, 1, 2, 2, 1, 3 4, 5, 6, 5, 4, 7, ...)

  for (let i = 0; i < numSubdivisions; ++i) {
    const ndxOffset = i * 2;

    // first triangle
    indexData[ndx++] = ndxOffset;
    indexData[ndx++] = ndxOffset + 1;
    indexData[ndx++] = ndxOffset + 2;

    // second triangle
    indexData[ndx++] = ndxOffset + 2;
    indexData[ndx++] = ndxOffset + 1;
    indexData[ndx++] = ndxOffset + 3;
  }

I'm not seeing a misleading comment. That that winding switched is irrelevant. The winding doesn't matter because culling is off. Culling off is the default. The tutorials don't care about culling because turning it on clutters the lesson. Culling off is normal for drawing 2d (see Skia reference). The tutorials don't turn it on until it actually has a point of being turned on.

greggman commented 2 months ago

I changed the comment on the indices to this

  // 1st tri  2nd tri  3rd tri  4th tri
  // 0 1 2    2 1 3    4 5 6    6 5 7
  //
  // 0--2        2     4--6        6  .....
  // | /        /|     | /        /|
  // |/        / |     |/        / |
  // 1        1--3     5        5--7  .....