feat: add dynamic collider force at position#3039
Conversation
luzhuang
force-pushed
the
feat/physics-force-at-position
branch
from
ef1ef42 to
20629f9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3039 +/- ##
========================================
Coverage 79.17% 79.17%
========================================
Files 914 914
Lines 101666 101695 +29
Branches 11209 11217 +8
========================================
+ Hits 80489 80520 +31
+ Misses 20990 20989 -1
+ Partials 187 186 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds ChangesapplyForceAtPosition Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
luzhuang
force-pushed
the
feat/physics-force-at-position
branch
from
20629f9 to
55d2f44
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/physics-lite/src/LiteDynamicCollider.ts (1)
55-56: ⚡ Quick winThrow an
Errorobject instead of a string literal.At Line 56, string throws make downstream error handling less reliable (
instanceof Error, stack, message shape). Use anErrorinstance here for consistent runtime behavior.Proposed fix
addForceAtPosition(force: Vector3, position: Vector3): void { - throw "Physics-lite don't support addForceAtPosition. Use Physics-PhysX instead!"; + throw new Error("Physics-lite don't support addForceAtPosition. Use Physics-PhysX instead!"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/physics-lite/src/LiteDynamicCollider.ts` around lines 55 - 56, In the addForceAtPosition method, replace the string literal being thrown with a proper Error object instance. Change the throw statement from throwing a plain string to throwing new Error with the same message text. This ensures proper error handling downstream with instanceof checks and provides correct stack trace information.notes/physics/2026-06-17-physx-force-at-position.md (1)
21-32: ⚡ Quick winConsider reformatting the Verification section as a pre-merge/release checklist.
The verification section documents steps already taken, which is valuable. However, for maintainers reviewing before merge or release, it would be clearer to present this as an actionable checklist with pre-merge gates explicitly called out.
Suggestion: Add a structured checklist format (e.g., markdown checkbox list or table) that clarifies:
- Which steps are pre-merge blockers (e.g., CDN runtime validation per the Rollout Gate section).
- Which steps are verification-only (e.g., coverage runs).
- Whether each step is a one-time verification or a repeatable gating check.
Example structure:
## Pre-Merge Checklist - [ ] **CDN runtime**: Default `PhysXPhysics` runtime URLs point to CDN with `PxRigidDynamic.prototype.addForceAtPos` and related helpers. - [ ] **Local build & smoke test**: Built and smoke-tested local runtime. - [ ] **Tests**: Run `HEADLESS=true pnpm exec vitest run --config tests/vitest.config.ts tests/src/core/physics/DynamicCollider.test.ts` (expect 27 passed). - [ ] **Coverage**: Run `npm run coverage` (expect 111 files and 1449 tests passed). - [ ] **Type build**: Run `npm run b:types` (expect success).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notes/physics/2026-06-17-physx-force-at-position.md` around lines 21 - 32, The Verification section in the file currently documents steps already taken in a bullet-list format, but it should be restructured as an actionable pre-merge checklist for future maintainers. Replace the existing Verification section with a new "Pre-Merge Checklist" section that uses markdown checkboxes and clearly categorizes each step. Distinguish between pre-merge blockers (such as CDN runtime validation) and verification-only steps (such as coverage runs). For each checklist item, include the command to run and expected outcome. This will make it explicit which gates must pass before merging and which are informational verifications.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@notes/physics/2026-06-17-physx-force-at-position.md`:
- Around line 21-32: The Verification section in the file currently documents
steps already taken in a bullet-list format, but it should be restructured as an
actionable pre-merge checklist for future maintainers. Replace the existing
Verification section with a new "Pre-Merge Checklist" section that uses markdown
checkboxes and clearly categorizes each step. Distinguish between pre-merge
blockers (such as CDN runtime validation) and verification-only steps (such as
coverage runs). For each checklist item, include the command to run and expected
outcome. This will make it explicit which gates must pass before merging and
which are informational verifications.
In `@packages/physics-lite/src/LiteDynamicCollider.ts`:
- Around line 55-56: In the addForceAtPosition method, replace the string
literal being thrown with a proper Error object instance. Change the throw
statement from throwing a plain string to throwing new Error with the same
message text. This ensures proper error handling downstream with instanceof
checks and provides correct stack trace information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7bf2b4b0-7b20-47d6-a702-e2e2bbc4bee3
⛔ Files ignored due to path filters (2)
packages/physics-physx/libs/physx.release.simd.wasmis excluded by!**/*.wasmpackages/physics-physx/libs/physx.release.wasmis excluded by!**/*.wasm
📒 Files selected for processing (7)
notes/physics/2026-06-17-physx-force-at-position.mdpackages/core/src/physics/DynamicCollider.tspackages/design/src/physics/IDynamicCollider.tspackages/physics-lite/src/LiteDynamicCollider.tspackages/physics-physx/src/PhysXDynamicCollider.tspackages/physics-physx/src/PhysXPhysics.tstests/src/core/physics/DynamicCollider.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/physics-physx/src/PhysXPhysics.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/design/src/physics/IDynamicCollider.ts
- packages/core/src/physics/DynamicCollider.ts
- packages/physics-physx/src/PhysXDynamicCollider.ts
- tests/src/core/physics/DynamicCollider.test.ts
luzhuang
force-pushed
the
feat/physics-force-at-position
branch
from
55d2f44 to
0beedf2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
luzhuang
force-pushed
the
feat/physics-force-at-position
branch
from
0beedf2 to
32582b2
Compare
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
第三轮（HEAD 32582b2f，相比上轮 55d2f448b）。相比上轮，实质代码逻辑（四层 API / lite throw / PhysX 委托 / CDN URL bump）完全无变化，唯一新增是 tests/src/core/physics/PhysicsScene.test.ts 里一个 lite 「unsupported behavior」测试（notes/ 文件只改了一行测试计数 1449→1450）。这个测试补上了我第一轮指出的 codecov「lite throw 分支未覆盖」缺口，方向正确。
问题
无 P0/P1/P2。
-
[P3]
PhysicsScene.test.ts:141新测试未root.destroy()清理 — 新测试const root = ...createRootEntity("root")+addComponent(DynamicCollider)，结尾没有root.destroy()。本 describe 块内几乎所有其他测试（removeShape167 行起，及 326/536/570/602/632/662/714/767/809… 全部）都在结尾root.destroy()，本测试偏离了这一既有惯例。已核实不构成正确性问题：该 collider 没addShape，lite_update只遍历 shape（LitePhysicsScene._updateWorldBox/ 碰撞检测走 shape 列表），无 shape 的 dynamic collider 不进 broadphase、不会被触碰，残留对后续removeShape等共享engineLite.activeScene的测试无干扰。建议结尾补root.destroy()与文件惯例对齐。不阻塞。 -
[P3]
notes/physics/2026-06-17-physx-force-at-position.md（前两轮已提，本轮仍未处理） — CR/工程笔记仍在引擎源码树里。再次核实：notes/在dev/2.0不存在、不在.gitignore、非既有约定。建议从 PR 移除（信息留 PR 描述 / commit / wiki），或要长期留存就改成 design doc 放进packages/。不阻塞。
新测试核对（已离线证伪，通过）
- 6 个断言字符串逐条对齐 lite 实际 throw — 测试映射
sleep()→"putToSleep"、wakeUp()→"wakeUp"、isSleeping()→"isSleeping"，与LiteDynamicCollider实际 throw（addForce:49 /addTorque:56 /putToSleep:70 /isSleeping:77 /wakeUp:205 / 新增addForceAtPosition）逐字一致。 - 守卫短路风险已排除 — core
DynamicCollider.applyForce/applyForceAtPosition/applyTorque带_phasedActiveInScene &&守卫（358/366 行），若 collider 非 active 则 native throw 不会触发、error留undefined、断言会 fail。已核实：beforeAll调了engineLite.run()，root 经createRootEntity默认 active、component 默认 enabled ⇒_phasedActiveInScene=true⇒ 守卫不短路。sleep/wakeUp/isSleeping（406/414/421 行）本就无守卫恒 throw。CI 1450 tests passed 佐证。 - 反向证伪成立 — 若 lite
addForceAtPosition不 throw，error为undefined，expect(undefined).eq("Physics-lite don't support addForceAtPosition...")会 fail ⇒ 是守得住的回归测试，非摆设。 - 非重复 — 全仓此前无任何测试断言 lite throw 字符串（
grep "Physics-lite don't support" tests/仅此一处），不与既有测试冗余。
其余维度（沿用前两轮结论，代码未变不再展开）
四层对称完整 / 物理等价 + delegation 测试守住接线 / CDN==checked-in 逐字节一致、sidecar 配对正确 / lite throw 字符串同族一致（CodeRabbit 改 Error 对象建议按项目惯例不采纳）—— 均已在前两轮闭环，本轮无变化、不重提。
LGTM。两个 P3 都不阻塞，移除 notes/（并顺手给新测试补 root.destroy()）后即可合并。
2 participants
Summary
DynamicCollider.applyForceAtPosition(force, position)for world-space force-at-position use cases.IDynamicColliderand the PhysX backend to delegate directly toPxRigidBody.addForceAtPos.PhysXPhysicsCDN runtime URLs to the same fix(README): Official Site Link #19 build sonew PhysXPhysics()exposesaddForceAtPoswithout customruntimeUrls.DynamicCollidercoverage for native delegation and force+torque motion equivalence.Related
Runtime Rollout Gate
The default CDN runtime has been uploaded as one zip so
physx.release*.jsand matching.wasmsidecars share the same CDN directory:https://mdn.alipayobjects.com/rms/uri/file/as/apwallet/1781696156399/suyi/physx.release.jshttps://mdn.alipayobjects.com/rms/uri/file/as/apwallet/1781696156399/suyi/physx.release.simd.jsBoth default CDN JS URLs were smoke-tested in Chromium after wasm initialization:
PxRigidDynamic.prototype.addForceAtPos,addImpulseAtPos, andgetVelocityAtPosare functions.Verification
npm run b:modulenpm run b:typesnpm run lint -- --quietHEADLESS=true pnpm exec vitest run --config tests/vitest.config.ts tests/src/core/physics/DynamicCollider.test.tsHEADLESS=true pnpm exec vitest --coverage tests/src/core/physics/DynamicCollider.test.tsnpm run coveragecurl -I -Lverified default CDN JS is served asapplication/x-javascript, wasm asapplication/wasm, with CORS enabled.emmake make -j1.addForceAtPosand related at-position helpers.Summary by CodeRabbit
applyForceAtPosition(force, position)to the dynamic collider API (world-space).