[Draft] PhysicsRegistry + SAT helpers + events + perf overlay + Project Newton scaffolding #44
Conversation
…wire into controller; emit on resize/scroll - Introduce PhysicsRegistry (cloth, rigid-static) with add/update/remove events - Controller subscribes; seeds static AABBs; removes on detach - Overlay AABBs refresh on changes - Tests: registry discovery/diffing/events refs #43
…itution + friction helpers
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdds a runtime event system and perf monitor, a DOM-driven PhysicsRegistry, SAT OBB-vs-AABB collision and a RigidSystem, integrates these into the cloth scene controller and debug UI, adds tests/docs, and updates CI and PR policy workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App.tsx
participant Debug as DebugPalette
participant Events as EventsPanel
participant Bus as globalEventBus
App->>Debug: provide showPerf & callbacks
Debug->>App: toggle showPerf
App->>Events: open=true
Events->>Bus: on(listener)
Engine->>Bus: emit(EngineEvent)
Bus->>Events: listener(event)
Events->>Events: append & truncate(100)
sequenceDiagram
participant Controller as ClothSceneController
participant Registry as PhysicsRegistry
participant DOM
participant Perf as perfMonitor
Controller->>Registry: discover(root)
Registry->>DOM: querySelectorAll(.cloth-enabled, .rigid-*)
DOM-->>Registry: elements[]
Registry-->>Controller: registry:add / registry:update / registry:remove
Controller->>Perf: begin()
Controller->>Engine: run systems (fixed/frame)
Controller->>Perf: end('system')
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that 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: 29
📜 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 (15)
.github/workflows/ci.yml(1 hunks).github/workflows/pr-policy.yml(1 hunks)docs/issues/physics-registry.md(1 hunks)src/App.tsx(7 hunks)src/app/EventsPanel.tsx(1 hunks)src/app/PerfOverlay.tsx(1 hunks)src/engine/events/__tests__/events.test.ts(1 hunks)src/engine/events/eventBus.ts(1 hunks)src/engine/events/types.ts(1 hunks)src/engine/perf/perfMonitor.ts(1 hunks)src/engine/registry/PhysicsRegistry.ts(1 hunks)src/engine/registry/__tests__/physicsRegistry.spec.ts(1 hunks)src/lib/__tests__/satObbAabb.test.ts(1 hunks)src/lib/clothSceneController.ts(7 hunks)src/lib/collision/satObbAabb.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/engine/events/__tests__/events.test.ts (2)
src/engine/events/types.ts (2)
EngineEvent(30-30)validateEngineEvent(40-63)src/engine/events/eventBus.ts (1)
EventBus(8-26)
src/app/PerfOverlay.tsx (1)
src/engine/perf/perfMonitor.ts (1)
perfMonitor(32-32)
src/engine/registry/__tests__/physicsRegistry.spec.ts (1)
src/engine/registry/PhysicsRegistry.ts (3)
describe(64-93)PhysicsRegistry(29-113)RegistryEvent(23-27)
src/engine/events/eventBus.ts (1)
src/engine/events/types.ts (2)
EngineEvent(30-30)validateEngineEvent(40-63)
src/lib/__tests__/satObbAabb.test.ts (1)
src/lib/collision/satObbAabb.ts (4)
OBB(2-2)AABB(3-3)obbVsAabb(12-65)applyRestitutionFriction(102-114)
src/app/EventsPanel.tsx (2)
src/engine/events/types.ts (1)
EngineEvent(30-30)src/engine/events/eventBus.ts (1)
globalEventBus(28-28)
src/lib/clothSceneController.ts (3)
src/engine/registry/PhysicsRegistry.ts (2)
PhysicsRegistry(29-113)RegistryEvent(23-27)src/engine/perf/perfMonitor.ts (1)
perfMonitor(32-32)src/engine/types.ts (2)
EngineSystem(22-30)EngineSystemOptions(3-7)
src/App.tsx (2)
src/app/PerfOverlay.tsx (1)
PerfOverlay(5-35)src/app/EventsPanel.tsx (1)
EventsPanel(6-68)
🪛 ast-grep (0.39.6)
src/engine/registry/__tests__/physicsRegistry.spec.ts
[warning] 23-23: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 GitHub Actions: CI
src/app/EventsPanel.tsx
[error] 54-54: ESLint: Unexpected any. Specify a different type. @typescript-eslint/no-explicit-any
🪛 GitHub Check: build-and-test
src/app/EventsPanel.tsx
[failure] 54-54:
Unexpected any. Specify a different type
🪛 markdownlint-cli2 (0.18.1)
docs/issues/physics-registry.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)
10-10: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
16-16: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 23-23: too many blank lines (1 > 0)
(empty-lines)
.github/workflows/pr-policy.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 42-42: too many blank lines (1 > 0)
(empty-lines)
🔇 Additional comments (19)
.github/workflows/ci.yml (2)
5-7: Wildcard branch patterns approved.Running CI on all branches for both push and pull_request events ensures comprehensive coverage. This is standard practice.
10-19: Configuration improvements look solid.The job rename to "build-and-test" is more descriptive. Adding
fetch-depth: 0provides full git history which may be required by tooling. The explicit string formats fornode-versionandcacheare clearer than implicit types..github/workflows/pr-policy.yml (3)
1-6: Workflow metadata is correct.The trigger configuration properly covers PR lifecycle events (opened, edited, synchronize). The YAMLlint truthy warning is a style preference and can be safely ignored.
15-24: PR context and milestone checks are properly implemented.The defensive check for PR context prevents unexpected failures. Requiring a milestone enforces project management discipline.
38-41: Acceptance criteria suggestion is a nice touch.Using
core.noticeinstead of failing makes this a helpful nudge rather than a blocker. Good balance.src/engine/events/types.ts (5)
1-4: Base event structure is sensible.Using numeric timestamps for performance is standard practice. The optional tag field provides flexibility for categorization.
6-15: CollisionEvent type is well-structured.The event captures all essential collision data: participants, contact geometry (normal, MTV), impulse magnitude, and material properties. The structure aligns with standard physics engine event models.
17-22: StateEvent discriminated union is properly typed.The state transition events are well-defined. The selective inclusion of
reasonon activate/deactivate (but not wake/sleep) reflects their different semantic purposes.
34-38:isVec2type guard is correctly implemented.The guard properly narrows
unknownto the required structure. The defensive checks are appropriate.
24-28: Type definition mismatch: RegistryEventRecord doesn't match actual event structure.The review analyzes
RegistryEventRecordwithpayload: unknown, but the actual code usesRegistryEvent(from PhysicsRegistry.ts) withcurrent/previousproperties. The handler in clothSceneController.ts correctly accessesevt.current?.typeand validates before field access. The types insrc/engine/events/types.tsandsrc/engine/registry/PhysicsRegistry.tsare misaligned—RegistryEventRecordis unused by the registry implementation and its payload structure is incorrect.Likely an incorrect or invalid review comment.
src/engine/events/eventBus.ts (2)
1-6: Type definitions are clean and well-scoped.Exporting
Unsubscribewhile keepingListenerinternal is appropriate encapsulation.
22-28: Subscription management and singleton are correctly implemented.The
onmethod properly returns an unsubscribe function, and the global singleton pattern is appropriate for a centralized event bus.docs/issues/physics-registry.md (1)
16-18: Test outlines are comprehensive.The test categories properly cover both unit-level descriptor logic and integration-level event propagation.
src/lib/__tests__/satObbAabb.test.ts (2)
1-11: Test helpers are clean and focused.The constructor helpers make test setup readable and maintainable.
14-19: Separation test is adequate.The test verifies non-collision detection, which is essential.
src/app/PerfOverlay.tsx (1)
26-27: Data formatting is correct.Using
toFixed(2)for millisecond precision is appropriate, and the ternary operator properly handles optional budget values.src/engine/registry/PhysicsRegistry.ts (3)
30-30: Verify memory management strategy.The strong reference to
HTMLElementin the Map prevents garbage collection of removed elements untildiscover()processes their removal. Confirm this latency is acceptable for your use case, or document the requirement to calldiscover()after DOM mutations.
115-119: LGTM.The
parseNumhelper correctly handles edge cases: undefined input, invalid strings,NaN, andInfinityare all filtered out. Solid defensive parsing.
49-49: Critical: Document.contains does not exist.
Documentdoes not have acontains()method—onlyNodeandElementdo. Whenrootisdocument, this line will throw a runtime error. The removal check will fail catastrophically.Apply this diff to fix:
- if (!root.contains(el)) { + const container = root instanceof Document ? root.documentElement : root + if (!container.contains(el)) { this.map.delete(el) this.emit({ type: 'registry:remove', previous: prev }) }Likely an incorrect or invalid review comment.
.github/workflows/pr-policy.yml
Outdated
| if (!/Acceptance Criteria/i.test(body)) { | ||
| core.notice('Consider adding an "Acceptance Criteria" section to the PR description.') | ||
| } | ||
There was a problem hiding this comment.
Remove the trailing blank line.
YAMLlint reports excessive blank lines at the end of the file.
}
-Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 42-42: too many blank lines (1 > 0)
(empty-lines)
🤖 Prompt for AI Agents
In .github/workflows/pr-policy.yml around line 42 (the end of the file), remove
the trailing blank line so the file ends immediately after the last YAML content
line; ensure there is no extra newline or blank row after that final line to
satisfy yamllint rules.
…y adds; stronger types/guards; UI polish - Move addSystem wrapper before adding camera/render/overlay systems - Process registry:add cloth after startup; clear pending list - Strengthen PhysicsRegistry event typing, data-phys-type validation, id handling, attrs equality; error isolation; docs minor - EventBus: listener isolation + portable env check - EventsPanel: remove any; use optional top-level tag - PerfOverlay: lazy init, error handling, theme color, empty state, interval prop - App toast effect: stable ref + cleanup - Remove stale collider on cloth removal - CI: allowlist + escape hatch for tests-first; trim trailing blank line - Tests: SAT stronger assertions; EventBus resilience
…ler via PhysicsRegistry - New RigidSystem (fixedUpdate): OBB dynamics vs static AABBs (SAT) - Controller installs rigid system and maps registry rigid-dynamic descriptors to bodies - Emits collision EngineEvents via global EventBus - Tests: rigidSystem integration + collision emit
|
Acceptance Criteria
|
There was a problem hiding this comment.
Actionable comments posted: 25
♻️ Duplicate comments (3)
src/lib/__tests__/satObbAabb.test.ts (1)
35-41: Rotated collision test is still a smoke test—validate MTV correctness.Lines 38-40 only check
collidedanddepth > 0. Past review requested MTV validation, which wasn't addressed. For a rotated OBB, you should verify:
- MTV magnitude equals depth:
Math.hypot(mtv.x, mtv.y) ≈ depth- MTV direction is reasonable (project back from OBB center to AABB center)
- Normal is unit:
Math.hypot(normal.x, normal.y) ≈ 1Add assertions:
expect(res.collided).toBe(true) expect(res.depth).toBeGreaterThan(0) + const mtvMag = Math.hypot(res.mtv.x, res.mtv.y) + expect(mtvMag).toBeCloseTo(res.depth, 3) + const normalMag = Math.hypot(res.normal.x, res.normal.y) + expect(normalMag).toBeCloseTo(1, 3) + // MTV should separate the shapes (dot product with separation direction should be positive) + const separation = { x: (box.min.x + box.max.x) / 2 - obb.center.x, y: (box.min.y + box.max.y) / 2 - obb.center.y } + const dot = res.normal.x * separation.x + res.normal.y * separation.y + expect(Math.abs(dot)).toBeGreaterThan(0) // normal aligns with separationsrc/engine/events/types.ts (1)
45-50: Unreadable boolean expression and missing tag validation.Line 48's condition is a cognitive nightmare: six boolean checks chained with
&&in a single expression. You're also not validating thata.tagandb.tagarestring | null | undefined—any other type will pass through.Apply this diff to improve readability and add tag validation:
case 'collision': { const a = (ev.a as UnknownRecord | undefined) const b = (ev.b as UnknownRecord | undefined) - const ok = !!(a && typeof a.id === 'string' && b && typeof b.id === 'string' && isVec2(ev.normal) && isVec2(ev.mtv) && typeof ev.impulse === 'number') - return ok + if (!a || typeof a.id !== 'string') return false + if (!b || typeof b.id !== 'string') return false + if (a.tag !== undefined && a.tag !== null && typeof a.tag !== 'string') return false + if (b.tag !== undefined && b.tag !== null && typeof b.tag !== 'string') return false + if (!isVec2(ev.normal)) return false + if (!isVec2(ev.mtv)) return false + if (typeof ev.impulse !== 'number') return false + return true }src/engine/registry/PhysicsRegistry.ts (1)
106-106: Theactivefield is hard-coded and never updated.Line 106 always sets
active: false, and the class exposes no method to flip it totrue. Either this field is dead code, or the activation contract is missing. If external code (e.g.,ClothSceneController) is expected to mutate descriptors directly, document that explicitly. Otherwise, provide a publicsetActive(id: string, active: boolean)method that updates the descriptor and emitsregistry:update.Do you want me to generate a
setActivemethod or clarify the lifecycle contract in the class documentation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.github/workflows/ci.yml(1 hunks).github/workflows/pr-policy.yml(1 hunks)src/App.tsx(7 hunks)src/app/EventsPanel.tsx(1 hunks)src/app/PerfOverlay.tsx(1 hunks)src/engine/events/__tests__/events.test.ts(1 hunks)src/engine/events/eventBus.ts(1 hunks)src/engine/events/types.ts(1 hunks)src/engine/registry/PhysicsRegistry.ts(1 hunks)src/engine/systems/__tests__/rigidSystem.test.ts(1 hunks)src/engine/systems/rigidSystem.ts(1 hunks)src/lib/__tests__/satObbAabb.test.ts(1 hunks)src/lib/clothSceneController.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/engine/events/__tests__/events.test.ts (2)
src/engine/events/types.ts (2)
EngineEvent(30-30)validateEngineEvent(40-69)src/engine/events/eventBus.ts (1)
EventBus(8-44)
src/engine/events/eventBus.ts (1)
src/engine/events/types.ts (2)
EngineEvent(30-30)validateEngineEvent(40-69)
src/app/EventsPanel.tsx (2)
src/engine/events/types.ts (1)
EngineEvent(30-30)src/engine/events/eventBus.ts (1)
globalEventBus(46-46)
src/lib/__tests__/satObbAabb.test.ts (1)
src/lib/collision/satObbAabb.ts (4)
OBB(2-2)AABB(3-3)obbVsAabb(12-65)applyRestitutionFriction(102-114)
src/lib/clothSceneController.ts (4)
src/engine/registry/PhysicsRegistry.ts (3)
PhysicsRegistry(28-141)RegistryEvent(23-26)PhysicsDescriptor(11-19)src/engine/systems/rigidSystem.ts (2)
RigidSystem(21-85)DynamicBody(9-19)src/engine/perf/perfMonitor.ts (1)
perfMonitor(32-32)src/engine/types.ts (2)
EngineSystem(22-30)EngineSystemOptions(3-7)
src/engine/systems/__tests__/rigidSystem.test.ts (3)
src/engine/systems/rigidSystem.ts (2)
AABB(7-7)RigidSystem(21-85)src/engine/events/types.ts (1)
EngineEvent(30-30)src/engine/events/eventBus.ts (1)
globalEventBus(46-46)
src/app/PerfOverlay.tsx (1)
src/engine/perf/perfMonitor.ts (1)
perfMonitor(32-32)
src/engine/systems/rigidSystem.ts (3)
src/engine/types.ts (1)
EngineSystem(22-30)src/lib/collision/satObbAabb.ts (3)
OBB(2-2)obbVsAabb(12-65)applyRestitutionFriction(102-114)src/engine/events/eventBus.ts (1)
globalEventBus(46-46)
src/App.tsx (2)
src/app/PerfOverlay.tsx (1)
PerfOverlay(5-47)src/app/EventsPanel.tsx (1)
EventsPanel(6-68)
🔇 Additional comments (32)
.github/workflows/ci.yml (4)
14-15: Verify whether full Git history is required.
fetch-depth: 0clones the entire repository history, which increases checkout latency. This is justified only if your workflow needs full history (e.g., semantic versioning, git-log analysis, or history-dependent tools).If full history is not needed, revert to the default shallow clone:
- uses: actions/checkout@v4 - with: - fetch-depth: 0
18-19: Node version and cache configuration is correct.Properly formatted as strings. No issues here.
21-21: Lint before test is a solid gate.Adding
npm run lintbeforenpm test -- --runenforces code quality checks upstream and aligns with the PR objective of establishing a lint+test workflow. This is good practice.
1-22: No trailing blank line detected.Past linting issue regarding excessive blank lines at EOF has been resolved. File structure is clean.
.github/workflows/pr-policy.yml (1)
55-59: LGTM—optional acceptance criteria nudge is appropriately non-blocking.The case-insensitive check and notice-level feedback strike the right balance for encouraging good PR hygiene without being draconian.
src/lib/__tests__/satObbAabb.test.ts (2)
5-11: LGTM—helpers improve test readability.The factory functions reduce boilerplate and make test setup clear.
43-51: LGTM—restitution and friction calculations are now validated precisely.Lines 48 and 50 correctly assert:
- Normal component:
-2 * -0.5 = 1(reversed and damped)- Tangential component:
1 * (1 - 0.2) = 0.8(friction applied)This addresses the past review's concern about weak assertions.
src/App.tsx (4)
118-120: Unnecessary function wrapping—pass the callback directly.Lines 118-120 wrap
onOpenChange(false)in an arrow function, butonOpenChangealready accepts a boolean. The Drawer'sonCloseprop expects() => void, so the wrapper is required.Wait—actually, checking the types:
onOpenChangeis(open: boolean) => void, and Drawer'sonCloseis() => void. The wrapper is needed.Never mind—this is correct.
147-161: LGTM—UI additions follow existing patterns and provide clear explanatory text.The wireframe explainer (lines 150-152) correctly clarifies the distinction between the simulation mesh and DOM element, which is a common user confusion point.
334-344: LGTM—toast effect state tracking is now correct.Line 340 updates
prevOpen.currenton every run, and the cleanup (lines 341-343) guards the timeout ID. This fixes the past review's concern about retriggering on unrelated renders.
390-394: LGTM—events panel shortcut follows established pattern.The Cmd/Ctrl+E handler mirrors the existing Cmd/Ctrl+J implementation (lines 385-389), maintaining consistency.
src/app/PerfOverlay.tsx (2)
5-19: LGTM—effect properly handles errors and cleanup.Lines 6, 10, and 12-16 address all past review concerns:
- Empty array initialization prevents render-time side effects
- Error handling prevents crashes if perfMonitor isn't ready
- Configurable interval (default 300ms) allows tuning
- Cleanup correctly clears the interval
21-46: LGTM—rendering uses theme colors and handles empty state gracefully.Lines 28-34 provide user-friendly empty state feedback, and line 38's
c="red"respects the theme system. Both past review concerns are resolved.src/app/EventsPanel.tsx (1)
50-56: LGTM—tag access is now type-safe, addressing past review.Line 54's type guard correctly handles events without a
tagfield (StateEvent) and those with optionaltag(CollisionEvent, RegistryEventRecord). No moreanycasts.src/engine/events/eventBus.ts (2)
26-38: LGTM—listener exception isolation is properly implemented.Lines 27-37 ensure a throwing listener doesn't stop propagation (try-catch), and lines 30-32 handle async rejection isolation. This fully addresses the past critical review.
40-43: LGTM—subscription pattern is standard and correct.Using a
Set(line 9) prevents duplicate listeners, and the unsubscribe function (line 42) directly removes the listener. Clean implementation.src/engine/events/types.ts (3)
1-30: LGTM: Event type definitions are well-structured.The discriminated unions are correctly typed, and optional fields are clearly marked. The use of union types for EngineEvent ensures exhaustive type checking at compile time.
34-38: LGTM: isVec2 type guard is correct.The null check and property validation are appropriate for runtime type narrowing.
56-65: LGTM: registry event validation is correct.The
registry:addandregistry:removecases correctly validate theidfield, andregistry:updateproperly checks the payload structure forpreviousandcurrentkeys.src/lib/clothSceneController.ts (6)
18-26: LGTM: New imports and fields are correctly typed.The addition of
perfMonitor,RigidSystem, andPhysicsRegistryfollows proper dependency injection patterns, and the field types are appropriate.Also applies to: 353-355
425-453: LGTM: Registry initialization correctly handles deferred and late additions.The
registryReadyflag and conditional logic inapplyEvent(lines 433-437) ensure that cloth nodes discovered after initialization are properly prepared. The past issue with accumulating unprepared cloth elements has been resolved.
429-449: LGTM: Registry event handling is comprehensive.The
applyEventhandler correctly dispatches add/remove/update events to the appropriate systems (collision system, rigid system, and cloth management). The call toremoveClothForElementon line 444 addresses the previous concern about cleaning up stale colliders.
530-560: LGTM: Teardown safely handles unsubscription errors.The try-catch block (lines 530-535) prevents unsubscription failures from disrupting the teardown sequence, and all new systems (render settings, rigid) are properly removed.
357-361: LGTM: onScroll triggers registry discovery.Calling
registry?.discover(document)on scroll ensures that layout changes (e.g., elements scrolling in/out of view) are detected and processed.
971-993: LGTM: Dynamic body creation validates and clamps parameters.The method correctly converts DOM coordinates to canonical space, applies sensible defaults, and clamps
mass,restitution, andfrictionto valid ranges. TheMath.max(1e-6, ...)guards prevent zero-sized collision volumes.src/engine/registry/PhysicsRegistry.ts (7)
1-27: LGTM: Type definitions use proper discriminated unions.The
RegistryEventtype (lines 23-26) correctly enforces that each event variant carries only the appropriate payload fields, preventing nonsensical states at compile time.
32-53: LGTM: discover() correctly handles add/update/remove lifecycle.The method properly detects new elements (line 40), changed elements (line 43), and removed elements (line 50), emitting the appropriate discriminated union variants.
55-63: LGTM: Public API has clear documentation.The JSDoc comments (lines 55, 60) describe the methods' contracts, and the multi-line formatting improves readability.
66-74: LGTM: Listener errors are isolated.The try-catch block (lines 68-72) ensures that one failing listener cannot prevent others from executing, addressing the observer pattern isolation requirement.
111-126: LGTM: Equality check avoids JSON.stringify pitfalls.Comparing individual fields (lines 120-124) prevents the key-order and numeric-precision issues that plagued the previous
JSON.stringifyapproach.
128-140: LGTM: autoId uses efficient sibling traversal.The
previousElementSiblingloop (lines 134-135) computes the index in O(index) time per ancestor, avoiding the O(siblings) cost ofArray.from(...).indexOf().
143-147: LGTM: parseNum safely converts strings to numbers.The helper correctly returns
undefinedfor invalid input and checksNumber.isFiniteto reject NaN and Infinity.
| fixedUpdate(dt: number) { | ||
| const aabbs = this.getAabbs() | ||
| for (const b of this.bodies) { | ||
| // Integrate velocity (gravity in -Y canonical) | ||
| b.velocity.y -= this.gravity * dt | ||
| // Integrate position | ||
| b.center.x += b.velocity.x * dt | ||
| b.center.y += b.velocity.y * dt | ||
|
|
||
| // Collide against static AABBs via SAT (OBB vs AABB) | ||
| const obb: OBB = { center: b.center, half: b.half, rotation: b.rotation } | ||
| for (const box of aabbs) { | ||
| const res = obbVsAabb(obb, box) | ||
| if (!res.collided) continue | ||
| // Separate along MTV | ||
| b.center.x += res.mtv.x | ||
| b.center.y += res.mtv.y | ||
| // Update velocity using restitution/friction | ||
| const vAfter = applyRestitutionFriction(b.velocity, res.normal, b.restitution, b.friction) | ||
| // Approx impulse magnitude (mass * deltaV along normal) | ||
| const vn = b.velocity.x * res.normal.x + b.velocity.y * res.normal.y | ||
| const vn2 = vAfter.x * res.normal.x + vAfter.y * res.normal.y | ||
| const impulse = Math.abs((vn2 - vn) * b.mass) | ||
| b.velocity = vAfter | ||
| // Emit event | ||
| globalEventBus.emit({ | ||
| type: 'collision', | ||
| time: Date.now(), | ||
| a: { id: b.id, tag: b.tag ?? undefined }, | ||
| b: { id: 'static', tag: null }, | ||
| normal: res.normal, | ||
| mtv: res.mtv, | ||
| impulse, | ||
| restitution: b.restitution, | ||
| friction: b.friction, | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Validate getAabbs() return value.
If getAabbs() returns malformed data (null, non-array, or AABBs with NaN coordinates), the simulation will crash or silently produce garbage. Add a defensive check.
Apply this diff:
fixedUpdate(dt: number) {
const aabbs = this.getAabbs()
+ if (!Array.isArray(aabbs)) {
+ console.error('getAabbs() returned non-array value')
+ return
+ }
for (const b of this.bodies) {I'll verify the review comment by examining the implementation and type safety of getAabbs(). Let me search the codebase:
🏁 Script executed:
#!/bin/bash
# Find getAabbs() method implementation and its type signature
rg -n 'getAabbs\s*\(' src/ -A 15 -B 2
# Also search for AABB type definition
rg -n 'type.*AABB|interface.*AABB' src/ -A 3Length of output: 2810
🏁 Script executed:
#!/bin/bash
# Find the rigidSystem file to understand class structure
cat src/engine/systems/rigidSystem.ts | head -100Length of output: 2621
🏁 Script executed:
#!/bin/bash
# Search for all callers of getAabbs() to see how it's used elsewhere
rg -n 'getAabbs' src/ -B 2 -A 2Length of output: 2046
🏁 Script executed:
#!/bin/bash
# Check if there are existing null/undefined/type checks in fixedUpdate or similar methods
rg -n 'if.*!Array|if.*null|console\.error' src/engine/systems/rigidSystem.ts -B 1 -A 1Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check all methods in RigidSystem for validation/defensive patterns
cat src/engine/systems/rigidSystem.ts | grep -n 'if.*\|throw\|console\|return' | head -20Length of output: 157
🏁 Script executed:
#!/bin/bash
# Find and examine collisionSystem.getStaticAABBs() to see if validation happens upstream
rg -n 'getStaticAABBs' src/ -A 10 -B 2Length of output: 4078
Fix addresses stated concern incompletely; proposed check insufficient.
The review identifies a valid defensive programming consideration, but the proposed Array.isArray() check only validates one of three stated concerns—it ignores the NaN coordinates threat that will "silently produce garbage." Additionally:
getStaticAABBs()implementation always returns an array via.map(), making null/non-array returns impossible without violating TypeScript's declared contract() => AABB[]- RigidSystem.ts contains zero error-handling patterns (no console.error, no early returns); this check introduces inconsistency
- Suggested fix doesn't validate coordinate validity, only wrapper type
If defensive programming is warranted here, a complete solution requires validating AABB structure (min/max presence, numeric coordinates), not just the outer Array. However, this responsibility arguably belongs upstream in collisionSystem.getStaticAABBs() which owns the data contract.
| const obb: OBB = { center: b.center, half: b.half, rotation: b.rotation } | ||
| for (const box of aabbs) { | ||
| const res = obbVsAabb(obb, box) | ||
| if (!res.collided) continue | ||
| // Separate along MTV | ||
| b.center.x += res.mtv.x | ||
| b.center.y += res.mtv.y | ||
| // Update velocity using restitution/friction | ||
| const vAfter = applyRestitutionFriction(b.velocity, res.normal, b.restitution, b.friction) | ||
| // Approx impulse magnitude (mass * deltaV along normal) | ||
| const vn = b.velocity.x * res.normal.x + b.velocity.y * res.normal.y | ||
| const vn2 = vAfter.x * res.normal.x + vAfter.y * res.normal.y | ||
| const impulse = Math.abs((vn2 - vn) * b.mass) | ||
| b.velocity = vAfter | ||
| // Emit event | ||
| globalEventBus.emit({ | ||
| type: 'collision', | ||
| time: Date.now(), | ||
| a: { id: b.id, tag: b.tag ?? undefined }, | ||
| b: { id: 'static', tag: null }, | ||
| normal: res.normal, | ||
| mtv: res.mtv, | ||
| impulse, | ||
| restitution: b.restitution, | ||
| friction: b.friction, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Collision loop is O(bodies × aabbs) without spatial partitioning.
For every body, you're testing collision against every static AABB. With 100 bodies and 100 obstacles, that's 10,000 tests per frame. Consider broad-phase culling (spatial hash, quad-tree, or AABB sweep) if you expect more than a handful of bodies or obstacles.
…ailing newline fixed
…scovery; emit update
…on cases; strengthen normal checks
…fer RigidSystem install; robust teardown of active cloth bodies
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
src/lib/__tests__/satObbAabb.test.ts (1)
35-41: Rotated OBB test remains minimal despite prior review suggestion.The previous review requested assertions on MTV direction and magnitude to make this test more robust, but only
collidedanddepthare verified. This won't catch bugs in the MTV calculation for rotated collisions.Strengthen the test with MTV and normal assertions:
const res = obbVsAabb(obb, box) expect(res.collided).toBe(true) expect(res.depth).toBeGreaterThan(0) + // Verify MTV properties + const mtvMag = Math.hypot(res.mtv.x, res.mtv.y) + expect(mtvMag).toBeCloseTo(res.depth, 3) + expect(mtvMag).toBeGreaterThan(0) + // Verify normal is unit-length + const normalMag = Math.hypot(res.normal.x, res.normal.y) + expect(normalMag).toBeCloseTo(1, 3) + // Verify normal and MTV point in the same direction + const dot = res.normal.x * res.mtv.x + res.normal.y * res.mtv.y + expect(dot).toBeGreaterThan(0)src/engine/systems/rigidSystem.ts (7)
2-3: Consolidate the duplicate imports.Two separate import statements from the same module clutter the imports section needlessly.
Apply this diff:
-import { obbVsAabb, type OBB } from '../../lib/collision/satObbAabb' -import { applyRestitutionFriction } from '../../lib/collision/satObbAabb' +import { obbVsAabb, applyRestitutionFriction, type OBB } from '../../lib/collision/satObbAabb'
6-7: Stop duplicating types—import from the source module.You're redefining
Vec2andAABBlocally when they should be imported fromsatObbAabb.tswhere the collision functions expect them. Structural typing masks this today, but when those upstream definitions evolve, these copies won't follow and TypeScript won't catch the divergence. Future maintainers will waste hours debugging phantom type incompatibilities.Import the canonical types instead:
-import { obbVsAabb, applyRestitutionFriction, type OBB } from '../../lib/collision/satObbAabb' +import { obbVsAabb, applyRestitutionFriction, type OBB, type Vec2, type AABB } from '../../lib/collision/satObbAabb' - -export type Vec2 = { x: number; y: number } -export type AABB = { min: Vec2; max: Vec2 }
29-32: Validate constructor parameters to prevent corruption.The constructor blindly accepts
opts.gravitywithout checking if it'sNaN,Infinity, or even a negative value. A garbage gravity value will silently corrupt every simulation frame thereafter. ThegetAabbscallback also isn't validated.Apply this diff:
constructor(opts: { getAabbs: () => AABB[]; gravity?: number }) { + if (typeof opts.getAabbs !== 'function') { + throw new TypeError('getAabbs must be a function') + } this.getAabbs = opts.getAabbs - if (typeof opts.gravity === 'number') this.gravity = opts.gravity + if (typeof opts.gravity === 'number') { + if (!Number.isFinite(opts.gravity)) { + throw new TypeError('gravity must be a finite number') + } + this.gravity = opts.gravity + } }
34-34: Validate gravity in setGravity.You're accepting any number—
NaN,Infinity, negative values—without a single sanity check. One bad call corrupts every subsequent simulation frame silently.Apply this diff:
- setGravity(g: number) { this.gravity = g } + setGravity(g: number) { + if (!Number.isFinite(g)) { + throw new TypeError('gravity must be a finite number') + } + this.gravity = g + }
36-38: Prevent duplicate body IDs—enforce consistency with the rest of the codebase.
addBodyallows multiple bodies with identical IDs to be registered. When a collision event fires, observers can't distinguish which body actually collided. This violates the pattern established bySimulationSystemandSimWorld, which both reject duplicate IDs at registration.Apply this diff:
addBody(b: DynamicBody) { + const existing = this.bodies.find((body) => body.id === b.id) + if (existing) { + throw new Error(`Body with id "${b.id}" already exists`) + } this.bodies.push(b) }
45-53: Guard against NaN/Infinity accumulation after integration.If a body's velocity or position ever becomes
NaNorInfinity(from bad input, extreme forces, or numeric overflow), the simulation silently corrupts and every subsequent frame compounds the damage. Add a sanity check after integration.Apply this diff:
b.center.x += b.velocity.x * dt b.center.y += b.velocity.y * dt + + // Guard against NaN/Infinity corruption + if (!Number.isFinite(b.center.x) || !Number.isFinite(b.center.y) || + !Number.isFinite(b.velocity.x) || !Number.isFinite(b.velocity.y)) { + console.error(`Body "${b.id}" corrupted: center=(${b.center.x}, ${b.center.y}), velocity=(${b.velocity.x}, ${b.velocity.y})`) + // Reset to prevent cascading corruption + b.center.x = 0 + b.center.y = 0 + b.velocity.x = 0 + b.velocity.y = 0 + }
55-81: Collision loop is O(bodies × aabbs)—consider broad-phase optimization later.Every body tests collision against every static AABB. With 100 bodies and 100 obstacles, that's 10,000 tests per frame. For the current scope this is acceptable, but if the system scales, consider spatial partitioning (quad-tree, spatial hash, or sweep-and-prune).
src/engine/registry/PhysicsRegistry.ts (1)
110-110: Validate dataset.physShape before using it.Line 110 casts
el.dataset.physShapeto'circle' | 'obb' | undefinedwithout checking if the runtime value actually matches one of those strings. An arbitrary value like"triangle"will silently propagate as a valid shape, violating type contracts downstream.Apply this diff:
- shape: (el.dataset.physShape as 'circle' | 'obb' | undefined) ?? undefined, + shape: (el.dataset.physShape === 'circle' || el.dataset.physShape === 'obb') ? el.dataset.physShape : undefined,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)docs/issues/physics-registry.md(1 hunks)src/engine/registry/PhysicsRegistry.ts(1 hunks)src/engine/systems/rigidSystem.ts(1 hunks)src/lib/__tests__/satObbAabb.test.ts(1 hunks)src/lib/clothSceneController.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/engine/systems/rigidSystem.ts (3)
src/engine/types.ts (1)
EngineSystem(22-30)src/lib/collision/satObbAabb.ts (3)
OBB(2-2)obbVsAabb(12-65)applyRestitutionFriction(102-114)src/engine/events/eventBus.ts (1)
globalEventBus(46-46)
src/lib/clothSceneController.ts (4)
src/engine/registry/PhysicsRegistry.ts (3)
PhysicsRegistry(28-154)RegistryEvent(23-26)PhysicsDescriptor(11-19)src/engine/systems/rigidSystem.ts (2)
RigidSystem(21-84)DynamicBody(9-19)src/engine/perf/perfMonitor.ts (1)
perfMonitor(32-32)src/engine/types.ts (2)
EngineSystem(22-30)EngineSystemOptions(3-7)
src/lib/__tests__/satObbAabb.test.ts (1)
src/lib/collision/satObbAabb.ts (4)
OBB(2-2)AABB(3-3)obbVsAabb(12-65)applyRestitutionFriction(102-114)
src/engine/registry/PhysicsRegistry.ts (1)
src/engine/entity/entity.ts (1)
id(20-22)
🪛 markdownlint-cli2 (0.18.1)
docs/issues/physics-registry.md
19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (21)
.gitignore (1)
25-25: Missing context and rationale forfeedback.mdentry.The addition of
feedback.mdlacks explanation and is disconnected from the PR objectives (PhysicsRegistry, SAT helpers, events, perf overlay). The entry provides no comment explaining why this file is excluded or whether it's a temporary development artifact.
- Is this change intentional or accidental drift?
- What is
feedback.mdand why should it be version-controlled excluded?- Should the pattern be more generic (e.g.,
*feedback*.md,*.feedback) to cover future variants?- Should this have a section comment explaining its purpose (e.g.,
# Development/Feedback artifacts)?Consider either:
- Adding a clarifying comment above the entry (e.g.,
# Feedback/notes (development only))- Grouping it with a similar category if applicable
- Using a pattern that covers related files if needed
src/lib/__tests__/satObbAabb.test.ts (5)
5-11: Helper functions are clean and fit for purpose.Both constructors clearly map parameters to the required shape structures.
14-19: X-axis separation test is correct.The AABB is clearly beyond the OBB's right edge with a 0.4-unit gap, and the additional Y-axis and corner tests added at lines 43-55 complete the separation coverage.
43-48: Y-axis separation test is correct.The AABB is positioned with a 0.7-unit gap above the OBB, cleanly verifying Y-axis separation as requested in the previous review.
50-55: Corner near-miss test is appropriately tight.The AABB placement at (0.76, 0.76) provides approximately 0.05 units of clearance from the rotated OBB's corner, which is an excellent edge case for verifying the SAT algorithm's precision.
57-65: Restitution and friction test is precise and correct.The normal component assertion verifies proper restitution bounce-back (−2 → +1 with e=0.5), and line 64 now correctly validates the exact tangential velocity after friction scaling (1 → 0.8 with μ=0.2), addressing the previous review's concern.
src/lib/clothSceneController.ts (7)
18-26: LGTM: Imports are clean and purposeful.The new imports for
perfMonitor,PhysicsRegistry,RigidSystem, and related types support the physics registry integration and performance instrumentation features.
353-361: Registry fields and scroll handler integration look correct.Fields are properly nullable and the scroll handler correctly triggers registry discovery to keep DOM state synchronized.
425-472: LGTM: Registry integration correctly handles late-arriving cloth elements.The
registryReadyflag gates whether new cloth elements discovered after initialization are prepared immediately (lines 433-437) or queued (line 436). After the initial batch is processed (line 469),pendingClothis cleared (line 470) andregistryReadyis set (line 471), ensuring subsequentregistry:addevents trigger immediate preparation. This fixes the previous critical issue where late cloth nodes accumulated in an unused array.
530-560: Dispose correctly tears down registry and systems.Registry unsubscription is wrapped in error isolation (lines 530-535), and all systems including the new
rigidSystemare properly removed (lines 557-560). Cleanup is thorough.
752-824: LGTM: Perf instrumentation now correctly wraps all systems.The
addSystemwrapper is installed at lines 756-776 before any new systems are registered (lines 799-823). Critically, thesimulationSystemadded in the constructor is explicitly wrapped at lines 778-796 with a__perfWrappedguard to prevent double-wrapping. This addresses the previous critical issue where the simulation system bypassed instrumentation entirely. All systems now report timing data toperfMonitor.
974-992: LGTM: Cloth removal now cleans up simulation and collision state.Lines 981-985 remove the adapter from the simulation system when
item.isActiveis true, and line 989 removes the static collider from the collision system. This addresses previous critical issues where dangling simulation bodies and ghost colliders were left behind.
1000-1022: Rigid-dynamic descriptor translation is clean and safe.Proper coordinate transformation (lines 1006-1009), sensible clamping of
mass,restitution, andfriction(lines 1017-1019), and straightforward body registration (line 1021). No issues detected.src/engine/registry/PhysicsRegistry.ts (8)
1-27: Type definitions are sound and type-safe.The
RegistryEventdiscriminated union (lines 23-26) correctly enforces that each event type carries only its appropriate payloads, preventing nonsensical states likeregistry:addwithout acurrentdescriptor. Type definitions align with the documented API contract.
28-31: Class fields are appropriately encapsulated.Using a
Mapfor element-to-descriptor tracking and aSetfor listeners is correct.
32-54: Discovery logic correctly handles add/update/remove lifecycle.Line 38 preserves the
activeflag across discovery runs, ensuring activation state survives DOM updates. Event emission is properly gated by descriptor equality checks (line 42) and element containment (line 49).
56-76: Public API is well-documented and properly formatted.The
on,entries, andsetActivemethods now have clear JSDoc comments, multi-line formatting, and correct implementations. ThesetActivemethod (lines 67-76) provides the activation lifecycle that was previously missing.
78-87: Emit correctly isolates listener errors.Each listener invocation is wrapped in a try-catch block (lines 81-85), ensuring one faulty listener can't prevent others from executing. Errors are logged for debugging.
124-139: Equality comparison is robust and explicit.All descriptor fields are compared directly (lines 125-138), avoiding the fragility of
JSON.stringifycomparison. This ensures accurate change detection without false positives from key ordering or precision issues.
141-153: AutoId generation is optimized and deterministic.Using
previousElementSiblingtraversal (lines 146-148) is O(index) per ancestor rather than O(siblings), providing good performance for deep trees. The path-based ID scheme is deterministic and stable.
156-160: parseNum helper is safe and correct.Returns
undefinedfor missing or non-finite values (line 159), preventingNaNorInfinityfrom entering physics attributes.
| - [ ] Diffing works: only changed fields emitted; stable ids across runs | ||
| - [ ] Inspector/activation can subscribe to the registry | ||
|
|
||
| ## Tests (write first) |
There was a problem hiding this comment.
Fix heading spacing to satisfy markdownlint.
Missing blank line below the "## Tests" heading violates MD022. Past comments addressed identical issues at lines 6, 9, and 16—this one slipped through.
Apply this diff:
## Tests (write first)
+
- [ ] physicsRegistry.spec: discovery from a test DOM, descriptor diffing, events on resize📝 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.
| ## Tests (write first) | |
| ## Tests (write first) | |
| - [ ] physicsRegistry.spec: discovery from a test DOM, descriptor diffing, events on resize |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In docs/issues/physics-registry.md around line 19, the "## Tests" heading is
missing the required blank line below it (MD022). Insert a single blank line
immediately after the "## Tests" heading so the heading is separated from the
following content, matching the fixes already applied at lines 6, 9, and 16.
| import { describe, it, expect } from 'vitest' | ||
|
|
||
| import { obbVsAabb, type OBB, type AABB, applyRestitutionFriction } from '../collision/satObbAabb' | ||
|
|
||
| function makeOBB(cx: number, cy: number, hx: number, hy: number, rad: number): OBB { | ||
| return { center: { x: cx, y: cy }, half: { x: hx, y: hy }, rotation: rad } | ||
| } | ||
|
|
||
| function makeAABB(minx: number, miny: number, maxx: number, maxy: number): AABB { | ||
| return { min: { x: minx, y: miny }, max: { x: maxx, y: maxy } } | ||
| } | ||
|
|
||
| describe('SAT OBB vs AABB', () => { | ||
| it('detects no contact when separated', () => { | ||
| const obb = makeOBB(0, 0, 1, 0.5, 0) | ||
| const box = makeAABB(1.4, -0.2, 2.2, 0.2) | ||
| const res = obbVsAabb(obb, box) | ||
| expect(res.collided).toBe(false) | ||
| }) | ||
|
|
||
| it('returns MTV for overlapping shapes (axis-aligned)', () => { | ||
| const obb = makeOBB(0, 0, 1, 0.5, 0) | ||
| const box = makeAABB(0.8, -0.3, 1.6, 0.3) | ||
| const res = obbVsAabb(obb, box) | ||
| expect(res.collided).toBe(true) | ||
| // MTV should push OBB minimally along +X (since overlap on right side is smaller) | ||
| expect(Math.abs(res.mtv.x) + Math.abs(res.mtv.y)).toBeGreaterThan(0) | ||
| expect(Math.abs(res.mtv.x)).toBeGreaterThan(Math.abs(res.mtv.y)) | ||
| // Normal should be axis-aligned, unit, and X-dominant when |mtv.x| > |mtv.y| | ||
| const mag1 = Math.abs(res.normal.x) + Math.abs(res.normal.y) | ||
| expect(mag1).toBeCloseTo(1, 3) | ||
| expect(Math.abs(res.normal.x)).toBeGreaterThan(Math.abs(res.normal.y)) | ||
| }) | ||
|
|
||
| it('handles rotated OBB collisions with AABB', () => { | ||
| const obb = makeOBB(0.9, 0.1, 1, 0.5, Math.PI / 6) | ||
| const box = makeAABB(0.8, -0.3, 1.6, 0.3) | ||
| const res = obbVsAabb(obb, box) | ||
| expect(res.collided).toBe(true) | ||
| expect(res.depth).toBeGreaterThan(0) | ||
| }) | ||
|
|
||
| it('detects separation when AABB is entirely above OBB (no collision)', () => { | ||
| const obb = makeOBB(0, 0, 0.5, 0.5, 0) | ||
| const box = makeAABB(-0.2, 1.2, 0.2, 1.6) | ||
| const res = obbVsAabb(obb, box) | ||
| expect(res.collided).toBe(false) | ||
| }) | ||
|
|
||
| it('near-miss corner with rotated OBB does not collide', () => { | ||
| const obb = makeOBB(0, 0, 0.5, 0.5, Math.PI / 4) | ||
| const box = makeAABB(0.76, 0.76, 1.2, 1.2) // just beyond the corner | ||
| const res = obbVsAabb(obb, box) | ||
| expect(res.collided).toBe(false) | ||
| }) | ||
|
|
||
| it('applies restitution and friction to velocity along contact', () => { | ||
| const normal = { x: 1, y: 0 } // contact normal to the right | ||
| const v = { x: -2, y: 1 } // incoming from left, with upward tangent | ||
| const out = applyRestitutionFriction(v, normal, 0.5, 0.2) | ||
| // Normal component should flip and be scaled by restitution | ||
| expect(out.x).toBeCloseTo(1, 3) // 0.5 * 2 = 1 | ||
| // Tangential component should be reduced by friction factor | ||
| expect(out.y).toBeCloseTo(0.8, 3) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider edge-case coverage for production robustness.
The test suite covers the happy paths and basic separation scenarios admirably. However, a truly bulletproof collision system should validate:
- Degenerate overlaps: Perfect OBB/AABB alignment and zero-gap near-touches to ensure MTV calculation doesn't produce NaN or zero-length normals.
- Physics boundary cases:
restitution=0(perfectly inelastic),restitution=1(perfectly elastic),friction=0(frictionless),friction=1(full tangent absorption). - Rotation consistency: Verify an OBB with
rotation=0produces identical results to a comparable AABB collision. - Zero/flipped normal input to
applyRestitutionFrictionto guard against divide-by-zero or sign errors.
Example edge-case tests:
it('handles perfect overlap at origin', () => {
const obb = makeOBB(0, 0, 0.5, 0.5, 0)
const box = makeAABB(-0.5, -0.5, 0.5, 0.5)
const res = obbVsAabb(obb, box)
expect(res.collided).toBe(true)
// MTV should be well-defined even for perfect overlap
const mtvMag = Math.hypot(res.mtv.x, res.mtv.y)
expect(mtvMag).toBeGreaterThan(0)
})
it('applies zero restitution (perfectly inelastic)', () => {
const normal = { x: 0, y: 1 }
const v = { x: 1, y: -3 }
const out = applyRestitutionFriction(v, normal, 0, 0)
expect(out.y).toBeCloseTo(0, 3) // no bounce
expect(out.x).toBeCloseTo(1, 3) // tangent preserved with no friction
})
it('applies full restitution (perfectly elastic)', () => {
const normal = { x: 0, y: 1 }
const v = { x: 0, y: -2 }
const out = applyRestitutionFriction(v, normal, 1, 0)
expect(out.y).toBeCloseTo(2, 3) // full reversal
})
it('applies full friction (tangent absorbed)', () => {
const normal = { x: 1, y: 0 }
const v = { x: -1, y: 2 }
const out = applyRestitutionFriction(v, normal, 0, 1)
expect(out.y).toBeCloseTo(0, 3) // tangent fully absorbed
})🤖 Prompt for AI Agents
In src/lib/__tests__/satObbAabb.test.ts around lines 1 to 66, add focused
edge-case tests to improve robustness: include a perfect-overlap test (aligned
OBB and AABB at origin) asserting collided=true and that the MTV magnitude > 0;
add restitution/friction boundary tests for restitution=0 and restitution=1, and
friction=0 and friction=1, verifying normal/tangent components behave as
expected; add a test that calls applyRestitutionFriction with a zero or
near-zero normal vector to assert it returns a safe result (no
NaN/divide-by-zero) or handles the case gracefully; ensure each test uses
existing helper constructors and uses toBeCloseTo/greater-than assertions
consistent with the suite.
| it('returns MTV for overlapping shapes (axis-aligned)', () => { | ||
| const obb = makeOBB(0, 0, 1, 0.5, 0) | ||
| const box = makeAABB(0.8, -0.3, 1.6, 0.3) | ||
| const res = obbVsAabb(obb, box) | ||
| expect(res.collided).toBe(true) | ||
| // MTV should push OBB minimally along +X (since overlap on right side is smaller) | ||
| expect(Math.abs(res.mtv.x) + Math.abs(res.mtv.y)).toBeGreaterThan(0) | ||
| expect(Math.abs(res.mtv.x)).toBeGreaterThan(Math.abs(res.mtv.y)) | ||
| // Normal should be axis-aligned, unit, and X-dominant when |mtv.x| > |mtv.y| | ||
| const mag1 = Math.abs(res.normal.x) + Math.abs(res.normal.y) | ||
| expect(mag1).toBeCloseTo(1, 3) | ||
| expect(Math.abs(res.normal.x)).toBeGreaterThan(Math.abs(res.normal.y)) | ||
| }) |
There was a problem hiding this comment.
PAST REVIEW COMMENT NOT ADDRESSED: L1 norm and missing sign checks.
Line 30 still uses L1 norm (Math.abs(x) + Math.abs(y)) instead of the L2 norm requested in the previous review. This is mathematically incorrect for verifying unit-length normals—it only works for axis-aligned vectors by coincidence. For a rotated normal like (0.707, 0.707), L1 = 1.414 while L2 = 1.
Additionally, the sign consistency checks between normal and mtv requested in the prior review are absent. The normal could point opposite to the MTV and this test would still pass.
Apply the diff from the previous review to fix both issues:
- const mag1 = Math.abs(res.normal.x) + Math.abs(res.normal.y)
+ const mag1 = Math.hypot(res.normal.x, res.normal.y)
expect(mag1).toBeCloseTo(1, 3)
expect(Math.abs(res.normal.x)).toBeGreaterThan(Math.abs(res.normal.y))
+ // Normal should point in MTV direction
+ if (Math.abs(res.mtv.x) > Math.abs(res.mtv.y)) {
+ expect(Math.sign(res.normal.x)).toBe(Math.sign(res.mtv.x))
+ expect(res.normal.y).toBeCloseTo(0, 3)
+ }📝 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.
| it('returns MTV for overlapping shapes (axis-aligned)', () => { | |
| const obb = makeOBB(0, 0, 1, 0.5, 0) | |
| const box = makeAABB(0.8, -0.3, 1.6, 0.3) | |
| const res = obbVsAabb(obb, box) | |
| expect(res.collided).toBe(true) | |
| // MTV should push OBB minimally along +X (since overlap on right side is smaller) | |
| expect(Math.abs(res.mtv.x) + Math.abs(res.mtv.y)).toBeGreaterThan(0) | |
| expect(Math.abs(res.mtv.x)).toBeGreaterThan(Math.abs(res.mtv.y)) | |
| // Normal should be axis-aligned, unit, and X-dominant when |mtv.x| > |mtv.y| | |
| const mag1 = Math.abs(res.normal.x) + Math.abs(res.normal.y) | |
| expect(mag1).toBeCloseTo(1, 3) | |
| expect(Math.abs(res.normal.x)).toBeGreaterThan(Math.abs(res.normal.y)) | |
| }) | |
| it('returns MTV for overlapping shapes (axis-aligned)', () => { | |
| const obb = makeOBB(0, 0, 1, 0.5, 0) | |
| const box = makeAABB(0.8, -0.3, 1.6, 0.3) | |
| const res = obbVsAabb(obb, box) | |
| expect(res.collided).toBe(true) | |
| // MTV should push OBB minimally along +X (since overlap on right side is smaller) | |
| expect(Math.abs(res.mtv.x) + Math.abs(res.mtv.y)).toBeGreaterThan(0) | |
| expect(Math.abs(res.mtv.x)).toBeGreaterThan(Math.abs(res.mtv.y)) | |
| // Normal should be axis-aligned, unit, and X-dominant when |mtv.x| > |mtv.y| | |
| const mag1 = Math.hypot(res.normal.x, res.normal.y) | |
| expect(mag1).toBeCloseTo(1, 3) | |
| expect(Math.abs(res.normal.x)).toBeGreaterThan(Math.abs(res.normal.y)) | |
| // Normal should point in MTV direction | |
| if (Math.abs(res.mtv.x) > Math.abs(res.mtv.y)) { | |
| expect(Math.sign(res.normal.x)).toBe(Math.sign(res.mtv.x)) | |
| expect(res.normal.y).toBeCloseTo(0, 3) | |
| } | |
| }) |
flyingrobots
changed the title
Project: Newton — Unified Physics Spec (Draft)
Status: Draft
Owner: @flyingrobots
Branch: feat/physics-registry (PR #44)
Milestone: PROJECT: Newton
1. Goals (What/Why)
PhysicsSystem.Rationale: The demo needs believable rigid interactions, robust cloth, and a clean mental model. A single system with two lanes (rigid + cloth) makes order and perf explicit and testable.
2. Scope / Out of Scope
3. Coordinate System & Time
4. System Architecture
4.1 PhysicsSystem (EngineSystem)
4.2 WorldXform (Component)
GetPosition(): Vector3GetRotationMat3(): Matrix3x3GetRotationQuat(): QuatAs4x4(): Matrix4(GPU‑friendly)WorldToLocal(p: Vector3): Vector3LocalToWorld(p: Vector3): Vector3ComputeWorldAABB(colliderLocal): AABB2(cached each step)ComputeWorldSphere(colliderLocal): { center: Vec2, radius: number }(cached each step)4.3 PhysicsBody (Local Space)
collider: { kind: 'obb'|'circle'|'polygon', ...local params }{ mass, friction, restitution, density?, centerOfMass? }position, angle, velocity, angularVelocity, invMass, invInertiarayIntersect(localRay): number | null(t along ray)applyImpulse(impulse: Vec2, localContactPoint?: Vec2): voidgetExtents(): [axisX: Vec2, axisY: Vec2](SAT axes; polygon returns axes set)4.4 CollisionSystem (Service)
5. Rigid Dynamics (OBB v1)
j = -(1+e) * v_rel·n / (Σ(invMass + (r×n)^2 * invInertia))jtto|jt| ≤ μ·j.I = m*(w^2+h^2)/12;invInertia = 1/I(0 for infinite).6. Cloth ↔ Rigid Collisions
7. Picking
WorldToLocal.PhysicsBody.rayIntersect(localRay); pick nearestt.pickevent; overlay marker; inspector shows body props.8. Events
collision(rigid contacts; later cloth sphere contacts optionally):{ time, type:'collision', a:{id,tag?}, b:{id,tag?}, normal, mtv, impulse, restitution, friction }wake|sleep|activate|deactivate(per body)pick{ time, type:'pick', id, tag?, rayWorld:{origin,dir}, hitLocal:{x,y}, t }9. Perf & FPS
physics:rigid:fixed,physics:cloth:fixed,render:frame,overlay:frame.10. UI / Debug
11. Testing
12. Milestones (Issues, all under PROJECT: Newton)
13. Acceptance Criteria
Appendix A: Initial Shape Set
center:{x,y}, half:{x,y}, angle(rad)center:{x,y}, radiuspoints: Vec2[](convex)Appendix B: Ray Conventions
origin + t*dir,t ≥ 0, withdirnormalized;rayIntersectreturns smallesttornull.