-
-
Notifications
You must be signed in to change notification settings - Fork 36k
ExtrudeGeometry: Fix regression introduced by #30750. #30822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ExtrudeGeometry: Fix regression introduced by #30750. #30822
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
Maybe it would be worth having a test screenshot for bevelEnabled == false? I'm not familiar with the process for setting those up. |
These new changes appear to be causing additional regressions. Also check the console errors. ![]() https://threejs.org/examples/webgl_geometry_text.html // tested after merging this PR I would be inclined to revert #30750 at this point and allow time for proper testing. |
@WestLangley not seeing console errors, but I see the geometry bug (oddly, it's different on my end). I'll investigate. |
The variable "vertices" could probably use renaming for clarity.
Fixed. The change was using the wrong variable to populate contractedContourVertices, which would cause an error for shapes with holes when bevelEnabled == false. My bad, I should have tested the PR more thoroughly. |
The console errors occurred when the font was changed. Unrelated to this PR, but FYI, file |
Ah, yeah I did see those later. They're gone now that the variable is fixed.
@WestLangley It appears that the outside boundary is being classified as a hole, while the inside boundary is being classified as a contour. This is happening before it's passed to ExtrudeGeometry, so it's probably a defect with the SVG file itself, the SVGLoader, or path.toShapes(). The issue seems to predate #30750 though. |
@gkjohnson I have done the best I can to identify the failure points. Someone other than myself will have to approve the code changes. 🙏 |
Sorry I missed that there were already other reviews on this PR somehow. I'm not an expert in this file but as this PR is addressing it looks like the tringulated arrays were not being populated if there were no bevel points. Using the "contour" and "holes" arrays should revert the behavior back to what it was before #30750 when bevels were disabled. This change also seems to work. let faces;
if ( contractedContourVertices.length === 0 ) {
faces = ShapeUtils.triangulateShape( contour, holes );
} else {
faces = ShapeUtils.triangulateShape( contractedContourVertices, expandedHoleVertices );
} |
@gkjohnson Good call. I've restructured this fix to check bevelSegments early, and avoid calculating contractedContourVertices and expandedHoleVertices when not necessary. The diffs show up poorly due to indentation changes, but the structure is now: let faces;
if ( bevelSegments == 0 ) {
faces = ShapeUtils.triangulateShape( contour, holes );
} else {
// Calculate contractedContourVertices and expandedHoleVertices
// ...
faces = ShapeUtils.triangulateShape( contractedContourVertices, expandedHoleVertices );
} |
Now branches based on bevelSegments ?= 0, bypassing contractedContourVertices/expandedHoleVertices generation when there is no bevel.
e6484d2
to
9091141
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good and ready to merge.
Fixed #30819
Description
Changes introduced by #30750 resulted in the inputs to ShapeUtils.triangulateGeometry not being initialized when bevelSegments == 0 (which happens when bevelEnabled == false), causing triangulation to fail. This PR adds code to manually populate those lists if they are empty before triangulation, fixing the issue. See related issue for examples.