Improve CSG material handling for subtract operations#510
Open
tracygardner wants to merge 2 commits intomainfrom
Open
Improve CSG material handling for subtract operations#510tracygardner wants to merge 2 commits intomainfrom
tracygardner wants to merge 2 commits intomainfrom
Conversation
Two bugs prevented the subtract mesh's material (e.g. box5's flowers texture/teal color) from appearing on the interior faces of a subtracted shape: 1. backFaceCulling was only disabled on the base mesh material; any subtract-mesh submaterial preserved in the MultiMaterial result kept backFaceCulling=true, hiding interior faces. 2. When Babylon CSG2 assigned a "default material" to interior cap faces, applyResultMeshProperties replaced it with the base mesh material (pink walls) instead of the subtract mesh material. Fix: pass subtract mesh materials into applyResultMeshProperties; use them (in order) when replacing default-material slots; set backFaceCulling=false on all preserved submaterials in the MultiMaterial result. https://claude.ai/code/session_01TnumLdbUkFMgiducy37Zne
📝 WalkthroughWalkthroughAdds explicit subtract-material tracking to CSG operations: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/csg.js (1)
765-772: Consider extracting duplicate material collection logic.This block is identical to lines 622-629 in
subtractMeshesMerge. Consider extracting a helper function to reduce duplication.♻️ Proposed refactor to extract helper
Add a helper at module level or within
flockCSG:function collectSubtractMaterials(validMeshes) { return validMeshes.map((m) => { if (m.metadata?.modelName) { const mwm = flock._findFirstDescendantWithMaterial(m); if (mwm) return mwm.material; } return m.material; }).filter(Boolean); }Then use it in both functions:
- const subtractMaterials = validMeshes.map((m) => { - if (m.metadata?.modelName) { - const mwm = flock._findFirstDescendantWithMaterial(m); - if (mwm) return mwm.material; - } - return m.material; - }).filter(Boolean); + const subtractMaterials = collectSubtractMaterials(validMeshes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 765 - 772, The duplicate material-collection logic used to build subtractMaterials should be extracted into a single helper (e.g., collectSubtractMaterials) so both places reuse it; implement a module-level or flockCSG-scoped function named collectSubtractMaterials(validMeshes) that encapsulates the mapping/filtering (using m.metadata?.modelName, flock._findFirstDescendantWithMaterial(m), and falling back to m.material) and then replace the inline blocks in both the current function and subtractMeshesMerge to call collectSubtractMaterials(validMeshes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/csg.js`:
- Around line 1068-1070: In applyResultMeshProperties, avoid a potential NPE by
checking referenceMesh.material before accessing backFaceCulling; update the
code in applyResultMeshProperties to guard accesses to referenceMesh.material
(e.g., if (referenceMesh.material) { referenceMesh.material.backFaceCulling =
false; ... }) and ensure any subsequent code that reads or writes properties on
referenceMesh.material similarly verifies the material exists or creates/assigns
a default material as needed.
---
Nitpick comments:
In `@api/csg.js`:
- Around line 765-772: The duplicate material-collection logic used to build
subtractMaterials should be extracted into a single helper (e.g.,
collectSubtractMaterials) so both places reuse it; implement a module-level or
flockCSG-scoped function named collectSubtractMaterials(validMeshes) that
encapsulates the mapping/filtering (using m.metadata?.modelName,
flock._findFirstDescendantWithMaterial(m), and falling back to m.material) and
then replace the inline blocks in both the current function and
subtractMeshesMerge to call collectSubtractMaterials(validMeshes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+1068
to
1070
| applyResultMeshProperties(resultMesh, referenceMesh, modelId, blockId, subtractMaterials = []) { | ||
| // Copy transformation properties | ||
| referenceMesh.material.backFaceCulling = false; |
There was a problem hiding this comment.
Add null check for referenceMesh.material to prevent NPE.
Line 1070 accesses referenceMesh.material.backFaceCulling unconditionally. If referenceMesh.material is null (e.g., a mesh without an assigned material), this will throw a null pointer exception.
🛡️ Proposed fix
applyResultMeshProperties(resultMesh, referenceMesh, modelId, blockId, subtractMaterials = []) {
// Copy transformation properties
- referenceMesh.material.backFaceCulling = false;
+ if (referenceMesh.material) {
+ referenceMesh.material.backFaceCulling = false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| applyResultMeshProperties(resultMesh, referenceMesh, modelId, blockId, subtractMaterials = []) { | |
| // Copy transformation properties | |
| referenceMesh.material.backFaceCulling = false; | |
| applyResultMeshProperties(resultMesh, referenceMesh, modelId, blockId, subtractMaterials = []) { | |
| // Copy transformation properties | |
| if (referenceMesh.material) { | |
| referenceMesh.material.backFaceCulling = false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/csg.js` around lines 1068 - 1070, In applyResultMeshProperties, avoid a
potential NPE by checking referenceMesh.material before accessing
backFaceCulling; update the code in applyResultMeshProperties to guard accesses
to referenceMesh.material (e.g., if (referenceMesh.material) {
referenceMesh.material.backFaceCulling = false; ... }) and ensure any subsequent
code that reads or writes properties on referenceMesh.material similarly
verifies the material exists or creates/assigns a default material as needed.
Log the material structure of the CSG result mesh to understand what toMesh() actually produces - whether MultiMaterial with real submaterials or default material fallbacks. https://claude.ai/code/session_01TnumLdbUkFMgiducy37Zne
Deploying flockdev with
|
| Latest commit: |
f0c5206
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c6bb3e56.flockdev.pages.dev |
| Branch Preview URL: | https://claude-fix-material-visibili.flockdev.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
api/csg.js (1)
1068-1132:⚠️ Potential issue | 🟠 MajorStill need to guard
referenceMesh.materialthroughout this helper.Line 1070 still dereferences
referenceMesh.materialunconditionally, and the fallback clone paths make the same assumption later in the method. Meshes can have valid geometry without an assigned material, so this can still throw before any fallback is applied. Please guard all of those accesses together or create a default material first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 1068 - 1132, The helper applyResultMeshProperties is still dereferencing referenceMesh.material unsafely (e.g., referenceMesh.material.backFaceCulling and referenceMesh.material.clone() in replaceMaterial and the fallback branch); ensure you guard all uses by first obtaining a safe baseMaterial (e.g., const baseMaterial = referenceMesh.material || new flock.BABYLON.StandardMaterial("csgDefaultMaterial", flock.scene)) then use baseMaterial for .backFaceCulling and .clone() calls (and set backFaceCulling=false on the new default), or add null checks before each referenceMesh.material access inside applyResultMeshProperties and in replaceMaterial so no path assumes material exists.
🧹 Nitpick comments (1)
api/csg.js (1)
1106-1110: Please gate or remove the diagnostic logging before merge.These
console.logcalls will fire for every CSG result and get noisy quickly in normal use. If you still need them, hide them behind an explicit debug flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 1106 - 1110, Remove or gate the noisy diagnostic console.log calls in api/csg.js so they don't run on every CSG result; either delete the three console.log lines or wrap them behind an explicit debug switch (e.g., a DEBUG_CSG boolean or existing logging facility) and ensure the default is false/disabled. Specifically, protect the logs that inspect resultMesh (resultMesh.material, resultMesh.subMeshes) and the MultiMaterial branch that references flock.BABYLON.MultiMaterial so they only execute when the debug flag is enabled or routed through a proper logger at debug level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/csg.js`:
- Around line 623-629: The subtractMaterials array is built from one material
per validMeshes entry which mismatches the actual descendant traversal used by
subtractDuplicates/allToolParts; change the construction of subtractMaterials to
mirror the same traversal (e.g., use the same collectMaterialMeshesDeep() or the
exact descendant iteration used for subtractDuplicates/allToolParts) so that you
collect every material-bearing descendant in the same order and count as
subtractDuplicates/allToolParts do; locate the current subtractMaterials
creation in api/csg.js (the block using validMeshes.map and
flock._findFirstDescendantWithMaterial) and replace it with a traversal that
iterates the same descendants (calling collectMaterialMeshesDeep or reusing the
iterator used for allToolParts/subtractDuplicates) and push each
descendant.material (or descendant's material via
flock._findFirstDescendantWithMaterial when appropriate), then filter(Boolean)
to preserve existing filtering.
- Around line 1112-1128: The single-material branch leaves non-default materials
with their original one-sided culling, causing interior faces to disappear;
after handling resultMesh.material in the else-if/isDefaultMaterial branch and
in the non-default case, ensure preserved single materials also render
double-sided by setting resultMesh.material.backFaceCulling = false when
resultMesh.material is not replaced. Update the logic around resultMesh.material
(alongside existing checks using flock.BABYLON.MultiMaterial, isDefaultMaterial,
and replaceMaterial) so that whenever a preserved (non-replaced) material is
kept, you explicitly set backFaceCulling = false; keep replaceMaterial()
behavior unchanged for default-material replacements.
---
Duplicate comments:
In `@api/csg.js`:
- Around line 1068-1132: The helper applyResultMeshProperties is still
dereferencing referenceMesh.material unsafely (e.g.,
referenceMesh.material.backFaceCulling and referenceMesh.material.clone() in
replaceMaterial and the fallback branch); ensure you guard all uses by first
obtaining a safe baseMaterial (e.g., const baseMaterial = referenceMesh.material
|| new flock.BABYLON.StandardMaterial("csgDefaultMaterial", flock.scene)) then
use baseMaterial for .backFaceCulling and .clone() calls (and set
backFaceCulling=false on the new default), or add null checks before each
referenceMesh.material access inside applyResultMeshProperties and in
replaceMaterial so no path assumes material exists.
---
Nitpick comments:
In `@api/csg.js`:
- Around line 1106-1110: Remove or gate the noisy diagnostic console.log calls
in api/csg.js so they don't run on every CSG result; either delete the three
console.log lines or wrap them behind an explicit debug switch (e.g., a
DEBUG_CSG boolean or existing logging facility) and ensure the default is
false/disabled. Specifically, protect the logs that inspect resultMesh
(resultMesh.material, resultMesh.subMeshes) and the MultiMaterial branch that
references flock.BABYLON.MultiMaterial so they only execute when the debug flag
is enabled or routed through a proper logger at debug level.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+623
to
+629
| const subtractMaterials = validMeshes.map((m) => { | ||
| if (m.metadata?.modelName) { | ||
| const mwm = flock._findFirstDescendantWithMaterial(m); | ||
| if (mwm) return mwm.material; | ||
| } | ||
| return m.material; | ||
| }).filter(Boolean); |
There was a problem hiding this comment.
Collect materials from the same parts you actually subtract.
This records at most one material per validMeshes entry, but both subtract paths operate on every material-bearing descendant returned by collectMaterialMeshesDeep(). A composite cutter with multiple materialized children will therefore exhaust subtractMaterials early, and later default slots will fall back to the base material instead of the remaining tool materials. Build subtractMaterials from the same descendant traversal/order used for subtractDuplicates / allToolParts.
Also applies to: 766-772
🧰 Tools
🪛 GitHub Actions: Prettier
[warning] Prettier reported formatting warning (file is not formatted according to Prettier).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/csg.js` around lines 623 - 629, The subtractMaterials array is built from
one material per validMeshes entry which mismatches the actual descendant
traversal used by subtractDuplicates/allToolParts; change the construction of
subtractMaterials to mirror the same traversal (e.g., use the same
collectMaterialMeshesDeep() or the exact descendant iteration used for
subtractDuplicates/allToolParts) so that you collect every material-bearing
descendant in the same order and count as subtractDuplicates/allToolParts do;
locate the current subtractMaterials creation in api/csg.js (the block using
validMeshes.map and flock._findFirstDescendantWithMaterial) and replace it with
a traversal that iterates the same descendants (calling
collectMaterialMeshesDeep or reusing the iterator used for
allToolParts/subtractDuplicates) and push each descendant.material (or
descendant's material via flock._findFirstDescendantWithMaterial when
appropriate), then filter(Boolean) to preserve existing filtering.
Comment on lines
1112
to
1128
| if (resultMesh.material) { | ||
| if (resultMesh.material instanceof flock.BABYLON.MultiMaterial) { | ||
| resultMesh.material.subMaterials = resultMesh.material.subMaterials.map( | ||
| (subMaterial) => { | ||
| if (subMaterial && isDefaultMaterial(subMaterial)) { | ||
| return replaceMaterial(); | ||
| } | ||
| // Ensure all preserved submaterials also render from both sides | ||
| if (subMaterial) { | ||
| subMaterial.backFaceCulling = false; | ||
| } | ||
| return subMaterial; | ||
| }, | ||
| ); | ||
| } else if (isDefaultMaterial(resultMesh.material)) { | ||
| resultMesh.material = replaceMaterial(); | ||
| resultMesh.material.backFaceCulling = false; | ||
| } |
There was a problem hiding this comment.
Preserved single-material results still keep one-sided culling.
The MultiMaterial branch updates preserved subMaterials, but the single-material branch only replaces "default material". If resultMesh.material comes back as a real non-default material, its backFaceCulling value is left unchanged and the new interior faces can still disappear.
🧰 Tools
🪛 GitHub Actions: Prettier
[warning] Prettier reported formatting warning (file is not formatted according to Prettier).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/csg.js` around lines 1112 - 1128, The single-material branch leaves
non-default materials with their original one-sided culling, causing interior
faces to disappear; after handling resultMesh.material in the
else-if/isDefaultMaterial branch and in the non-default case, ensure preserved
single materials also render double-sided by setting
resultMesh.material.backFaceCulling = false when resultMesh.material is not
replaced. Update the logic around resultMesh.material (alongside existing checks
using flock.BABYLON.MultiMaterial, isDefaultMaterial, and replaceMaterial) so
that whenever a preserved (non-replaced) material is kept, you explicitly set
backFaceCulling = false; keep replaceMaterial() behavior unchanged for
default-material replacements.
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
Enhanced the CSG boolean operation material handling to properly preserve and apply materials from subtracted meshes to the resulting geometry's interior faces.
Key Changes
_findFirstDescendantWithMaterial()applyResultMeshProperties()to accept an optionalsubtractMaterialsarray parameterreplaceMaterial()function to prioritize using collected subtract mesh materials for interior faces, falling back to cloned reference mesh material when unavailablebackFaceCullingset tofalsefor proper two-sided renderingImplementation Details
applyResultMeshProperties()subtractMaterialIdxcounter tracks which subtract material to use next, cycling through available materials as default slots are replacedhttps://claude.ai/code/session_01TnumLdbUkFMgiducy37Zne
Summary by CodeRabbit