Support box-projected UVs for CSG subtractMeshes#511
Conversation
Deploying flockdev with
|
| Latest commit: |
941ca1c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7bb70675.flockdev.pages.dev |
| Branch Preview URL: | https://codex-investigate-material-v.flockdev.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional box-projection UV generation to CSG subtraction results, makes normalization warnings conditional, and adds pre-checks to skip CSG merges for meshes with non-finite or mismatched vertex attribute kinds, falling back to Mesh.MergeMeshes. Subtraction APIs now accept an options object forwarded to implementations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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.
🧹 Nitpick comments (1)
tests/materials.test.js (1)
757-823: Good test coverage for UV projection feature.The test properly validates that:
- UV data is generated when
uvProjection: "box"is specified- The
uvScaleparameter affects UV magnitude proportionallyOne consideration: if CSG operations fail (return
null),getMeshByNamewould returnnullandgetVerticesDatawould throw. You could add a guard assertion to improve error diagnostics:const meshA = flock.scene.getMeshByName("uvSubtractA"); const meshB = flock.scene.getMeshByName("uvSubtractB"); expect(meshA, "uvSubtractA mesh should exist after subtraction").to.exist; expect(meshB, "uvSubtractB mesh should exist after subtraction").to.exist;This would provide clearer failure messages if CSG operations fail for environment-specific reasons.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/materials.test.js` around lines 757 - 823, The test calls getVerticesData on meshes returned by flock.scene.getMeshByName("uvSubtractA") and "uvSubtractB" without guarding for null if CSG failed; add explicit assertions such as expect(meshA, "uvSubtractA mesh should exist after subtraction").to.exist and expect(meshB, "uvSubtractB mesh should exist after subtraction").to.exist before calling meshA.getVerticesData(...) / meshB.getVerticesData(...) so failures report clear diagnostics (referenced symbols: flock.scene.getMeshByName, meshA, meshB, getVerticesData, "uvSubtractA", "uvSubtractB").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/materials.test.js`:
- Around line 757-823: The test calls getVerticesData on meshes returned by
flock.scene.getMeshByName("uvSubtractA") and "uvSubtractB" without guarding for
null if CSG failed; add explicit assertions such as expect(meshA, "uvSubtractA
mesh should exist after subtraction").to.exist and expect(meshB, "uvSubtractB
mesh should exist after subtraction").to.exist before calling
meshA.getVerticesData(...) / meshB.getVerticesData(...) so failures report clear
diagnostics (referenced symbols: flock.scene.getMeshByName, meshA, meshB,
getVerticesData, "uvSubtractA", "uvSubtractB").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd82ef66-87a5-480a-bf6e-05f789241429
📒 Files selected for processing (2)
api/csg.jstests/materials.test.js
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)
1-1:⚠️ Potential issue | 🟡 MinorAddress Prettier formatting warning.
The pipeline indicates Prettier found formatting issues. Run
prettier --write api/csg.jsto fix the style violations before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` at line 1, Prettier reported style violations in api/csg.js (the top-level variable declaration "flock"); run the formatter and commit the result: execute prettier --write api/csg.js (or your project's formatting task) to reformat the file, verify that the change only affects style (e.g., spacing/semicolons around the "let flock;" declaration), then add and commit the updated file so the pipeline warning is resolved.
🧹 Nitpick comments (1)
api/csg.js (1)
196-197: NegativeuvScalevalues will pass validation and produce negativetexturePhysicalSize.The check
uvScale !== 0allows negative values through, which would result in a negativetexturePhysicalSize. Depending on howsetSizeBasedBoxUVshandles this, it could produce inverted or unexpected UV mapping.🔧 Proposed fix to reject non-positive scales
- const scale = Number.isFinite(uvScale) && uvScale !== 0 ? uvScale : 1; + const scale = Number.isFinite(uvScale) && uvScale > 0 ? uvScale : 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 196 - 197, The current validation for uvScale uses uvScale !== 0 which allows negative values and yields a negative texturePhysicalSize; update the logic that computes scale so it only accepts positive finite uvScale (e.g. Number.isFinite(uvScale) && uvScale > 0) and fall back to 1 otherwise, ensuring texturePhysicalSize = 4 / scale is always positive; adjust any downstream assumptions in setSizeBasedBoxUVs or callers to rely on a guaranteed positive texturePhysicalSize.
🤖 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`:
- Line 1: Prettier reported style violations in api/csg.js (the top-level
variable declaration "flock"); run the formatter and commit the result: execute
prettier --write api/csg.js (or your project's formatting task) to reformat the
file, verify that the change only affects style (e.g., spacing/semicolons around
the "let flock;" declaration), then add and commit the updated file so the
pipeline warning is resolved.
---
Nitpick comments:
In `@api/csg.js`:
- Around line 196-197: The current validation for uvScale uses uvScale !== 0
which allows negative values and yields a negative texturePhysicalSize; update
the logic that computes scale so it only accepts positive finite uvScale (e.g.
Number.isFinite(uvScale) && uvScale > 0) and fall back to 1 otherwise, ensuring
texturePhysicalSize = 4 / scale is always positive; adjust any downstream
assumptions in setSizeBasedBoxUVs or callers to rely on a guaranteed positive
texturePhysicalSize.
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1-1:⚠️ Potential issue | 🟡 MinorAddress Prettier formatting issue to pass CI.
The pipeline reports a Prettier formatting issue. Run
npx prettier --write api/csg.jsto fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` at line 1, The file declares the top-level variable "flock" and fails CI due to Prettier formatting; run the formatter (e.g., execute `npx prettier --write api/csg.js`) or apply the project's Prettier config to reformat api/csg.js so the declaration for "flock" and the rest of the file conform to the repository's style rules, then commit the formatted file.
🤖 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 481-485: Update the console.warn message inside the mergeMeshes
logic to include the missing word "to": change the string passed to console.warn
in the mergeMeshes branch from "[mergeMeshes] Skipping CSG merge due non-finite
positions; using Mesh.MergeMeshes fallback." to "[mergeMeshes] Skipping CSG
merge due to non-finite positions; using Mesh.MergeMeshes fallback." so the
warning reads correctly.
---
Outside diff comments:
In `@api/csg.js`:
- Line 1: The file declares the top-level variable "flock" and fails CI due to
Prettier formatting; run the formatter (e.g., execute `npx prettier --write
api/csg.js`) or apply the project's Prettier config to reformat api/csg.js so
the declaration for "flock" and the rest of the file conform to the repository's
style rules, then commit the formatted file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
api/csg.js
Outdated
| } else { | ||
| console.warn( | ||
| "[mergeMeshes] Skipping CSG merge due non-finite positions; using Mesh.MergeMeshes fallback.", | ||
| ); | ||
| emptyMeshes.forEach((m) => m.dispose()); | ||
| console.warn("[mergeMeshes] CSG merge attempt failed:", error); | ||
| csgSucceeded = false; | ||
| } |
There was a problem hiding this comment.
Minor typo in warning message.
Missing "to" in the console warning.
Proposed fix
} else {
console.warn(
- "[mergeMeshes] Skipping CSG merge due non-finite positions; using Mesh.MergeMeshes fallback.",
+ "[mergeMeshes] Skipping CSG merge due to non-finite positions; using Mesh.MergeMeshes fallback.",
);
}📝 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.
| } else { | |
| console.warn( | |
| "[mergeMeshes] Skipping CSG merge due non-finite positions; using Mesh.MergeMeshes fallback.", | |
| ); | |
| emptyMeshes.forEach((m) => m.dispose()); | |
| console.warn("[mergeMeshes] CSG merge attempt failed:", error); | |
| csgSucceeded = false; | |
| } | |
| } else { | |
| console.warn( | |
| "[mergeMeshes] Skipping CSG merge due to non-finite positions; using Mesh.MergeMeshes fallback.", | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/csg.js` around lines 481 - 485, Update the console.warn message inside
the mergeMeshes logic to include the missing word "to": change the string passed
to console.warn in the mergeMeshes branch from "[mergeMeshes] Skipping CSG merge
due non-finite positions; using Mesh.MergeMeshes fallback." to "[mergeMeshes]
Skipping CSG merge due to non-finite positions; using Mesh.MergeMeshes
fallback." so the warning reads correctly.
1 participant
Motivation
uvProjectionanduvScale) intosubtractMeshesand its internal merge/individual flows.Description
applyBoxProjectionUV(mesh, uvScale)which computes box-projected UVs from vertex positions and normals and sets theUVvertex buffer on the mesh.subtractMeshessignature to accept either anapproachstring or an options object and forwardoptionsintosubtractMeshesMergeandsubtractMeshesIndividualas applicable.applyBoxProjectionUVto theresultMeshin both merge and individual subtraction code paths whenoptions.uvProjection === "box", usingoptions.uvScalefor scaling.tests/materials.test.jsthat exercises box-projected UVs forsubtractMeshesand asserts UV presence and scaling behavior.Testing
tests/materials.test.jswhich contains the newshould support box-projected UVs for subtractMeshestest; all tests passed.Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Tests