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.94% +0.77%
===========================================
Files 914 919 +5
Lines 101666 102602 +936
Branches 11209 11378 +169
===========================================
+ Hits 80489 82028 +1539
+ Misses 20990 20393 -597
+ Partials 187 181 -6
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:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ChangesapplyForceAtPosition Feature
Possibly related issues
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 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 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()）后即可合并。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
第四轮（HEAD b3cbdaf5b，相比上轮 32582b2f）。两个新 commit：
5f304ebc— 把我前三轮反复提的两个 P3 一并修掉：删除notes/physics/2026-06-17-physx-force-at-position.md、给PhysicsScene.test.ts的 lite unsupported 测试补上root.destroy()。b3cbdaf5b（本轮唯一实质代码改动）—perf: avoid vector wrappers for PhysX forces：把addForce/addTorque/addForceAtPosition里传给原生绑定的{ x: v.x, y: v.y, z: v.z }字面量改成直接传引擎Vector3/Vector3对象。
方向正确，本轮无 P0/P1/P2/P3，前轮两个 P3 已闭环。
已闭环（前轮提出，本轮确认修复，不再展开）
- [P3 已修]
notes/CR 笔记 — 已从源码树删除（前三轮均提）。 - [P3 已修] lite unsupported 测试缺
root.destroy()—PhysicsScene.test.ts:165已补root.destroy()，与本 describe 块内其它测试惯例对齐。
本轮新增 perf 改动核对（已离线证伪，通过）
this._pxActor.addForce({ x: force.x, y: force.y, z: force.z }) → this._pxActor.addForce(force)（addTorque/addForceAtPos 同改）。逐项核实是正确且与代码库主流一致的简化，非风险：
- emscripten value_object 绑定按
.x/.y/.z属性读取，不要求特定类 — 这两行{x,y,z}包裹是整个 physx 包里仅有的两处这种写法（grep "{ x:" packages/physics-physx/src/只命中PhysXDynamicCollider.ts:218/225），是 outlier。其余所有传向同一_pxActor的调用早就直接传Vector3/Quaternion：setLinearVelocity(value)（:59）、setAngularVelocity(_tempTranslation)（:83）、setCMassLocalPose(position)（:105）、setKinematicTarget(positionOrRotation, rotation)（:233/241/243）、PhysXJoint.setLocalPose(actor, position, rotation)（:114）、PhysXCollider.setGlobalPose(...)（:52）。本改动是把这 3 个方法对齐到既有主流写法，而非引入新模式。 Vector3的x/y/z是 getter（返回_x/_y/_z，Vector3.ts:327/339/351），native 属性访问命中 getter，与{x,y,z}字面量逐字段等价 — 不存在「private 字段读不到」问题。- 省一次 per-call 对象字面量分配 — force/torque 可能每帧调用，去掉临时对象是真收益。
addForceAtPosition原本一次调用分配 2 个字面量，现在 0。 - 真实绑定路径被等价测试守住 —
DynamicCollider.test.ts:317「produces the same motion as an equivalent force and torque」跑真 PhysX_update(1)，断言linearVelocity/angularVelocity三分量逐一等于applyForce + applyTorque((0,0,-1))的参考分解（com=(0,0,0)、force=(1,0,0)、pos=(0,1,0)⇒torque = pos×force = (0,0,-1)）。若直接传Vector3导致绑定读不到坐标，速度会偏离/归零 → 测试 fail。CI（build×3 OS / e2e 4/4 / lint）全绿佐证绑定按 getter 正确读取。 - perf commit scope 干净 — 仅
PhysXDynamicCollider.ts3 行，未重新 touch wasm/js（正确：wasm 绑定在前序 commit 已就位，且按.x/.y/.z读取与 JS 对象的类无关）。
其余维度（沿用前三轮结论，代码未变不再展开）
四层（design 接口 / core 公开 API / physx 后端 / lite throw）对称完整 / 物理等价 + delegation 测试守住接线 / CDN==checked-in 逐字节一致、sidecar 配对正确 / lite throw 字符串同族一致（CodeRabbit 改 Error 对象建议按项目惯例不采纳）—— 均已闭环。
LGTM，可合并。
3 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
New Features
Tests