Skip to content

fix(physics): make PhysX CCD transitions valid#3041

Open
luzhuang wants to merge 5 commits into
dev/2.0from
fix/physics-kinematic-sync
Open

fix(physics): make PhysX CCD transitions valid#3041
luzhuang wants to merge 5 commits into
dev/2.0from
fix/physics-kinematic-sync

Conversation

@luzhuang

@luzhuang luzhuang commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

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:

  • eKINEMATIC together with swept CCD (eENABLE_CCD);
  • swept CCD together with speculative CCD (eENABLE_SPECULATIVE_CCD).

This occurs in two normal public-API flows:

  1. A dynamic collider using Continuous or ContinuousDynamic becomes kinematic. The old implementation raised eKINEMATIC while swept CCD was still enabled. PhysX reported an invalid parameter and filtered CCD itself.
  2. A collider switches from ContinuousSpeculative to 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 collisionDetectionMode while 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

  • Mirror the native kinematic state in PhysXDynamicCollider.
  • Before raising eKINEMATIC, disable swept CCD and CCD friction when swept CCD is currently enabled.
  • While the actor is kinematic, defer requested swept modes by applying an effective Discrete mode. ContinuousSpeculative remains enabled because PhysX supports speculative CCD on kinematic bodies.
  • Keep Core's requested mode unchanged so its existing dynamic-transition resync restores the requested swept mode.
  • Apply mutually exclusive CCD families in disable-old-before-enable-new order for every transition.
  • Replace the repeated four-case flag switch with one compact flag mapping while preserving the required native write order.

The change is confined to the PhysX adapter. It does not alter the public DynamicCollider API, transform synchronization, or lifecycle behavior.

PhysX source evidence

NpRigidBodyTemplate::setRigidBodyFlagsInternal explicitly reports and filters both invalid combinations:

  • kinematic plus eENABLE_CCD;
  • eENABLE_CCD plus eENABLE_SPECULATIVE_CCD.

Because validation happens inside each setRigidBodyFlag call, 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.0 merge-base implementation of PhysXDynamicCollider.ts, after rebuilding module artifacts:

  • defers swept CCD across kinematic transitions failed because the old adapter did not disable swept CCD before enabling eKINEMATIC. PhysX also emitted kinematic bodies with CCD enabled are not supported.
  • switches between swept and speculative CCD without overlapping flags failed because the old adapter did not disable speculative CCD before enabling swept CCD. PhysX also emitted eENABLE_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.ts suite 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:

  • the bundled PhysX runtime is a Release build where PX_CHECK_AND_RETURN(destination.isSane()) is compiled out;
  • NpRigidDynamic::setKinematicTarget then calls destination.getNormalized() unconditionally;
  • a public behavior test using the old adapter produced the correct unit-length target orientation.

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:module
  • pnpm b:types
  • HEADLESS=true pnpm vitest run tests/src/core/physics/DynamicCollider.test.ts --reporter=verbose — 29 passed
  • production ESLint
  • focused Prettier check
  • git diff --check

Scope and independence

This PR targets dev/2.0 and changes only:

  • packages/physics-physx/src/PhysXDynamicCollider.ts
  • tests/src/core/physics/DynamicCollider.test.ts

It has no changed-file overlap with #3025 or #3042 and has no merge-order dependency on either PR.

Summary by CodeRabbit

  • Bug Fixes
    • Improved collision detection handling when toggling dynamic colliders between kinematic and non-kinematic states.
    • Prevented incompatible continuous collision detection configurations from being applied simultaneously, enforcing correct swept vs speculative CCD behavior and CCD friction rules.
  • Tests
    • Added new test coverage to verify PhysX CCD flag transitions when changing kinematic state and when switching between swept (continuous) and speculative (continuous speculative) modes.

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 9a4057d5-ed11-4ab3-82dc-dd5eb713bb01

📥 Commits

Reviewing files that changed from the base of the PR and between b413f41 and d98ffe1.

📒 Files selected for processing (1)
  • packages/physics-physx/src/PhysXDynamicCollider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/physics-physx/src/PhysXDynamicCollider.ts

Walkthrough

PhysXDynamicCollider now tracks kinematic state, applies compatible CCD flags, and validates swept and speculative CCD transitions through PhysX rigid-body flag tests.

Changes

PhysX dynamic collider behavior

Layer / File(s) Summary
Kinematic CCD state and flags
packages/physics-physx/src/PhysXDynamicCollider.ts, tests/src/core/physics/DynamicCollider.test.ts
Kinematic state now affects collision-mode handling, CCD flags are applied through a shared helper, and tests verify flag transitions, ordering, and non-overlapping CCD modes.

Estimated code review effort: 3 (Moderate) | ~15 minutes

Poem

A rabbit checks each CCD light,
Swept and spec’tral flags unite.
Kinematic hops keep modes in line,
PhysX gets a state that’s fine.
Hop, hop—tests make it right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: fixing invalid PhysX CCD transitions in the physics collider.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/physics-kinematic-sync

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

GuoLei1990

This comment was marked as outdated.

