fix(physics): make PhysX CCD transitions valid#3041
Conversation
|
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesPhysX dynamic collider behavior
Estimated code review effort: 3 (Moderate) | ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
b84a476 to
3ef3825
Compare
3ef3825 to
173ba85
Compare
173ba85 to
a4b1840
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3041 +/- ##
===========================================
+ Coverage 79.52% 79.79% +0.26%
===========================================
Files 904 904
Lines 101168 101289 +121
Branches 11377 11400 +23
===========================================
+ Hits 80456 80819 +363
+ Misses 20528 20288 -240
+ Partials 184 182 -2
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:
|
GuoLei1990
left a comment
There was a problem hiding this comment.
三轮 review @ b413f41bb(线性追加 1 commit,--comment)
相对二轮的变化:二轮我审的 tip
b474e58f5之上线性追加了b413f41bb refactor(physics): drop redundant target normalization(compare报ahead 1 / behind 0,无 force-push)。该 commit 删除了move()里的 JS 侧 quat 归一化——两处 rotation 入口从.normalized ? 直传 : tempRotation.copyFrom().normalize()改回裸转发setKinematicTarget(pos, rot),并删除对应的normalizes non-unit kinematic targets...白盒测试 + 未用的Quaternionimport(净-10/+3源、-29/+1测试)。
二轮结论的自我修正
二轮我把这套 JS 侧归一化判为"正确"。本轮基于 PhysX 真源核对后确认:作者删得对,我二轮的"正确"评价过誉了——那段归一化是重复 PhysX 自身工作的冗余防御代码。 这正是"前几轮 review 可能都在错方向上加固"的一个实例,据实修正。
删除安全性核对(回到构建 WASM 的 PhysX 真源,逐条证伪 PR 描述三条论断)
PR 描述称"bundled runtime 是 Release build、isSane() 被编译掉、setKinematicTarget 无条件 getNormalized()"。不信描述,直接读构建 physx.release.wasm 的 C++ 真源(/Users/chenmo/Code/Galacean/physX.js):
getNormalized()无条件执行 —NpRigidDynamic.cpp:144setKinematicTargetInternal(destination.getNormalized());在任何#ifguard 之外,release/checked 都跑。getNormalized()(PxQuat.h)按真实 magnitude 除,对任意有限非零 quat 产出正规单位四元数。- Release 关掉
PX_CHECKED—emscripten/CMakeLists.txtRELEASE 编译定义仅NDEBUG;PX_SUPPORT_PVD=0,不含PX_CHECKED→PX_CHECK_AND_RETURN(destination.isSane(), ...)(NpRigidDynamic.cpp:130)经CmPhysXCommon.h:68展开为空。所以isSane()的拒绝不会触发。⚠️ 值得一提:isSane()(PxQuat.h:142-146)不止查有限性,还查|magnitude−1| < 1e-2——它本会拒非单位 quat;只有在 release 把它编译掉后,非单位 quat 才得以走到 :144 被getNormalized()收拾。两个条件缺一,删除都不安全,此处恰好都成立。 - binding 层裸转发 —
ActorBinding.h:112-115emscripten glue 把(pos,rot)直接包PxTransform转发原生调用,无任何预归一化/校验,完全依赖原生 :144。 physx.release.wasm是唯一默认产物(libs/只有 release + release.simd,无 debug/checked 变体;build.sh只build release)。
结论:用户传入的有限非零非单位 quat → 原生 getNormalized() 修正 → 旋转正确,JS 侧归一化确属冗余。退化零 quat((0,0,0,0)→NaN)old/new 都不防(旧 Quaternion.normalize 有零守卫会原样透传零 quat,PhysX getNormalized 反而产 NaN),但零 quat 作为旋转 target 本就无意义、非本 PR 引入的回归、非现实输入。
剩余路径核对(均通过)
move()三路径(PhysXDynamicCollider.ts:230-251):both-args / rotation-only 现裸传用户 quat(原生归一化兜底,安全);position-only 复用getWorldTransform→getGlobalPose()读回值(PhysX 存的即单位 quat,无需归一化)。逻辑自洽。- 删测正确、无孤儿:被删测试测的正是被删的 JS 归一化行为,代码与测试同删是对的;剩下的两个 CCD 契约测试(
defers swept CCD across kinematic transitions/switches between swept and speculative CCD...)本轮未动、仍真守 mutual-exclusivity 与延迟 CCD 的原生边界。删掉的Quaternionimport 同步清理干净。 - CCD 状态机(二轮已审部分)本轮零改动,无需复审。CI 全绿(
codecov真跑 vitest / e2e 4/4 / build ×3)。
与 #3040 / #3025 的关系
按 diff 核对:本 PR 两文件(PhysXDynamicCollider.ts / DynamicCollider.test.ts)与 #3040(clone 重构,packages/core/*,无 physics-physx)、#3025(fix/physics-shaderlab-split,改 PhysXPhysicsScene.ts/Collision.ts 等,不含 PhysXDynamicCollider.ts)零文件交叉,无 merge-order 依赖。PR 描述的独立性声明属实。
问题
- [P3](二轮遗留,部分闭环)
PhysXDynamicCollider.ts:185,265— 二轮标的 4 条带句号单行//注释里,move()的 2 条随归一化删除已消失;剩 2 条仍带句末句号(...eKINEMATIC is raised./...at the same time.)。本包惯例单行//不加句号(对照PhysXPhysics.ts:286,290、PhysXCharacterController.ts:91,93)。建议顺手去掉两处句号。非阻塞。
结论
无 P0/P1/P2,--comment。 本轮 delta 是一次正确的冗余消除——JS 侧 quat 归一化经 PhysX 真源核对确属重复原生 getNormalized() 的防御代码,删除安全(两个必要条件"getNormalized 无条件 + release 关 PX_CHECKED"均成立)、测试同步删除无孤儿、剩余 CCD 契约测试仍守得住、CI 全绿。仅一条二轮遗留的注释句号 P3 未闭环。
GuoLei1990
left a comment
There was a problem hiding this comment.
四轮 review @ d98ffe1ee(线性追加 1 commit,--comment)
相对三轮的变化:三轮我审的 tip
b413f41bb之上线性追加了唯一一个 commitd98ffe1ee style(physics): align inline comments(compare报ahead 1 / behind 0,无 force-push),仅动PhysXDynamicCollider.ts一个文件、净-0/+0行、纯改注释文本。
唯一 delta:三轮遗留 P3 已闭环
三轮我标的最后一条 [P3] 是 PhysXDynamicCollider.ts:185,265 两条单行 // 注释带句末句号,偏离本包惯例(对照 PhysXPhysics.ts:286,290、PhysXCharacterController.ts:91,93 单行 // 均不加句号)。本 commit 逐字采纳:
- // PhysX rejects swept CCD when eKINEMATIC is raised.
+ // PhysX rejects swept CCD when eKINEMATIC is raised
- // PhysX rejects enabling swept and speculative CCD at the same time.
+ // PhysX rejects enabling swept and speculative CCD at the same time核对当前 tree(d98ffe1ee):全文件现仅剩这 2 条 // 注释,无任何 // 注释以句号结尾,与包内惯例对齐。P3 闭环。
逻辑零改动、无需复审
本 commit 不触碰任何代码路径——CCD 状态机(kinematic × swept 互斥、swept × speculative 互斥、_applyCollisionDetectionFlags 先关旧族再开新族的写序)、move() 三路径裸转发 + native getNormalized() 兜底、两个 CCD 契约测试均与三轮 approve 时逐字一致。三轮已逐条闭环的实质问题本轮不重审。CI 全绿(build ×3 / codecov 真跑 vitest / e2e 4/4 / lint)。
结论
无 P0/P1/P2/P3,--comment。 本轮 delta 是三轮遗留注释句号 P3 的纯风格闭环,逐字采纳、无任何逻辑改动、无新增问题。此前四轮累积的所有 finding 现已全部关闭,PR 从 review 视角处于可合状态(base 正确落在 dev/2.0、与 #3025/#3040/#3042 零文件交叉、无 merge-order 依赖)。
Problem
PhysX validates rigid-body flags on every native write, not only after the caller has finished configuring a body. The previous wrapper wrote each collision mode as three independent flags in an order that could temporarily submit combinations PhysX explicitly rejects:
eKINEMATICtogether with swept CCD (eENABLE_CCD);eENABLE_SPECULATIVE_CCD).This occurs in two normal public-API flows:
ContinuousorContinuousDynamicbecomes kinematic. The old implementation raisedeKINEMATICwhile swept CCD was still enabled. PhysX reported an invalid parameter and filtered CCD itself.ContinuousSpeculativeto a swept mode. The old implementation enabled swept CCD before disabling speculative CCD. PhysX again reported an invalid parameter and filtered one of the flags.Core deliberately retains the requested
collisionDetectionModewhile the collider is kinematic and reapplies it when the collider becomes dynamic. The PhysX backend must therefore distinguish the requested public mode from the flags that are legal for the actor's current native state.Solution
PhysXDynamicCollider.eKINEMATIC, disable swept CCD and CCD friction when swept CCD is currently enabled.Discretemode.ContinuousSpeculativeremains enabled because PhysX supports speculative CCD on kinematic bodies.The change is confined to the PhysX adapter. It does not alter the public
DynamicColliderAPI, transform synchronization, or lifecycle behavior.PhysX source evidence
NpRigidBodyTemplate::setRigidBodyFlagsInternalexplicitly reports and filters both invalid combinations:eENABLE_CCD;eENABLE_CCDpluseENABLE_SPECULATIVE_CCD.Because validation happens inside each
setRigidBodyFlagcall, reaching the correct final flags is insufficient: the intermediate write order must also be legal.Red-test evidence
The final two regression tests were run unchanged against the exact
dev/2.0merge-base implementation ofPhysXDynamicCollider.ts, after rebuilding module artifacts:defers swept CCD across kinematic transitionsfailed because the old adapter did not disable swept CCD before enablingeKINEMATIC. PhysX also emittedkinematic bodies with CCD enabled are not supported.switches between swept and speculative CCD without overlapping flagsfailed because the old adapter did not disable speculative CCD before enabling swept CCD. PhysX also emittedeENABLE_CCD can't be raised at the same time as eENABLE_SPECULATIVE_CCD.Result before the fix: 2 failed. Result with this PR: 2 passed; the full
DynamicCollider.test.tssuite is 29 passed.These are native-boundary protocol tests, not string-matching tests. They assert the required flag order and the resulting native flags through the public Core setters.
Investigated and intentionally excluded
Non-unit kinematic-target quaternions were also investigated. A boundary-only test could be made red by observing a length-2 quaternion entering
setKinematicTarget, but that does not prove a release-runtime bug:PX_CHECK_AND_RETURN(destination.isSane())is compiled out;NpRigidDynamic::setKinematicTargetthen callsdestination.getNormalized()unconditionally;The JavaScript-side normalization and its white-box test were therefore removed. Keeping them would duplicate PhysX work in an engine path without a demonstrated release behavior failure.
Verification
pnpm b:modulepnpm b:typesHEADLESS=true pnpm vitest run tests/src/core/physics/DynamicCollider.test.ts --reporter=verbose— 29 passedgit diff --checkScope and independence
This PR targets
dev/2.0and changes only:packages/physics-physx/src/PhysXDynamicCollider.tstests/src/core/physics/DynamicCollider.test.tsIt has no changed-file overlap with #3025 or #3042 and has no merge-order dependency on either PR.
Summary by CodeRabbit