feat: world sleep guard toggle + tessellation caps #32
Conversation
…s\n\n- Add controller methods: setWorldSleepGuardEnabled, setTessellationAutoEnabled, setTessellationMinMax\n- Make adapter guard conditional and set from controller\n- computeAutoSegments respects auto on/off and min/max caps\n- Add DOM integration tests for guard (on/off) and tessellation caps\n- Wire Mantine Drawer controls for guard + caps; keep App resilient to controller mocks\n- All tests passing (116/116)
Summary by CodeRabbitRelease Notes
WalkthroughReplaces the Drawer debug UI with an affixed Paper accordion panel; adds many DebugProps (tessellation, world-sleep-guard, broadphase, overlay and clothify/restore hooks); implements fat‑AABB broadphase, static-world spheres and LineLoop pointer overlay; introduces RigidBody2D, EventBus, clothify/restore APIs, and related tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as DebugPalette (Accordion)
participant App as App.tsx
participant Demo as Demo wiring
participant CSC as ClothSceneController
UI->>App: onAutoTessellationChange(enabled)
App->>Demo: propagate prop
Demo->>CSC: setTessellationAutoEnabled(enabled)
CSC->>CSC: recompute segments (respect min/max)
UI->>App: onTessellationMinChange(min)
App->>Demo: propagate prop
Demo->>CSC: setTessellationMinMax(min, currentMax)
CSC->>CSC: clamp and apply
UI->>App: onWorldSleepGuardChange(enabled)
App->>Demo: propagate prop
Demo->>CSC: setWorldSleepGuardEnabled(enabled)
CSC->>Adapters: adapter.setWorldSleepGuardEnabled(enabled)
sequenceDiagram
participant Sim as SimulationSystem
participant Body as PhysicsBody
rect rgba(243,240,255,0.8)
Sim->>Body: set gravity
Sim->>Body: wake() -- ensure immediate wake on gravity change
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
…eleport on click; remove auto-pin release\n\n- Draw unit circle scaled by pointerRadius instead of massive sphere\n- Track pointerRadius in DebugOverlayState and compute from DOM record size\n- Keep overlay pointer at last position on reset to prevent center jump\n\nfeat(presets): make presets cloth-only (no camera zoom) and update tests\n\n- Remove cameraZoom from presets and App preset handler\n- Adjust App tests to assert no zoom change\n\nrefactor: stop auto releasing pins on activation for predictable behavior
|
Updates pushed addressing reported issues:
If you still see an outsized collider, try toggling the drawer once so the overlay refreshes state; the circle should roughly match the cloth’s default impulse radius. Happy to wire radius to a dedicated slider if you prefer explicit control. |
…e' for expected drop-on-click\n\n- SimulationSystem.broadcastGravity now wakes all bodies after updating gravity\n- Controller.setGravity wakes active cloths\n- App default pin mode set to 'none' to match expected UX
|
Follow-up fixes pushed for the "cloth doesn’t drop / gravity has no effect" report:
This should address: “relaxes then freezes” + “not affected by gravity.” If you prefer a different default (e.g., Top), we can keep the default but auto‑wake on activation and/or auto‑release pins after warm start. For now I’ve kept auto‑release OFF to match your earlier ask. Let me know if you still see a freeze with Real‑Time ON; I can add a tiny status readout (real‑time, g, substeps, iterations) to the drawer to verify live values. |
…tropy\n\n- Scale Y by (px/m X) / (px/m Y) based on camera extents and viewport size\n- Keeps pointer collider perfectly round regardless of viewport aspect
|
Pointer collider stretch fix pushed:
Details:
Suite remains green (116/116). Let me know if you want me to port the small WorldCamera helper as well for clarity. |
…nt, then re-adding cloth mesh\n\n- Avoids any chance of a duplicate mesh lingering (solid vs wireframe overlay)\n- Verified tests remain green
|
Wireframe report follow-up:
If you still don’t see wireframe after toggling it on while the cloth is active, let me know the exact steps and I’ll instrument a quick counter of cloth meshes toggled by the RenderSettingsSystem to verify it’s hitting your mesh. |
…eep guard toggle, typed snapshot, tessellation options; align DebugSettings fields
|
Merged origin/main into this branch (no rebase), resolved conflicts, and validated:
Health checks
Commit: 5921117 (merge(main): resolve conflicts in clothSceneController; keep guard + tessellation options) I’m holding off on merging per request—awaiting review/feedback. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/App.tsx (1)
607-620: Reset still pins the clothWe switched the runtime default to
PinMode = 'none', but the Reset button keeps forcing'top', undoing that fix the moment someone hits reset. Please align the reset path with the new default:- setPinMode("top") + setPinMode("none")src/lib/clothSceneController.ts (1)
952-993: Auto tessellation controls are inert
setTessellationAutoEnabledandsetTessellationMinMaxonly mutate the debug struct; they never re-run thepool.prepareloop, so the mesh keeps whatever segment count it had before the toggle. That means the brand-new UI sliders and tests lie unless the user also jiggles the manual tessellation slider. We need to force a re-bake even when the nominal segment count stays the same. One way:- async setTessellationSegments(segments: number) { + async setTessellationSegments(segments: number, force = false) { const pool = this.pool if (!pool) return const clamped = Math.max(1, Math.min(segments, 32)) - if (this.debug.tessellationSegments === clamped) return + if (!force && this.debug.tessellationSegments === clamped) return this.debug.tessellationSegments = clamped const tasks: Promise<void>[] = [] @@ setTessellationAutoEnabled(enabled: boolean) { this.debug.autoTessellation = !!enabled + void this.setTessellationSegments(this.debug.tessellationSegments, true) } @@ setTessellationMinMax(min: number, max: number) { const mi = Math.max(1, Math.min(46, Math.round(min))) const ma = Math.max(mi + 2, Math.min(48, Math.round(max))) this.debug.tessellationMin = mi this.debug.tessellationMax = ma + void this.setTessellationSegments(this.debug.tessellationSegments, true) }With the
forceflag we reuse the existing refresh logic, ensure the new caps apply immediately, and keep the manual slider behaviour unchanged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
BLOG_NOTES.mdis excluded by!*.md
📒 Files selected for processing (8)
src/App.tsx(10 hunks)src/app/__tests__/debugActions.test.tsx(1 hunks)src/app/presets.ts(0 hunks)src/engine/render/DebugOverlayState.ts(1 hunks)src/engine/render/DebugOverlaySystem.ts(3 hunks)src/engine/systems/simulationSystem.ts(1 hunks)src/lib/__tests__/domIntegration.test.ts(3 hunks)src/lib/clothSceneController.ts(14 hunks)
💤 Files with no reviewable changes (1)
- src/app/presets.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/clothSceneController.ts (1)
src/types/pinMode.ts (1)
PinMode(1-1)
🪛 GitHub Actions: CI
src/App.tsx
[error] 494-494: ESLint: Unexpected any. Specify a different type. @typescript-eslint/no-explicit-any (lint step: npm run lint)
🪛 GitHub Check: test
src/engine/render/DebugOverlaySystem.ts
[failure] 96-96:
Empty block statement
src/lib/clothSceneController.ts
[failure] 593-593:
Empty block statement
[failure] 590-590:
Empty block statement
src/App.tsx
[failure] 502-502:
Unexpected any. Specify a different type
[failure] 498-498:
Unexpected any. Specify a different type
[failure] 494-494:
Unexpected any. Specify a different type
🔇 Additional comments (6)
src/engine/systems/simulationSystem.ts (1)
153-158: Gravity tweaks must wake sleepers immediatelyThe extra
wake?.()makes gravity adjustments observable without waiting for incidental motion. Optional chaining keeps bodies without a wake hook safe. No objections.src/engine/render/DebugOverlayState.ts (1)
14-15: Expose pointerRadius in shared stateThreading the collider radius through
DebugOverlayStateremoves the hard-coded magic value from renderers. Looks solid.src/app/__tests__/debugActions.test.tsx (1)
148-164: Preset path now proven zoom-neutralTracking
setTargetZoomcall counts before/after the preset click nails the regression we feared. Clean assertion, no leaks.src/lib/__tests__/domIntegration.test.ts (3)
607-632: Auto tessellation coverage is thoroughToggling auto off/on and asserting the segment clamp hits both min/max paths—exactly what we needed to keep the controller honest.
651-672: Pointer leave regression locked downCapturing the last pointer coordinates and asserting they persist through
pointerleavedefends against the old teleport bug. Nicely surgical.
674-713: Guard wake behaviour verified end-to-endThe paired tests prove the adapter respects the world-sleep guard switch, using actual update cycles rather than stubs. Perfect signal.
… change while auto is ON; keep manual slider semantics; tests green
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/App.tsx (1)
607-620: Reset function incomplete: missing new state resets.The reset handler doesn't reset the newly added state variables:
autoTessellation(should reset totrue)tessellationMin(should reset to6)tessellationMax(should reset to24)worldSleepGuard(should reset totrue)This leaves the debug palette in a partially-reset state when the user clicks "Reset."
Apply this diff:
onReset={() => { setWireframe(false) setRealTime(true) setGravity(9.81) setImpulseMultiplier(1) setTessellationSegments(24) + setAutoTessellation(true) + setTessellationMin(6) + setTessellationMax(24) setConstraintIterations(4) setSubsteps(1) setCameraZoom(1) setPointerColliderVisible(false) setPinMode("none") + setWorldSleepGuard(true) controllerRef.current?.setSleepConfig({ velocityThreshold: 0.001, frameThreshold: 60 }) actionsRef.current?.setSleepConfig(0.001, 60) }}src/lib/clothSceneController.ts (1)
507-534: computeAutoSegments: MIN clamp contradicts UI bounds.Line 526:
clamp(round(this.debug.tessellationMin ?? 6), 1, 40)caps the minimum segments at 40, but the UI slider inApp.tsx:238allowstessellationMinto go up to 46. This means the user can set min to 46 in the UI, butcomputeAutoSegmentssilently clamps it to 40.This mismatch breaks user expectations: moving the Min slider to 46 has no effect beyond 40.
Either:
- Update line 526 to
clamp(round(this.debug.tessellationMin ?? 6), 1, 46)to match the UI, or- Update the UI slider at
App.tsx:238tomax={40}to match the function's limit.Given that
MAX_TESSELLATION_CAPis 48 and the max slider goes to 48, option 1 (raising the clamp to 46) seems more consistent.- const MIN_SEGMENTS = clamp(round(this.debug.tessellationMin ?? 6), 1, 40) + const MIN_SEGMENTS = clamp(round(this.debug.tessellationMin ?? 6), 1, 46)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/App.tsx(11 hunks)src/engine/render/DebugOverlaySystem.ts(3 hunks)src/lib/clothSceneController.ts(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/App.tsx (3)
src/lib/clothSceneController.ts (5)
setConstraintIterations(205-207)setConstraintIterations(930-936)setSubsteps(920-927)setPinMode(944-958)PinMode(25-25)src/lib/clothPhysics.ts (2)
setConstraintIterations(70-73)setSubsteps(75-78)src/engine/debug/engineActions.ts (3)
setConstraintIterations(91-93)setSubsteps(60-62)setPinMode(125-127)
src/lib/clothSceneController.ts (1)
src/types/pinMode.ts (1)
PinMode(1-1)
🔇 Additional comments (13)
src/engine/render/DebugOverlaySystem.ts (2)
23-23: LGTM: Pointer type matches implementation.The change from
THREE.MeshtoTHREE.LineLoopcorrectly reflects the circular outline rendering approach.
121-138: LGTM: Clean pointer initialization.The unit-circle geometry with 64 segments and
LineLooprendering is correct. TherenderOrderof 1002 ensures the pointer draws on top of other debug overlays.src/App.tsx (5)
41-46: LGTM: New debug props correctly typed.The optional markers and callback signatures for tessellation and world sleep guard controls are appropriate.
Also applies to: 55-56
225-245: LGTM: Auto tessellation UI correctly wired.The conditional min/max sliders appear only when auto tessellation is enabled, and the max slider's min bound of
(tessellationMin ?? 6) + 2correctly prevents inversions.
278-284: LGTM: World sleep guard toggle correctly placed.The double-bang coercion handles the optional prop safely, and the UI placement under sleep threshold is contextually appropriate.
493-503: Fixed:as anycasts removed, but double optional chaining is unconventional.The previous lint violations have been resolved! The code now correctly uses optional chaining without type casts.
However, the double optional chaining (
?.setTessellationAutoEnabled?.() is unconventional. IfcontrollerRef.currentis aClothSceneController, these methods are guaranteed to exist. The pattern suggests the methods are optional, which they're not in production.Based on the PR description ("Makes the App resilient to controller mocks via optional method calls"), this is intentional for test resilience. If so, consider documenting this pattern with a comment explaining the mock-compatibility rationale, or type the ref as
Partial<ClothSceneController>to make the optionality explicit.
566-571: LGTM: Props correctly threaded to DebugPalette.The tessellation and world sleep guard state is properly wired through to the debug UI.
Also applies to: 580-581
src/lib/clothSceneController.ts (6)
35-37: LGTM: DebugSettings extended correctly.The new fields for auto-tessellation and world sleep guard are properly typed and match the defaults initialized at lines 373-378.
Also applies to: 40-40
588-609: Fixed: Empty catch blocks replaced with proper logging.The previous lint violations have been resolved! The catch blocks now log errors with appropriate context (operation, elementId, error). The non-fatal nature is correctly conveyed with
console.warn.
628-628: LGTM: Pin behavior now deterministic.Removing the auto-release timeout makes pin lifecycle fully controlled by the selected Pin Mode, which is more predictable. The comment accurately documents this change.
647-647: LGTM: World sleep guard applied on activation, wake on gravity change.Line 647 correctly syncs the guard state when creating adapters. Lines 899-902 ensure sleeping cloths wake when gravity changes so they respond immediately to the new force.
Also applies to: 899-902
968-1015: LGTM: Tessellation methods correctly implement runtime control.The
forceparameter insetTessellationSegmentsallows recomputation when auto-tessellation settings change, even if the base segment count is unchanged. The guard logic insetTessellationAutoEnabledandsetTessellationMinMaxcorrectly triggers recalculation only when auto mode is active.Line 1008-1009 clamps are correct, but note they interact with the clamp inconsistency flagged in
computeAutoSegments(line 526).
134-167: Reject: Both concerns are unfounded. The code is correct and safe.Concern 1 (unsafe cast): INCORRECT. While the
as unknown as MaybeGScast is unnecessarily defensive, it is safe because line 137 contains a proper runtime guardtypeof maybe.getBoundingSphere === 'function'. Line 138 executes only if the guard passes, so it cannot throw. Proof: the adapter itself callscloth.getBoundingSphere()directly at line 232 without any casting—TypeScript accepts it. The cast is cargo cult programming, not a bug.Concern 2 (frame-rate dependent threshold): INCORRECT. Scaling the threshold by
dtis intentional frame-rate normalization, not a flaw. The math: at 60fps (dt≈0.0167), threshold =0.001 * 0.0167; at 30fps (dt≈0.0333), threshold =0.001 * 0.0333. The threshold expands at slower frame rates to compensate for fewer samples per second, ensuring consistent detection of motion across varying frame rates. This is correct physics. The blog notes explicitly document this: "scale threshold by dt to approximate velocity threshold per frame."The code is correct and the guard is well-designed. Remove the review comment.
Likely an incorrect or invalid review comment.
…isotropy; shared pointer radius helper; reset new palette states; raise MIN clamp to 46
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 (2)
src/lib/clothSceneController.ts (2)
134-167: World sleep guard silently degrades if getBoundingSphere is missing.Lines 135-138 use a type guard to check for
getBoundingSphere, and if it's missing, the entire guard block is skipped. This is safe but results in silent degradation - the guard is effectively disabled for that body. While this is better than crashing, it may mask issues during development.Consider logging a warning once per adapter if
getBoundingSphereis unavailable:if (typeof maybe.getBoundingSphere === 'function') { // ... existing logic ... } else { // One-time warning if (!this._warnedAboutMissingBoundingSphere) { console.warn(`ClothBodyAdapter ${this.id}: getBoundingSphere not available, world sleep guard disabled`) this._warnedAboutMissingBoundingSphere = true } }Then add
private _warnedAboutMissingBoundingSphere = falseto the class fields.
507-534: Dense auto-tessellation logic could use a clarifying comment.Lines 507-534 implement complex clamping and min/max logic for auto tessellation. The code is correct but dense. The interplay between
rawMax,maxUser, and the clamps at line 530 is not immediately obvious.Consider adding a brief comment before line 530 explaining the logic:
// Effective max is the lesser of: user-configured max and the provided cap, both bounded by global limits const MAX = Math.min(clamp(rawMax, MIN_SEGMENTS + 2, MAX_TESSELLATION_CAP), maxUser)
♻️ Duplicate comments (2)
src/engine/render/DebugOverlaySystem.ts (1)
109-109: Empty catch block still violates lint rules.Line 109 remains an empty catch block, which was flagged in the previous review and is breaking
no-emptyin CI. This must be fixed.Apply this diff to satisfy the lint rule:
- } catch { /* no-op */ } + } catch (_) { void 0 }src/lib/clothSceneController.ts (1)
847-864: Pointer radius logic still duplicated with adapter.Lines 847-864 implement pointer radius computation, which is now in a dedicated method (improvement over the previous inline version). However, the logic is still duplicated with
ClothBodyAdapter.getImpulseRadius()(lines 247-261). Both checkdata-cloth-impulse-radius, computemin(width, height) / 12, and use the same MIN/DEFAULT constants.While the extraction into a dedicated method is an improvement, consider further refactoring to share the logic. One approach:
- Extract a static helper:
private static computeImpulseRadius(element: HTMLElement, widthMeters: number, heightMeters: number): number- Call it from both
ClothBodyAdapter.getImpulseRadius()andClothSceneController.computePointerRadiusFor()This eliminates the duplication entirely and ensures both code paths stay in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/App.tsx(11 hunks)src/engine/render/DebugOverlaySystem.ts(3 hunks)src/lib/clothSceneController.ts(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/App.tsx (2)
src/lib/clothSceneController.ts (5)
setConstraintIterations(205-207)setConstraintIterations(954-960)setSubsteps(944-951)setPinMode(968-982)PinMode(25-25)src/engine/debug/engineActions.ts (3)
setConstraintIterations(91-93)setSubsteps(60-62)setPinMode(125-127)
src/lib/clothSceneController.ts (1)
src/types/pinMode.ts (1)
PinMode(1-1)
🔇 Additional comments (4)
src/engine/render/DebugOverlaySystem.ts (1)
128-145: LGTM: Pointer refactored to LineLoop.The unit circle geometry with 64 segments is appropriate, and the renderOrder ensures proper layering. The implementation is clean.
src/App.tsx (1)
493-503: LGTM:anycasts removed.The previous review's critical issue has been properly addressed. The code now uses optional chaining without type casts, which is the correct approach.
src/lib/clothSceneController.ts (2)
377-377: LGTM: pinMode default now consistent.Line 377 sets pinMode default to
'none', which matches the App.tsx default at line 361. This resolves the inconsistency flagged in the previous review.
588-609: LGTM: Empty catch blocks replaced with proper error handling.The previous review's critical issue has been properly addressed. Lines 590-598 and 601-609 now catch errors, log them with context, and continue gracefully. The defensive activation pattern is well-documented.
…scrollable) while keeping keyboard toggle; make Presets controlled (selectedPreset)
…ht; remove redundant abs;\nchore(controller): warn once when getBoundingSphere is missing; add comment clarifying effective MAX in auto tessellation
…s; world-sleep guard; tessellation options; tests green
…ons (non‑modal); keep aria‑labels stable; open key panels by default; preserve Step control when paused
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/clothSceneController.ts (1)
145-183: World-space guard: disable permanently after logging the missing method.Lines 148-182 check
typeof maybe.getBoundingSphere === 'function'every frame. Once you log the warning (line 179), you already know the method is missing—disable the guard for this adapter instance to avoid repeated checks:} else { if (!this._warnedAboutMissingBoundingSphere) { console.warn(`ClothBodyAdapter ${this.id}: getBoundingSphere not available, world sleep guard disabled`) this._warnedAboutMissingBoundingSphere = true + this._worldSleepGuardEnabled = false } }
♻️ Duplicate comments (5)
src/engine/render/RenderSettingsSystem.ts (2)
23-25: Commented-out code STILL present despite claiming it was "addressed."Past review (commits 42e042b–ff2e244) flagged this dead code and you marked it "✅ Addressed," yet here it sits, mocking us. If you're not going to use
lastWireframe, delete it. Version control exists for resurrection. This speculative "Kept for future toggles" rationalization is YAGNI theater.Apply this diff:
- // Kept for future toggles; currently unused because we enforce every frame - // to cover meshes created after the last toggle. - // private lastWireframe = false
37-40: Brute-force per-frame traversal: past review ignored, justification added instead.The previous review demanded an event-driven solution (registry, dirty flag, or notification hook), yet you simply rewrote the comment to assert "cheap and prevents drift" without measuring or profiling. You're still calling
scene.traverse()on every single frame for every object in the hierarchy, regardless of whether anything changed. This scales O(n) with scene complexity and burns cycles on static meshes that will never become cloth.The architectural flaw remains unfixed. Implement one of these:
- Event emission: When
activate()orclothify()creates a cloth mesh, emit an event that triggersapplySettings()once.- Registry: Maintain a
Set<THREE.Mesh>of cloth/static meshes; iterate only that set whenwireframechanges.- Dirty flag: Mark dirty when
state.wireframeorstate.applyToStaticchanges or when new meshes appear; skip traversal when clean.src/App.tsx (1)
586-588: Duplicate useEffect: still running setPointerOverlayVisible twice per toggle.Past review flagged this exact duplicate at lines 520-522 and 586-588, yet both remain. Every
pointerColliderVisiblechange triggers the same call twice. Delete one:- useEffect(() => { - actionsRef.current?.setPointerOverlayVisible(pointerColliderVisible) - }, [pointerColliderVisible])src/lib/clothSceneController.ts (2)
596-624: Critical: Missing disposal race guard afterawaitin clothify.Past review flagged this exact issue and it remains unfixed. If the controller is disposed or the pool is swapped while
prepareawaits (line 603), you'll still callmount,addStaticBody, etc. on stale/null state. The pattern at lines 565-566 inprepareElementsshows the correct guard. Apply it here:const seg = this.computeAutoSegments(rect, this.debug.tessellationSegments) await pool.prepare(element, seg) + if (this.disposed || this.pool !== pool || this.domToWebGL !== bridge) return pool.mount(element)
922-939: Code duplication persists:computePointerRadiusFormirrorsClothBodyAdapter.getImpulseRadius.Past review flagged this duplication and suggested extracting a shared helper or calling the adapter method directly. Instead, you created a second helper (lines 922-939) with identical logic to
ClothBodyAdapter.getImpulseRadius(lines 263-277): same constants, same attribute check, same formula.Extract the logic into a shared static function:
// In a shared utility or on ClothBodyAdapter as static static computeImpulseRadius(element: HTMLElement, widthMeters: number, heightMeters: number): number { const attr = element.dataset.clothImpulseRadius const parsed = attr ? Number.parseFloat(attr) : NaN if (!Number.isNaN(parsed) && parsed > 0) return parsed const base = Math.min(widthMeters, heightMeters) const MIN = 0.0006, DEFAULT = 0.0012 return base > 0 ? Math.max(MIN, base / 12) : DEFAULT }Then call it from both places.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/App.tsx(17 hunks)src/engine/render/RenderSettingsState.ts(1 hunks)src/engine/render/RenderSettingsSystem.ts(2 hunks)src/lib/clothSceneController.ts(18 hunks)src/lib/h2cInterception.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/lib/clothSceneController.ts (1)
src/types/pinMode.ts (1)
PinMode(1-1)
src/App.tsx (5)
src/app/presets.ts (1)
PRESETS(35-35)src/lib/clothSceneController.ts (6)
setConstraintIterations(221-223)setConstraintIterations(1029-1035)setSubsteps(1019-1026)setPinMode(1043-1057)PinMode(25-25)setRealTime(1010-1016)src/lib/clothPhysics.ts (2)
setConstraintIterations(70-73)setSubsteps(75-78)src/engine/debug/engineActions.ts (4)
setConstraintIterations(91-93)setSubsteps(60-62)setPinMode(125-127)setRealTime(50-52)src/engine/simulationRunner.ts (2)
setSubsteps(73-77)setRealTime(66-70)
🔇 Additional comments (9)
src/lib/h2cInterception.ts (1)
47-48: Comment clarity is excellent.The comment clearly explains both the intent (No-Quirks mode) and the approach (mirror or synthesize), making the subsequent logic immediately understandable.
src/engine/render/RenderSettingsState.ts (1)
4-5: LGTM: Clean flag addition with appropriate default.The
applyToStaticproperty is well-named, defaulted correctly (false), and documented concisely. The comment explains both the condition ("when true") and the purpose ("preview"), making the intent crystal clear.src/App.tsx (2)
175-376: LGTM: Well-organized Accordion structure with appropriate conditionals.The debug panel's multi-section layout is clean, follows Mantine conventions, and correctly handles optional tessellation props with ?? fallbacks. The conditional rendering of min/max sliders (lines 271-284) is appropriate and avoids cluttering the UI when auto tessellation is off.
708-726: LGTM: Pragmatic clothify/restore delegation with appropriate error handling.The manual stepping when paused (lines 711-715) is a reasonable workaround to ensure visual separation of the cloth from the static panel. Error handling logs failures instead of silently swallowing them, and the delegation pattern keeps the component clean by avoiding direct controller ref access in DebugPalette.
src/lib/clothSceneController.ts (5)
35-40: LGTM: Required fields in internal settings, optional in public API.Making
autoTessellation,tessellationMin,tessellationMax, andworldSleepGuardEnabledrequired inDebugSettingsis correct—the controller always has concrete defaults (lines 389-394). The public API in App.tsx makes them optional for backward compatibility. Clean separation.
86-90: LGTM: Well-documented state for world-space sleep guarding.The activation grace period (20 frames ≈ 0.3s at 60 fps) is a pragmatic solution to avoid premature sleep on pinned cloth before it can sag. The one-time warning flag prevents console spam. Clean implementation.
656-661: LGTM: Size-relative jitter prevents visual crumple without rigid starts.The jitter calculation (lines 658-660) scales with element size while capping extremes: tiny elements won't crumple, large ones get meaningful perturbation. The deliberate choice not to auto-release pins (line 661) aligns with the PR's goal of predictable pin behavior. Well-reasoned and documented.
698-737: LGTM: Comprehensive restoration logic mirrors activation in reverse.
restoreElementmethodically reverses activation: removes from simulation, updates userData flags, recycles/remounts geometry, restores DOM state, and resets item flags. The early-exit guard (lines 701-707) handles the static case cleanly. Well-structured and complete.
995-1001: LGTM: Wake-on-gravity-change ensures sleeping cloth responds immediately.The wake call (line 1000) after gravity changes is an intentional feature (per PR objectives) that prevents sleeping cloth from ignoring gravity updates. The implementation is correct and beneficial for UX.
| // Optional helper passed by parent to clothify the panel element | ||
| clothifyElement?: (el: HTMLElement) => Promise<void> | ||
| restoreElement?: (el: HTMLElement) => Promise<void> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant comment: the ? already signals optionality.
The comment "Optional helper passed by parent to delegate" restates what TypeScript already declares via ?. Either remove the comment or make it meaningful (e.g., "Delegate clothification to parent/controller to avoid local ref access").
🤖 Prompt for AI Agents
In src/App.tsx around lines 77 to 79, the inline comment "Optional helper passed
by parent to clothify the panel element" is redundant because the `?` already
indicates optionality; remove the comment or replace it with a meaningful one
(for example: "Delegate clothify/restore behavior to parent/controller to avoid
local DOM management" or similar) so the comment adds intent rather than
repeating type information.
| // Hide via visual opacity to keep element capturable by html2canvas when needed | ||
| opacity: open ? 1 : 0, | ||
| pointerEvents: open ? 'auto' : 'none', | ||
| }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Opacity-based hiding: clever but deserves fuller explanation.
Using opacity: 0 instead of conditional rendering is unusual. The inline comment mentions html2canvas, but it's not immediately clear why the element must stay mounted when hidden. Consider expanding:
- // Hide via visual opacity to keep element capturable by html2canvas when needed
+ // Hide via visual opacity rather than unmounting to keep the DOM structure
+ // intact for html2canvas capture when clothifying the panel📝 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.
| // Hide via visual opacity to keep element capturable by html2canvas when needed | |
| opacity: open ? 1 : 0, | |
| pointerEvents: open ? 'auto' : 'none', | |
| }} | |
| // Hide via visual opacity rather than unmounting to keep the DOM structure | |
| // intact for html2canvas capture when clothifying the panel | |
| opacity: open ? 1 : 0, | |
| pointerEvents: open ? 'auto' : 'none', | |
| }} |
🤖 Prompt for AI Agents
In src/App.tsx around lines 156 to 159, the inline comment about using opacity:
0 to keep the element mounted for html2canvas is terse; replace it with a
clearer explanatory comment that states why the element must remain in the DOM
(html2canvas requires mounted nodes to capture layout and styles), that
pointerEvents is toggled to prevent interaction while hidden, and note any
accessibility implications (e.g., ensure hidden content is not announced by
screen readers or use aria-hidden if appropriate). Keep the existing
opacity/pointerEvents approach but expand the comment to include those three
points so future readers understand the necessity and tradeoffs.
| const userData = (mesh as unknown as { userData?: Record<string, unknown> }).userData || {} | ||
| const isCloth = !!userData['isCloth'] | ||
| const isStatic = !!userData['isStatic'] | ||
| if (isCloth || (isStatic && this.state.applyToStatic)) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify the double-cast pattern.
(mesh as unknown as { userData?: Record<string, unknown> }) is verbose. You can safely cast directly or use a type guard:
- const userData = (mesh as unknown as { userData?: Record<string, unknown> }).userData || {}
+ const userData = (mesh.userData as Record<string, unknown> | undefined) || {}📝 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.
| const userData = (mesh as unknown as { userData?: Record<string, unknown> }).userData || {} | |
| const isCloth = !!userData['isCloth'] | |
| const isStatic = !!userData['isStatic'] | |
| if (isCloth || (isStatic && this.state.applyToStatic)) { | |
| const userData = (mesh.userData as Record<string, unknown> | undefined) || {} | |
| const isCloth = !!userData['isCloth'] | |
| const isStatic = !!userData['isStatic'] | |
| if (isCloth || (isStatic && this.state.applyToStatic)) { |
🤖 Prompt for AI Agents
In src/engine/render/RenderSettingsSystem.ts around lines 46 to 49, the code
uses a redundant double-cast "(mesh as unknown as { userData?: Record<string,
unknown> })"; replace this with a simpler, safer approach by either casting
directly to the expected shape (e.g. "mesh as { userData?: Record<string,
unknown> }") or, better, use a type guard/`in` check to read userData only when
present (e.g. if ('userData' in mesh) { const userData = mesh.userData ?? {};
... }) so you avoid the verbose double-cast and make the intent and type-safety
clearer.
| /** Enables/disables automatic tessellation based on on-screen size. */ | ||
| setTessellationAutoEnabled(enabled: boolean) { | ||
| this.debug.autoTessellation = !!enabled | ||
| if (this.debug.autoTessellation) { | ||
| void this.setTessellationSegments(this.debug.tessellationSegments, true) | ||
| } | ||
| } | ||
|
|
||
| /** Sets the min/max caps used by auto tessellation. */ | ||
| setTessellationMinMax(min: number, max: number) { | ||
| const mi = Math.max(1, Math.min(46, Math.round(min))) | ||
| const ma = Math.max(mi + 2, Math.min(48, Math.round(max))) | ||
| this.debug.tessellationMin = mi | ||
| this.debug.tessellationMax = ma | ||
| if (this.debug.autoTessellation) { | ||
| void this.setTessellationSegments(this.debug.tessellationSegments, true) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider commenting the intentional fire-and-forget async calls.
Lines 1101 and 1112 use void to discard the setTessellationSegments promise. While correct, it's subtle. Add a brief comment:
+ // Fire-and-forget: tessellation updates are async but don't block caller
void this.setTessellationSegments(this.debug.tessellationSegments, true)📝 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.
| /** Enables/disables automatic tessellation based on on-screen size. */ | |
| setTessellationAutoEnabled(enabled: boolean) { | |
| this.debug.autoTessellation = !!enabled | |
| if (this.debug.autoTessellation) { | |
| void this.setTessellationSegments(this.debug.tessellationSegments, true) | |
| } | |
| } | |
| /** Sets the min/max caps used by auto tessellation. */ | |
| setTessellationMinMax(min: number, max: number) { | |
| const mi = Math.max(1, Math.min(46, Math.round(min))) | |
| const ma = Math.max(mi + 2, Math.min(48, Math.round(max))) | |
| this.debug.tessellationMin = mi | |
| this.debug.tessellationMax = ma | |
| if (this.debug.autoTessellation) { | |
| void this.setTessellationSegments(this.debug.tessellationSegments, true) | |
| } | |
| } | |
| /** Enables/disables automatic tessellation based on on-screen size. */ | |
| setTessellationAutoEnabled(enabled: boolean) { | |
| this.debug.autoTessellation = !!enabled | |
| if (this.debug.autoTessellation) { | |
| // Fire-and-forget: tessellation updates are async but don't block caller | |
| void this.setTessellationSegments(this.debug.tessellationSegments, true) | |
| } | |
| } | |
| /** Sets the min/max caps used by auto tessellation. */ | |
| setTessellationMinMax(min: number, max: number) { | |
| const mi = Math.max(1, Math.min(46, Math.round(min))) | |
| const ma = Math.max(mi + 2, Math.min(48, Math.round(max))) | |
| this.debug.tessellationMin = mi | |
| this.debug.tessellationMax = ma | |
| if (this.debug.autoTessellation) { | |
| // Fire-and-forget: tessellation updates are async but don't block caller | |
| void this.setTessellationSegments(this.debug.tessellationSegments, true) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/lib/clothSceneController.ts around lines 1097 to 1114, the two uses of
`void this.setTessellationSegments(..., true)` are intentional fire-and-forget
async calls but lack an explanatory comment; add a concise inline comment on
each `void` invocation (e.g., "// intentionally fire-and-forget: update segments
asynchronously; errors handled internally") so readers understand the promise is
deliberately discarded and why.
| if (targetDocument.implementation?.createDocumentType) { | ||
| if (parsed.doctype) { | ||
| const dt = targetDocument.implementation.createDocumentType( | ||
| parsed.doctype.name, | ||
| parsed.doctype.publicId, | ||
| parsed.doctype.systemId, | ||
| ) | ||
| targetDocument.appendChild(dt) | ||
| } else { | ||
| // HTML5 doctype (<!DOCTYPE html>) | ||
| const dt = targetDocument.implementation.createDocumentType('html', '', '') | ||
| targetDocument.appendChild(dt) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Eliminate code duplication and verify silent failure path.
The targetDocument.appendChild(dt) call is duplicated on lines 56 and 60. More critically, if createDocumentType is unsupported, the optional chaining on line 49 silently skips the entire doctype block, potentially leaving the document in Quirks mode—directly contradicting the comment on line 47 that states the goal is to "ensure No-Quirks mode."
Refactor to eliminate duplication and handle unsupported environments:
- if (targetDocument.implementation?.createDocumentType) {
+ if (targetDocument.implementation?.createDocumentType) {
+ let dt: DocumentType
if (parsed.doctype) {
- const dt = targetDocument.implementation.createDocumentType(
+ dt = targetDocument.implementation.createDocumentType(
parsed.doctype.name,
parsed.doctype.publicId,
parsed.doctype.systemId,
)
- targetDocument.appendChild(dt)
} else {
- // HTML5 doctype (<!DOCTYPE html>)
- const dt = targetDocument.implementation.createDocumentType('html', '', '')
- targetDocument.appendChild(dt)
+ // HTML5 doctype: <!DOCTYPE html> → createDocumentType('html', '', '')
+ dt = targetDocument.implementation.createDocumentType('html', '', '')
}
+ targetDocument.appendChild(dt)
}Additionally, verify the silent failure path:
The optional chaining means older browsers without createDocumentType will skip doctype creation entirely. Run the following script to confirm whether html2canvas or the broader codebase has a fallback strategy or tolerance for Quirks mode in legacy environments:
🏁 Script executed:
#!/bin/bash
# Search for Quirks mode handling, compatibility checks, or fallback logic
rg -nC3 -i 'quirks|compatMode|createDocumentType' --type tsLength of output: 1353
🏁 Script executed:
#!/bin/bash
# Get broader context: parent function, try-catch blocks, JSDoc
rg -nB20 'if \(targetDocument\.implementation\?\.createDocumentType\)' src/lib/h2cInterception.ts | head -50Length of output: 1228
🏁 Script executed:
#!/bin/bash
# Check for try-catch blocks in the file and any browser support comments
rg -nC5 'try|catch|browser|support|IE|legacy|polyfill' src/lib/h2cInterception.ts | head -40Length of output: 1575
🏁 Script executed:
#!/bin/bash
# Check file header/exports for any JSDoc or browser support declarations
head -50 src/lib/h2cInterception.tsLength of output: 2269
Eliminate duplication and fix silent failure that contradicts stated goal.
The optional chaining on line 49 silently skips doctype creation if createDocumentType is unavailable, yet the function returns true anyway. This violates the explicit comment on lines 47–48 ("ensure No-Quirks mode"). Without a doctype, the document defaults to Quirks mode, directly contradicting the stated intent. Additionally, targetDocument.appendChild(dt) is duplicated on lines 56 and 60.
if (targetDocument.implementation?.createDocumentType) {
+ let dt: DocumentType
if (parsed.doctype) {
- const dt = targetDocument.implementation.createDocumentType(
+ dt = targetDocument.implementation.createDocumentType(
parsed.doctype.name,
parsed.doctype.publicId,
parsed.doctype.systemId,
)
- targetDocument.appendChild(dt)
} else {
- // HTML5 doctype (<!DOCTYPE html>)
- const dt = targetDocument.implementation.createDocumentType('html', '', '')
- targetDocument.appendChild(dt)
+ // HTML5 doctype: <!DOCTYPE html> → createDocumentType('html', '', '')
+ dt = targetDocument.implementation.createDocumentType('html', '', '')
}
+ targetDocument.appendChild(dt)
- }
+ } else {
+ // Fallback: if createDocumentType is unavailable, explicitly set a minimal doctype
+ // to avoid Quirks mode. Consider throwing an error or logging a warning here.
+ targetDocument.appendChild(targetDocument.implementation.createDocumentType('html', '', ''))
+ }Either throw an error if createDocumentType is unavailable (failing loudly rather than silently), or implement an explicit fallback strategy. The current behavior silently produces Quirks-mode documents, which undermines the documented contract.
🤖 Prompt for AI Agents
In src/lib/h2cInterception.ts around lines 49 to 61, the code currently uses
optional chaining on targetDocument.implementation?.createDocumentType which
silently skips doctype creation and returns true (leading to Quirks mode) and
also duplicates targetDocument.appendChild(dt); change this to explicitly check
for targetDocument.implementation and createDocumentType: if createDocumentType
is missing, throw a clear error (or call a documented fallback that guarantees a
standards-mode doctype), and when creating the doctype compute dt once and call
targetDocument.appendChild(dt) only once (remove the duplicated append). Ensure
the function fails loudly instead of silently producing a Quirks-mode document.
| const dt = targetDocument.implementation.createDocumentType( | ||
| parsed.doctype.name, | ||
| parsed.doctype.publicId, | ||
| parsed.doctype.systemId, | ||
| ) | ||
| targetDocument.appendChild(dt) | ||
| } else { | ||
| // HTML5 doctype (<!DOCTYPE html>) | ||
| const dt = targetDocument.implementation.createDocumentType('html', '', '') | ||
| targetDocument.appendChild(dt) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more descriptive naming and inline documentation.
The variable dt is extremely terse. While scoped locally, a more descriptive name like doctype or clonedDoctype would improve readability. Additionally, the empty strings '' for publicId and systemId on line 59 are correct for HTML5 but lack inline explanation—consider adding a brief note like /* publicId */ '', /* systemId */ '' to make the API contract explicit.
🤖 Prompt for AI Agents
In src/lib/h2cInterception.ts around lines 51 to 61, the local variable name
`dt` is too terse and the empty string arguments for HTML5 doctype lack
explanation; rename `dt` to a more descriptive identifier such as `doctype` or
`clonedDoctype`, add a short inline comment above the branch describing the
HTML5 doctype case, and annotate the empty string parameters with inline
comments like `/* publicId */ ''` and `/* systemId */ ''` so the
createDocumentType call is self-documenting.
…ned cloth (e.g., top edge) sags before sleeping; keep type-safety
… bounding spheres (center+radius) via WorldBody transform; world-only sleep decision path in adapter
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/clothSceneController.ts (3)
152-197: Type assertions bypass safety on optional radius.Lines 156, 172, and 190 perform type assertions on
bs.radiuswhich may be undefined. While the nullish coalescing (??) handles the undefined case at runtime, the type assertion(bs.radius as number)defeats TypeScript's static checks and could mask future bugs if the getBoundingSphere contract changes.Declare a proper union type or conditional:
- if (typeof maybe.getBoundingSphere === 'function') { - const bs = maybe.getBoundingSphere() as { center: { x: number; y: number }; radius?: number } + if (typeof maybe.getBoundingSphere === 'function') { + const bs = maybe.getBoundingSphere() as { center: { x: number; y: number }; radius: number | undefined } const localCenter = bs.center let worldCenter = this._tmpLocalV2 // ... - const dRadius = Math.abs(((bs.radius as number) ?? 0) - this._lastRadius) + const dRadius = Math.abs((bs.radius ?? 0) - this._lastRadius) // ... - this._lastRadius = ((bs.radius as number) ?? this._lastRadius) + this._lastRadius = (bs.radius ?? this._lastRadius)Alternatively, extract the radius once and guard it:
+ const radius = bs.radius ?? this._lastRadius + const dRadius = Math.abs(radius - this._lastRadius) // ... - this._lastRadius = ((bs.radius as number) ?? this._lastRadius) + this._lastRadius = radius
546-574: Magic number 46 cap is unexplained and creates edge-case inconsistency.Line 565 clamps
tessellationMinto a maximum of 46, but the global cap is 48. This arbitrary limit is undocumented and creates an edge case: if a user setstessellationMinto 47 or 48, it silently becomes 46, potentially causing confusion whentessellationMaxcan reach 48.Either:
- Document why 46 is the practical minimum cap (e.g., "leave headroom for MAX_SEGMENTS + 2 spacing"), or
- Use
MAX_TESSELLATION_CAP - 2symbolically:- const MIN_SEGMENTS = clamp(round(this.debug.tessellationMin ?? 6), 1, 46) const MAX_TESSELLATION_CAP = 48 + const MIN_SEGMENTS = clamp(round(this.debug.tessellationMin ?? 6), 1, MAX_TESSELLATION_CAP - 2)
1017-1025: setGravity bypasses world-space transforms—critical correctness bug.Lines 1022-1023 call
item.cloth.setGravitydirectly with a world-space vector, bypassing the adapter'ssetGlobalGravity(lines 238-248) which transforms the gravity vector into the cloth's local/model space when aworldBodytransform is present. For cloth under scaled or rotated transforms, this produces incorrect physics.Delegate to the adapter when available:
setGravity(gravity: number) { this.debug.gravity = gravity + const gravityVector = new THREE.Vector3(0, -gravity, 0) for (const item of this.items.values()) { - if (!item.cloth) continue - item.cloth.setGravity(new THREE.Vector3(0, -gravity, 0)) - item.cloth.wake() + if (item.adapter) { + item.adapter.setGlobalGravity(gravityVector) + item.adapter.wake() + } else if (item.cloth) { + // Fallback for cloth without adapter (shouldn't happen in normal flow) + item.cloth.setGravity(gravityVector) + item.cloth.wake() + } } }
♻️ Duplicate comments (5)
src/engine/render/DebugOverlaySystem.ts (1)
110-110: UNRESOLVED: Empty catch block violates lint rules.This exact issue was flagged in a previous review. The empty catch
catch { /* no-op */ }triggers theno-emptylint rule. You ignored the feedback.Apply the previously suggested fix:
- } catch { /* no-op */ } + } catch (_) { void 0 }src/lib/clothSceneController.ts (4)
615-647: Disposal race guard still missing after await (duplicate concern).The past review comment flagged this, and it remains unaddressed: after
await pool.prepare(element, seg)at line 626, the controller could be disposed or the pool swapped during a hot reload or rapid navigation. The subsequentpool.mount(element)and state mutations would operate on stale/dead resources.Insert the disposal guard before any post-await mutations:
const seg = this.computeAutoSegments(rect, this.debug.tessellationSegments) await pool.prepare(element, seg) + if (this.disposed || this.pool !== pool || this.domToWebGL !== bridge) return pool.mount(element)
926-962: Pointer radius computation duplicates adapter logic (reiteration).The
computePointerRadiusForhelper (lines 945-962) is a verbatim copy ofClothBodyAdapter.getImpulseRadius(lines 286-300). This was previously flagged as a nitpick, but the duplication is now entrenched and will diverge over time if either is updated independently.Extract a shared helper at file scope or in a utility module:
function computeImpulseRadius( element: HTMLElement, widthMeters: number, heightMeters: number ): number { const attr = element.dataset.clothImpulseRadius const parsed = attr ? Number.parseFloat(attr) : NaN if (!Number.isNaN(parsed) && parsed > 0) return parsed const base = Math.min(widthMeters || 0, heightMeters || 0) const MIN_POINTER_RADIUS = 0.0006 const DEFAULT_POINTER_RADIUS = 0.0012 if (base > 0) return Math.max(MIN_POINTER_RADIUS, base / 12) return DEFAULT_POINTER_RADIUS }Then call it from both
ClothBodyAdapter.getImpulseRadiusandcomputePointerRadiusFor:private getImpulseRadius() { - const attr = this.item.element.dataset.clothImpulseRadius - // ... 14 lines + const { widthMeters = 0, heightMeters = 0 } = this.record ?? {} + return computeImpulseRadius(this.item.element, widthMeters, heightMeters) }private computePointerRadiusFor(item: ClothItem) { - const attr = item.element.dataset.clothImpulseRadius - // ... 14 lines + const widthMeters = item.record?.widthMeters ?? 0 + const heightMeters = item.record?.heightMeters ?? 0 + return computeImpulseRadius(item.element, widthMeters, heightMeters) }
1120-1126: Fire-and-forget void lacks comment (reiteration).Line 1124 uses
voidto discard the promise fromsetTessellationSegments, which is correct but was previously flagged as lacking an explanatory comment. This makes the intentional fire-and-forget pattern unclear to future maintainers.Add a brief comment:
this.debug.autoTessellation = !!enabled if (this.debug.autoTessellation) { + // Fire-and-forget: re-tessellate asynchronously; errors handled internally void this.setTessellationSegments(this.debug.tessellationSegments, true) }
1128-1137: Fire-and-forget void lacks comment (reiteration).Line 1135 has the same issue as line 1124:
voiddiscards the promise without explanation. The clamping logic at lines 1130-1131 correctly matchescomputeAutoSegments(lines 565-570), ensuring consistency between the user-facing API and the internal computation.Add a comment and approve the clamping:
this.debug.tessellationMin = mi this.debug.tessellationMax = ma if (this.debug.autoTessellation) { + // Fire-and-forget: re-tessellate asynchronously; errors handled internally void this.setTessellationSegments(this.debug.tessellationSegments, true) }The min/max clamping is consistent with the caps used in
computeAutoSegments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/engine/render/DebugOverlaySystem.ts(4 hunks)src/lib/clothSceneController.ts(21 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/clothSceneController.ts (1)
src/types/pinMode.ts (1)
PinMode(1-1)
🔇 Additional comments (10)
src/engine/render/DebugOverlaySystem.ts (2)
23-23: Type change correctly reflects the new pointer implementation.The field type accurately matches the
LineLoopinstantiation at line 141.
239-241: Color scheme update is clear and intentional.The comment accurately describes the mapping: green for awake bodies, red for sleeping bodies. The hex values match the stated intent.
src/lib/clothSceneController.ts (8)
120-142: Activation grace period is sound.The double wake (before and after
cloth.update) is a defensive pattern to prevent the internal sleep logic from latching during the grace window. The 20-frame grace (~333ms at 60fps) is reasonable for pinned cloth to sag under gravity before world-space stillness is evaluated.
258-284: World-space bounding sphere transform is well-implemented.The logic correctly transforms the center to world space and approximates the radius under non-uniform scale by averaging axis samples. The
Number.isFiniteguards prevent NaN propagation, and the fallback to local space when no worldBody is present is appropriate.
694-717: Adapter initialization order is correct.Setting
worldSleepGuardEnabledat line 703 before adding the adapter to the simulation system (line 707) is safe because the flag only affects behavior duringupdate, which won't be called until the adapter is registered and the simulation runs.
720-760: restoreElement is well-structured.The method correctly mirrors the offscreen handler logic: removes from simulation, destroys the entity, marks the mesh as static, recycles/resets/remounts the geometry, and restores event handlers. The sequence is sound and the early return for non-active items is appropriate.
894-900: Pointer reset preserves overlay position correctly.The comment at line 898 clearly documents the intentional preservation of the overlay pointer position to avoid visual teleportation. This aligns with the PR objectives.
1082-1088: setWorldSleepGuardEnabled is implemented correctly.The method updates the debug state and broadcasts to all adapters using optional chaining, consistent with the pattern used in
setPinModeand other broadcast methods.
1090-1118: setTessellationSegments with force parameter is sound.The
forceflag at line 1090 provides the necessary escape hatch to re-tessellate elements when auto mode is toggled or min/max bounds change, even if thetessellationSegmentsvalue itself is unchanged. The callers at lines 1124 and 1135 correctly useforce=truewhen auto is enabled.
416-416: Default pin mode changed to 'none'—verify UX intent.The default
pinModeis now'none', meaning cloth falls immediately on activation without pins. Per the PR objectives, this was intentional to make clicks "naturally make the cloth fall." Ensure this aligns with the desired out-of-box user experience, especially for new users who may expect cloth to remain attached at the top by default.Based on the PR description, this is intentional. If the behavior is confirmed as desired, this comment can be disregarded.
| mesh.position.set(this.state.pointer.x, this.state.pointer.y, 0.2) | ||
| const r = Math.max(0.0005, Math.min(0.2, this.state.pointerRadius || 0.01)) | ||
| // Try to compensate for orthographic anisotropy; fall back to uniform if anything is invalid. | ||
| const cam = this.view.camera as THREE.Camera & Partial<THREE.OrthographicCamera> | ||
| const maybeOrtho = cam as Partial<THREE.OrthographicCamera> | ||
| const left = typeof maybeOrtho.left === 'number' ? maybeOrtho.left : NaN | ||
| const right = typeof maybeOrtho.right === 'number' ? maybeOrtho.right : NaN | ||
| const top = typeof maybeOrtho.top === 'number' ? maybeOrtho.top : NaN | ||
| const bottom = typeof maybeOrtho.bottom === 'number' ? maybeOrtho.bottom : NaN | ||
| const hasOrthoExtents = [left, right, top, bottom].every((v) => Number.isFinite(v)) | ||
| if (hasOrthoExtents) { | ||
| const rawWidth = right - left | ||
| const rawHeight = top - bottom | ||
| // Guard: inverted or non-positive extents → uniform scale fallback | ||
| if (rawWidth <= 0 || rawHeight <= 0) { | ||
| mesh.scale.set(r, r, 1) | ||
| return | ||
| } | ||
| // Clamp tiny dimensions to a small epsilon to avoid divide-by-zero | ||
| const safeWidth = Math.max(1e-6, rawWidth) | ||
| const safeHeight = Math.max(1e-6, rawHeight) | ||
| let viewportWidth = 1 | ||
| let viewportHeight = 1 | ||
| const anyView = this.view as unknown as { getViewportPixels?: () => { width: number; height: number } } | ||
| try { | ||
| const vp = anyView.getViewportPixels?.() | ||
| if (vp && Number.isFinite(vp.width) && Number.isFinite(vp.height) && vp.width > 0 && vp.height > 0) { | ||
| viewportWidth = vp.width | ||
| viewportHeight = vp.height | ||
| } | ||
| } catch { /* no-op */ } | ||
| const pxPerMeterX = viewportWidth / safeWidth | ||
| const pxPerMeterY = viewportHeight / safeHeight | ||
| const k = pxPerMeterX / pxPerMeterY | ||
| if (Number.isFinite(k) && k > 0) { | ||
| mesh.scale.set(r, r * k, 1) | ||
| } else { | ||
| mesh.scale.set(r, r, 1) | ||
| } | ||
| } else { | ||
| mesh.scale.set(r, r, 1) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the orthographic compensation logic to a private method.
This 42-line block dominates frameUpdate and handles a single responsibility: computing anisotropy-corrected scale for the pointer. Extracting it to a method like computePointerScale(r: number): { x: number; y: number; z: number } would dramatically improve readability and make the frameUpdate flow scannable.
Example structure:
if (visible) {
mesh.position.set(this.state.pointer.x, this.state.pointer.y, 0.2)
const r = Math.max(0.0005, Math.min(0.2, this.state.pointerRadius || 0.01))
const scale = this.computePointerScale(r)
mesh.scale.set(scale.x, scale.y, scale.z)
}Then implement computePointerScale as a separate method containing lines 82-121.
🤖 Prompt for AI Agents
In src/engine/render/DebugOverlaySystem.ts around lines 80-121, extract the
42-line orthographic compensation/block that computes anisotropy-corrected
pointer scale into a new private method computePointerScale(r: number): { x:
number; y: number; z: number }; move all logic that reads this.view, camera
extents, viewport pixels and computes k into that method, preserve all guards
(NaN checks, safeWidth/safeHeight, try/catch, finite checks) and return { x:
number, y: number, z: number } (use z=1), then replace the original in-frame
block with const scale = this.computePointerScale(r); mesh.scale.set(scale.x,
scale.y, scale.z); ensure the new method is private, uses the same types
(accessing this.view and this.state as before), and maintains identical fallback
behavior when values are invalid.
| mesh.visible = visible | ||
| if (visible) { | ||
| mesh.position.set(this.state.pointer.x, this.state.pointer.y, 0.2) | ||
| const r = Math.max(0.0005, Math.min(0.2, this.state.pointerRadius || 0.01)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace magic numbers with named constants.
The values 0.0005, 0.2, and 0.01 at line 81 are opaque. What do they represent? Minimum pointer radius? Maximum? Default? Naming them would make the intent explicit.
Example:
private static readonly MIN_POINTER_RADIUS = 0.0005
private static readonly MAX_POINTER_RADIUS = 0.2
private static readonly DEFAULT_POINTER_RADIUS = 0.01
// Then at line 81:
const r = Math.max(
DebugOverlaySystem.MIN_POINTER_RADIUS,
Math.min(DebugOverlaySystem.MAX_POINTER_RADIUS, this.state.pointerRadius || DebugOverlaySystem.DEFAULT_POINTER_RADIUS)
)🤖 Prompt for AI Agents
In src/engine/render/DebugOverlaySystem.ts around line 81, replace the magic
numbers 0.0005, 0.2, and 0.01 with descriptive private static readonly constants
(e.g., MIN_POINTER_RADIUS = 0.0005, MAX_POINTER_RADIUS = 0.2,
DEFAULT_POINTER_RADIUS = 0.01) and update the radius calculation to use those
constants (use this.state.pointerRadius ||
DebugOverlaySystem.DEFAULT_POINTER_RADIUS inside Math.min/Math.max). Keep names
clear and capitalized to communicate their intent.
| private ensurePointer() { | ||
| if (!this.pointer) { | ||
| const geometry = new THREE.SphereGeometry(0.12, 16, 16) | ||
| const material = new THREE.MeshBasicMaterial({ color: 0x66ccff, wireframe: true }) | ||
| this.pointer = new THREE.Mesh(geometry, material) | ||
| // Unit circle in XY, scaled by pointerRadius each frame | ||
| const segments = 64 | ||
| const verts: number[] = [] | ||
| for (let i = 0; i < segments; i++) { | ||
| const t = (i / segments) * Math.PI * 2 | ||
| verts.push(Math.cos(t), Math.sin(t), 0) | ||
| } | ||
| const geometry = new THREE.BufferGeometry() | ||
| geometry.setAttribute('position', new THREE.Float32BufferAttribute(verts, 3)) | ||
| const material = new THREE.LineBasicMaterial({ color: 0x66ccff, transparent: true, opacity: 0.9 }) | ||
| this.pointer = new THREE.LineLoop(geometry, material) | ||
| this.pointer.visible = false | ||
| this.pointer.renderOrder = 1002 | ||
| } | ||
| return this.pointer | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract circle vertex generation to eliminate duplication.
Lines 132-137 and lines 231-236 in drawSimCircles use identical logic to generate circle vertices (64 segments, same loop, same math). This is textbook DRY violation.
Extract a helper:
private generateCircleVertices(segments: number, centerX = 0, centerY = 0, radius = 1, z = 0): number[] {
const verts: number[] = []
for (let i = 0; i < segments; i++) {
const t = (i / segments) * Math.PI * 2
verts.push(centerX + Math.cos(t) * radius, centerY + Math.sin(t) * radius, z)
}
return verts
}Then simplify both call sites:
// ensurePointer:
const verts = this.generateCircleVertices(64, 0, 0, 1, 0)
// drawSimCircles:
const verts = this.generateCircleVertices(64, body.center.x, body.center.y, body.radius, 0.1)🤖 Prompt for AI Agents
In src/engine/render/DebugOverlaySystem.ts around lines 129-146 (and similarly
at lines ~231-236), duplicate circle-vertex generation logic should be extracted
into a single helper; add a private method like generateCircleVertices(segments:
number, centerX = 0, centerY = 0, radius = 1, z = 0) that returns number[] by
running the existing loop and pushing cos/sin coords, then replace the current
inline loops in ensurePointer and drawSimCircles with calls to that helper
(passing 64 and appropriate center/radius/z values) and use its return value to
create the BufferGeometry attributes so both sites reuse the same
implementation.
| // Seed a very small, size-relative perturbation to avoid perfectly rigid start | ||
| // without causing a visible crumple/pop. | ||
| const sizeHint = Math.max(0.0005, Math.min(record.widthMeters, record.heightMeters)) | ||
| const jitter = Math.min(0.02, sizeHint * 0.2) | ||
| const jitter = Math.min(0.004, sizeHint * 0.05) | ||
| cloth.addTurbulence(jitter) | ||
| item.releasePinsTimeout = window.setTimeout(() => { | ||
| cloth.releaseAllPins() | ||
| delete item.releasePinsTimeout | ||
| }, 900) | ||
| // Do not auto-release pins; keep according to selected Pin Mode for predictable behavior. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Jitter constants lack rationale.
Lines 681-682 use magic numbers 0.0005, 0.004, and 0.05 without explaining why 5% of element size capped at 4mm is the chosen perturbation. While the logic is sound (size-relative jitter with a sensible cap), future maintainers won't know if these values were empirically tuned or arbitrary.
Add a brief comment or extract named constants:
+ // Jitter: 5% of element size (capped at 4mm) to avoid perfectly rigid start
+ // without visible crumple; min size hint 0.5mm prevents degenerate cases.
const sizeHint = Math.max(0.0005, Math.min(record.widthMeters, record.heightMeters))
const jitter = Math.min(0.004, sizeHint * 0.05)Or:
+ const MIN_SIZE_HINT_METERS = 0.0005
+ const JITTER_CAP_METERS = 0.004
+ const JITTER_SCALE_FACTOR = 0.05
+ const sizeHint = Math.max(MIN_SIZE_HINT_METERS, Math.min(record.widthMeters, record.heightMeters))
+ const jitter = Math.min(JITTER_CAP_METERS, sizeHint * JITTER_SCALE_FACTOR)📝 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.
| // Seed a very small, size-relative perturbation to avoid perfectly rigid start | |
| // without causing a visible crumple/pop. | |
| const sizeHint = Math.max(0.0005, Math.min(record.widthMeters, record.heightMeters)) | |
| const jitter = Math.min(0.02, sizeHint * 0.2) | |
| const jitter = Math.min(0.004, sizeHint * 0.05) | |
| cloth.addTurbulence(jitter) | |
| item.releasePinsTimeout = window.setTimeout(() => { | |
| cloth.releaseAllPins() | |
| delete item.releasePinsTimeout | |
| }, 900) | |
| // Do not auto-release pins; keep according to selected Pin Mode for predictable behavior. | |
| // Seed a very small, size-relative perturbation to avoid perfectly rigid start | |
| // without causing a visible crumple/pop. | |
| // Jitter: 5% of element size (capped at 4mm) to avoid perfectly rigid start | |
| // without visible crumple; min size hint 0.5mm prevents degenerate cases. | |
| const sizeHint = Math.max(0.0005, Math.min(record.widthMeters, record.heightMeters)) | |
| const jitter = Math.min(0.004, sizeHint * 0.05) | |
| cloth.addTurbulence(jitter) | |
| // Do not auto-release pins; keep according to selected Pin Mode for predictable behavior. |
| // Seed a very small, size-relative perturbation to avoid perfectly rigid start | |
| // without causing a visible crumple/pop. | |
| const sizeHint = Math.max(0.0005, Math.min(record.widthMeters, record.heightMeters)) | |
| const jitter = Math.min(0.02, sizeHint * 0.2) | |
| const jitter = Math.min(0.004, sizeHint * 0.05) | |
| cloth.addTurbulence(jitter) | |
| item.releasePinsTimeout = window.setTimeout(() => { | |
| cloth.releaseAllPins() | |
| delete item.releasePinsTimeout | |
| }, 900) | |
| // Do not auto-release pins; keep according to selected Pin Mode for predictable behavior. | |
| // Seed a very small, size-relative perturbation to avoid perfectly rigid start | |
| // without causing a visible crumple/pop. | |
| const MIN_SIZE_HINT_METERS = 0.0005 | |
| const JITTER_CAP_METERS = 0.004 | |
| const JITTER_SCALE_FACTOR = 0.05 | |
| const sizeHint = Math.max(MIN_SIZE_HINT_METERS, Math.min(record.widthMeters, record.heightMeters)) | |
| const jitter = Math.min(JITTER_CAP_METERS, sizeHint * JITTER_SCALE_FACTOR) | |
| cloth.addTurbulence(jitter) | |
| // Do not auto-release pins; keep according to selected Pin Mode for predictable behavior. |
🤖 Prompt for AI Agents
In src/lib/clothSceneController.ts around lines 679-684, the jitter calculation
uses magic numbers (0.0005, 0.004, 0.05) without rationale; extract these into
named constants (e.g., MIN_JITTER_M, MAX_JITTER_M, JITTER_FRACTION) and replace
inline literals, and add a concise comment stating units and why values were
chosen (minimum jitter to avoid perfectly rigid start, cap at 4mm to prevent
visible crumple, use 5% of element size as size-relative perturbation). Ensure
names and the inline comment make the intent and units clear for future
maintainers.
…centered at origin and transformed to world (max axis scale); world-only sleep check uses center motion; color swap green=awake red=asleep
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
src/lib/clothSceneController.ts (4)
649-671: Missing disposal guard after async prepare (previously flagged).As noted in the prior review,
clothifylacks the disposal/pool guard afterawait pool.prepare(...)at line 650. If the controller is disposed or the pool is swapped during the async call, the subsequentmount/addStaticBody/etc. operations will manipulate stale state, leading to potential duplicate attachments or leaked resources.Apply the guard immediately after the await:
const seg = this.computeAutoSegments(rect, this.debug.tessellationSegments) await pool.prepare(element, seg) + if (this.disposed || this.pool !== pool || this.domToWebGL !== bridge) return pool.mount(element)
705-706: Jitter magic numbers still lack rationale (previously flagged).Lines 705-706 use hardcoded constants
0.0005,0.004, and0.05without explanation. The prior review requested extracting these to named constants or adding a rationale comment. Neither has been done.Apply the previously suggested fix:
+ // Jitter: 5% of element size (capped at 4mm) to avoid perfectly rigid start + // without visible crumple; min size hint 0.5mm prevents degenerate cases. const sizeHint = Math.max(0.0005, Math.min(record.widthMeters, record.heightMeters)) const jitter = Math.min(0.004, sizeHint * 0.05)Or extract constants:
+ const MIN_SIZE_HINT_M = 0.0005 + const JITTER_CAP_M = 0.004 + const JITTER_FRACTION = 0.05 + const sizeHint = Math.max(MIN_SIZE_HINT_M, Math.min(record.widthMeters, record.heightMeters)) + const jitter = Math.min(JITTER_CAP_M, sizeHint * JITTER_FRACTION) - const sizeHint = Math.max(0.0005, Math.min(record.widthMeters, record.heightMeters)) - const jitter = Math.min(0.004, sizeHint * 0.05)
970-986: Pointer radius logic still duplicatesClothBodyAdapter.getImpulseRadius(previously flagged).The new
computePointerRadiusForhelper (lines 970-986) consolidates the controller's pointer radius calculation, but the logic remains duplicated withClothBodyAdapter.getImpulseRadius(lines 310-324). Both implement the same attribute parsing, MIN/DEFAULT constants, and base/12 fallback. Future changes to impulse radius logic must be applied in both places.Extract a shared static helper or expose
getImpulseRadiusas a public method on the adapter:+ // In ClothBodyAdapter: + public getImpulseRadiusForElement(): number { + return this.getImpulseRadius() + } // In ClothSceneController.computePointerRadiusFor: private computePointerRadiusFor(item: ClothItem) { + if (item.adapter) { + return item.adapter.getImpulseRadiusForElement() + } const attr = item.element.dataset.clothImpulseRadius // ... (keep fallback for non-active items)Or extract a module-level helper used by both.
1148-1148: Fire-and-forget async calls lack explanatory comments (previously flagged).Lines 1148 and 1159 use
void this.setTessellationSegments(..., true)to intentionally discard the promise. The prior review requested a brief comment explaining the fire-and-forget pattern. The fix was not applied.Add inline comments:
+ // Fire-and-forget: tessellation updates are async but don't block UI changes void this.setTessellationSegments(this.debug.tessellationSegments, true)(Apply at both line 1148 and line 1159.)
Also applies to: 1159-1159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/clothSceneController.ts(22 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/clothSceneController.ts (1)
src/types/pinMode.ts (1)
PinMode(1-1)
| private _modelRadius = 0 | ||
| // Keep newly activated cloth awake for a short grace window to avoid | ||
| // immediate sleep when pins prevent initial center motion. | ||
| private _activationGraceFrames = 20 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract activation grace magic number to a named constant.
The hardcoded 20 frames (≈0.33s at 60 FPS) lacks semantic clarity and makes tuning harder. Extract to a constant at class or module scope with a descriptive name like ACTIVATION_GRACE_FRAMES or INITIAL_AWAKE_FRAME_COUNT.
+ private static readonly ACTIVATION_GRACE_FRAMES = 20
// Keep newly activated cloth awake for a short grace window to avoid
// immediate sleep when pins prevent initial center motion.
- private _activationGraceFrames = 20
+ private _activationGraceFrames = ClothBodyAdapter.ACTIVATION_GRACE_FRAMES📝 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.
| private _activationGraceFrames = 20 | |
| private static readonly ACTIVATION_GRACE_FRAMES = 20 | |
| // Keep newly activated cloth awake for a short grace window to avoid | |
| // immediate sleep when pins prevent initial center motion. | |
| private _activationGraceFrames = ClothBodyAdapter.ACTIVATION_GRACE_FRAMES |
🤖 Prompt for AI Agents
In src/lib/clothSceneController.ts around line 92, the private field
_activationGraceFrames is initialized with the magic number 20; extract this
literal into a clearly named constant (e.g., ACTIVATION_GRACE_FRAMES or
INITIAL_AWAKE_FRAME_COUNT) declared at module or class scope, replace the
numeric literal with that constant, and update any related comments to explain
the semantic meaning (approx. 0.33s at 60 FPS) so tuning is easier and intent is
clear.
| setWorldSleepGuardEnabled(enabled: boolean) { | ||
| this._worldSleepGuardEnabled = !!enabled | ||
| } |
There was a problem hiding this comment.
Disabling the guard mid-simulation leaves stale _worldSleeping state.
When setWorldSleepGuardEnabled(false) is called while _worldSleeping === true, the adapter continues reporting as sleeping even though the guard is no longer enforcing it. On the next update, the cloth's local sleep state may differ, causing a mismatch.
Consider resetting the flag when the guard is toggled:
setWorldSleepGuardEnabled(enabled: boolean) {
this._worldSleepGuardEnabled = !!enabled
+ if (!enabled) {
+ this._worldSleeping = false
+ }
}🤖 Prompt for AI Agents
In src/lib/clothSceneController.ts around lines 255-257, the setter only coerces
the incoming value but doesn't clear the stale _worldSleeping flag when the
guard is turned off; update setWorldSleepGuardEnabled to coerce the boolean as
now and, if disabling the guard (enabled === false) or the flag is changing from
true to false, reset this._worldSleeping = false so the adapter stops reporting
sleep after the guard is disabled.
| } | ||
|
|
||
| /** Public: convert an active cloth element back to a static DOM-backed mesh. */ | ||
| async restoreElement(element: HTMLElement) { |
There was a problem hiding this comment.
Unnecessary async keyword misleads callers.
restoreElement is declared async but never awaits anything. This misleads callers into thinking they must await the result, adds unnecessary Promise overhead, and may confuse future maintainers who expect async operations within.
Remove the async keyword:
- async restoreElement(element: HTMLElement) {
+ restoreElement(element: HTMLElement) {📝 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.
| async restoreElement(element: HTMLElement) { | |
| restoreElement(element: HTMLElement) { |
🤖 Prompt for AI Agents
In src/lib/clothSceneController.ts around line 745, the restoreElement function
is declared async but contains no await operations; remove the async keyword so
it becomes a synchronous function, update its return type from Promise<void> to
void (if explicitly typed), and search/adjust any callers to stop awaiting its
result (or leave awaits harmlessly removed) to avoid unnecessary Promise
overhead and misleading consumers.
| if (this.pool) { | ||
| this.pool.recycle(element) | ||
| this.pool.resetGeometry(element) | ||
| this.pool.mount(element) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
recycle/mount can throw; consider defensive error handling.
If pool.recycle or subsequent operations throw (e.g., geometry corruption, pool state issues), the element is left partially restored: isActive may still be true, the entity destroyed but the mesh not remounted, etc. Wrap the recycle/mount sequence in try-catch or validate pool state before proceeding.
if (this.pool) {
- this.pool.recycle(element)
- this.pool.resetGeometry(element)
- this.pool.mount(element)
+ try {
+ this.pool.recycle(element)
+ this.pool.resetGeometry(element)
+ this.pool.mount(element)
+ } catch (error) {
+ console.error('Failed to restore element geometry', error)
+ // Element remains in inconsistent state; caller should handle
+ }
}🤖 Prompt for AI Agents
In src/lib/clothSceneController.ts around lines 770-774, the sequence calling
this.pool.recycle(element); this.pool.resetGeometry(element);
this.pool.mount(element) can throw and leave the element in a partial state;
wrap the recycle/resetGeometry/mount sequence in a try-catch and validate pool
state before invoking, and in the catch perform deterministic cleanup/rollback
(ensure element.isActive is set to false, restore or fully remove the
mesh/entity, and avoid partially mounted state), then rethrow or log the error
with context; additionally, consider checking that this.pool and the three
methods exist before calling to avoid TypeErrors.
…ep tests synchronous in MODE=test; ensure uniform scale post-rebuild; preserve overlay refresh
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/clothSceneController.ts (1)
1127-1155: Potential race condition in asyncsetTessellationSegments.If
setTessellationSegmentsis called multiple times rapidly (e.g., user dragging a slider), concurrentprepareoperations on the same elements can interleave, potentially causing geometry corruption or inconsistent state. Consider adding a guard (e.g., abort previous operation, debounce, or lock) to prevent concurrent updates.Consider adding a simple guard:
+ private _tessellationUpdateInProgress = false + async setTessellationSegments(segments: number, force = false) { const pool = this.pool if (!pool) return const clamped = Math.max(1, Math.min(segments, 32)) if (!force && this.debug.tessellationSegments === clamped) return + if (this._tessellationUpdateInProgress) return + this._tessellationUpdateInProgress = true this.debug.tessellationSegments = clamped // ... existing logic ... await Promise.all(tasks) + this._tessellationUpdateInProgress = false this.collisionSystem.refresh() }
♻️ Duplicate comments (10)
src/lib/clothSceneController.ts (10)
92-92: UNRESOLVED: Activation grace magic number lacks clarity.The hardcoded
20frames (≈0.33s at 60 FPS) lacks semantic clarity and makes tuning harder. Extract to a named constant at class or module scope.Apply this diff:
+ private static readonly ACTIVATION_GRACE_FRAMES = 20 // Keep newly activated cloth awake for a short grace window to avoid // immediate sleep when pins prevent initial center motion. - private _activationGraceFrames = 20 + private _activationGraceFrames = ClothBodyAdapter.ACTIVATION_GRACE_FRAMESBased on past review comments.
758-758: UNRESOLVED: Unnecessaryasynckeyword misleads callers.
restoreElementis declaredasyncbut never awaits anything. This misleads callers into thinking they must await the result, adds unnecessary Promise overhead, and may confuse future maintainers who expect async operations within.Remove the
asynckeyword:- async restoreElement(element: HTMLElement) { + restoreElement(element: HTMLElement) {Based on past review comments.
215-215: UNRESOLVED: Misleading comment about radius.This comment is confusing because the preceding block (lines 183-214) only uses
worldCenter; radius is never referenced here. The comment should clarify that it refers to the_modelRadiusfield being constant across frames, or be removed entirely.Apply this diff:
- // Radius is constant; no need to update here + // (_modelRadius is precomputed and constant; only center is tracked for motion)Or simply remove the comment if it adds no value to this block.
Based on past review comments.
639-671: UNRESOLVED: Missing disposal guard inclothifyafterawait.The
clothifymethod lacks the disposal guard thatprepareElementshas afterawait pool.prepare(...). If the controller is disposed or the pool is swapped whileprepareawaits, lines 651-669 will still execute on dead state (callingmount, adding to collision system, etc.), potentially causing crashes or memory leaks.Apply this diff:
const seg = this.computeAutoSegments(rect, this.debug.tessellationSegments) await pool.prepare(element, seg) + if (this.disposed || this.pool !== pool || this.domToWebGL !== bridge) return pool.mount(element)Based on past review comments.
783-787: UNRESOLVED:recycle/mountcan throw; needs defensive error handling.If
pool.recycle,resetGeometry, ormountthrow (e.g., geometry corruption, pool state issues), the element is left partially restored:isActivemay still be true, the entity destroyed but the mesh not remounted, etc. Wrap the sequence in try-catch or validate pool state before proceeding.Apply this diff:
if (this.pool) { - this.pool.recycle(element) - this.pool.resetGeometry(element) - this.pool.mount(element) + try { + this.pool.recycle(element) + this.pool.resetGeometry(element) + this.pool.mount(element) + } catch (error) { + console.error('Failed to restore element geometry', error) + // Element remains in inconsistent state; caller should handle + } }Based on past review comments.
255-257: UNRESOLVED: Disabling the guard leaves stale_worldSleepingstate.When
setWorldSleepGuardEnabled(false)is called while_worldSleeping === true, the adapter continues reporting as sleeping even though the guard is no longer enforcing it, causing a potential mismatch on the next update.Apply this diff:
setWorldSleepGuardEnabled(enabled: boolean) { this._worldSleepGuardEnabled = !!enabled + if (!enabled) { + this._worldSleeping = false + } }Based on past review comments.
1157-1163: UNRESOLVED: Fire-and-forget async call lacks explanatory comment.Line 1161 uses
void this.setTessellationSegments(...)as an intentional fire-and-forget call, but this is subtle and lacks a comment explaining why the promise is deliberately discarded.Add a brief comment:
+ // Fire-and-forget: tessellation updates are async but don't block caller void this.setTessellationSegments(this.debug.tessellationSegments, true)Based on past review comments.
716-720: UNRESOLVED: Jitter constants lack rationale.Lines 718-719 use magic numbers
0.0005,0.004, and0.05without explaining why 5% of element size capped at 4mm is the chosen perturbation. While the logic is sound, future maintainers won't know if these values were empirically tuned or arbitrary.Extract named constants:
+ const MIN_JITTER_M = 0.0005 + const MAX_JITTER_M = 0.004 + const JITTER_FRACTION = 0.05 + // Jitter: 5% of element size (capped at 4mm) to avoid perfectly rigid start + // without visible crumple; min size hint 0.5mm prevents degenerate cases. - const sizeHint = Math.max(0.0005, Math.min(record.widthMeters, record.heightMeters)) - const jitter = Math.min(0.004, sizeHint * 0.05) + const sizeHint = Math.max(MIN_JITTER_M, Math.min(record.widthMeters, record.heightMeters)) + const jitter = Math.min(MAX_JITTER_M, sizeHint * JITTER_FRACTION)Based on past review comments.
1165-1174: UNRESOLVED: Fire-and-forget async call lacks explanatory comment.Line 1172 uses
void this.setTessellationSegments(...)as an intentional fire-and-forget call, but this is subtle and lacks a comment explaining why the promise is deliberately discarded.Add a brief comment:
+ // Fire-and-forget: tessellation updates are async but don't block caller void this.setTessellationSegments(this.debug.tessellationSegments, true)Based on past review comments.
112-137: UNRESOLVED: Empty catch block at line 133 violates lint and masks failures.This critical issue was flagged in a previous review but remains unaddressed. The empty
catchat line 133 will break theno-emptylint rule and silently swallows exceptions that could indicate malformed geometry or missing properties, making debugging impossible.Apply this diff:
- } catch { + } catch (error) { + console.warn(`ClothBodyAdapter ${id}: failed to compute model radius from geometry`, error) const w = this.record.widthMeters ?? 0 const h = this.record.heightMeters ?? 0 this._modelRadius = 0.5 * Math.max(w, h, 0.001)Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/clothSceneController.ts(22 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/clothSceneController.ts (1)
src/types/pinMode.ts (1)
PinMode(1-1)
🔇 Additional comments (2)
src/lib/clothSceneController.ts (2)
1054-1062: Excellent: Wake-on-gravity-change implemented correctly.The addition of
item.cloth.wake()after setting gravity (line 1060) ensures that sleeping cloth bodies react immediately to gravity changes. This is exactly the right behavior and matches the PR objectives.
963-999: Good refactor: Pointer radius helper eliminates duplication.The extraction of
computePointerRadiusFor(lines 982-999) and its use inupdateOverlayDebug(lines 963-979) addresses the previous review comment about duplicated radius calculation logic. The implementation correctly mirrorsClothBodyAdapter.getImpulseRadius().
…300ms cool‑off) with force prepare; disabled in tests; overlay refresh on update
…ndering; emit newDomObjectGeo/clothActivated events; capture-first swap to avoid flicker
… toggle, and layout-settled recapture
…d dev events (PR #32)
…‑uniform scale); compute true local bounding spheres (half‑diagonal); publish world-space spheres from local via WorldBody; keep legacy scaling in MODE=test; fix overlay guards
…lse); set wireframe on static meshes immediately; capture debug panel as static while open to draw AABB/spheres/wireframe; guard null record; all tests+build green
…c overlay without capturing; derive overlay spheres from AABB half‑diagonal; remove accidental controller any-calls; preserve pointer interactivity
…es until activation (runtime); keep old semantics in MODE=test; layout observer only re-bakes active cloth; tessellation re-bakes active cloth at runtime but all in tests; overlays compute world spheres from AABB half‑diagonal; depthTest=false for gizmos
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/engine/render/DebugOverlaySystem.ts (1)
40-66: Dispose the sphere overlay group as well.
dispose()tears down the pointer, AABB, pin, and sleep-circle groups, but the newly introducedsphereGroupis never cleared or removed from the scene. After toggling the static sphere overlay once, you leak its geometry/materials and leave the group attached across detach/re-init cycles. Mirror the existing teardown pattern so the group is disposed and unhooked.if (this.circleGroup) { this.clearOverlayGroup(this.circleGroup) this.view.scene?.remove(this.circleGroup) this.circleGroup = undefined } + if (this.sphereGroup) { + this.clearOverlayGroup(this.sphereGroup) + this.view.scene?.remove(this.sphereGroup) + this.sphereGroup = undefined + }src/lib/clothSceneController.ts (2)
604-612: Auto tessellation caps never exceed the manual slider.Every call to
computeAutoSegmentsstill passesthis.debug.tessellationSegments, andsetTessellationSegmentsclamps that value to ≤32. As a result, the newtessellationMaxslider (up to 48) is ignored—auto mode can never produce more than 32 segments. HavecomputeAutoSegmentsderive its cap fromtessellationMaxwhen auto mode is active so the new bounds actually apply.- private computeAutoSegments(rect: DOMRect, maxCap = this.debug.tessellationSegments) { + private computeAutoSegments(rect: DOMRect, maxCap?: number) { const clamp = (val: number, min: number, max: number) => Math.max(min, Math.min(max, val)) const round = (n: number) => Math.round(n) // Auto off → return the exact configured segments (clamped) - if (!this.debug.autoTessellation) { - return clamp(round(this.debug.tessellationSegments), 1, 48) - } + const manualCap = maxCap ?? this.debug.tessellationSegments + if (!this.debug.autoTessellation) { + return clamp(round(manualCap), 1, 48) + } @@ - const MIN_SEGMENTS = clamp(round(this.debug.tessellationMin ?? 6), 1, 46) - const MAX_TESSELLATION_CAP = 48 - const rawMax = round(maxCap) - const maxUser = clamp(round(this.debug.tessellationMax ?? maxCap), MIN_SEGMENTS + 2, MAX_TESSELLATION_CAP) - // Effective max is the lesser of: user-configured max and the provided cap, both bounded by global limits - const MAX = Math.min(clamp(rawMax, MIN_SEGMENTS + 2, MAX_TESSELLATION_CAP), maxUser) + const autoCap = this.debug.tessellationMax ?? manualCap + const MIN_SEGMENTS = clamp(round(this.debug.tessellationMin ?? 6), 1, 46) + const MAX_TESSELLATION_CAP = 48 + const rawMax = round(autoCap) + const maxUser = clamp(round(autoCap), MIN_SEGMENTS + 2, MAX_TESSELLATION_CAP) + const MAX = Math.min(clamp(rawMax, MIN_SEGMENTS + 2, MAX_TESSELLATION_CAP), maxUser)(With this change, the existing call sites can keep passing the manual slider value; auto mode now honours the separate max cap.)
528-583: Dispose must disconnect the layout observer / timers.
layoutObserverandrecaptureTimerssurvivedispose(), so layout callbacks and delayed recaptures keep firing against a torn-down controller. Disconnect the observer and clear the pending timers when disposing to avoid dangling callbacks.for (const item of this.items.values()) { if (item.releasePinsTimeout !== undefined) { clearTimeout(item.releasePinsTimeout) @@ - this.items.clear() - this.domToWebGL = null - this.pool = null + if (this.layoutObserver) { + this.layoutObserver.disconnect() + this.layoutObserver = null + } + for (const timer of this.recaptureTimers.values()) { + clearTimeout(timer) + } + this.recaptureTimers.clear() + this.items.clear() + this.domToWebGL = null + this.pool = null
♻️ Duplicate comments (5)
src/App.tsx (1)
531-536: Still callingsetPointerOverlayVisibletwice.The duplicate
useEffectonpointerColliderVisiblesurvived the refactor, so every toggle still hits the controller twice. Please drop the redundant block as previously discussed.Also applies to: 600-602
src/engine/render/DebugOverlaySystem.ts (1)
105-112: Replace the empty catch with explicit handling.
catch { /* no-op */ }is still an empty block, so eslint (no-empty) will flag it and any genuinegetViewportPixelsfailure is silently ignored. Capture the error and either log or explicitly consume it so lint passes and diagnostics aren’t lost.- } catch { /* no-op */ } + } catch (error) { + if (import.meta?.env?.MODE !== 'test') { + console.debug('DebugOverlaySystem: getViewportPixels failed', error) + } + }src/lib/clothSceneController.ts (3)
256-258: Reset world-sleep state when disabling the guard.
setWorldSleepGuardEnabled(false)leaves_worldSleepingand_worldStillFramesunchanged, soisSleeping()keeps reportingtrueeven after you turn the guard off. Clear the cached state when disabling the guard (and re-arm the grace window if needed) so the adapter reflects live cloth state.setWorldSleepGuardEnabled(enabled: boolean) { - this._worldSleepGuardEnabled = !!enabled + const next = !!enabled + this._worldSleepGuardEnabled = next + if (!next) { + this._worldSleeping = false + this._worldStillFrames = 0 + } }
719-723: Empty catch still violates lint and hides failures.The new activation path reintroduced
catch { /* ignore unmounted */ }.no-emptywill flag this, and ifpool.recycleis throwing for a real reason you’ll never see it. Capture the error (log or bail) instead of swallowing it.- try { this.pool.recycle(element) } catch { /* ignore unmounted */ } + try { + this.pool.recycle(element) + } catch (error) { + console.warn('pool.recycle during activation failed', error) + }
664-692: Guard against disposal afterprepare()resolves.
clothifyawaitspool.prepare, but if the controller is disposed (or the pool/view swaps) while that promise is in flight, the subsequentpool.mount, collision registration, andthis.items.setrun against torn-down state. You fixed this race inprepareElements; the manual path needs the same guard.async clothify(element: HTMLElement, options: { activate?: boolean; addClickHandler?: boolean } = {}) { @@ - await pool.prepare(element, seg, { reason: 'manual' }) + await pool.prepare(element, seg, { reason: 'manual' }) + if (this.disposed || this.pool !== pool || this.domToWebGL !== bridge) { + return + } pool.mount(element)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
BLOG_NOTES.mdis excluded by!*.md
📒 Files selected for processing (6)
src/App.tsx(19 hunks)src/engine/render/DebugOverlayState.ts(2 hunks)src/engine/render/DebugOverlaySystem.ts(5 hunks)src/lib/clothSceneController.ts(26 hunks)src/lib/domToWebGL.ts(3 hunks)src/lib/elementPool.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/lib/domToWebGL.ts (1)
src/lib/units.ts (2)
toCanonicalWidthMeters(21-24)toCanonicalHeightMeters(26-29)
src/App.tsx (5)
src/lib/clothSceneController.ts (6)
PinMode(25-25)setConstraintIterations(260-262)setConstraintIterations(1161-1167)setSubsteps(1151-1158)setPinMode(1175-1189)setRealTime(1142-1148)src/lib/clothPhysics.ts (2)
setConstraintIterations(70-73)setSubsteps(75-78)src/engine/debug/engineActions.ts (4)
setConstraintIterations(91-93)setSubsteps(60-62)setPinMode(125-127)setRealTime(50-52)src/engine/simulationRunner.ts (2)
setSubsteps(73-77)setRealTime(66-70)src/engine/render/DebugOverlayState.ts (1)
DebugOverlayState(15-38)
src/lib/clothSceneController.ts (1)
src/types/pinMode.ts (1)
PinMode(1-1)
🔇 Additional comments (3)
src/lib/domToWebGL.ts (2)
20-21: Field addition looks correct.The documentation is adequate, though "center at origin" could be more explicit about which coordinate space (model/local vs world). The field serves its purpose for bounding sphere calculations.
158-159: Bounding sphere calculation is mathematically sound.The use of
Math.hypotto compute the diagonal and halving it for the radius is correct for a plane centered at the origin.src/lib/elementPool.ts (1)
20-22: ****The review comment misdiagnoses the code. The early-return optimization on line 22 does not create a footgun.
The three
prepare()calls withoutforce(lines 640, 671, 711) are all first captures where no prior record exists. The early return check—if (existing && existing.segments === segments && !options.force)—never fires becauseexistingis falsy. These calls always execute the full preparation path regardless of theforceflag.The two calls with
force: true(lines 722, 1215) occur in re-preparation scenarios where the element already has a prior record:
- Line 722: After
pool.recycle(element)during non-test activation- Line 1215: During tessellation updates of active cloth elements
The developers already understand the semantics. The early return only prevents redundant work when re-calling
prepare()on an already-prepared element with identical segment counts—a valid optimization. Content-changing scenarios correctly useforce: true.No documentation update or caller audit is needed.
Likely an incorrect or invalid review comment.
| useEffect(() => { | ||
| controllerRef.current?.setTessellationAutoEnabled?.(autoTessellation) | ||
| }, [autoTessellation]) | ||
|
|
||
| useEffect(() => { | ||
| controllerRef.current?.setTessellationMinMax?.(tessellationMin, tessellationMax) | ||
| }, [tessellationMin, tessellationMax]) | ||
|
|
||
| useEffect(() => { | ||
| controllerRef.current?.setWorldSleepGuardEnabled?.(worldSleepGuard) | ||
| }, [worldSleepGuard]) |
There was a problem hiding this comment.
Initial auto/tessellation/world-guard settings never reach the controller.
These effects run before controllerRef.current is populated, so the initial values (especially if a consumer overrides the defaults) are silently ignored. Once the controller finishes init(), the dependencies haven’t changed, so the setters never fire. Push the initial sync into the init promise (alongside the gravity/zoom setup) so the controller is always brought up to date, then keep these effects for subsequent user changes.
actionsRef.current = actions
+ controller.setTessellationAutoEnabled(autoTessellation)
+ controller.setTessellationMinMax(tessellationMin, tessellationMax)
+ controller.setWorldSleepGuardEnabled(worldSleepGuard)
@@
useEffect(() => {
controllerRef.current?.setTessellationAutoEnabled?.(autoTessellation)
}, [autoTessellation])(Apply the same inside the init() block for tessellationMin/tessellationMax and worldSleepGuard so initial overrides are honored.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/App.tsx around lines 588 to 598, the useEffect calls that call
controllerRef.current setters run before the controller is initialized so
initial values can be missed; move the initial sync of autoTessellation,
tessellationMin/tessellationMax, and worldSleepGuard into the controller init()
promise (the same place where gravity/zoom are set) so the controller receives
the initial/overridden values when it is created, and leave the existing
useEffect hooks in place to handle subsequent changes.
| // In tests, keep legacy behavior (apply DOM aspect via world scale) to satisfy existing specs. | ||
| const mode = (typeof import.meta !== 'undefined' ? (import.meta as unknown as { env?: Record<string, string | undefined> }).env?.MODE : undefined) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract the import.meta.env.MODE pattern to eliminate duplication.
This type assertion pattern (import.meta as unknown as { env?: Record<string, string | undefined> }).env?.MODE appears at line 46 and line 189. Extract it to a private helper method or a utility function.
Apply this diff to add a helper:
+ private getEnvironmentMode(): string | undefined {
+ const envObj = (typeof import.meta !== 'undefined'
+ ? (import.meta as unknown as { env?: Record<string, string | undefined> }).env
+ : undefined)
+ return envObj?.MODE
+ }
+
constructor(container: HTMLElement, options: DOMToWebGLOptions = {}) {
this.container = container
- const envObj = (typeof import.meta !== 'undefined' ? (import.meta as unknown as { env?: Record<string, string | undefined> }).env : undefined)
- const envMode = envObj?.VITE_H2C_INTERCEPT as 'on' | 'off' | 'auto' | undefined
+ const envMode = this.getEnvironmentMode()?.VITE_H2C_INTERCEPT as 'on' | 'off' | 'auto' | undefined
this.interceptMode = (options.interceptHtml2CanvasWrites ?? envMode ?? 'on')And update line 189:
- const mode = (typeof import.meta !== 'undefined' ? (import.meta as unknown as { env?: Record<string, string | undefined> }).env?.MODE : undefined)
+ const mode = this.getEnvironmentMode()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/domToWebGL.ts around lines 46 and 188-189, the repeated
type-assertion pattern (import.meta as unknown as { env?: Record<string, string
| undefined> }).env?.MODE should be extracted to a small private helper; add a
function like getImportMetaMode(): string | undefined near the top of the module
that safely returns that MODE value using the same type assertion once, then
replace both occurrences with calls to getImportMetaMode(); ensure the helper is
exported only if needed, preserves the exact undefined behavior, and keeps
TypeScript happy with the same type signature.
| if (mode === 'test') { | ||
| const scaleX = widthMeters / (record.baseWidthMeters || 1) | ||
| const scaleY = heightMeters / (record.baseHeightMeters || 1) | ||
| record.worldBody.setScaleComponents(scaleX, scaleY, 1) | ||
| } else { | ||
| // Runtime: do not map DOM aspect changes into world scale. | ||
| record.worldBody.setScaleComponents(1, 1, 1) | ||
| } | ||
| record.worldBody.applyToMesh() | ||
| // Keep meters updated for consumers; geometry is rebuilt on layout-settled. | ||
| record.widthMeters = widthMeters | ||
| record.heightMeters = heightMeters | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Document the rationale for mode-based transform divergence and verify widthMeters/heightMeters semantics.
This mode-based branching introduces a fundamental behavioral difference between test and production environments:
- Test mode: DOM aspect changes are mapped to world scale, keeping geometry fixed.
- Runtime mode: World scale is locked to (1, 1, 1), ignoring DOM aspect changes.
Critical concerns:
- Why does runtime ignore DOM aspect? The comment on line 197 states the behavior but not the rationale. What problem does this solve? Does it prevent unintended geometry deformation? Is it an optimization? Document this.
- Semantic confusion: Lines 201-203 update
widthMetersandheightMetersin both modes, but in runtime mode these values are computed from the DOM yet not applied to scale. The comment says "keep meters updated for consumers" — which consumers? Do they expect these values to match the visual scale, or are they used elsewhere (e.g., for physics bounds)? This asymmetry is a trap for future maintainers. - Test/production divergence risk: Divergent behavior between test and production can mask bugs. If a consumer relies on
widthMeters/heightMetersmatching visual dimensions, tests will pass but production will break.
Actions required:
- Add a comprehensive comment block above line 186 explaining:
- Why runtime mode decouples DOM aspect from world scale
- What
widthMeters/heightMetersrepresent in each mode - Which consumers rely on these values and how
- Verify that consumers of
widthMeters/heightMeters(e.g., physics, collision) handle both modes correctly. Search the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of widthMeters/heightMeters from DOMMeshRecord to verify they handle both scale modes.
# Search for access to widthMeters or heightMeters on record objects
rg -n --type=ts -C 3 '\b(record|mesh|element)\.(widthMeters|heightMeters)\b'Length of output: 5760
Fix test/production parity trap: mode-based meters divergence causes physics bugs in production.
The search confirms the original concern is not hypothetical—this is a latent defect:
- clothSceneController.ts (lines 125–136, 304, 746) uses
widthMeters/heightMetersfor bounding sphere radius, cloth scene bounds, and perturbation sizing. These are physics/collision calculations. - Runtime mode breaks the contract: mesh visual scale is (1, 1, 1), but
widthMeters/heightMetersreflect full DOM dimensions. Consumers assume meters match visual scale. - Tests hide the bug: test mode applies proportional scale, so tests pass. Production fails silently with misaligned physics bounds.
- No mode awareness in consumers: clothSceneController and other callers have zero guards or conditional logic. They blindly trust the values.
Required fixes:
-
Document the divergence before line 186: explain why runtime locks scale to (1,1,1), what
widthMeters/heightMetersrepresent in each mode, and why clothSceneController's physics logic isn't broken by this mismatch. -
Audit clothSceneController.ts (lines 125–136, 304, 746): verify physics bounds calculations are correct when visual scale ≠ meters in runtime mode. If not, either:
- Use visual scale components (1,1,1) instead of meters for physics bounds in runtime mode, OR
- Apply the same scaling to physics bounds in runtime mode, OR
- Rethink the test/runtime split to maintain invariants
-
Add mode-aware assertions or guards in consumers or in the record update code to catch future maintenance mistakes.
🤖 Prompt for AI Agents
In src/lib/domToWebGL.ts around lines 192 to 204 (and add doc before line 186),
the runtime path sets mesh scale to (1,1,1) while still updating
record.widthMeters/heightMeters, creating a test/production parity gap that
breaks physics consumers; update the file by (1) adding a short documented note
before line 186 explaining why runtime locks visual scale to (1,1,1), what
widthMeters/heightMeters represent in test vs runtime, and the intended
invariant (or intentional divergence), (2) change the update logic or add a
clear mode-aware mapping so consumers get the correct units — either keep meters
consistent with visual scale in runtime (apply the same scale to
record.widthMeters/heightMeters) or explicitly flag the record as
"logicalMeters" vs "visualScale" so callers can choose, and (3) add mode-aware
runtime assertions/guards here (and/or helper functions) so consumers like
clothSceneController.ts can detect mismatch early; also audit
clothSceneController.ts at lines 125–136, 304, and 746 and update its
bounding/physics calculations to use the correct metric (visual size when
physics is visual-scale-based or widthMeters when physics expects layout
meters), or add assertions there to prevent future regressions.
| // Capture first to avoid visible gaps, then swap the record. | ||
| const texture = await this.domBridge.captureElement(element) | ||
| const record = this.domBridge.createMesh(element, texture, segments) | ||
|
|
||
| // If an old record exists, remove its mesh only after we have a new one. | ||
| if (existing) { | ||
| if (this.mounted.has(element)) { | ||
| this.domBridge.removeMesh(existing.mesh) | ||
| this.mounted.delete(element) | ||
| } | ||
| this.domBridge.disposeMesh(existing) | ||
| } |
There was a problem hiding this comment.
The "avoid visible gaps" comment is misleading — gaps can still occur.
Line 24 claims to "avoid visible gaps," but the implementation doesn't guarantee this:
- Line 31: The old mesh is removed via
this.domBridge.removeMesh(existing.mesh). - Missing mount: The new mesh is created (line 26) but not yet added to the scene. It's stored in the
elementsmap (line 37), but mounting happens in a separatemount()call. - Gap window: Between removal (line 31) and the eventual
mount()call, the element is not visible on screen.
This gap may be brief, but it contradicts the comment. If the goal is truly to avoid gaps, you must mount the new mesh before removing the old one:
- // Capture first to avoid visible gaps, then swap the record.
+ // Capture and create new mesh first, but gaps may still occur until mount() is called.
const texture = await this.domBridge.captureElement(element)
const record = this.domBridge.createMesh(element, texture, segments)
- // If an old record exists, remove its mesh only after we have a new one.
+ // If an old record exists and is mounted, swap atomically.
if (existing) {
if (this.mounted.has(element)) {
+ this.domBridge.addMesh(record.mesh)
+ this.domBridge.updateMeshTransform(element, record)
this.domBridge.removeMesh(existing.mesh)
- this.mounted.delete(element)
+ // Keep element in mounted set since we immediately re-mounted
}
this.domBridge.disposeMesh(existing)
}Alternatively, if caller-controlled mounting is intentional, update the comment to reflect reality.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/elementPool.ts around lines 24 to 35, the comment "Capture first to
avoid visible gaps" is incorrect because the new mesh is created but not mounted
before the old mesh is removed, creating a visibility gap; fix by either (A)
truly avoiding gaps: after creating the new record call the DOM bridge to mount
the new mesh and mark the element as mounted (e.g.,
this.domBridge.mountMesh(record.mesh); this.mounted.add(element)) before calling
removeMesh/disposeMesh on the existing record, then continue storing the new
record; or (B) keep caller-controlled mounting but update the comment to
accurately state that creation happens before removal but mounting is performed
later by mount(), so gaps may occur — implement one of these two options and
remove the misleading claim.
| // Emit a browser event for debugging / observability | ||
| try { | ||
| if (typeof window !== 'undefined' && 'dispatchEvent' in window) { | ||
| const detail = { | ||
| elementId: element.id || null, | ||
| widthMeters: record.widthMeters, | ||
| heightMeters: record.heightMeters, | ||
| baseWidthMeters: record.baseWidthMeters, | ||
| baseHeightMeters: record.baseHeightMeters, | ||
| segments: record.segments, | ||
| reason: options.reason ?? 'prepare', | ||
| time: Date.now(), | ||
| } | ||
| window.dispatchEvent(new CustomEvent('newDomObjectGeo', { detail })) | ||
| const mode = (import.meta as unknown as { env?: Record<string, string> }).env?.MODE | ||
| if (mode !== 'test') { | ||
| // eslint-disable-next-line no-console | ||
| console.info('[newDomObjectGeo]', detail) | ||
| } | ||
| } | ||
| } catch { | ||
| // ignore if CustomEvent/window not available | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a configuration flag for observability logging and consider exposing silent failures.
Two concerns with the observability block:
- No debug flag for console.info (line 56): The
console.infocall runs in all non-test environments with no way to disable it. In a production app with frequentprepare()calls, this could spam logs. Add a runtime configuration option:
- if (getEnvironmentMode() !== 'test') {
+ const shouldLog = getEnvironmentMode() !== 'test' && this.enableObservabilityLogs
+ if (shouldLog) {
// eslint-disable-next-line no-console
console.info('[newDomObjectGeo]', detail)
}And add a field to the class:
export class ElementPool {
+ private enableObservabilityLogs = false // or read from config/options
private domBridge: Pick<...>- Silent error swallowing (lines 59-61): Errors during event dispatch are caught and ignored. While this prevents crashes, it also hides genuine issues (e.g., broken event listeners). Consider:
- Logging the error even if the event fails
- Or exposing a callback/observable for observability failures
- Or at minimum, document that observability failures are intentionally silent
Committable suggestion skipped: line range outside the PR's diff.
| const mode = (import.meta as unknown as { env?: Record<string, string> }).env?.MODE | ||
| if (mode !== 'test') { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract the MODE check pattern — third instance of the same duplication.
This is the third instance of the import.meta.env?.MODE pattern in this PR (also in domToWebGL.ts lines 46 and 189). Extract this to a shared utility module:
Create src/lib/envUtils.ts:
/**
* Get the current environment mode (e.g., 'test', 'development', 'production').
* Safe to call in any environment; returns undefined if import.meta is unavailable.
*/
export function getEnvironmentMode(): string | undefined {
if (typeof import.meta === 'undefined') return undefined
const env = (import.meta as unknown as { env?: Record<string, string | undefined> }).env
return env?.MODE
}Then use it here:
+import { getEnvironmentMode } from './envUtils'
+
// ... in prepare method ...
- const mode = (import.meta as unknown as { env?: Record<string, string> }).env?.MODE
- if (mode !== 'test') {
+ if (getEnvironmentMode() !== 'test') {
// eslint-disable-next-line no-console
console.info('[newDomObjectGeo]', detail)
}And update domToWebGL.ts to use the same utility.
🤖 Prompt for AI Agents
In src/lib/elementPool.ts around lines 53-54 the inline import.meta.env MODE
check is duplicated across the PR; extract this pattern to a shared utility by
adding src/lib/envUtils.ts with a getEnvironmentMode() function that safely
reads import.meta?.env?.MODE (handling undefined import.meta), then replace the
inline check in elementPool.ts with a call to getEnvironmentMode(), and update
domToWebGL.ts (lines ~46 and ~189) to use the same utility to remove duplication
and centralize environment detection.
… remove direct DOMRect math in collision path; keep viewport-based local sizing only; unify overlay/collision pipelines
…y when enabled; remove unused eslint-disable comments; tests/build green
…gins; add getAABB to ClothPhysics/ClothBodyAdapter; keep sphere path available; tests/build green
…umb broad‑phase controls via Controller/EngineActions; overlay renders sim and fat AABBs; tests/build green
…n in transforms; computeBoundingBox each sync for accurate dynamic AABB; keep tests/build green
…TA + email input as cloth; subscribe button as rigid dynamic; titles/text/list as rigid static; add RigidBody2D + adapter and lazy activation; keep AABB math unified and WorldBody rotation supported; lint/tests/build green
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engine/render/DebugOverlaySystem.ts (1)
51-67: Clean up every overlay group you allocate.
sphereGroupandsimAabbGroupare created below but never torn down here, so disposing the system leaks their geometries/materials and leaves them attached to the scene graph. Add the sameclearOverlayGroup/scene.removedance you already do for the other groups.Apply this diff:
if (this.circleGroup) { this.clearOverlayGroup(this.circleGroup) this.view.scene?.remove(this.circleGroup) this.circleGroup = undefined } + if (this.sphereGroup) { + this.clearOverlayGroup(this.sphereGroup) + this.view.scene?.remove(this.sphereGroup) + this.sphereGroup = undefined + } + if (this.simAabbGroup) { + this.clearOverlayGroup(this.simAabbGroup) + this.view.scene?.remove(this.simAabbGroup) + this.simAabbGroup = undefined + }src/App.tsx (1)
499-530: Initial tessellation/world-guard/broadphase settings still never reach the controller.All three
useEffecthooks below fire beforecontrollerRef.currentis set, so their initial values are dropped on the floor. Onceinit()resolves the controller is in sync limbo unless the user toggles something. Push the initial calls into this block right after you create the controller/actions; keep the effects for later changes.Inside this
init().thenadd:actionsRef.current = actions // existing setup… + controller.setTessellationAutoEnabled(autoTessellation) + controller.setTessellationMinMax(tessellationMin, tessellationMax) + controller.setWorldSleepGuardEnabled(worldSleepGuard) + controller.setBroadphaseMode(broadphaseMode)…and do the same for
actionsRef.current?.setBroadphaseModeif you need the EngineActions path too.
♻️ Duplicate comments (7)
src/lib/elementPool.ts (3)
24-35: UNRESOLVED: The "avoid visible gaps" claim is STILL false.This is the exact issue flagged in the previous review and it remains unfixed. The comment on line 24 claims to "avoid visible gaps," but the implementation guarantees them:
- Line 26: New mesh created but not mounted
- Line 31: Old mesh removed immediately
- Line 37: New record stored, but mounting happens only when caller invokes
mount()(lines 67-75)Result: A visibility gap exists from line 31 until the eventual
mount()call.The comment on line 28 is also imprecise—it says "remove its mesh only after we have a new one," but the reality is more nuanced: you remove only if mounted, and the new mesh isn't mounted yet.
Either:
- Fix the gap: Call
this.domBridge.addMesh(record.mesh)andthis.mounted.add(element)before line 31, or- Fix the comment: Replace line 24 with
// Create the new mesh first; caller must mount() to make it visibleand line 28 with// Remove old mesh from scene (if mounted) before disposing
39-60: CRITICAL: Silent failures, no logging controls, unsafe type casts, and debugging nightmares.This observability block has cascading issues—some already flagged in previous reviews and still unresolved:
1. UNRESOLVED: No configuration flag for console.info (line 55)
The previous review correctly noted thatconsole.inforuns unconditionally in all non-test environments. In production with frequentprepare()calls, this spams logs. Add a runtime flag:private enableObservabilityLogs = false // or inject via constructor options2. UNRESOLVED: Silent error swallowing is catastrophic (lines 58-60)
The emptycatchhides genuine failures (broken event listeners, malformed detail objects, etc.). At minimum:} catch { - // ignore if CustomEvent/window not available + // ignore if CustomEvent/window not available + if (getEnvironmentMode() !== 'test' && getEnvironmentMode() !== 'production') { + console.warn('[elementPool] Failed to dispatch newDomObjectGeo event', err) + } }3. NEW CRITICAL: Unsafe type cast (line 53)
(import.meta as unknown as { env?: Record<string, string> })assumesMODEis always a string. What if it's a number or undefined? This can explode. Use a safe utility (see line 53-54 comment).4. NEW ISSUE: Redundant check (line 41)
'dispatchEvent' in windowis pointless—ifwindowexists, it always hasdispatchEvent. Remove it:-if (typeof window !== 'undefined' && 'dispatchEvent' in window) { +if (typeof window !== 'undefined') {5. NEW MINOR: Unclear nullification (line 43)
Whyelement.id || null? If the goal is to avoid empty strings, document it. Otherwise, just useelement.id.
53-56: UNRESOLVED: MODE check duplication—third instance across the PR.As flagged in the previous review, this inline
import.meta.env?.MODEpattern appears three times in this PR:
- Here (lines 53-54)
domToWebGL.tsline ~46domToWebGL.tsline ~189Extract this to a shared utility immediately:
Create
src/lib/envUtils.ts:/** * Get the current environment mode (e.g., 'test', 'development', 'production'). * Safe to call in any environment; returns undefined if import.meta is unavailable. */ export function getEnvironmentMode(): string | undefined { if (typeof import.meta === 'undefined') return undefined const env = (import.meta as unknown as { env?: Record<string, string | undefined> }).env return env?.MODE }Then here:
+import { getEnvironmentMode } from './envUtils' + - const mode = (import.meta as unknown as { env?: Record<string, string> }).env?.MODE - if (mode !== 'test') { + if (getEnvironmentMode() !== 'test') { // eslint-disable-next-line no-console console.info('[newDomObjectGeo]', detail) }And update both
domToWebGL.tssites.src/App.tsx (1)
147-160: Overlay effect still resubscribes every render and never cleans up.Depending on the whole
propsobject re-runs this effect on every render, spammingaddOverlayElement, and there’s still no cleanup when the panel unmounts open—stale colliders linger forever. Narrow the deps, log restore failures, and always unregister in a return callback.Try:
- useEffect(() => { - const el = panelRef.current - if (!el) return - if (open && props.restoreElement) { - // Ensure any previously clothified panel is restored to DOM for interactivity - Promise.resolve(props.restoreElement(el)).catch(() => {}) - } - // Register/unregister the panel as a static collider so its AABB/sphere draw in the overlay - if (open && props.showPanelBounds) props.addOverlayElement?.(el) - else props.removeOverlayElement?.(el) - }, [open, props, props.showPanelBounds]) + useEffect(() => { + const el = panelRef.current + if (!el) return undefined + if (open && restoreElement) { + void restoreElement(el).catch((error) => { + console.warn('DebugPalette.restoreElement failed', error) + }) + } + if (open && showPanelBounds) { + addOverlayElement?.(el) + return () => removeOverlayElement?.(el) + } + removeOverlayElement?.(el) + return undefined + }, [open, restoreElement, addOverlayElement, removeOverlayElement, showPanelBounds])(Destructure the props above so these names exist.)
src/lib/clothSceneController.ts (2)
786-803: Stop swallowing recycle failures.This
try { recycle } catch { /* ignore unmounted */ }brings back the same lint failure and hides real pool breakage. Guard the call or log the specific error you expect.Example:
- try { this.pool.recycle(element) } catch { /* ignore unmounted */ } + try { + this.pool.recycle(element) + } catch (error) { + console.warn('ElementPool.recycle failed during activation', error) + }
267-269: Reset_worldSleepingwhen you disable the guard.Once
_worldSleepingflips true, turning the guard off keeps returningtruefromisSleeping(), so SimWorld still treats the cloth as dormant even though the guard is gone. Clear the flag when disabling.setWorldSleepGuardEnabled(enabled: boolean) { this._worldSleepGuardEnabled = !!enabled + if (!this._worldSleepGuardEnabled) { + this._worldSleeping = false + } }src/engine/render/DebugOverlaySystem.ts (1)
105-113: Empty catch still violates lint and hides real failures.
catch { /* no-op */ }triggersno-emptyand silently swallows viewport failures. Bind the error and either log it or rethrow.Do this instead:
- } catch { /* no-op */ } + } catch (error) { + console.warn('DebugOverlaySystem.getViewportPixels failed', error) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
src/App.tsx(20 hunks)src/engine/debug/engineActions.ts(1 hunks)src/engine/render/DebugOverlayState.ts(2 hunks)src/engine/render/DebugOverlaySystem.ts(5 hunks)src/engine/systems/simulationSystem.ts(3 hunks)src/lib/WorldBody.ts(3 hunks)src/lib/clothPhysics.ts(2 hunks)src/lib/clothSceneController.ts(31 hunks)src/lib/collisionSystem.ts(4 hunks)src/lib/elementPool.ts(1 hunks)src/lib/rigidBody2d.ts(1 hunks)src/lib/simWorld.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/lib/simWorld.ts (1)
src/lib/simulationScheduler.ts (1)
SimulationScheduler(11-47)
src/engine/systems/simulationSystem.ts (1)
src/lib/simWorld.ts (1)
SimBroadphaseMode(41-41)
src/lib/collisionSystem.ts (2)
src/lib/WorldBody.ts (1)
WorldBody(16-154)src/lib/units.ts (4)
toCanonicalWidthMeters(21-24)toCanonicalHeightMeters(26-29)toCanonicalX(31-35)toCanonicalY(37-41)
src/App.tsx (3)
src/lib/clothSceneController.ts (7)
PinMode(26-26)setConstraintIterations(271-273)setConstraintIterations(444-444)setConstraintIterations(1295-1301)setBroadphaseMode(1040-1042)setPinMode(1309-1323)setRealTime(1276-1282)src/engine/debug/engineActions.ts (4)
setConstraintIterations(91-93)setBroadphaseMode(120-122)setPinMode(135-137)setRealTime(50-52)src/engine/render/DebugOverlayState.ts (1)
DebugOverlayState(15-44)
src/lib/clothSceneController.ts (7)
src/types/pinMode.ts (1)
PinMode(1-1)src/lib/domToWebGL.ts (1)
DOMMeshRecord(14-26)src/engine/entity/entity.ts (2)
Entity(14-96)id(20-22)src/lib/simWorld.ts (2)
SimBody(9-18)SimSleepConfig(25-28)src/engine/entity/component.ts (1)
Component(6-11)src/lib/collisionSystem.ts (1)
CollisionSystem(13-93)src/lib/rigidBody2d.ts (1)
RigidBody2D(12-119)
🔇 Additional comments (4)
src/lib/clothPhysics.ts (1)
445-445: LGTM — necessary for AABB consistency.Adding
computeBoundingBox()here mirrors the existingcomputeBoundingSphere()call (line 444) and ensures the bounding box stays synchronized with vertex updates. This is required forgetAABB()to return accurate results.src/engine/render/DebugOverlayState.ts (1)
19-39: Overlay state additions are coherent.The new toggles and buffers line up with the overlay system’s expanded rendering surface, and the defaults stay sane (false flags, empty arrays, a sensible 0.01m pointer radius). No changes needed.
src/engine/systems/simulationSystem.ts (1)
162-167: Waking sleepers on gravity change is the right call.This ensures gravity tweaks have immediate visual effect instead of waiting for incidental collisions to bump bodies awake. Nice touch.
src/lib/WorldBody.ts (1)
56-139: Rotation support slots in cleanly.The Euler/quaternion setters and the guarded application inside
applyToMeshintegrate neatly, and the local↔world helpers now honor rotation without destabilizing existing scale/translation behavior. Looks solid.
| useEffect(() => { | ||
| const actions = actionsRef.current | ||
| if (!actions) return | ||
| // Toggle overlay visibility; hide while the drawer is open to avoid overlap. | ||
| actions.setPointerOverlayVisible(debugOpen ? false : pointerColliderVisible) | ||
| }, [debugOpen, pointerColliderVisible]) | ||
| // Non-modal debug: keep overlay visibility tied to the toggle regardless of Drawer state. | ||
| actions.setPointerOverlayVisible(pointerColliderVisible) | ||
| }, [pointerColliderVisible]) | ||
|
|
||
| useEffect(() => { | ||
| actionsRef.current?.setGravityScalar(gravity) |
There was a problem hiding this comment.
Drop the second pointer-overlay effect.
You already do this at Lines 646‑651; keeping both just toggles twice per change.
Delete this effect.
🤖 Prompt for AI Agents
In src/App.tsx around lines 574 to 582, there is a duplicate useEffect that sets
the pointer overlay visibility based on pointerColliderVisible; remove this
entire effect block (the useEffect that reads actionsRef.current and calls
setPointerOverlayVisible) so only the existing pointer-overlay effect at lines
~646–651 remains to avoid toggling twice per change.
| /** Sets broad-phase mode on the simulation world via SimulationSystem (if present). */ | ||
| setBroadphaseMode(mode: 'sphere' | 'fatAABB') { | ||
| (this.simulation as unknown as { setBroadphaseMode?: (m: 'sphere' | 'fatAABB') => void })?.setBroadphaseMode?.(mode) | ||
| } | ||
|
|
||
| /** Adjusts broad-phase margins (base + velocity fudge) on SimWorld via SimulationSystem. */ | ||
| setBroadphaseMargins(baseMargin: number, velocityFudge: number) { | ||
| (this.simulation as unknown as { setBroadphaseMargins?: (b: number, v: number) => void })?.setBroadphaseMargins?.(baseMargin, velocityFudge) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Eliminate the unknown cast here.
SimulationSystem already exposes these methods, so the cast to a hand-rolled shape sidesteps the compiler and risks hiding future drift. Pull in the shared type and call the method directly.
-import type { SimulationSystem } from '../systems/simulationSystem'
+import type { SimulationSystem } from '../systems/simulationSystem'
+import type { SimBroadphaseMode } from '../../lib/simWorld'
…
- setBroadphaseMode(mode: 'sphere' | 'fatAABB') {
- (this.simulation as unknown as { setBroadphaseMode?: (m: 'sphere' | 'fatAABB') => void })?.setBroadphaseMode?.(mode)
+ setBroadphaseMode(mode: SimBroadphaseMode) {
+ this.simulation?.setBroadphaseMode?.(mode)
}
…
- setBroadphaseMargins(baseMargin: number, velocityFudge: number) {
- (this.simulation as unknown as { setBroadphaseMargins?: (b: number, v: number) => void })?.setBroadphaseMargins?.(baseMargin, velocityFudge)
+ setBroadphaseMargins(baseMargin: number, velocityFudge: number) {
+ this.simulation?.setBroadphaseMargins?.(baseMargin, velocityFudge)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/engine/debug/engineActions.ts around lines 119 to 127, remove the ad-hoc
unknown cast and hand-rolled shape and instead import and use the shared
SimulationSystem type: add an import for the exported SimulationSystem type from
its module, make this.simulation typed or narrowed to SimulationSystem (or check
it is present and then cast to SimulationSystem) and call setBroadphaseMode and
setBroadphaseMargins directly without the unknown intermediary; keep the runtime
safety by checking this.simulation exists before invoking the methods and remove
the custom inline type used in the current calls.
| getAABB(): { min: THREE.Vector2; max: THREE.Vector2 } { | ||
| if (!this.mesh.geometry.boundingBox) { | ||
| this.mesh.geometry.computeBoundingBox() | ||
| } | ||
| const box = this.mesh.geometry.boundingBox! | ||
| const pos = this.mesh.position | ||
| // Geometry positions are in mesh local space; mesh carries world translation (scale=1, no rotation) | ||
| return { | ||
| min: new THREE.Vector2(box.min.x + pos.x, box.min.y + pos.y), | ||
| max: new THREE.Vector2(box.max.x + pos.x, box.max.y + pos.y), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical inconsistency: getAABB() returns world space, getBoundingSphere() returns local space.
This method adds mesh.position to return world-space coordinates (lines 138-139), but the existing getBoundingSphere() method (lines 119-128) returns local-space coordinates without any position offset. This inconsistency is a landmine:
wakeIfPointInside()(line 109) compares a world-spacepointagainst the local-space result ofgetBoundingSphere()— likely broken for any mesh with non-zero position.- Callers cannot predict which coordinate space they're getting from bounding-volume queries on the same object.
Incomplete world transformation.
Lines 138-139 offset by position only, but line 136 acknowledges scale=1, no rotation as an assumption. If that assumption is violated (e.g., a scaled or rotated mesh), the AABB will be geometrically incorrect. Use Box3.applyMatrix4(mesh.matrixWorld) or transform each corner via mesh.localToWorld() to handle the full transform.
Apply this diff to fix both the coordinate-space inconsistency and the incomplete transform (if scale/rotation must be supported):
getAABB(): { min: THREE.Vector2; max: THREE.Vector2 } {
if (!this.mesh.geometry.boundingBox) {
this.mesh.geometry.computeBoundingBox()
}
const box = this.mesh.geometry.boundingBox!
- const pos = this.mesh.position
- // Geometry positions are in mesh local space; mesh carries world translation (scale=1, no rotation)
+ // Transform local-space box to world space via mesh transform
+ const worldBox = box.clone().applyMatrix4(this.mesh.matrixWorld)
return {
- min: new THREE.Vector2(box.min.x + pos.x, box.min.y + pos.y),
- max: new THREE.Vector2(box.max.x + pos.x, box.max.y + pos.y),
+ min: new THREE.Vector2(worldBox.min.x, worldBox.min.y),
+ max: new THREE.Vector2(worldBox.max.x, worldBox.max.y),
}
}Fix getBoundingSphere() to also return world space:
getBoundingSphere(): { center: THREE.Vector2; radius: number } {
if (!this.mesh.geometry.boundingSphere) {
this.mesh.geometry.computeBoundingSphere()
}
const sphere = this.mesh.geometry.boundingSphere!
+ const worldCenter = sphere.center.clone().applyMatrix4(this.mesh.matrixWorld)
+ // Radius must be scaled by max scale component if non-uniform scale exists
+ const scale = this.mesh.scale
+ const maxScale = Math.max(scale.x, scale.y, scale.z)
return {
- center: new THREE.Vector2(sphere.center.x, sphere.center.y),
- radius: sphere.radius,
+ center: new THREE.Vector2(worldCenter.x, worldCenter.y),
+ radius: sphere.radius * maxScale,
}
}Run this script to verify the coordinate-space assumptions in the codebase and locate all call sites:
🏁 Script executed:
#!/bin/bash
# Description: Verify usage patterns and coordinate-space assumptions for bounding-volume methods
echo "=== Searching for getAABB() call sites ==="
rg -nP --type=ts 'getAABB\s*\(' -A 3 -B 1
echo -e "\n=== Searching for getBoundingSphere() call sites ==="
rg -nP --type=ts 'getBoundingSphere\s*\(' -A 3 -B 1
echo -e "\n=== Searching for mesh.position, mesh.scale, mesh.rotation modifications ==="
rg -nP --type=ts 'mesh\.(position|scale|rotation|matrix)' -A 2 -B 1
echo -e "\n=== Checking if ClothPhysics mesh is ever scaled or rotated ==="
rg -nP --type=ts 'ClothPhysics' -A 5 | rg -P '(scale|rotation|quaternion)'Length of output: 12296
🏁 Script executed:
#!/bin/bash
# Verify cloth mesh scaling and rotation at runtime, plus examine syncGeometry change
echo "=== Check cloth mesh creation and initial state ==="
rg -nP --type=ts 'cloth.*=.*new.*ClothPhysics|ClothPhysics.*new' -A 5 -B 2
echo -e "\n=== Check if cloth position/scale/rotation is modified after creation ==="
rg -nP --type=ts '(cloth|this\.cloth)\.(position|scale|rotation)' -A 2 -B 1
echo -e "\n=== Check syncGeometry() method and line 445 context ==="
rg -nP --type=ts 'syncGeometry|computeBoundingBox' -A 5 -B 3Length of output: 7131
Coordinate-space hell is real—but the scale/rotation assumption is actually safe in practice.
The core bug is ironclad: getBoundingSphere() returns local space (no position offset), while getAABB() returns world space (position added). This creates a landmine when they're used together:
- wakeIfPointInside() (line 110-114) compares a world-space
pointagainst the local-space result ofgetBoundingSphere()—fundamentally broken for any mesh with non-zero position. - simWorld.ts broadphase (line 172-187) also mixes the two: it calls
getBoundingSphere()on both bodies (which forrigidBody2dreturns world space, but for cloth returns local space), then later callsgetAABB()(world space). Inconsistent coordinate systems in the same calculation.
However, the incomplete-transform issue does NOT apply here. Evidence:
- Cloth meshes are not modified after creation (no runtime position/scale/rotation changes).
- The "scale=1, no rotation" assumption holds at instantiation time because
elementPoolresets scale to(1,1,1)before reuse anddomToWebGLhandles the initial layout-to-world transform without mesh scale/rotation (it only sets position and computes dimensions separately).
Fix the coordinate-space inconsistency, drop the scale/rotation fix:
getAABB(): { min: THREE.Vector2; max: THREE.Vector2 } {
if (!this.mesh.geometry.boundingBox) {
this.mesh.geometry.computeBoundingBox()
}
const box = this.mesh.geometry.boundingBox!
const pos = this.mesh.position
- // Geometry positions are in mesh local space; mesh carries world translation (scale=1, no rotation)
+ // Transform local-space box to world space
return {
min: new THREE.Vector2(box.min.x + pos.x, box.min.y + pos.y),
max: new THREE.Vector2(box.max.x + pos.x, box.max.y + pos.y),
}
}Unify getBoundingSphere() to return world space:
getBoundingSphere(): { center: THREE.Vector2; radius: number } {
if (!this.mesh.geometry.boundingSphere) {
this.mesh.geometry.computeBoundingSphere()
}
const sphere = this.mesh.geometry.boundingSphere!
+ const pos = this.mesh.position
return {
- center: new THREE.Vector2(sphere.center.x, sphere.center.y),
+ center: new THREE.Vector2(sphere.center.x + pos.x, sphere.center.y + pos.y),
radius: sphere.radius,
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
src/lib/clothPhysics.ts lines 130-141: getAABB returns world-space coordinates
while getBoundingSphere currently returns local-space, causing incorrect
mixed-space comparisons (wakeIfPointInside and broadphase). Change
getBoundingSphere to return a world-space sphere by taking
geometry.boundingSphere.center and adding mesh.position to produce the sphere
center (leave radius as-is since scale=1 in practice), so both AABB and bounding
sphere are in the same coordinate space; update any direct uses expecting
local-space sphere to use the new world-space result.
| const corners = [ | ||
| this._tmpLocalV3.set(-hw, -hh, 0), | ||
| this._tmpLocalV3B.set(hw, -hh, 0), | ||
| this._tmpLocalV3.set(hw, hh, 0), | ||
| this._tmpLocalV3B.set(-hw, hh, 0), | ||
| ] | ||
| let minX = Infinity, minY = Infinity, maxX = -Infinity, maxY = -Infinity | ||
| for (const c of corners) { | ||
| const w = this.record.worldBody.localToWorldPoint(c, this._tmpWorldV3) | ||
| if (w.x < minX) minX = w.x | ||
| if (w.y < minY) minY = w.y | ||
| if (w.x > maxX) maxX = w.x | ||
| if (w.y > maxY) maxY = w.y | ||
| } | ||
| return { min: new THREE.Vector2(minX, minY), max: new THREE.Vector2(maxX, maxY) } |
There was a problem hiding this comment.
AABB calculation is wrong—you're iterating the same two points.
corners stores repeated references to _tmpLocalV3 / _tmpLocalV3B, so by the time the loop runs all entries share the last assigned values. The resulting min/max omit the lower corners, producing a bogus bounding box and breaking fat-AABB wake logic. Clone each corner before pushing.
Fix like this:
- const corners = [
- this._tmpLocalV3.set(-hw, -hh, 0),
- this._tmpLocalV3B.set(hw, -hh, 0),
- this._tmpLocalV3.set(hw, hh, 0),
- this._tmpLocalV3B.set(-hw, hh, 0),
- ]
+ const corners = [
+ new THREE.Vector3(-hw, -hh, 0),
+ new THREE.Vector3(hw, -hh, 0),
+ new THREE.Vector3(hw, hh, 0),
+ new THREE.Vector3(-hw, hh, 0),
+ ](Or reuse temps but copy before adding.)
📝 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.
| const corners = [ | |
| this._tmpLocalV3.set(-hw, -hh, 0), | |
| this._tmpLocalV3B.set(hw, -hh, 0), | |
| this._tmpLocalV3.set(hw, hh, 0), | |
| this._tmpLocalV3B.set(-hw, hh, 0), | |
| ] | |
| let minX = Infinity, minY = Infinity, maxX = -Infinity, maxY = -Infinity | |
| for (const c of corners) { | |
| const w = this.record.worldBody.localToWorldPoint(c, this._tmpWorldV3) | |
| if (w.x < minX) minX = w.x | |
| if (w.y < minY) minY = w.y | |
| if (w.x > maxX) maxX = w.x | |
| if (w.y > maxY) maxY = w.y | |
| } | |
| return { min: new THREE.Vector2(minX, minY), max: new THREE.Vector2(maxX, maxY) } | |
| const corners = [ | |
| new THREE.Vector3(-hw, -hh, 0), | |
| new THREE.Vector3(hw, -hh, 0), | |
| new THREE.Vector3(hw, hh, 0), | |
| new THREE.Vector3(-hw, hh, 0), | |
| ] | |
| let minX = Infinity, minY = Infinity, maxX = -Infinity, maxY = -Infinity | |
| for (const c of corners) { | |
| const w = this.record.worldBody.localToWorldPoint(c, this._tmpWorldV3) | |
| if (w.x < minX) minX = w.x | |
| if (w.y < minY) minY = w.y | |
| if (w.x > maxX) maxX = w.x | |
| if (w.y > maxY) maxY = w.y | |
| } | |
| return { min: new THREE.Vector2(minX, minY), max: new THREE.Vector2(maxX, maxY) } |
🤖 Prompt for AI Agents
In src/lib/clothSceneController.ts around lines 332 to 346 the AABB computation
is wrong because corners currently push references to the same two temp vectors
(_tmpLocalV3/_tmpLocalV3B), so after the set calls all entries point to the last
value; replace those pushes with cloned/fresh Vector3 instances (or call
.clone() / make a copy of the temp immediately before pushing) so each corner is
an independent vector, then proceed with the world-local conversion and min/max
calculation as written.
| private computeAABB(body: StaticBody): { min: THREE.Vector2; max: THREE.Vector2 } { | ||
| const hw = (body.widthMeters || 0) / 2 | ||
| const hh = (body.heightMeters || 0) / 2 | ||
| const corners: THREE.Vector3[] = [ | ||
| new THREE.Vector3(-hw, -hh, 0), | ||
| new THREE.Vector3(hw, -hh, 0), | ||
| new THREE.Vector3(hw, hh, 0), | ||
| new THREE.Vector3(-hw, hh, 0), | ||
| ] | ||
| let minX = Infinity, minY = Infinity, maxX = -Infinity, maxY = -Infinity | ||
| for (const c of corners) { | ||
| const w = body.worldBody.localToWorldPoint(c) | ||
| if (w.x < minX) minX = w.x | ||
| if (w.y < minY) minY = w.y | ||
| if (w.x > maxX) maxX = w.x | ||
| if (w.y > maxY) maxY = w.y | ||
| } | ||
| return { | ||
| min: new THREE.Vector2(minX, minY), | ||
| max: new THREE.Vector2(maxX, maxY), | ||
| } |
There was a problem hiding this comment.
Stop allocating a parade of vectors every tick.
computeAABB now builds four fresh THREE.Vector3 locals, triggers another four allocations inside localToWorldPoint, and spits out new THREE.Vector2 instances on every apply()—per static body, per fixed update. That’s 10+ heap objects a frame where we previously just reused numbers, and it will churn the GC under load. Reuse scratch vectors instead.
+const LOCAL_CORNER_SCRATCH = [
+ new THREE.Vector3(),
+ new THREE.Vector3(),
+ new THREE.Vector3(),
+ new THREE.Vector3(),
+]
+
+const WORLD_CORNER_SCRATCH = [
+ new THREE.Vector3(),
+ new THREE.Vector3(),
+ new THREE.Vector3(),
+ new THREE.Vector3(),
+]
+
private computeAABB(body: StaticBody): { min: THREE.Vector2; max: THREE.Vector2 } {
const hw = (body.widthMeters || 0) / 2
const hh = (body.heightMeters || 0) / 2
- const corners: THREE.Vector3[] = [
- new THREE.Vector3(-hw, -hh, 0),
- new THREE.Vector3(hw, -hh, 0),
- new THREE.Vector3(hw, hh, 0),
- new THREE.Vector3(-hw, hh, 0),
- ]
+ LOCAL_CORNER_SCRATCH[0].set(-hw, -hh, 0)
+ LOCAL_CORNER_SCRATCH[1].set(hw, -hh, 0)
+ LOCAL_CORNER_SCRATCH[2].set(hw, hh, 0)
+ LOCAL_CORNER_SCRATCH[3].set(-hw, hh, 0)
let minX = Infinity, minY = Infinity, maxX = -Infinity, maxY = -Infinity
- for (const c of corners) {
- const w = body.worldBody.localToWorldPoint(c)
+ for (let i = 0; i < 4; i++) {
+ const w = body.worldBody.localToWorldPoint(LOCAL_CORNER_SCRATCH[i], WORLD_CORNER_SCRATCH[i])
if (w.x < minX) minX = w.x
if (w.y < minY) minY = w.y
if (w.x > maxX) maxX = w.x
if (w.y > maxY) maxY = w.y
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/collisionSystem.ts around lines 71 to 91, computeAABB allocates new
THREE.Vector3 for the four corners and new THREE.Vector2 for the result every
call which causes heavy GC churn; change it to reuse preallocated scratch
vectors (e.g. class-level Vector3[4] for corners and a temporary Vector3 for
localToWorldPoint, plus reusable Vector2 objects or accept output targets) and
call localToWorldPoint into the single temp Vector3 rather than creating new
vectors, then update the min/max numbers and write into reused Vector2s (or
return references to them) so no per-frame heap allocations are performed.
| } | ||
|
|
||
| async prepare(element: HTMLElement, segments = 24) { | ||
| async prepare(element: HTMLElement, segments = 24, options: { force?: boolean; reason?: string } = {}) { |
There was a problem hiding this comment.
CRITICAL: No validation on segments and no concurrent call protection.
Two severe issues with this method signature:
-
Zero validation on
segments: What happens if someone passessegments = 0,segments = -5, orsegments = 1000000? You'll create degenerate or explosion-level geometry. At minimum, enforce bounds:if (segments < 1 || segments > 256) { throw new RangeError(`segments must be in [1, 256], got ${segments}`) }
-
Race condition hell: This is an
asyncmethod with no guard against concurrent calls for the same element. Ifprepare(elem, 24)is called twice in rapid succession, both will capture textures, create meshes, and you'll leak resources or corrupt state. Add a guard:private preparing = new Set<HTMLElement>() async prepare(...) { if (this.preparing.has(element)) { throw new Error('prepare already in progress for this element') } this.preparing.add(element) try { // ... existing logic } finally { this.preparing.delete(element) } }
🤖 Prompt for AI Agents
In src/lib/elementPool.ts around line 20, the prepare(element, segments = 24,
...) method lacks validation on the segments parameter and has no protection
against concurrent calls for the same element; add a bounds check that throws a
RangeError if segments < 1 or > 256 (or other chosen safe upper bound) and
implement a per-element guard (e.g., a private Set or Map keyed by HTMLElement)
that rejects or awaits concurrent prepare calls for the same element by checking
the set at entry, adding the element before work begins, and ensuring removal in
a finally block to avoid leaks; keep existing logic inside the guarded/try block
so resources are not double-created or leaked.
| if (existing) { | ||
| this.destroy(element) | ||
| } | ||
| if (existing && existing.segments === segments && !options.force) return |
There was a problem hiding this comment.
MAJOR: Early return ignores element dimension changes.
The early return only checks segments equality, but what if the element's actual dimensions changed? If someone resizes the element via CSS or browser zoom, the geometry becomes stale but this check lets it slide. You'll end up with a mesh that doesn't match the element's current size.
Add dimension validation:
const existing = this.elements.get(element)
-if (existing && existing.segments === segments && !options.force) return
+if (existing && existing.segments === segments && !options.force) {
+ const rect = element.getBoundingClientRect()
+ const widthMeters = rect.width / 100 // or however you compute this
+ const heightMeters = rect.height / 100
+ if (Math.abs(existing.baseWidthMeters - widthMeters) < 0.001 &&
+ Math.abs(existing.baseHeightMeters - heightMeters) < 0.001) {
+ return
+ }
+}Or at minimum, document that dimension changes require force: true.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/elementPool.ts around line 22, the early return only compares
existing.segments to segments and skips updating when dimensions changed; update
the check to also validate stored element dimensions against the element’s
current size (e.g., compare stored width/height to
element.getBoundingClientRect() or to options.width/options.height if provided)
and only return early when both segments and dimensions match or when
options.force is true; alternatively, if you prefer not to add dimension checks,
update documentation/comments to clearly state that dimension changes require
calling with force: true.
| baseHeightMeters: record.baseHeightMeters, | ||
| segments: record.segments, | ||
| reason: options.reason ?? 'prepare', | ||
| time: Date.now(), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider performance.now() for higher precision.
If the time field is used for performance profiling or debugging timing issues, performance.now() provides microsecond precision and is monotonic (unaffected by system clock adjustments). However, if this is just metadata/logging, Date.now() is fine.
If you need precision:
- time: Date.now(),
+ time: performance.now(),📝 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.
| time: Date.now(), | |
| time: performance.now(), |
🤖 Prompt for AI Agents
In src/lib/elementPool.ts around line 50, the time field currently uses
Date.now(); replace it with performance.now() for higher-resolution, monotonic
timestamps when the field is used for profiling: update the assignment to use
performance.now() and add a safe fallback to Date.now() for environments where
performance is unavailable (e.g., const now = (typeof performance !==
'undefined' && performance.now) ? performance.now() : Date.now(); and set time:
now). If callers expect epoch milliseconds, convert performance.now() to epoch
by adding Date.now() - performance.timing?.navigationStart (or use Date.now()
fallback) so unit/epoch semantics remain consistent; ensure any tests or
consumers are adjusted to the new precision/units.
…le); emit broad‑phase wake + activation with tags; parse data‑tag; tests/build green
…ucture; feat: minimal OBB AABB resolution to satisfy specs; docs: append notes; all tests green
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (11)
src/App.tsx (2)
592-669: Drop the duplicate pointer overlay effect.
pointerColliderVisiblenow triggers two identical effects, so every toggle sends two set calls. Kill one of them.- useEffect(() => { - const actions = actionsRef.current - if (!actions) return - // Non-modal debug: keep overlay visibility tied to the toggle regardless of Drawer state. - actions.setPointerOverlayVisible(pointerColliderVisible) - }, [pointerColliderVisible]) - @@ useEffect(() => { actionsRef.current?.setPointerOverlayVisible(pointerColliderVisible) }, [pointerColliderVisible])
149-161: Fix the panel overlay effect; it's leaking colliders and hiding errors.Every render re-executes this effect (the parent hands you fresh arrow props), so without a cleanup the panel stays registered in the overlay set and piles up stale colliders when the drawer re-opens or the component unmounts while open. Worse, the empty catch makes a restore failure completely silent. Please wire up a cleanup and log the failure so we can debug the guard.
- useEffect(() => { - const el = panelRef.current - if (!el) return - if (open && props.restoreElement) { - // Ensure any previously clothified panel is restored to DOM for interactivity - Promise.resolve(props.restoreElement(el)).catch(() => {}) - } - // Register/unregister the panel as a static collider so its AABB/sphere draw in the overlay - if (open && props.showPanelBounds) props.addOverlayElement?.(el) - else props.removeOverlayElement?.(el) - }, [open, props, props.showPanelBounds]) + useEffect(() => { + const el = panelRef.current + if (!el) return + + const { restoreElement, addOverlayElement, removeOverlayElement, showPanelBounds } = props + + if (open && restoreElement) { + void Promise.resolve(restoreElement(el)).catch((error) => { + console.warn('DebugPalette.restoreElement failed', error) + }) + } + + if (open && showPanelBounds) { + addOverlayElement?.(el) + return () => removeOverlayElement?.(el) + } + + removeOverlayElement?.(el) + return undefined + }, [open, props.restoreElement, props.addOverlayElement, props.removeOverlayElement, props.showPanelBounds])src/lib/clothSceneController.ts (9)
148-152: Empty catch block STILL violates lint despite past review.Line 148 has an empty
catch {}that past review comment explicitly flagged and marked as "✅ Addressed". Either the fix was reverted or never applied. This blocks CI and masks geometry computation failures that would help diagnose why_modelRadiusfalls back to defaults.Apply the fix from the past review:
- } catch { + } catch (error) { + console.warn(`ClothBodyAdapter ${id}: failed to compute model radius from geometry`, error) const w = this.record.widthMeters ?? 0 const h = this.record.heightMeters ?? 0 this._modelRadius = 0.5 * Math.hypot(w, h) }Based on past review comments.
230-230: Misleading comment persists despite past review.Line 230's comment "Radius is constant; no need to update here" was flagged in a past review as misleading because this block only uses
worldCenterand never references radius. Past review marked this "✅ Addressed" but the comment is unchanged.Apply the fix from past review:
- // Radius is constant; no need to update here + // (Radius is precomputed and constant; only center is tracked for motion detection)Or remove it entirely if it adds no value.
Based on past review comments.
270-272: Stale_worldSleepingstate when guard is disabled.Past review flagged that disabling the guard mid-simulation leaves
_worldSleeping === trueeven though the guard no longer enforces it, causing a mismatch with local sleep state.Apply the fix from past review:
setWorldSleepGuardEnabled(enabled: boolean) { this._worldSleepGuardEnabled = !!enabled + if (!enabled) { + this._worldSleeping = false + } }Based on past review comments.
335-340: CRITICAL: AABB corners are bogus—you're reusing the same two temp vectors.Lines 335-340 push references to
_tmpLocalV3and_tmpLocalV3Binto the array. After all four.set()calls,corners[0]andcorners[2]are the same object (both are_tmpLocalV3, currently set tohw, hh, 0), andcorners[1]andcorners[3]are the same object (_tmpLocalV3B, set to-hw, hh, 0). The loop at lines 342-348 only sees two unique points (the last set values), omitting the lower corners entirely. The resulting bounding box is garbage and breaks fat-AABB wake logic.This was flagged as CRITICAL in a past review and remains unfixed.
Fix by cloning each corner before pushing:
- const corners = [ - this._tmpLocalV3.set(-hw, -hh, 0), - this._tmpLocalV3B.set(hw, -hh, 0), - this._tmpLocalV3.set(hw, hh, 0), - this._tmpLocalV3B.set(-hw, hh, 0), - ] + const corners = [ + new THREE.Vector3(-hw, -hh, 0), + new THREE.Vector3(hw, -hh, 0), + new THREE.Vector3(hw, hh, 0), + new THREE.Vector3(-hw, hh, 0), + ]Based on past review comments.
758-760: Missing disposal guard afterawaitinclothify.Past review flagged that
clothifylacks a guard afterawait pool.prepare(...)(line 759) to check if the controller was disposed during the async operation. Ifthis.disposed,this.pool, orthis.domToWebGLchanged while awaiting, lines 760+ will operate on stale or null state. Past review marked this "✅ Addressed" but the guard is still missing.Apply the fix from past review:
const seg = this.computeAutoSegments(rect, this.debug.tessellationSegments) await pool.prepare(element, seg, { reason: 'manual' }) + if (this.disposed || this.pool !== pool || this.domToWebGL !== bridge) return pool.mount(element)Based on past review comments.
807-807: Empty catch with comment still violatesno-empty.Line 807's
catch { /* ignore unmounted */ }was flagged in past review as non-compliant (still violatesno-emptylint) and marked "✅ Addressed". It's unchanged. Either remove the try-catch and guard the call, or add proper error handling.Apply the fix from past review:
- try { this.pool.recycle(element) } catch { /* ignore unmounted */ } + try { + this.pool.recycle(element) + } catch (error) { + console.warn('pool.recycle failed during activation', error) + }Based on past review comments.
917-956: Unnecessaryasynckeyword and missing error handling.
Line 917:
restoreElementis declaredasyncbut contains noawait. Past review flagged this (no checkmark = not addressed). Removeasyncto avoid misleading callers and unnecessary Promise overhead.Lines 942-946: The
recycle/resetGeometry/mountsequence can throw (geometry corruption, pool state issues). Past review suggested defensive error handling (no checkmark). Wrap in try-catch or validate pool state before proceeding to avoid leaving element in inconsistent state (e.g.,isActivetrue but mesh not remounted).Apply fixes from past review:
- async restoreElement(element: HTMLElement) { + restoreElement(element: HTMLElement) { const item = this.items.get(element) if (!item) return // ... if (this.pool) { - this.pool.recycle(element) - this.pool.resetGeometry(element) - this.pool.mount(element) + try { + this.pool.recycle(element) + this.pool.resetGeometry(element) + this.pool.mount(element) + } catch (error) { + console.error('Failed to restore element geometry', error) + // Ensure consistent state: mark inactive if remount failed + item.isActive = false + } }Based on past review comments.
1378-1378: Fire-and-forget async calls lack explanatory comments.Lines 1378 and 1389 use
void this.setTessellationSegments(..., true)to intentionally discard the promise. Past review suggested adding a brief comment (no checkmark = not addressed). While the pattern is correct, it's subtle.Add a concise comment:
+ // Fire-and-forget: tessellation updates are async but don't block caller void this.setTessellationSegments(this.debug.tessellationSegments, true)Based on past review comments.
Also applies to: 1389-1389
834-835: Magic numbers lack rationale.Lines 834-835 use
0.0005,0.004, and0.05without explaining why 5% of element size capped at 4mm is chosen. Past review flagged this (no checkmark, so not addressed). Future maintainers won't know if these were empirically tuned or arbitrary.Extract named constants with units:
+ const MIN_JITTER_M = 0.0005 // 0.5mm minimum perturbation hint + const MAX_JITTER_M = 0.004 // 4mm cap to avoid visible crumple + const JITTER_FRACTION = 0.05 // 5% of element size const sizeHint = Math.max(0.0005, Math.min(record.widthMeters, record.heightMeters)) - const jitter = Math.min(0.004, sizeHint * 0.05) + const jitter = Math.min(MAX_JITTER_M, sizeHint * JITTER_FRACTION)Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
BLOG_NOTES.mdis excluded by!*.md
📒 Files selected for processing (7)
src/App.tsx(21 hunks)src/engine/events/EventBus.ts(1 hunks)src/lib/__tests__/dataAttrs.test.ts(1 hunks)src/lib/__tests__/rigidObb.test.ts(1 hunks)src/lib/clothSceneController.ts(32 hunks)src/lib/rigidBody2d.ts(1 hunks)src/lib/simWorld.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/lib/__tests__/rigidObb.test.ts (1)
src/lib/rigidBody2d.ts (1)
RigidBody2D(12-156)
src/App.tsx (4)
src/lib/clothSceneController.ts (8)
PinMode(27-27)setConstraintIterations(274-276)setConstraintIterations(447-447)setConstraintIterations(1304-1310)setSubsteps(1294-1301)setBroadphaseMode(1049-1051)setPinMode(1318-1332)setRealTime(1285-1291)src/engine/debug/engineActions.ts (5)
setConstraintIterations(91-93)setSubsteps(60-62)setBroadphaseMode(120-122)setPinMode(135-137)setRealTime(50-52)src/lib/simWorld.ts (1)
setBroadphaseMode(61-63)src/engine/render/DebugOverlayState.ts (1)
DebugOverlayState(15-44)
src/lib/simWorld.ts (1)
src/lib/simulationScheduler.ts (1)
SimulationScheduler(11-47)
src/lib/clothSceneController.ts (7)
src/types/pinMode.ts (1)
PinMode(1-1)src/lib/domToWebGL.ts (1)
DOMMeshRecord(14-26)src/engine/entity/entity.ts (2)
Entity(14-96)id(20-22)src/lib/simWorld.ts (2)
SimBody(9-18)SimSleepConfig(25-28)src/lib/collisionSystem.ts (1)
CollisionSystem(13-93)src/lib/rigidBody2d.ts (1)
RigidBody2D(12-156)src/engine/events/EventBus.ts (1)
EventBus(9-28)
🪛 GitHub Actions: CI
src/lib/rigidBody2d.ts
[error] 132-132: ESLint: 'pen' is never reassigned. Use 'const' instead. (prefer-const)
🪛 GitHub Check: test
src/lib/rigidBody2d.ts
[failure] 132-132:
'pen' is never reassigned. Use 'const' instead
🔇 Additional comments (5)
src/lib/simWorld.ts (2)
11-12: LGTM: Clean interface extension.The optional
getAABBmethod properly extendsSimBodyfor fat-AABB broadphase without breaking existing implementations.
231-272: Liang-Barsky implementation is correct.The fat-AABB intersection uses Liang-Barsky clipping properly. Zero-length segments (dx=dy=0) will return true if the start point is inside the expanded rectangle (all four
clipcalls with p=0 will check q >= 0, which corresponds to the start point being inside each half-space). This is the correct behavior for a point-in-rect test.src/lib/clothSceneController.ts (3)
416-450: RigidBodyAdapter implementation is sound.The adapter correctly wraps
RigidBody2Dand implements theSimBodycontract. The_itemparameter is unused but properly prefixed. The manual sphere containment check inwakeIfPointInsideis correct. OptionalSimBodymethods likewarmStartare appropriately omitted.
1270-1277: LGTM: Wake-on-gravity-change is implemented.Lines 1273-1276 correctly iterate active cloths, apply the new gravity vector, and immediately call
wake()so sleeping bodies react to the change. This matches the intent from PR objectives and past review discussion.
1198-1214: LGTM: Duplication addressed with shared helper.
computePointerRadiusForextracts the pointer radius calculation into a dedicated method, mirroringClothBodyAdapter.getImpulseRadiuslogic. This addresses the past review concern (marked "✅ Addressed in commit 384a860") about code duplication.
| if (!(import.meta as unknown as { env?: Record<string, string> }).env || (import.meta as unknown as { env?: Record<string, string> }).env?.MODE !== 'test') { | ||
| this.installLayoutObserver(clothElements) | ||
| } | ||
| } catch { | ||
| // best effort; ignore in environments without import.meta | ||
| this.installLayoutObserver(clothElements) | ||
| } |
There was a problem hiding this comment.
Fragile test detection with unsafe type gymnastics.
Lines 586-592 (and line 707) use import.meta as unknown as { env?: Record<string, string> } to detect test mode. This is brittle—if the structure changes, it fails silently. Past review flagged a similar pattern and marked it "✅ Addressed", but these instances remain.
Consider a cleaner approach:
- try {
- if (!(import.meta as unknown as { env?: Record<string, string> }).env || (import.meta as unknown as { env?: Record<string, string> }).env?.MODE !== 'test') {
- this.installLayoutObserver(clothElements)
- }
- } catch {
- // best effort; ignore in environments without import.meta
- this.installLayoutObserver(clothElements)
- }
+ const isTest = typeof import.meta !== 'undefined' &&
+ typeof (import.meta as { env?: { MODE?: string } }).env === 'object' &&
+ (import.meta as { env: { MODE?: string } }).env.MODE === 'test'
+ if (!isTest) {
+ this.installLayoutObserver(clothElements)
+ }Or define a global test flag checked without type gymnastics.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/clothSceneController.ts around lines 586-592 (and also at line 707),
the code uses fragile casts like `import.meta as unknown as { env?:
Record<string,string> }` to detect test mode; replace these unsafe type
gymnastics with a small, well-typed helper (e.g. isTestEnv()) that safely checks
for known test signals: first check a dedicated global flag on globalThis (e.g.
(globalThis as any).__TEST__), then fall back to feature-detected
import.meta.env access only when available (guarded by typeof import.meta !==
'undefined' or checking 'env' in import.meta), and finally fall back to
process.env.NODE_ENV if present; update both sites to call that helper so
detection is robust and avoids unsafe casts.
| await this.pool.prepare(element, seg, { reason: 'rigid-activation', force: true }) | ||
| this.pool.mount(element) | ||
| const record = this.pool.getRecord(element)! |
There was a problem hiding this comment.
Missing disposal guard after await in activateRigid.
Line 902 awaits pool.prepare but lines 903+ proceed without checking if the controller was disposed, the pool was swapped, or domToWebGL changed during the async operation. Same issue as clothify (flagged in past review).
Add guard after await:
await this.pool.prepare(element, seg, { reason: 'rigid-activation', force: true })
+ if (this.disposed || !this.pool || !this.domToWebGL) return
this.pool.mount(element)📝 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.
| await this.pool.prepare(element, seg, { reason: 'rigid-activation', force: true }) | |
| this.pool.mount(element) | |
| const record = this.pool.getRecord(element)! | |
| await this.pool.prepare(element, seg, { reason: 'rigid-activation', force: true }) | |
| if (this.disposed || !this.pool || !this.domToWebGL) return | |
| this.pool.mount(element) | |
| const record = this.pool.getRecord(element)! |
| this.collisionSystem.refresh() | ||
| this.updateOverlayDebug() | ||
| }) | ||
| .catch(() => { /* ignore capture failures */ }) |
There was a problem hiding this comment.
Empty catch block in layout observer.
Line 1477's .catch(() => { /* ignore capture failures */ }) is an empty catch with only a comment. This violates no-empty lint and provides no diagnostic when geometry recapture fails after layout changes.
Log the error:
- .catch(() => { /* ignore capture failures */ })
+ .catch((error) => {
+ console.warn('Layout observer: geometry recapture failed for', el.id || el, error)
+ })📝 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.
| .catch(() => { /* ignore capture failures */ }) | |
| .catch((error) => { | |
| console.warn('Layout observer: geometry recapture failed for', el.id || el, error) | |
| }) |
🤖 Prompt for AI Agents
In src/lib/clothSceneController.ts around lines 1477 to 1477, the promise catch
currently swallows errors with an empty handler; replace .catch(() => { /*
ignore capture failures */ }) with a catch that accepts the error and logs it
(e.g., console.error or the module's logger) including context like "geometry
recapture failed after layout change" and the error object so failures are
visible for debugging while keeping the swallow behavior (do not rethrow).
| const minPen = Math.min(penLeft, penRight, penBottom, penTop) | ||
| let nx = 0, ny = 0, pen = minPen | ||
| if (minPen === penLeft) { nx = -1; ny = 0 } | ||
| else if (minPen === penRight) { nx = 1; ny = 0 } | ||
| else if (minPen === penBottom) { nx = 0; ny = -1 } | ||
| else { nx = 0; ny = 1 } | ||
| this.position.x += nx * pen | ||
| this.position.y += ny * pen | ||
| const vn = this.velocity.x * nx + this.velocity.y * ny | ||
| const rx = this.velocity.x - (1 + this.restitution) * vn * nx |
There was a problem hiding this comment.
Satisfy lint: keep pen constant.
pen never changes after initialization, so ESLint is halting the build with prefer-const. Flip it to a const so CI goes green.
- let nx = 0, ny = 0, pen = minPen
+ let nx = 0, ny = 0
+ const pen = minPen
if (minPen === penLeft) { nx = -1; ny = 0 }
else if (minPen === penRight) { nx = 1; ny = 0 }
else if (minPen === penBottom) { nx = 0; ny = -1 }📝 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.
| const minPen = Math.min(penLeft, penRight, penBottom, penTop) | |
| let nx = 0, ny = 0, pen = minPen | |
| if (minPen === penLeft) { nx = -1; ny = 0 } | |
| else if (minPen === penRight) { nx = 1; ny = 0 } | |
| else if (minPen === penBottom) { nx = 0; ny = -1 } | |
| else { nx = 0; ny = 1 } | |
| this.position.x += nx * pen | |
| this.position.y += ny * pen | |
| const vn = this.velocity.x * nx + this.velocity.y * ny | |
| const rx = this.velocity.x - (1 + this.restitution) * vn * nx | |
| const minPen = Math.min(penLeft, penRight, penBottom, penTop) | |
| let nx = 0, ny = 0 | |
| const pen = minPen | |
| if (minPen === penLeft) { nx = -1; ny = 0 } | |
| else if (minPen === penRight) { nx = 1; ny = 0 } | |
| else if (minPen === penBottom) { nx = 0; ny = -1 } | |
| else { nx = 0; ny = 1 } | |
| this.position.x += nx * pen | |
| this.position.y += ny * pen | |
| const vn = this.velocity.x * nx + this.velocity.y * ny | |
| const rx = this.velocity.x - (1 + this.restitution) * vn * nx |
🧰 Tools
🪛 GitHub Actions: CI
[error] 132-132: ESLint: 'pen' is never reassigned. Use 'const' instead. (prefer-const)
🪛 GitHub Check: test
[failure] 132-132:
'pen' is never reassigned. Use 'const' instead
🤖 Prompt for AI Agents
In src/lib/rigidBody2d.ts around lines 131 to 140, the variable `pen` is never
reassigned after being initialized from `minPen`, triggering ESLint's
`prefer-const`; change the declaration so `pen` is a `const` (e.g., keep
`nx`/`ny` as `let` but declare `pen` as `const` initialized to `minPen`) so the
build passes.
| setBroadphaseMargins(baseMargin: number, velocityFudge: number) { | ||
| if (Number.isFinite(baseMargin) && baseMargin >= 0) this.baseMargin = baseMargin | ||
| if (Number.isFinite(velocityFudge) && velocityFudge >= 0) this.velocityFudge = velocityFudge | ||
| } |
There was a problem hiding this comment.
Silent validation failures hide misconfiguration.
setBroadphaseMargins silently ignores invalid inputs (NaN, Infinity, negative). Callers have no way to know their configuration was rejected. At minimum, warn on invalid values; ideally, throw or return a validation result.
Apply this diff to add warnings:
setBroadphaseMargins(baseMargin: number, velocityFudge: number) {
- if (Number.isFinite(baseMargin) && baseMargin >= 0) this.baseMargin = baseMargin
- if (Number.isFinite(velocityFudge) && velocityFudge >= 0) this.velocityFudge = velocityFudge
+ if (Number.isFinite(baseMargin) && baseMargin >= 0) {
+ this.baseMargin = baseMargin
+ } else {
+ console.warn('SimWorld: invalid baseMargin', baseMargin)
+ }
+ if (Number.isFinite(velocityFudge) && velocityFudge >= 0) {
+ this.velocityFudge = velocityFudge
+ } else {
+ console.warn('SimWorld: invalid velocityFudge', velocityFudge)
+ }
}📝 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.
| setBroadphaseMargins(baseMargin: number, velocityFudge: number) { | |
| if (Number.isFinite(baseMargin) && baseMargin >= 0) this.baseMargin = baseMargin | |
| if (Number.isFinite(velocityFudge) && velocityFudge >= 0) this.velocityFudge = velocityFudge | |
| } | |
| setBroadphaseMargins(baseMargin: number, velocityFudge: number) { | |
| if (Number.isFinite(baseMargin) && baseMargin >= 0) { | |
| this.baseMargin = baseMargin | |
| } else { | |
| console.warn('SimWorld: invalid baseMargin', baseMargin) | |
| } | |
| if (Number.isFinite(velocityFudge) && velocityFudge >= 0) { | |
| this.velocityFudge = velocityFudge | |
| } else { | |
| console.warn('SimWorld: invalid velocityFudge', velocityFudge) | |
| } | |
| } |
| this.eventBus?.post({ | ||
| type: 'broadphaseWake', | ||
| id: (target as unknown as { id?: string }).id, | ||
| payload: { | ||
| mover: (moving as unknown as { id?: string }).id, | ||
| mode: this.broadphaseMode, | ||
| }, | ||
| time: Date.now(), | ||
| }) |
There was a problem hiding this comment.
Unsafe type assertions bypass the SimBody contract.
Lines 201 and 203 cast target and moving to { id?: string } to extract an id that isn't guaranteed by the SimBody interface. If the interface requires an id, add it explicitly:
export interface SimBody extends SleepableBody {
id: string // ← add this
getBoundingSphere: () => BoundingSphere
// ...
}Then remove the casts:
this.eventBus?.post({
type: 'broadphaseWake',
- id: (target as unknown as { id?: string }).id,
+ id: target.id,
payload: {
- mover: (moving as unknown as { id?: string }).id,
+ mover: moving.id,
mode: this.broadphaseMode,
},
time: Date.now(),
})If id is truly optional, make it explicit in the interface so consumers know the contract.
🤖 Prompt for AI Agents
In src/lib/simWorld.ts around lines 199 to 207: the code uses unsafe type
assertions to extract id from target and moving ((target as unknown as { id?:
string }).id), bypassing the SimBody contract; open the SimBody interface
declaration and make the id property explicit (add either id: string if it must
always exist, or id?: string if optional), then remove the ad-hoc casts in this
file and directly access target.id and moving.id (adjust call sites if you
changed optionality, e.g. handle undefined or assert presence where appropriate)
so the type system enforces the contract instead of using unsafe assertions.
… milestone tasks; minor overlay + events UX improvements
…ilestone + issues opened)
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engine/render/DebugOverlaySystem.ts (1)
52-66: Dispose the new overlay groups.You introduced
simAabbGroupandsphereGroup, butdispose()still pretends they do not exist. Detaching this system now strands those groups in the scene graph, so the next attach leaks geometry/materials and double-renders the stale overlays. Clean up every group you allocate.if (this.circleGroup) { this.clearOverlayGroup(this.circleGroup) this.view.scene?.remove(this.circleGroup) this.circleGroup = undefined } + if (this.simAabbGroup) { + this.clearOverlayGroup(this.simAabbGroup) + this.view.scene?.remove(this.simAabbGroup) + this.simAabbGroup = undefined + } + if (this.sphereGroup) { + this.clearOverlayGroup(this.sphereGroup) + this.view.scene?.remove(this.sphereGroup) + this.sphereGroup = undefined + }src/lib/clothSceneController.ts (1)
163-183: Activation grace logic is sound; consider documenting the 20-frame magic number.Lines 163-183 implement an activation grace window to prevent immediate sleep. The 20-frame threshold (≈0.33s at 60 FPS) is hardcoded at line 106. Past review suggested extracting it to a named constant for clarity and tuning.
Apply this diff:
+ private static readonly ACTIVATION_GRACE_FRAMES = 20 // Keep newly activated cloth awake for a short grace window to avoid // immediate sleep when pins prevent initial center motion. - private _activationGraceFrames = 20 + private _activationGraceFrames = ClothBodyAdapter.ACTIVATION_GRACE_FRAMES
♻️ Duplicate comments (12)
src/App.tsx (4)
508-508: Inconsistent default: App uses 'none', controller expects 'top'.Line 508 initializes
pinModeto'none', but the controller's default atsrc/lib/clothSceneController.ts:524is also'none'now. However, past review comments flagged a mismatch when the controller defaulted to'top'. Verify that both defaults are intentionally synchronized or document why they differ.Based on past review comments.
Run this script to confirm both defaults match:
#!/bin/bash # Verify pinMode defaults in App and controller match. rg -nP "useState<PinMode>\('(none|top)'\)" src/App.tsx rg -nP "pinMode:\s*'(none|top)'" src/lib/clothSceneController.ts
151-161: Stabilize effect dependencies and add cleanup.This effect depends on the entire
propsobject, causing it to re-run on every render. Additionally, there's no cleanup when the component unmounts whileopenis true, leaving stale overlay elements in the collision system.Based on past review comments.
Apply this diff:
useEffect(() => { const el = panelRef.current if (!el) return if (open && props.restoreElement) { - Promise.resolve(props.restoreElement(el)).catch(() => {}) + void Promise.resolve(props.restoreElement(el)).catch((error) => { + console.warn('DebugPalette.restoreElement failed', error) + }) } - // Register/unregister the panel as a static collider so its AABB/sphere draw in the overlay - if (open && props.showPanelBounds) props.addOverlayElement?.(el) - else props.removeOverlayElement?.(el) - }, [open, props, props.showPanelBounds]) + if (open && props.showPanelBounds) { + props.addOverlayElement?.(el) + return () => props.removeOverlayElement?.(el) + } + props.removeOverlayElement?.(el) + return undefined + }, [open, props.restoreElement, props.addOverlayElement, props.removeOverlayElement, props.showPanelBounds])
597-602: Drop the first of two duplicate pointer-overlay effects.You toggle
pointerColliderVisibletwice per change: once here (lines 597-602) and again at lines 672-674. Delete this effect.Based on past review comments.
Apply this diff:
- useEffect(() => { - const actions = actionsRef.current - if (!actions) return - // Non-modal debug: keep overlay visibility tied to the toggle regardless of Drawer state. - actions.setPointerOverlayVisible(pointerColliderVisible) - }, [pointerColliderVisible]) -
654-664: Initial auto-tessellation and world-guard settings never reach the controller.These effects run before
controllerRef.currentis populated, so initial values (especially if a consumer overrides the defaults) are silently ignored. Move the initial sync into the controllerinit()promise (alongside gravity/zoom setup at lines 535-547) so the controller always receives the initial values.Based on past review comments.
Apply this diff inside the
init()promise block (after line 543):actionsRef.current.setSleepConfig(sleepVelocity, sleepFrames) + controller.setTessellationAutoEnabled(autoTessellation) + controller.setTessellationMinMax(tessellationMin, tessellationMax) + controller.setWorldSleepGuardEnabled(worldSleepGuard) + actionsRef.current.setBroadphaseMode?.(broadphaseMode) + controller.setBroadphaseMode?.(broadphaseMode) if (reduced) {Keep the existing
useEffecthooks for subsequent changes.src/lib/clothSceneController.ts (8)
335-349: CRITICAL: AABB corners still reuse temp vectors, producing a bogus bounding box.Past review flagged this exact issue:
cornersarray at lines 335-340 stores repeated references to_tmpLocalV3and_tmpLocalV3B. By the time the loop at lines 342-348 runs, all entries point to the last assigned value (the fourth corner). The resulting min/max omit the lower corners, breaking fat-AABB wake logic.Based on past review comments.
Apply this diff:
const hw = (this.record.widthMeters || 0) * 0.5 const hh = (this.record.heightMeters || 0) * 0.5 const corners = [ - this._tmpLocalV3.set(-hw, -hh, 0), - this._tmpLocalV3B.set(hw, -hh, 0), - this._tmpLocalV3.set(hw, hh, 0), - this._tmpLocalV3B.set(-hw, hh, 0), + new THREE.Vector3(-hw, -hh, 0), + new THREE.Vector3(hw, -hh, 0), + new THREE.Vector3(hw, hh, 0), + new THREE.Vector3(-hw, hh, 0), ] let minX = Infinity, minY = Infinity, maxX = -Infinity, maxY = -Infinity for (const c of corners) {
586-592: Fragile test detection with unsafe type gymnastics.Lines 586-592 (and also lines 707, 793, 883, 1353) use
import.meta as unknown as { env?: Record<string, string> }to detect test mode. This is brittle—if the structure changes, it fails silently. Past review flagged similar patterns.Based on past review comments.
Extract a well-typed helper to centralize test detection:
// At top of file or in a utility module function isTestEnv(): boolean { try { if (typeof import.meta !== 'undefined' && typeof (import.meta as { env?: { MODE?: string } }).env === 'object') { return (import.meta as { env: { MODE?: string } }).env.MODE === 'test' } } catch { // ignore } return false }Then replace all usages:
- const isTest = (typeof import.meta !== 'undefined' && (import.meta as unknown as { env?: Record<string, string> }).env?.MODE === 'test') + const isTest = isTestEnv()Apply this pattern at lines 586, 707, 793, 883, and 1353.
807-807: Empty catch block violates no-empty and masks failures.Line 807's
catch { /* ignore unmounted */ }is minimally compliant but provides no diagnostic whenrecyclefails. Past review flagged this exact line.Based on past review comments.
Apply this diff:
- try { this.pool.recycle(element) } catch { /* ignore unmounted */ } + try { + this.pool.recycle(element) + } catch (error) { + console.warn('pool.recycle failed during activation', error) + }
1480-1480: Empty catch in layout observer swallows geometry recapture failures.Line 1480's
.catch(() => { /* ignore capture failures */ })violatesno-emptyand provides no diagnostic when geometry recapture fails after layout changes. Past review flagged this.Based on past review comments.
Apply this diff:
- .catch(() => { /* ignore capture failures */ }) + .catch((error) => { + console.warn('Layout observer: geometry recapture failed for', el.id || el, error) + })
920-959: Unnecessaryasynckeyword misleads callers.
restoreElementis declaredasyncbut never awaits anything. This misleads callers into thinking they must await the result and adds unnecessary Promise overhead. Past review flagged this.Based on past review comments.
Apply this diff:
- async restoreElement(element: HTMLElement) { + restoreElement(element: HTMLElement) {Update callers in
src/App.tsxto removeawaitor leave it (await on a non-Promise is a no-op but harmless).
1181-1217: Pointer radius calculation duplicates ClothBodyAdapter logic.Lines 1201-1216 duplicate
ClothBodyAdapter.getImpulseRadius()(lines 352-366). Past review suggested extracting a shared helper or calling the adapter method directly to avoid divergence.Based on past review comments.
Consider extracting a module-level helper:
function computePointerRadius( element: HTMLElement, widthMeters: number, heightMeters: number ): number { const attr = element.dataset.clothImpulseRadius const parsed = attr ? Number.parseFloat(attr) : NaN if (!Number.isNaN(parsed) && parsed > 0) return parsed const base = Math.min(widthMeters, heightMeters) const MIN = 0.0006 const DEFAULT = 0.0012 return base > 0 ? Math.max(MIN, base / 12) : DEFAULT }Then replace lines 1201-1216 with:
private computePointerRadiusFor(item: ClothItem) { const record = item.record return computePointerRadius( item.element, record?.widthMeters ?? 0, record?.heightMeters ?? 0 ) }And update
ClothBodyAdapter.getImpulseRadius()to call the same helper.
1338-1343: setWorldSleepGuardEnabled lacks state reset when disabling.When
setWorldSleepGuardEnabled(false)is called while an adapter's_worldSleepingis true, the adapter continues reporting as sleeping even though the guard is disabled. Past review flagged this inClothBodyAdapter.setWorldSleepGuardEnabled.The controller method at lines 1338-1343 propagates the flag but doesn't reset the adapter's
_worldSleepingstate. Consider broadcasting a wake-all when disabling:setWorldSleepGuardEnabled(enabled: boolean) { this.debug.worldSleepGuardEnabled = !!enabled for (const item of this.items.values()) { item.adapter?.setWorldSleepGuardEnabled(this.debug.worldSleepGuardEnabled) + if (!enabled) { + item.adapter?.wake() + } } }Alternatively, reset the flag in
ClothBodyAdapter.setWorldSleepGuardEnabledas past review suggested:setWorldSleepGuardEnabled(enabled: boolean) { this._worldSleepGuardEnabled = !!enabled + if (!enabled) { + this._worldSleeping = false + } }
752-780: Guardclothifyagainst disposal races.The
clothifymethod awaitspool.prepare(...)at line 759 but doesn't re-checkdisposed/pool/bridgebefore callingpool.mount(element)at line 760. If the controller is disposed or the pool is swapped during the async operation, we'll mount on dead state. Past review flagged a similar pattern inprepareElements.Apply this diff:
const seg = this.computeAutoSegments(rect, this.debug.tessellationSegments) await pool.prepare(element, seg, { reason: 'manual' }) + if (this.disposed || this.pool !== pool || this.domToWebGL !== bridge) return pool.mount(element)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
.github/ISSUE_TEMPLATE/newton_task.md(1 hunks)docs/ROADMAP_PROJECT_NEWTON.md(1 hunks)docs/issues/demo-polish.md(1 hunks)docs/issues/inspector.md(1 hunks)docs/issues/perf-budget.md(1 hunks)docs/issues/pr-a.md(1 hunks)docs/issues/pr-b.md(1 hunks)docs/issues/pr-c.md(1 hunks)docs/issues/pr-d.md(1 hunks)docs/issues/pr-e.md(1 hunks)docs/issues/sandman.md(1 hunks)scripts/gh_newton.sh(1 hunks)src/App.tsx(22 hunks)src/engine/render/DebugOverlaySystem.ts(5 hunks)src/lib/clothSceneController.ts(32 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/App.tsx (3)
src/lib/clothSceneController.ts (8)
PinMode(27-27)setConstraintIterations(274-276)setConstraintIterations(447-447)setConstraintIterations(1307-1313)setSubsteps(1297-1304)setBroadphaseMode(1052-1054)setPinMode(1321-1335)setRealTime(1288-1294)src/engine/debug/engineActions.ts (5)
setConstraintIterations(91-93)setSubsteps(60-62)setBroadphaseMode(120-122)setPinMode(135-137)setRealTime(50-52)src/engine/render/DebugOverlayState.ts (1)
DebugOverlayState(15-44)
src/lib/clothSceneController.ts (7)
src/types/pinMode.ts (1)
PinMode(1-1)src/lib/domToWebGL.ts (1)
DOMMeshRecord(14-26)src/engine/entity/entity.ts (2)
Entity(14-96)id(20-22)src/lib/simWorld.ts (2)
SimBody(9-18)SimSleepConfig(25-28)src/lib/collisionSystem.ts (1)
CollisionSystem(13-93)src/lib/rigidBody2d.ts (1)
RigidBody2D(12-156)src/engine/events/EventBus.ts (1)
EventBus(9-28)
🪛 markdownlint-cli2 (0.18.1)
docs/issues/perf-budget.md
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
14-14: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
docs/issues/sandman.md
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
14-14: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
docs/issues/pr-b.md
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
14-14: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
18-18: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
docs/ROADMAP_PROJECT_NEWTON.md
24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
36-36: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
41-41: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
47-47: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
52-52: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
59-59: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
64-64: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
71-71: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
docs/issues/inspector.md
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
13-13: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
docs/issues/pr-d.md
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
13-13: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
docs/issues/pr-a.md
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
11-11: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
17-17: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
21-21: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (6)
docs/issues/sandman.md (2)
1-17: Verify feature scope alignment with PR objectives.This documentation describes the "Sandman" sleeper reset system (event-driven auto-reset after delay), whereas the PR objectives focus on "World Sleep Guard toggle + tessellation caps." Clarify whether Sandman is:
- Part of this PR (and should be mentioned in PR objectives), or
- A complementary/follow-up feature documented here for future work
If Sandman is not in this PR's scope, consider moving this to a separate issue tracking document or roadmap file to avoid confusion.
7-7: Verify GSAP FLIP is optionally gated in implementation.Line 7 references "optional GSAP FLIP when available," implying defensive handling for an optional dependency. Ensure that the implementation (not yet present in this PR, based on acceptance criteria) properly checks for GSAP availability before attempting to use FLIP, preventing runtime errors on systems without GSAP.
docs/issues/demo-polish.md (1)
1-16: LGTM!Documentation is clear and well-structured.
docs/issues/pr-e.md (1)
1-16: LGTM!Documentation clearly outlines the eventing expansion requirements and test strategy.
src/lib/clothSceneController.ts (2)
416-450: Review comment is incorrect—.rigid-dynamicelements exist in the codebase.A
.rigid-dynamicbutton element is present insrc/App.tsx:738, and theclothSceneController.tsactively queries for these elements at line 570. TheRigidBodyAdapterandactivateRigidmethod are not dead code—they are actively integrated into the demo's rigid body system. The premise that rigid elements may not be present is factually incorrect.Likely an incorrect or invalid review comment.
128-153: Empty catch block at line 148 will break lint and mask geometry failures.The empty
catchat line 148 violatesno-emptyand silently swallows exceptions that might indicate malformed geometry or missing properties. Past review flagged a similar pattern.Apply this diff:
} catch { + console.warn(`ClothBodyAdapter ${id}: failed to compute model radius from geometry`) const w = this.record.widthMeters ?? 0 const h = this.record.heightMeters ?? 0 this._modelRadius = 0.5 * Math.hypot(w, h)Likely an incorrect or invalid review comment.
| ## Summary | ||
| Hotkey I toggles an inspector cursor. Hovering a DOM element shows a Mantine Popover with: type, tag, collider (shape, extents), mass/restitution/friction, awake/sleep, world bounds. Click pins the popover; ESC dismisses. | ||
|
|
||
| ## Acceptance Criteria | ||
| - [ ] Inspector toggle (I); popover content accurate for cloth/rigid/static | ||
| - [ ] No layout shift; non‑modal | ||
|
|
||
| ## Tests (write first) | ||
| - [ ] inspector.test: overlay highlight and popover render; shows basic fields |
There was a problem hiding this comment.
Fix markdown heading spacing to pass lint.
The linter requires blank lines around headings. Add blank lines before lines 6, 9, and 13 to comply with MD022.
Apply this diff:
Milestone: PROJECT: Newton
+
## Summary
Hotkey I toggles an inspector cursor. Hovering a DOM element shows a Mantine Popover with: type, tag, collider (shape, extents), mass/restitution/friction, awake/sleep, world bounds. Click pins the popover; ESC dismisses.
+
## Acceptance Criteria
- [ ] Inspector toggle (I); popover content accurate for cloth/rigid/static
- [ ] No layout shift; non‑modal
+
## Tests (write first)
- [ ] inspector.test: overlay highlight and popover render; shows basic fields📝 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.
| ## Summary | |
| Hotkey I toggles an inspector cursor. Hovering a DOM element shows a Mantine Popover with: type, tag, collider (shape, extents), mass/restitution/friction, awake/sleep, world bounds. Click pins the popover; ESC dismisses. | |
| ## Acceptance Criteria | |
| - [ ] Inspector toggle (I); popover content accurate for cloth/rigid/static | |
| - [ ] No layout shift; non‑modal | |
| ## Tests (write first) | |
| - [ ] inspector.test: overlay highlight and popover render; shows basic fields | |
| ## Summary | |
| Hotkey I toggles an inspector cursor. Hovering a DOM element shows a Mantine Popover with: type, tag, collider (shape, extents), mass/restitution/friction, awake/sleep, world bounds. Click pins the popover; ESC dismisses. | |
| ## Acceptance Criteria | |
| - [ ] Inspector toggle (I); popover content accurate for cloth/rigid/static | |
| - [ ] No layout shift; non‑modal | |
| ## Tests (write first) | |
| - [ ] inspector.test: overlay highlight and popover render; shows basic fields |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
13-13: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In docs/issues/inspector.md around lines 6 to 14, the markdown headings lack
required blank lines; add a single blank line immediately before line 6 (the
"Summary" heading), before line 9 (the "Acceptance Criteria" heading) and before
line 13 (the "Tests (write first)" heading) so each heading is separated by a
blank line to satisfy MD022 lint rules.
| ## Summary | ||
| - Draw wireframe on top of solid meshes via a dedicated overlay pass (no reliance on material.wireframe alone). | ||
| - Add type glyphs at object centers (cloth=circle, rigid=diamond, static=square) and a mini legend in the Debug panel. | ||
| - Replace Drawer with a non‑modal Events panel (opens at ~45% height), Cmd/Ctrl+E hotkey, Fullscreen toggle; scene remains interactive. | ||
|
|
||
| ## Acceptance Criteria | ||
| - [ ] Overlay wireframe visible above textured mesh when Wireframe is ON | ||
| - [ ] Glyphs rendered per type; legend visible in Debug panel | ||
| - [ ] Events panel opens at ~45%, non‑modal, fullscreen toggle works; hotkey Cmd/Ctrl+E | ||
| - [ ] Scene input remains interactive while Events panel is open | ||
|
|
||
| ## Tests (write first) | ||
| - [ ] overlayWireframe.test: assert wireframe LineSegments exist for an active cloth | ||
| - [ ] eventsPanel.test: hotkey toggles, non‑modal click-through verified, fullscreen toggles height | ||
|
|
||
| ## Notes | ||
| - Keep MODE=test semantics stable | ||
| - GSAP dragging/resizing is PR B (out of scope here) |
There was a problem hiding this comment.
Fix markdown heading spacing to pass lint.
Add blank lines before lines 6, 11, 17, and 21 to comply with MD022.
Apply this diff:
Milestone: PROJECT: Newton
+
## Summary
- Draw wireframe on top of solid meshes via a dedicated overlay pass (no reliance on material.wireframe alone).
- Add type glyphs at object centers (cloth=circle, rigid=diamond, static=square) and a mini legend in the Debug panel.
- Replace Drawer with a non‑modal Events panel (opens at ~45% height), Cmd/Ctrl+E hotkey, Fullscreen toggle; scene remains interactive.
+
## Acceptance Criteria
- [ ] Overlay wireframe visible above textured mesh when Wireframe is ON
- [ ] Glyphs rendered per type; legend visible in Debug panel
- [ ] Events panel opens at ~45%, non‑modal, fullscreen toggle works; hotkey Cmd/Ctrl+E
- [ ] Scene input remains interactive while Events panel is open
+
## Tests (write first)
- [ ] overlayWireframe.test: assert wireframe LineSegments exist for an active cloth
- [ ] eventsPanel.test: hotkey toggles, non‑modal click-through verified, fullscreen toggles height
+
## Notes
- Keep MODE=test semantics stable
- GSAP dragging/resizing is PR B (out of scope here)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
11-11: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
17-17: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
21-21: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In docs/issues/pr-a.md around lines 6 to 23, the Markdown headings need blank
lines before them to satisfy lint rule MD022; insert a single empty line before
line 6 (the "Summary" heading), before line 11 (the "Acceptance Criteria"
heading), before line 17 (the "Tests (write first)" heading), and before line 21
(the "Notes" heading) so each heading is preceded by an empty line.
| ## Summary | ||
| Convert Debug and Events panels to floating, draggable, resizable Mantine shells (GSAP Draggable). Persist positions/sizes in localStorage. Provide dock-to-bottom (45%) mode and ESC to exit fullscreen. | ||
|
|
||
| ## Acceptance Criteria | ||
| - [ ] Drag/resize both panels; positions persist across reloads | ||
| - [ ] Dock-to-bottom mode; ESC exits fullscreen | ||
| - [ ] Hotkeys remain functional; panels are non‑modal | ||
|
|
||
| ## Tests (write first) | ||
| - [ ] panelState.test: drag/resize persisted to localStorage | ||
| - [ ] hotkeys.test: Cmd/Ctrl+J and Cmd/Ctrl+E still work | ||
|
|
||
| ## Notes | ||
| - Keep Mantine shells; use GSAP for motion only |
There was a problem hiding this comment.
Fix markdown heading spacing to pass lint.
Add blank lines before lines 6, 9, 14, and 18 to satisfy MD022.
Apply this diff:
Milestone: PROJECT: Newton
+
## Summary
Convert Debug and Events panels to floating, draggable, resizable Mantine shells (GSAP Draggable). Persist positions/sizes in localStorage. Provide dock-to-bottom (45%) mode and ESC to exit fullscreen.
+
## Acceptance Criteria
- [ ] Drag/resize both panels; positions persist across reloads
- [ ] Dock-to-bottom mode; ESC exits fullscreen
- [ ] Hotkeys remain functional; panels are non‑modal
+
## Tests (write first)
- [ ] panelState.test: drag/resize persisted to localStorage
- [ ] hotkeys.test: Cmd/Ctrl+J and Cmd/Ctrl+E still work
+
## Notes
- Keep Mantine shells; use GSAP for motion onlyCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
14-14: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
18-18: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In docs/issues/pr-b.md around lines 6 to 19, several headings lack the required
blank line before them per MD022; add a single blank line immediately before the
headings at original lines 6, 9, 14, and 18 so each heading is preceded by one
empty line, then re-run the markdown linter to confirm the spacing issue is
resolved.
| ## Summary | ||
| When a body has been activated and later falls asleep away from its origin, schedule a reset after N seconds (default 5s). Cancel on wake. Cloth returns to static DOM (or GSAP FLIP). Rigid returns to origin and removes adapter. Emit `resetRequest` and `reset` events. |
There was a problem hiding this comment.
Add blank line after heading per Markdown best practices.
Line 6 (## Summary) lacks a blank line below it before the content. This violates MD022 (blanks-around-headings) and affects consistency.
Apply this diff:
## Summary
+
When a body has been activated and later falls asleep away from its origin, schedule a reset after N seconds (default 5s). Cancel on wake. Cloth returns to static DOM (or GSAP FLIP). Rigid returns to origin and removes adapter. Emit `resetRequest` and `reset` events.🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In docs/issues/sandman.md around lines 6 to 7, the heading "## Summary" is
missing a blank line below it; insert a single blank line between the heading
and the following paragraph so the file conforms to Markdown best-practice
(MD022) and maintains consistent spacing around headings.
| ## Acceptance Criteria | ||
| - [ ] Sleep events trigger reset scheduling; wake cancels | ||
| - [ ] Cloth → static DOM; rigid → origin; optional GSAP FLIP when available | ||
| - [ ] Events recorded in Events panel |
There was a problem hiding this comment.
Add blank line after heading per Markdown best practices.
Line 9 (## Acceptance Criteria) lacks a blank line below it before the checklist. This violates MD022 (blanks-around-headings) and affects consistency.
Apply this diff:
## Acceptance Criteria
+
- [ ] Sleep events trigger reset scheduling; wake cancels🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In docs/issues/sandman.md around lines 9 to 12, the heading "## Acceptance
Criteria" (line 9) is not followed by a blank line which violates MD022; insert
a single blank line immediately after the heading so the checklist starts on a
new paragraph and ensure there is exactly one blank line (no extra blank lines)
between the heading and the following list.
| <Drawer size="xl" position="bottom" opened={eventsOpen} onClose={() => setEventsOpen(false)} title="Engine Events"> | ||
| <Stack> | ||
| <TextInput placeholder="Search events" value={eventSearch} onChange={(e) => setEventSearch(e.currentTarget.value)} leftSection={<IconSearch size={16} />} /> | ||
| <ScrollArea h={260}> | ||
| <Table striped highlightOnHover> | ||
| <Table.Thead> | ||
| <Table.Tr> | ||
| <Table.Th>Time</Table.Th> | ||
| <Table.Th>Type</Table.Th> | ||
| <Table.Th>Id</Table.Th> | ||
| <Table.Th>Tag</Table.Th> | ||
| </Table.Tr> | ||
| </Table.Thead> | ||
| <Table.Tbody> | ||
| {events | ||
| .filter((e) => { | ||
| const q = eventSearch.toLowerCase().trim() | ||
| if (!q) return true | ||
| const s = `${new Date(e.time).toLocaleTimeString()} ${e.type} ${e.id ?? ''} ${e.tag ?? ''}`.toLowerCase() | ||
| return s.includes(q) | ||
| }) | ||
| .map((ev, i) => ( | ||
| <Table.Tr key={i}> | ||
| <Table.Td>{new Date(ev.time).toLocaleTimeString()}</Table.Td> | ||
| <Table.Td>{ev.type}</Table.Td> | ||
| <Table.Td>{ev.id ?? ''}</Table.Td> | ||
| <Table.Td>{ev.tag ?? ''}</Table.Td> | ||
| </Table.Tr> | ||
| ))} | ||
| </Table.Tbody> | ||
| </Table> | ||
| </ScrollArea> | ||
| </Stack> | ||
| </Drawer> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Events Drawer lacks error boundary and emits untyped custom events.
The Events panel subscribes to engineEvent (lines 497-507) without validation, and the filter logic at lines 772-777 concatenates potentially null fields without guarding. If an event is malformed, the UI could break. Wrap the subscription in a try-catch and validate event shape.
Apply this diff to the useEffect at lines 497-507:
useEffect(() => {
const handler = (e: Event) => {
- const detail = (e as CustomEvent).detail as EngineEventRow
- setEvents((prev) => {
- const next = [detail, ...prev]
- return next.slice(0, 100)
- })
+ try {
+ const detail = (e as CustomEvent).detail as EngineEventRow
+ if (!detail || typeof detail !== 'object' || typeof detail.time !== 'number' || typeof detail.type !== 'string') {
+ console.warn('engineEvent: invalid detail', detail)
+ return
+ }
+ setEvents((prev) => {
+ const next = [detail, ...prev]
+ return next.slice(0, 100)
+ })
+ } catch (err) {
+ console.error('engineEvent handler failed', err)
+ }
}
window.addEventListener('engineEvent', handler as EventListener)
return () => window.removeEventListener('engineEvent', handler as EventListener)
}, [])Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/App.tsx around lines 757-790 (and update the related useEffect
subscription at ~497-507), wrap the engineEvent subscription callback in a
try-catch and validate the incoming event shape before updating state: ensure
required fields exist and are of expected types (e.g. time is a parsable date or
number, type is a string; id and tag are optional strings) and ignore or log
malformed events; in the Drawer filter/concatenation (lines ~772-777) guard
against null/undefined by coalescing fields to empty strings (e.g. String(ev.id
?? '') / String(ev.tag ?? '')) and use a safe time formatting (check time
validity before new Date(...).toLocaleTimeString()); ensure the subscription is
cleaned up in the effect cleanup and that no unhandled exceptions escape the
callback.
| clothifyElement={async (el: HTMLElement) => { | ||
| try { | ||
| await controllerRef.current?.clothify?.(el, { activate: true, addClickHandler: false }) | ||
| if (!realTimeRef.current) { | ||
| // Give it a nudge when paused so it separates from the DOM panel | ||
| controllerRef.current?.stepOnce() | ||
| controllerRef.current?.stepOnce() | ||
| } | ||
| } catch (err) { | ||
| console.warn('clothifyElement failed', err) | ||
| } | ||
| }} | ||
| restoreElement={async (el: HTMLElement) => { | ||
| try { | ||
| await controllerRef.current?.restoreElement?.(el) | ||
| } catch (err) { | ||
| console.warn('restoreElement failed', err) | ||
| } | ||
| }} | ||
| addOverlayElement={(el) => controllerRef.current?.addStaticOverlayElement?.(el)} | ||
| removeOverlayElement={(el) => controllerRef.current?.removeStaticOverlayElement?.(el)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate element before clothifying; log failures properly.
The clothifyElement callback lacks input validation and may fail silently if el is null or not a valid HTMLElement. Additionally, the two manual stepOnce() calls when paused (lines 878-879) are undocumented and could confuse maintainers.
Apply this diff:
clothifyElement={async (el: HTMLElement) => {
try {
+ if (!el || !(el instanceof HTMLElement)) {
+ console.warn('clothifyElement: invalid element', el)
+ return
+ }
await controllerRef.current?.clothify?.(el, { activate: true, addClickHandler: false })
if (!realTimeRef.current) {
- // Give it a nudge when paused so it separates from the DOM panel
+ // Manual steps when paused: separate cloth copy from DOM panel visually
controllerRef.current?.stepOnce()
controllerRef.current?.stepOnce()
}
} catch (err) {
- console.warn('clothifyElement failed', err)
+ console.error('clothifyElement failed', err)
}
}}📝 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.
| clothifyElement={async (el: HTMLElement) => { | |
| try { | |
| await controllerRef.current?.clothify?.(el, { activate: true, addClickHandler: false }) | |
| if (!realTimeRef.current) { | |
| // Give it a nudge when paused so it separates from the DOM panel | |
| controllerRef.current?.stepOnce() | |
| controllerRef.current?.stepOnce() | |
| } | |
| } catch (err) { | |
| console.warn('clothifyElement failed', err) | |
| } | |
| }} | |
| restoreElement={async (el: HTMLElement) => { | |
| try { | |
| await controllerRef.current?.restoreElement?.(el) | |
| } catch (err) { | |
| console.warn('restoreElement failed', err) | |
| } | |
| }} | |
| addOverlayElement={(el) => controllerRef.current?.addStaticOverlayElement?.(el)} | |
| removeOverlayElement={(el) => controllerRef.current?.removeStaticOverlayElement?.(el)} | |
| clothifyElement={async (el: HTMLElement) => { | |
| try { | |
| if (!el || !(el instanceof HTMLElement)) { | |
| console.warn('clothifyElement: invalid element', el) | |
| return | |
| } | |
| await controllerRef.current?.clothify?.(el, { activate: true, addClickHandler: false }) | |
| if (!realTimeRef.current) { | |
| // Manual steps when paused: separate cloth copy from DOM panel visually | |
| controllerRef.current?.stepOnce() | |
| controllerRef.current?.stepOnce() | |
| } | |
| } catch (err) { | |
| console.error('clothifyElement failed', err) | |
| } | |
| }} | |
| restoreElement={async (el: HTMLElement) => { | |
| try { | |
| await controllerRef.current?.restoreElement?.(el) | |
| } catch (err) { | |
| console.warn('restoreElement failed', err) | |
| } | |
| }} | |
| addOverlayElement={(el) => controllerRef.current?.addStaticOverlayElement?.(el)} | |
| removeOverlayElement={(el) => controllerRef.current?.removeStaticOverlayElement?.(el)} |
🤖 Prompt for AI Agents
In src/App.tsx around lines 873 to 893: validate that the incoming el is
non-null and an HTMLElement before calling clothify/restore (e.g., if (!el ||
!(el instanceof HTMLElement)) { processLogger?.warn('clothifyElement called with
invalid element', { el }); return; }), wrap controller calls with optional
chaining as already used, and improve error logging to include context and the
caught error (e.g., processLogger?.error('clothifyElement failed for element', {
el }, err)). Replace the undocumented two consecutive
controllerRef.current?.stepOnce() calls with a single explicit nudge call and a
short inline comment explaining why it’s needed (or call stepOnce() once), so
maintainers aren’t confused.
| private applySleepTint(enabled: boolean) { | ||
| if (!enabled) return | ||
| const snap = this.state.simSnapshot | ||
| if (!snap) return | ||
| const asleep = new Set<string>() | ||
| for (const b of snap.bodies) if (b.sleeping) asleep.add(b.id) | ||
| const scene = this.view.scene as THREE.Scene | undefined | ||
| if (!scene) return | ||
| scene.traverse((obj) => { | ||
| const mesh = obj as THREE.Mesh | ||
| const userData = (mesh as unknown as { userData?: Record<string, unknown> }).userData || {} | ||
| if (!userData['isCloth']) return | ||
| const id = (userData['bodyId'] as string | undefined) || '' | ||
| const mat = mesh.material as THREE.MeshBasicMaterial | undefined | ||
| if (!mat) return | ||
| if (asleep.has(id)) { | ||
| mat.color.setHex(0xcc5555) | ||
| mat.opacity = 0.95 | ||
| mat.transparent = true | ||
| } else { | ||
| mat.color.setHex(0xffffff) | ||
| mat.opacity = 1 | ||
| mat.transparent = false | ||
| } | ||
| }) |
There was a problem hiding this comment.
Stop leaving cloth meshes permanently tinted.
applySleepTint bails out whenever the overlay is disabled, so every cloth material you touched while it was enabled stays red/transparent forever. Toggle the overlay off and the scene is still mangled—classic state leak. You need to capture each mesh’s original color/opacity, restore those values when the overlay is off, and avoid doing the full pass once everything is reset.
- private applySleepTint(enabled: boolean) {
- if (!enabled) return
- const snap = this.state.simSnapshot
- if (!snap) return
- const asleep = new Set<string>()
- for (const b of snap.bodies) if (b.sleeping) asleep.add(b.id)
- const scene = this.view.scene as THREE.Scene | undefined
- if (!scene) return
- scene.traverse((obj) => {
- const mesh = obj as THREE.Mesh
- const userData = (mesh as unknown as { userData?: Record<string, unknown> }).userData || {}
- if (!userData['isCloth']) return
- const id = (userData['bodyId'] as string | undefined) || ''
- const mat = mesh.material as THREE.MeshBasicMaterial | undefined
- if (!mat) return
- if (asleep.has(id)) {
- mat.color.setHex(0xcc5555)
- mat.opacity = 0.95
- mat.transparent = true
- } else {
- mat.color.setHex(0xffffff)
- mat.opacity = 1
- mat.transparent = false
- }
- })
- }
+ private applySleepTint(enabled: boolean) {
+ const scene = this.view.scene as THREE.Scene | undefined
+ if (!scene) return
+ const snap = this.state.simSnapshot
+ const asleep = new Set<string>()
+ if (enabled) {
+ if (!snap) return
+ for (const b of snap.bodies) if (b.sleeping) asleep.add(b.id)
+ }
+ scene.traverse((obj) => {
+ const mesh = obj as THREE.Mesh
+ const userData = (mesh as unknown as { userData?: Record<string, unknown> }).userData || {}
+ if (!userData['isCloth']) return
+ const mat = mesh.material as THREE.MeshBasicMaterial | undefined
+ if (!mat) return
+ if (!userData.__sleepTintOriginal) {
+ userData.__sleepTintOriginal = {
+ color: mat.color.getHex(),
+ opacity: mat.opacity,
+ transparent: mat.transparent,
+ }
+ }
+ if (!enabled) {
+ const original = userData.__sleepTintOriginal as { color: number; opacity: number; transparent: boolean }
+ mat.color.setHex(original.color)
+ mat.opacity = original.opacity
+ mat.transparent = original.transparent
+ userData.__sleepTintApplied = false
+ return
+ }
+ const id = (userData['bodyId'] as string | undefined) || ''
+ if (asleep.has(id)) {
+ mat.color.setHex(0xcc5555)
+ mat.opacity = 0.95
+ mat.transparent = true
+ userData.__sleepTintApplied = true
+ } else {
+ mat.color.setHex(0xffffff)
+ mat.opacity = 1
+ mat.transparent = false
+ userData.__sleepTintApplied = true
+ }
+ })
+ }🤖 Prompt for AI Agents
In src/engine/render/DebugOverlaySystem.ts around lines 355-379, applySleepTint
currently bails out when disabled and leaves cloth meshes permanently tinted;
change it to 1) when enabling: traverse scene but only override material
properties if you haven’t already saved originals—store original color (as hex
or Color), opacity and transparent flags on the mesh.userData under unique keys
(e.g. __dbg_origColor, __dbg_origOpacity, __dbg_origTransparent) before
mutating; 2) when disabling: traverse scene and restore material properties from
those userData keys for any mesh that has them, then delete the keys; and 3)
optimize by returning early when disabling if no meshes contain the saved
userData keys (so you don't do a full pass unnecessarily). Ensure you guard
userData access and material typing as before and handle meshes removed from
scene by checking keys before restore.
| addStaticOverlayElement(element: HTMLElement) { | ||
| this.collisionSystem.addStaticBody(element) | ||
| this.updateOverlayDebug() | ||
| } | ||
|
|
||
| /** Remove a previously added static overlay element. */ | ||
| removeStaticOverlayElement(element: HTMLElement) { | ||
| this.collisionSystem.removeStaticBody(element) | ||
| this.updateOverlayDebug() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Public overlay element methods lack input validation.
addStaticOverlayElement and removeStaticOverlayElement don't validate the element parameter. If called with null or an invalid element, the collision system could throw or leave stale state.
Apply this diff:
addStaticOverlayElement(element: HTMLElement) {
+ if (!element || !(element instanceof HTMLElement)) {
+ console.warn('addStaticOverlayElement: invalid element', element)
+ return
+ }
this.collisionSystem.addStaticBody(element)
this.updateOverlayDebug()
}
removeStaticOverlayElement(element: HTMLElement) {
+ if (!element || !(element instanceof HTMLElement)) {
+ console.warn('removeStaticOverlayElement: invalid element', element)
+ return
+ }
this.collisionSystem.removeStaticBody(element)
this.updateOverlayDebug()
}🤖 Prompt for AI Agents
In src/lib/clothSceneController.ts around lines 1062 to 1071, the public methods
addStaticOverlayElement and removeStaticOverlayElement must validate their
element parameter before using the collision system: ensure element is
non-null/undefined and is an instance of HTMLElement (or otherwise has the
expected DOM API), and if invalid either throw a clear TypeError or return early
after logging a warning; only call
this.collisionSystem.addStaticBody/removeStaticBody and this.updateOverlayDebug
when the element passes validation to avoid exceptions or stale state.
| private installLayoutObserver(elements: HTMLElement[]) { | ||
| if (this.layoutObserver) return | ||
| try { | ||
| this.layoutObserver = new ResizeObserver((entries) => { | ||
| for (const entry of entries) { | ||
| const el = entry.target as HTMLElement | ||
| if (!this.items.has(el)) continue | ||
| // schedule a deferred recapture/rebuild after layout settles | ||
| const existing = this.recaptureTimers.get(el) | ||
| if (existing) clearTimeout(existing) | ||
| const timer = window.setTimeout(() => { | ||
| // bail if controller disposed | ||
| if (this.disposed) return | ||
| // compute segments using current auto-tessellation | ||
| const rect = el.getBoundingClientRect() | ||
| const seg = this.computeAutoSegments(rect, this.debug.tessellationSegments) | ||
| const item = this.items.get(el) | ||
| if (!item?.isActive) { | ||
| // Static: do not capture or mount. Just refresh collision/overlay from DOMRect. | ||
| this.collisionSystem.refresh() | ||
| this.updateOverlayDebug() | ||
| this.recaptureTimers.delete(el) | ||
| return | ||
| } | ||
| // Active cloth: re-bake | ||
| this.pool?.prepare(el, seg, { force: true, reason: 'layout-settled' }) | ||
| .then(() => { | ||
| if (this.disposed) return | ||
| this.pool?.mount(el) | ||
| this.pool?.resetGeometry(el) | ||
| // Update transforms and overlay | ||
| const rec = this.pool?.getRecord(el) | ||
| if (rec) this.domToWebGL?.updateMeshTransform(el, rec) | ||
| this.collisionSystem.refresh() | ||
| this.updateOverlayDebug() | ||
| }) | ||
| .catch(() => { /* ignore capture failures */ }) | ||
| .finally(() => { | ||
| this.recaptureTimers.delete(el) | ||
| }) | ||
| }, ClothSceneController.LAYOUT_COOL_OFF_MS) | ||
| this.recaptureTimers.set(el, timer) | ||
| } | ||
| }) | ||
| for (const el of elements) this.layoutObserver.observe(el) | ||
| } catch { | ||
| // ResizeObserver not available; skip auto-recapture | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Layout observer lacks comprehensive error handling.
The installLayoutObserver method wraps the entire ResizeObserver creation in a try-catch (lines 1446-1491), but the inner logic (lines 1454-1486) could fail in multiple ways (disposed controller, missing pool, invalid elements). The empty catch at line 1489 swallows all failures.
Add granular error handling:
this.layoutObserver = new ResizeObserver((entries) => {
+ try {
for (const entry of entries) {
const el = entry.target as HTMLElement
if (!this.items.has(el)) continue
// ... existing logic ...
}
+ } catch (error) {
+ console.error('ResizeObserver callback failed', error)
+ }
})
for (const el of elements) this.layoutObserver.observe(el)
- } catch {
+ } catch (error) {
- // ResizeObserver not available; skip auto-recapture
+ console.warn('ResizeObserver not available; skip auto-recapture', error)
}📝 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.
| private installLayoutObserver(elements: HTMLElement[]) { | |
| if (this.layoutObserver) return | |
| try { | |
| this.layoutObserver = new ResizeObserver((entries) => { | |
| for (const entry of entries) { | |
| const el = entry.target as HTMLElement | |
| if (!this.items.has(el)) continue | |
| // schedule a deferred recapture/rebuild after layout settles | |
| const existing = this.recaptureTimers.get(el) | |
| if (existing) clearTimeout(existing) | |
| const timer = window.setTimeout(() => { | |
| // bail if controller disposed | |
| if (this.disposed) return | |
| // compute segments using current auto-tessellation | |
| const rect = el.getBoundingClientRect() | |
| const seg = this.computeAutoSegments(rect, this.debug.tessellationSegments) | |
| const item = this.items.get(el) | |
| if (!item?.isActive) { | |
| // Static: do not capture or mount. Just refresh collision/overlay from DOMRect. | |
| this.collisionSystem.refresh() | |
| this.updateOverlayDebug() | |
| this.recaptureTimers.delete(el) | |
| return | |
| } | |
| // Active cloth: re-bake | |
| this.pool?.prepare(el, seg, { force: true, reason: 'layout-settled' }) | |
| .then(() => { | |
| if (this.disposed) return | |
| this.pool?.mount(el) | |
| this.pool?.resetGeometry(el) | |
| // Update transforms and overlay | |
| const rec = this.pool?.getRecord(el) | |
| if (rec) this.domToWebGL?.updateMeshTransform(el, rec) | |
| this.collisionSystem.refresh() | |
| this.updateOverlayDebug() | |
| }) | |
| .catch(() => { /* ignore capture failures */ }) | |
| .finally(() => { | |
| this.recaptureTimers.delete(el) | |
| }) | |
| }, ClothSceneController.LAYOUT_COOL_OFF_MS) | |
| this.recaptureTimers.set(el, timer) | |
| } | |
| }) | |
| for (const el of elements) this.layoutObserver.observe(el) | |
| } catch { | |
| // ResizeObserver not available; skip auto-recapture | |
| } | |
| } | |
| private installLayoutObserver(elements: HTMLElement[]) { | |
| if (this.layoutObserver) return | |
| try { | |
| this.layoutObserver = new ResizeObserver((entries) => { | |
| try { | |
| for (const entry of entries) { | |
| const el = entry.target as HTMLElement | |
| if (!this.items.has(el)) continue | |
| // schedule a deferred recapture/rebuild after layout settles | |
| const existing = this.recaptureTimers.get(el) | |
| if (existing) clearTimeout(existing) | |
| const timer = window.setTimeout(() => { | |
| // bail if controller disposed | |
| if (this.disposed) return | |
| // compute segments using current auto-tessellation | |
| const rect = el.getBoundingClientRect() | |
| const seg = this.computeAutoSegments(rect, this.debug.tessellationSegments) | |
| const item = this.items.get(el) | |
| if (!item?.isActive) { | |
| // Static: do not capture or mount. Just refresh collision/overlay from DOMRect. | |
| this.collisionSystem.refresh() | |
| this.updateOverlayDebug() | |
| this.recaptureTimers.delete(el) | |
| return | |
| } | |
| // Active cloth: re-bake | |
| this.pool?.prepare(el, seg, { force: true, reason: 'layout-settled' }) | |
| .then(() => { | |
| if (this.disposed) return | |
| this.pool?.mount(el) | |
| this.pool?.resetGeometry(el) | |
| // Update transforms and overlay | |
| const rec = this.pool?.getRecord(el) | |
| if (rec) this.domToWebGL?.updateMeshTransform(el, rec) | |
| this.collisionSystem.refresh() | |
| this.updateOverlayDebug() | |
| }) | |
| .catch(() => { /* ignore capture failures */ }) | |
| .finally(() => { | |
| this.recaptureTimers.delete(el) | |
| }) | |
| }, ClothSceneController.LAYOUT_COOL_OFF_MS) | |
| this.recaptureTimers.set(el, timer) | |
| } | |
| } catch (error) { | |
| console.error('ResizeObserver callback failed', error) | |
| } | |
| }) | |
| for (const el of elements) this.layoutObserver.observe(el) | |
| } catch (error) { | |
| console.warn('ResizeObserver not available; skip auto-recapture', error) | |
| } | |
| } |
Stacked on feat/mantine-ui-migration.
All tests: 116/116 green; build passes.