Automatically clean shape data passed to ExtrudeGeometry

[mlasala45]

Description

Before triangulating a shape, ExtrudeGeometry now welds any index-adjacent points within a small hardcoded distance threshold of each other. This resolves common triangulation artifacts caused by improperly formatted shape data.

Also improved variable names and commenting in the affected files.

Related issue: #20317
Original PR with discussion: #30698

Screenshots

Before change (prominent triangulation errors):

With vertex welding (bevelOffset is negative, same settings, no errors):

Triangulation still breaks if you use physically invalid parameters (bevelOffset negative and larger than the 2D thickness of the contour shape):

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

	Before	After	Diff
WebGL	336.02 78.26	336.02 78.26	+0 B +0 B
WebGPU	525.13 146.26	525.13 146.26	+0 B +0 B
WebGPU Nodes	524.59 146.16	524.59 146.16	+0 B +0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

	Before	After	Diff
WebGL	465.07 112.14	465.07 112.14	+0 B +0 B
WebGPU	597.91 162.53	597.91 162.53	+0 B +0 B
WebGPU Nodes	553.03 152.01	553.03 152.01	+0 B +0 B

[gkjohnson]

Might to get someone who's more familiar with this code to review, as well.

[gkjohnson]

Can we call this something like "mergePoints" so it's more descriptive of what it's doing?

[mlasala45]

Works for me. It's derived from older logic that removed duplicate verts, but only identical ones, and only from the start and end. Hence "clean" rather than "weld" or "merge". But for the current version it makes sense to change the name.

[gkjohnson]

How was this threshold value determined?

[mlasala45]

Arbitrary value based on the relative positions of duplicate verts in my test cases. I think I mentioned in the original PR that it should probably be a user-specified argument rather than hardcoded, which I can add to the PR.

[gkjohnson]

This threshold seems fairly large to me. I'm seeing that 1e-7 works just fine for the example from #20317 (and 1e-8 works when not using squared distances). Ideally we should be able to pick a value that works for the floating point error sensitivity of the operations being done in this function so users don't have to configure this. Really close values, for example, look like they will cause issues when calculating the getBevelVec function resulting in vertices shifting in unexpected directions.

And since floating point error increases as the numbers get larger this threshold could change depending on the scale of values being used.

[mlasala45]

It all depends on whatever originally generated the user's geometry data, though. If a certain program spits out vertices that are further apart than our threshold, they would need to manually weld them, or be able to raise the threshold for that extrusion.

I'm in favor of a sensible/informed default value, that can still be overridden by the user if desired. If we'd rather not expose that setting though, it should be documented somewhere obvious.

[gkjohnson]

I'm not following - the reason we need to merge the vertices is because follow on operations are sensitive to certain amounts of floating point error or singularities. Why would we not be able to select a threshold that ensures the "cleaned" contour only includes points that are at least a distance apart that works well for those operations.

[mlasala45]

I was approaching it from "The duplicate vertices are unintentional, and don't represent the intended layout of the mesh," which could cause unintended behavior in the triangulator. For example a duplicate vert that is offset in the wrong direction could produce self-intersecting geometry, despite not running into precision errors.

[gkjohnson]

When would this condition be needed? When i === points.length (the final iteration) then currentIndex will be 0 so it doesn't seem like this is necessary?

[mlasala45]

You're right. This was a stopgap for a situation that caused an infinite loop, but it appears to function without it now.

[gkjohnson]

This is what this PR looks like on the example from #20317 with the default -5 bevel. I expect this must be because the original shape is contracted so far that the "hole" and outer edge cross. I suppose this is inevitable when expanding or contracting shapes like this. Is there a better expected behavior?

And with -4 bevel:

[mlasala45]

Is there a better expected behavior?

Ideally, we could clamp the bevelOffset to prevent it going into invalid ranges, and document to the user that the function does that. Or, we could throw an error rather than let the triangulation go through.

However, both of those would require some programmatic way of detecting the fault, or the valid offset range. This seems infeasible, given the variety of possible situations and the amount of computation required. Plus, it's probably something the user could easily determine experimentally for their mesh.

[gkjohnson]

Plus, it's probably something the user could easily determine experimentally for their mesh.

Many of the use cases for these tools include cases where the shape is not known in advance and therefore cannot be tested or checked ahead of time. Ie procedurally generated shapes, end-user drawn shapes, dragged and dropped files, etc. And the developer cannot check whether the shape is valid (eg no crossing lines which will break earcut) because they don't have access to the expanded or contracted shape.

I agree this is difficult and doesn't need to be handled here but it's not as letting the user deal with it.

[mlasala45]

Many of the use cases for these tools include cases where the shape is not known in advance and therefore cannot be tested or checked ahead of time.

Good point, that slipped my mind. I think we're on the same page, it's undesirable but not necessary to solve here.