Attempt manifold 3D text mesh creation with verbose logging and fallback#524
Attempt manifold 3D text mesh creation with verbose logging and fallback#524tracygardner wants to merge 8 commits intomainfrom
Conversation
Deploying flockxr with
|
| Latest commit: |
5b0d1a6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bd449522.flockxr.pages.dev |
| Branch Preview URL: | https://codex-find-alternatives-for.flockxr.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:
📝 WalkthroughWalkthroughcreate3DText now records whether manifold was requested, logs manifold attempt/success or explicit fallback reason, and recomputes normals on successful manifold geometry. CSG subtraction gains per-model monotonic trace steps, propagates traceId/traceStep to subtraction implementations, and adds gated console.info instrumentation for subtraction steps and failures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 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 |
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/shapes.js (1)
724-806:⚠️ Potential issue | 🟡 MinorFix formatting to clear CI
Prettier is failing for this file in CI. Please run formatting on
api/shapes.jsbefore merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/shapes.js` around lines 724 - 806, The file fails CI due to formatting issues; run the project's Prettier/formatter on api/shapes.js (or invoke the npm script like npm run format) and reformat the create3DText block (symbols to check: create3DText, useManifold, manifoldRequested, meshId, createManifoldTextMesh, meshData, vertexData) so indentation, trailing commas, and line breaks match project style, then stage the formatted file and push the commit.
🤖 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/shapes.js`:
- Around line 724-806: The file fails CI due to formatting issues; run the
project's Prettier/formatter on api/shapes.js (or invoke the npm script like npm
run format) and reformat the create3DText block (symbols to check: create3DText,
useManifold, manifoldRequested, meshId, createManifoldTextMesh, meshData,
vertexData) so indentation, trailing commas, and line breaks match project
style, then stage the formatted file and push the commit.
Deploying flockdev with
|
| Latest commit: |
5b0d1a6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://615fcfcf.flockdev.pages.dev |
| Branch Preview URL: | https://codex-find-alternatives-for.flockdev.pages.dev |
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 | 🟡 MinorPrettier check is currently failing for this file
CI reports formatting/style issues in
api/csg.js. Please run Prettier on this file before merge.🤖 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 is failing for api/csg.js due to formatting/style issues; run Prettier on the file (or your project's formatting script) and commit the updated formatting so the file (including the top-level declaration like the variable "flock") matches the project's code style; ensure your editor/CI config uses the project's Prettier settings and re-run lint/CI to confirm the file passes.
🧹 Nitpick comments (1)
api/csg.js (1)
792-855: Gate high-volumeconsole.infocalls behind a debug flagLine 792 onward and Line 993 onward run inside per-tool/per-subtraction loops; unconditional info logging here can become noisy and expensive in normal usage. Consider gating these logs with a single debug switch (for example
options.debugCSG || flock?.materialsDebug).Proposed refactor pattern
+const shouldLogCSGInfo = options?.debugCSG === true || flock?.materialsDebug === true; +const logCSGInfo = (...args) => { + if (shouldLogCSGInfo) console.info(...args); +}; - console.info(`[subtractMeshes][merge] toolIndex=${meshIndex} ...`); + logCSGInfo(`[subtractMeshes][merge] toolIndex=${meshIndex} ...`); - console.info(`[subtractMeshes][merge] subtractionIndex=${idx} status=ok tool=${m.name}`); + logCSGInfo(`[subtractMeshes][merge] subtractionIndex=${idx} status=ok tool=${m.name}`);Also applies to: 993-1018
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 792 - 855, The info logs inside subtractMeshes (the per-tool loop that builds subtractDuplicates and the subtraction loop that uses outerCSG) are unconditionally verbose; wrap the console.info/console.warn calls in a debug guard (e.g. if (options.debugCSG || flock?.materialsDebug)) so they only run when debugging is enabled. Locate the logging calls around the partClones/unified handling (where subtractDuplicates is pushed) and in the subtraction loop that constructs meshCSG and calls outerCSG.subtract, and conditionally execute those console.* statements behind the debug flag; keep the debug flag check centralized (reuse options.debugCSG || flock?.materialsDebug) to avoid duplicating logic.
🤖 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 is failing for api/csg.js due to formatting/style issues; run
Prettier on the file (or your project's formatting script) and commit the
updated formatting so the file (including the top-level declaration like the
variable "flock") matches the project's code style; ensure your editor/CI config
uses the project's Prettier settings and re-run lint/CI to confirm the file
passes.
---
Nitpick comments:
In `@api/csg.js`:
- Around line 792-855: The info logs inside subtractMeshes (the per-tool loop
that builds subtractDuplicates and the subtraction loop that uses outerCSG) are
unconditionally verbose; wrap the console.info/console.warn calls in a debug
guard (e.g. if (options.debugCSG || flock?.materialsDebug)) so they only run
when debugging is enabled. Locate the logging calls around the
partClones/unified handling (where subtractDuplicates is pushed) and in the
subtraction loop that constructs meshCSG and calls outerCSG.subtract, and
conditionally execute those console.* statements behind the debug flag; keep the
debug flag check centralized (reuse options.debugCSG || flock?.materialsDebug)
to avoid duplicating logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/csg.js (1)
711-713: Extract shared trace-context initialization to avoid drift.Both subtraction variants duplicate the same
traceId/traceStep/verboseTracesetup. A tiny helper will keep behavior aligned and reduce maintenance churn.Also applies to: 938-940
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 711 - 713, Duplicate trace-context initialization (traceId, traceStep, verboseTrace) should be extracted into a small helper function (e.g., initTraceContext or buildTraceContext) that accepts the existing options object and returns an object containing traceId, traceStep, and verboseTrace; replace the repeated blocks that set const traceId = options.traceId || "no-trace-id", const traceStep = Number.isFinite(options.traceStep) ? options.traceStep : 1, and const verboseTrace = traceStep <= 3 with a single call to this helper wherever those three lines are duplicated (both occurrences shown in the diff), and update call sites to destructure/use the returned traceId, traceStep, and verboseTrace.
🤖 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 1122-1125: flock._subtractTraceSteps is an unbounded Map and can
grow indefinitely for new traceKey values; change it to a bounded cache by
replacing the plain Map with a size-limited structure (e.g., an LRU or simple
FIFO-evicting Map) so when adding a new traceKey in the block that references
flock._subtractTraceSteps and traceKey you evict the oldest entry once a
maxEntries threshold is reached (use a constant like MAX_SUBTRACT_TRACE_STEPS)
and then set the new nextStep; update the initialization (where
flock._subtractTraceSteps is created) and the increment logic that uses nextStep
to maintain that bound.
---
Nitpick comments:
In `@api/csg.js`:
- Around line 711-713: Duplicate trace-context initialization (traceId,
traceStep, verboseTrace) should be extracted into a small helper function (e.g.,
initTraceContext or buildTraceContext) that accepts the existing options object
and returns an object containing traceId, traceStep, and verboseTrace; replace
the repeated blocks that set const traceId = options.traceId || "no-trace-id",
const traceStep = Number.isFinite(options.traceStep) ? options.traceStep : 1,
and const verboseTrace = traceStep <= 3 with a single call to this helper
wherever those three lines are duplicated (both occurrences shown in the diff),
and update call sites to destructure/use the returned traceId, traceStep, and
verboseTrace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if (!flock._subtractTraceSteps) flock._subtractTraceSteps = new Map(); | ||
| const traceKey = typeof modelId === "string" ? modelId : String(modelId); | ||
| const nextStep = (flock._subtractTraceSteps.get(traceKey) || 0) + 1; | ||
| flock._subtractTraceSteps.set(traceKey, nextStep); |
There was a problem hiding this comment.
Bound trace-state map to prevent unbounded memory growth.
flock._subtractTraceSteps only grows and is never pruned. In long-running sessions with many unique modelIds, this can accumulate indefinitely.
♻️ Suggested bounded-cache fix
if (!flock._subtractTraceSteps) flock._subtractTraceSteps = new Map();
const traceKey = typeof modelId === "string" ? modelId : String(modelId);
+const TRACE_STEP_CACHE_LIMIT = 1000;
+const isNewTraceKey = !flock._subtractTraceSteps.has(traceKey);
+if (isNewTraceKey && flock._subtractTraceSteps.size >= TRACE_STEP_CACHE_LIMIT) {
+ const oldestKey = flock._subtractTraceSteps.keys().next().value;
+ flock._subtractTraceSteps.delete(oldestKey);
+}
const nextStep = (flock._subtractTraceSteps.get(traceKey) || 0) + 1;
flock._subtractTraceSteps.set(traceKey, nextStep);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/csg.js` around lines 1122 - 1125, flock._subtractTraceSteps is an
unbounded Map and can grow indefinitely for new traceKey values; change it to a
bounded cache by replacing the plain Map with a size-limited structure (e.g., an
LRU or simple FIFO-evicting Map) so when adding a new traceKey in the block that
references flock._subtractTraceSteps and traceKey you evict the oldest entry
once a maxEntries threshold is reached (use a constant like
MAX_SUBTRACT_TRACE_STEPS) and then set the new nextStep; update the
initialization (where flock._subtractTraceSteps is created) and the increment
logic that uses nextStep to maintain that bound.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/csg.js (1)
1138-1158:⚠️ Potential issue | 🟠 MajorReset or bound
_subtractTraceSteps; it currently leaks state across operations.This map only grows, and its stale entries also make future subtracts for the same
modelIdlook like repeated steps forever. That means the new "initial trace" diagnostics stop appearing for later independent operations, in addition to the memory growth in long-running sessions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 1138 - 1158, The _subtractTraceSteps Map grows unbounded and makes future calls appear as repeated steps; fix by implementing TTL-based eviction and bounding: add a companion Map (e.g., flock._subtractTraceTimers) to store setTimeout handles keyed by traceKey, clear any existing timer when updating flock._subtractTraceSteps in subtractMeshes, and schedule a timer (e.g., 30–60s) to delete the traceKey from flock._subtractTraceSteps (and its timer) so state is reset; additionally, enforce a hard cap (e.g., 1000 entries) on flock._subtractTraceSteps and evict oldest keys if exceeded. Ensure you update code around traceKey/nextStep/traceId handling in subtractMeshes to clear/set timers and evict when necessary.
🤖 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 863-870: The per-subtraction console.info trace logs are still
emitted on every failure; update the catch blocks in subtractMeshesMerge (and
the other similar catch) to only emit the console.info when initialTrace is
truthy — i.e., add a guard like if (initialTrace) around the console.info(...)
calls that currently log `[subtractMeshes][merge] trace=...` so repeated retries
don't spam per-step noise while preserving the console.warn(e.message) failure
signal.
---
Duplicate comments:
In `@api/csg.js`:
- Around line 1138-1158: The _subtractTraceSteps Map grows unbounded and makes
future calls appear as repeated steps; fix by implementing TTL-based eviction
and bounding: add a companion Map (e.g., flock._subtractTraceTimers) to store
setTimeout handles keyed by traceKey, clear any existing timer when updating
flock._subtractTraceSteps in subtractMeshes, and schedule a timer (e.g., 30–60s)
to delete the traceKey from flock._subtractTraceSteps (and its timer) so state
is reset; additionally, enforce a hard cap (e.g., 1000 entries) on
flock._subtractTraceSteps and evict oldest keys if exceeded. Ensure you update
code around traceKey/nextStep/traceId handling in subtractMeshes to clear/set
timers and evict when necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/csg.js (1)
1081-1084:⚠️ Potential issue | 🟠 MajorBound trace-state map to prevent unbounded memory growth.
flock._subtractTraceStepsonly grows and is never pruned. In long-running sessions with many uniquemodelIds (e.g., from block regeneration per Context snippet 2), this accumulates indefinitely.♻️ Suggested bounded-cache fix
if (!flock._subtractTraceSteps) flock._subtractTraceSteps = new Map(); const traceKey = typeof modelId === "string" ? modelId : String(modelId); +const TRACE_STEP_CACHE_LIMIT = 1000; +if (!flock._subtractTraceSteps.has(traceKey) && flock._subtractTraceSteps.size >= TRACE_STEP_CACHE_LIMIT) { + const oldestKey = flock._subtractTraceSteps.keys().next().value; + flock._subtractTraceSteps.delete(oldestKey); +} const nextStep = (flock._subtractTraceSteps.get(traceKey) || 0) + 1; flock._subtractTraceSteps.set(traceKey, nextStep);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 1081 - 1084, flock._subtractTraceSteps is an unbounded Map keyed by traceKey (derived from modelId) that can grow without limit; replace it with a bounded cache (e.g., an LRU or size-capped Map) or implement eviction logic so entries are pruned when capacity is exceeded. Update the initialization site that currently sets flock._subtractTraceSteps = new Map() to instead create a bounded cache (or wrap the Map with logic to remove the oldest/least-recently-used key when size > MAX_STEPS), and ensure the existing accessors (the traceKey computation, nextStep increment, and flock._subtractTraceSteps.set/get usage) continue to work with the bounded structure.
🧹 Nitpick comments (2)
api/csg.js (2)
841-843: Gate failure trace logs behindinitialTraceto suppress repeated noise.This
console.infois emitted on every failed subtraction regardless of trace step. If a model triggers many retries, this produces per-step noise that theinitialTracegating was designed to suppress. Theconsole.warnon lines 837-840 already preserves the failure signal.♻️ Suggested fix
} catch (e) { console.warn( `[subtractMeshesMerge] Subtraction ${idx} failed:`, e.message, ); - console.info( - `[subtractMeshes][merge] trace=${traceId} subtractionIndex=${idx} status=failed tool=${m.name}`, - ); + if (initialTrace) { + console.info( + `[subtractMeshes][merge] trace=${traceId} subtractionIndex=${idx} status=failed tool=${m.name}`, + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 841 - 843, The console.info call in the subtractMeshes merge path is emitting a per-step failure log (traceId/subtractionIndex) for every retry; gate this message behind the same initialTrace check used for the console.warn so repeated retries don't spam logs: in the function subtractMeshes (merge handling) wrap or condition the console.info(`[subtractMeshes][merge] trace=${traceId} subtractionIndex=${idx} status=failed tool=${m.name}`) so it only runs when initialTrace is true (same condition the existing console.warn uses), preserving the single warning but suppressing noisy per-step info logs.
994-996: Gate failure trace logs behindinitialTracehere as well.Same concern as the merge implementation—this
console.infowill emit on every failure regardless of trace step, while theconsole.warnon line 993 already signals the failure.♻️ Suggested fix
} catch (e) { console.warn(e); - console.info( - `[subtractMeshes][individual] trace=${traceId} subtractionIndex=${index} status=failed tool=${part.name}`, - ); + if (initialTrace) { + console.info( + `[subtractMeshes][individual] trace=${traceId} subtractionIndex=${index} status=failed tool=${part.name}`, + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 994 - 996, The extra console.info in the subtractMeshes flow is emitting redundant failure trace logs regardless of the trace step; update the logging so the info-level trace (the console.info call using traceId, subtractionIndex and part.name) is only executed when the tracing gate variable initialTrace is truthy (i.e., mirror the merge implementation’s behavior) to avoid duplicating the console.warn failure log—locate the console.info in subtractMeshes and wrap it in a conditional check for initialTrace (or gate the log behind the same trace-check used elsewhere).
🤖 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`:
- Line 978: The forEach callback currently declares an unused parameter named
meshIndex which violates ESLint; update the callback signature used on
validMeshes.forEach to either remove the unused second parameter or rename it to
start with an underscore (e.g., _meshIndex) so it satisfies the project's lint
rule; locate the validMeshes.forEach(...) callback and change the parameter
accordingly in that anonymous function.
---
Duplicate comments:
In `@api/csg.js`:
- Around line 1081-1084: flock._subtractTraceSteps is an unbounded Map keyed by
traceKey (derived from modelId) that can grow without limit; replace it with a
bounded cache (e.g., an LRU or size-capped Map) or implement eviction logic so
entries are pruned when capacity is exceeded. Update the initialization site
that currently sets flock._subtractTraceSteps = new Map() to instead create a
bounded cache (or wrap the Map with logic to remove the
oldest/least-recently-used key when size > MAX_STEPS), and ensure the existing
accessors (the traceKey computation, nextStep increment, and
flock._subtractTraceSteps.set/get usage) continue to work with the bounded
structure.
---
Nitpick comments:
In `@api/csg.js`:
- Around line 841-843: The console.info call in the subtractMeshes merge path is
emitting a per-step failure log (traceId/subtractionIndex) for every retry; gate
this message behind the same initialTrace check used for the console.warn so
repeated retries don't spam logs: in the function subtractMeshes (merge
handling) wrap or condition the console.info(`[subtractMeshes][merge]
trace=${traceId} subtractionIndex=${idx} status=failed tool=${m.name}`) so it
only runs when initialTrace is true (same condition the existing console.warn
uses), preserving the single warning but suppressing noisy per-step info logs.
- Around line 994-996: The extra console.info in the subtractMeshes flow is
emitting redundant failure trace logs regardless of the trace step; update the
logging so the info-level trace (the console.info call using traceId,
subtractionIndex and part.name) is only executed when the tracing gate variable
initialTrace is truthy (i.e., mirror the merge implementation’s behavior) to
avoid duplicating the console.warn failure log—locate the console.info in
subtractMeshes and wrap it in a conditional check for initialTrace (or gate the
log behind the same trace-check used elsewhere).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9e4062d-a8c3-44b0-84e9-f27e1fd58e5d
📒 Files selected for processing (2)
api/csg.jsapi/shapes.js
🚧 Files skipped from review as they are similar to previous changes (1)
- api/shapes.js
| let outerCSG = flock.BABYLON.CSG2.FromMesh(baseDuplicate, false); | ||
| const allToolParts = []; | ||
| validMeshes.forEach((mesh) => { | ||
| validMeshes.forEach((mesh, meshIndex) => { |
There was a problem hiding this comment.
Fix ESLint violation: unused meshIndex parameter.
The pipeline is failing because meshIndex is defined but never used. Per the project's ESLint configuration, unused arguments must be prefixed with _.
🔧 Proposed fix
- validMeshes.forEach((mesh, meshIndex) => {
+ validMeshes.forEach((mesh, _meshIndex) => {Alternatively, if the index is not needed at all:
- validMeshes.forEach((mesh, meshIndex) => {
+ validMeshes.forEach((mesh) => {📝 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.
| validMeshes.forEach((mesh, meshIndex) => { | |
| validMeshes.forEach((mesh, _meshIndex) => { |
🧰 Tools
🪛 GitHub Actions: ESLint
[error] 978-978: ESLint (no-unused-vars): 'meshIndex' is defined but never used. Allowed unused args must match /^_/u
🪛 GitHub Check: eslint
[failure] 978-978:
'meshIndex' is defined but never used. Allowed unused args must match /^_/u
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/csg.js` at line 978, The forEach callback currently declares an unused
parameter named meshIndex which violates ESLint; update the callback signature
used on validMeshes.forEach to either remove the unused second parameter or
rename it to start with an underscore (e.g., _meshIndex) so it satisfies the
project's lint rule; locate the validMeshes.forEach(...) callback and change the
parameter accordingly in that anonymous function.
Motivation
createManifoldTextMeshsucceeds.Description
manifoldRequestedflag and additionalconsole.info/console.warnlogs to trace whether manifold or standard generation is used and why.createManifoldTextMesh(including mapping.jsonfont names tofonts/FreeSansBold.ttf), computes normals, recenters vertex positions, and applies the resultingVertexDatato a newMeshwhen successful.MeshBuilder.CreateTextpath when the manifold approach throws, and records the fallback reason (manifold_failedormanifold_disabled).Testing
npm test, and it completed successfully.Codex Task
Summary by CodeRabbit