Add isManifoldText metadata to track manifold-created 3D text#525
Open
tracygardner wants to merge 2 commits intomainfrom
Open
Add isManifoldText metadata to track manifold-created 3D text#525tracygardner wants to merge 2 commits intomainfrom
tracygardner wants to merge 2 commits intomainfrom
Conversation
…r failures Manifold-created text meshes call flipFaces() to render correctly in Babylon.js's left-handed coordinate system. When used as a CSG subtract tool in subtractMeshesMerge, this flip was not undone, so Manifold saw an inverted tool solid. The resulting mesh had incorrect winding on the text cavity walls — fine visually (backFaceCulling=false) but written as-is to STL, causing strict slicers to reject the file. Fix: tag manifold text with metadata.isManifoldText=true and flip its faces in subtractMeshesMerge alongside the existing modelName flip for GLB imports. https://claude.ai/code/session_01FJgv9x8zCFiEuyMEyFJokp
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR ensures CSG subtraction outputs have normals by calling Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/csg.js (1)
812-824:⚠️ Potential issue | 🟡 Minor
isManifoldTextflip is skipped in the direct-geometry fallback path.At Line [820], direct tool clones bypass the new metadata-based face flip. If a tool has geometry but no material, subtraction can still run with inverted winding.
💡 Proposed fix
} else if (meshHasGeometry) { // Direct mesh without children (e.g., manifold text mesh) const clone = cloneForCSG(mesh, `direct_tool_${meshIndex}`); + if ( + (mesh.metadata?.modelName || mesh.metadata?.isManifoldText) && + typeof clone.flipFaces === "function" + ) { + clone.flipFaces(); + } subtractDuplicates.push(clone); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 812 - 824, Direct-geometry clones created in the fallback path (when meshHasGeometry is true) skip the metadata-based face flip logic; update the direct path that creates the clone via cloneForCSG (variable name clone) so that if clone.metadata?.modelName || clone.metadata?.isManifoldText is truthy and typeof clone.flipFaces === "function" you call clone.flipFaces() before pushing it into subtractDuplicates, matching the flip behavior used for unified children.
🧹 Nitpick comments (1)
tests/shapes.test.js (1)
225-260: Use returned IDs instead of hardcoded names to reduce test flakiness.
createBox/subtractMeshescan return reserved IDs. Using literals for readiness and cleanup can target the wrong mesh if names are remapped.♻️ Suggested test hardening
- flock.createBox("subtractBase", { + const baseId = flock.createBox("subtractBase", { color: "#ff0000", width: 4, height: 2, depth: 1, position: [0, 0, 0], }); - createdIds.push("subtractBase"); + createdIds.push(baseId); ... - flock.whenModelReady("subtractBase", resolve); + flock.whenModelReady(baseId, resolve); ... const resultId = await flock.subtractMeshes( "textSubtractResult", - "subtractBase", + baseId, [textId], ); - createdIds.push("textSubtractResult"); + createdIds.push(resultId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/shapes.test.js` around lines 225 - 260, The test hardcodes mesh names which can be remapped; instead capture and use returned IDs from createBox/create3DText/subtractMeshes: store the createBox return in a variable (e.g., boxId) and push that to createdIds; use that boxId and the previously stored textId when calling whenModelReady and subtractMeshes; store the subtractMeshes return in resultId and push that to createdIds instead of using the literal "subtractBase" or "textSubtractResult", ensuring all readiness checks and cleanup reference those returned variables (createBox, create3DText, whenModelReady, subtractMeshes, createdIds, textId, resultId).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/csg.js`:
- Around line 812-824: Direct-geometry clones created in the fallback path (when
meshHasGeometry is true) skip the metadata-based face flip logic; update the
direct path that creates the clone via cloneForCSG (variable name clone) so that
if clone.metadata?.modelName || clone.metadata?.isManifoldText is truthy and
typeof clone.flipFaces === "function" you call clone.flipFaces() before pushing
it into subtractDuplicates, matching the flip behavior used for unified
children.
---
Nitpick comments:
In `@tests/shapes.test.js`:
- Around line 225-260: The test hardcodes mesh names which can be remapped;
instead capture and use returned IDs from createBox/create3DText/subtractMeshes:
store the createBox return in a variable (e.g., boxId) and push that to
createdIds; use that boxId and the previously stored textId when calling
whenModelReady and subtractMeshes; store the subtractMeshes return in resultId
and push that to createdIds instead of using the literal "subtractBase" or
"textSubtractResult", ensuring all readiness checks and cleanup reference those
returned variables (createBox, create3DText, whenModelReady, subtractMeshes,
createdIds, textId, resultId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f6ccc65-8b6c-453a-93dc-ba4027b4905e
📒 Files selected for processing (3)
api/csg.jsapi/shapes.jstests/shapes.test.js
The previous commit incorrectly flipped the text tool mesh faces before the boolean subtract, which caused Manifold CSG to treat the text as an inverted solid (intersection instead of subtraction). Reverted: isManifoldText condition removed from the flipFaces call in subtractMeshesMerge. The isManifoldText metadata flag (shapes.js) is kept for potential future use. Added: resultMesh.createNormals(true) after outerCSG.toMesh() in both subtractMeshesMerge and subtractMeshesIndividual, matching the pattern already used in mergeMeshes. This ensures vertex normals are freshly computed from the actual face geometry rather than relying on whatever the Manifold → Babylon.js conversion produces. https://claude.ai/code/session_01FJgv9x8zCFiEuyMEyFJokp
Deploying flockxr with
|
| Latest commit: |
ca4c136
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cf0a917b.flockxr.pages.dev |
| Branch Preview URL: | https://claude-fix-3d-text-stl-expor.flockxr.pages.dev |
Deploying flockdev with
|
| Latest commit: |
ca4c136
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://052ecbf4.flockdev.pages.dev |
| Branch Preview URL: | https://claude-fix-3d-text-stl-expor.flockdev.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds metadata tracking for 3D text objects created using the Manifold approach, enabling proper handling of these objects in mesh operations like subtraction.
Key Changes
isManifoldTextmetadata flag on meshes created via the Manifold approach increate3DText()isManifoldTextmetadata flag in addition tomodelNameisManifoldTextmetadata is correctly set on manifold-created textImplementation Details
The
isManifoldTextflag is set on the mesh metadata after successful Manifold geometry creation. This flag is then used in the CSG subtraction pipeline to determine whether face flipping should be applied, ensuring proper mesh orientation and validity when performing boolean operations with manifold-created text objects.https://claude.ai/code/session_01FJgv9x8zCFiEuyMEyFJokp
Summary by CodeRabbit
Bug Fixes
Tests