@GuoLei1990 GuoLei1990 marked this pull request as draft July 1, 2026 08:49
@luzhuang luzhuang force-pushed the fix/physics-kinematic-sync branch from b84a476 to 3ef3825 Compare July 13, 2026 09:14
@luzhuang luzhuang changed the base branch from fix/physics-shaderlab-split to dev/2.0 July 13, 2026 09:14
@luzhuang luzhuang force-pushed the fix/physics-kinematic-sync branch from 3ef3825 to 173ba85 Compare July 13, 2026 09:25
@luzhuang luzhuang force-pushed the fix/physics-kinematic-sync branch from 173ba85 to a4b1840 Compare July 13, 2026 09:38
@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.79%. Comparing base (75af66f) to head (d98ffe1).
⚠️ Report is 1 commits behind head on dev/2.0.

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     
Flag Coverage Δ
unittests 79.79% <100.00%> (+0.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@luzhuang luzhuang changed the title fix(physics): preserve kinematic sync semantics fix(physics): distinguish kinematic motion from teleport sync Jul 13, 2026
@luzhuang luzhuang changed the title fix(physics): distinguish kinematic motion from teleport sync fix(physics): harden PhysX kinematic CCD and targets Jul 13, 2026
@luzhuang luzhuang marked this pull request as ready for review July 13, 2026 11:57
GuoLei1990

This comment was marked as outdated.

@luzhuang luzhuang changed the title fix(physics): harden PhysX kinematic CCD and targets fix(physics): make PhysX CCD transitions valid Jul 13, 2026

@GuoLei1990 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

三轮 review @ b413f41bb(线性追加 1 commit,--comment

相对二轮的变化:二轮我审的 tip b474e58f5 之上线性追加了 b413f41bb refactor(physics): drop redundant target normalizationcompareahead 1 / behind 0,无 force-push)。该 commit 删除了 move() 里的 JS 侧 quat 归一化——两处 rotation 入口从 .normalized ? 直传 : tempRotation.copyFrom().normalize() 改回裸转发 setKinematicTarget(pos, rot),并删除对应的 normalizes non-unit kinematic targets... 白盒测试 + 未用的 Quaternion import(净 -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):

  1. getNormalized() 无条件执行NpRigidDynamic.cpp:144 setKinematicTargetInternal(destination.getNormalized());任何 #if guard 之外,release/checked 都跑。getNormalized()PxQuat.h)按真实 magnitude 除,对任意有限非零 quat 产出正规单位四元数。
  2. Release 关掉 PX_CHECKEDemscripten/CMakeLists.txt RELEASE 编译定义仅 NDEBUG;PX_SUPPORT_PVD=0不含 PX_CHECKEDPX_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() 收拾。两个条件缺一,删除都不安全,此处恰好都成立。
  3. binding 层裸转发ActorBinding.h:112-115 emscripten glue 把 (pos,rot) 直接包 PxTransform 转发原生调用,无任何预归一化/校验,完全依赖原生 :144。
  4. physx.release.wasm 是唯一默认产物libs/ 只有 release + release.simd,无 debug/checked 变体;build.shbuild 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 复用 getWorldTransformgetGlobalPose() 读回值(PhysX 存的即单位 quat,无需归一化)。逻辑自洽。
  • 删测正确、无孤儿:被删测试测的正是被删的 JS 归一化行为,代码与测试同删是对的;剩下的两个 CCD 契约测试(defers swept CCD across kinematic transitions / switches between swept and speculative CCD...本轮未动、仍真守 mutual-exclusivity 与延迟 CCD 的原生边界。删掉的 Quaternion import 同步清理干净。
  • CCD 状态机(二轮已审部分)本轮零改动,无需复审。CI 全绿(codecov 真跑 vitest / e2e 4/4 / build ×3)。

#3040 / #3025 的关系

按 diff 核对:本 PR 两文件(PhysXDynamicCollider.ts / DynamicCollider.test.ts)与 #3040(clone 重构,packages/core/*,无 physics-physx)、#3025fix/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,290PhysXCharacterController.ts:91,93)。建议顺手去掉两处句号。非阻塞。

结论

无 P0/P1/P2,--comment 本轮 delta 是一次正确的冗余消除——JS 侧 quat 归一化经 PhysX 真源核对确属重复原生 getNormalized() 的防御代码,删除安全(两个必要条件"getNormalized 无条件 + release 关 PX_CHECKED"均成立)、测试同步删除无孤儿、剩余 CCD 契约测试仍守得住、CI 全绿。仅一条二轮遗留的注释句号 P3 未闭环。

@GuoLei1990 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

四轮 review @ d98ffe1ee(线性追加 1 commit,--comment

相对三轮的变化:三轮我审的 tip b413f41bb 之上线性追加了唯一一个 commit d98ffe1ee style(physics): align inline commentscompareahead 1 / behind 0,无 force-push),仅动 PhysXDynamicCollider.ts 一个文件、净 -0/+0 行、纯改注释文本。

唯一 delta:三轮遗留 P3 已闭环

三轮我标的最后一条 [P3] 是 PhysXDynamicCollider.ts:185,265 两条单行 // 注释带句末句号,偏离本包惯例(对照 PhysXPhysics.ts:286,290PhysXCharacterController.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 依赖)。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants