fix(physics-physx): skip initial overlap in raycast/sweep + reuse query callbacks#2998
Conversation
PhysX 射线/sweep 起点在碰撞体内部时会返回 distance=0 的命中, 与 Unity 行为不一致。通过启用 POST_FILTER 并在 postFilter 中 过滤 distance<=0 的结果,使 PhysX 跳过 initial overlap 并继续 查找下一个最近命中。 - 启用 POST_FILTER query flag - raycast 和 sweep callback 添加 postFilter 过滤 distance<=0 - 更新 PhysX wasm CDN URL(对应 QueryBinding.h postFilter 改动) - 更新本地 wasm 产物
Commit 6ab7437 enabled POST_FILTER on the shared _pxFilterData. But overlap callbacks (overlapBoxAll/SphereAll/CapsuleAll) only define preFilter — without postFilter, PhysX wasm crashes attempting to invoke the missing callback for overlap queries. Split raycast/sweep filter data from overlap filter data: - _pxFilterData: STATIC | DYNAMIC | PRE_FILTER (overlap queries) - _pxRaycastFilterData: STATIC | DYNAMIC | PRE_FILTER | POST_FILTER (raycast and _sweepSingle, with their postFilter distance<=0 skip) Also update 4 existing tests that asserted the pre-fix behavior (ray origin inside collider returns true with distance=0). New behavior: initial overlap is skipped and raycast returns false unless there is a collider beyond.
The previous ray origin (3,3,3) on a (6,6,6) box centered at origin is the box corner — boundary point whose hit/miss depends on PhysX edge tolerance. Move the origin strictly inside so the test deterministically verifies the initial-overlap-skip behavior.
…epFilterData Field is used by both raycast and _sweepSingle; the new name accurately reflects scope.
Each raycast/sweep/overlap previously created a JS callback object and called PxQueryFilterCallback.implement(...) / .delete() per query, which crosses the wasm boundary twice via embind's emval handle pattern. For high-frequency game queries (FPS aim, AI vision, character controller sensors) this dominates query cost. Maintain three persistent callbacks at scene construction time. The user-provided filter function is swapped per call via a stack so reentrancy is preserved (e.g., calling raycast inside another raycast's onRaycast). Each query type owns its own stack so cross-type nesting (raycast inside sweep, etc.) doesn't conflict. try/finally guards stack push/pop against user filters that throw, so stack state stays clean across exceptions. Tests cover three reentrancy scenarios: same-type nesting, mixed-type nesting, and exception propagation.
…ere/capsule Mirror the existing raycast inside-collider regression for sweep paths. Each cast originates inside a near collider and must skip it (initial overlap) to hit a far collider beyond, exercising postFilter on _pxRaycastSweepFilterData for sweepSingle queries.
…yHitType - Move PxSweepHit.delete() to a finally block so a throwing outHitResult callback can no longer leak the wasm-side hit object. - Extract QueryHitType.NONE / .BLOCK from the magic 0/2 spread across the three persistent query callbacks; mirrors the existing QueryFlag enum in this file and removes the inline "eBLOCK : eNONE" comments.
The two postFilter arrows triggered prettier-recommended lint failures. Collapsing the ternary onto a single parenthesised line matches the formatter and unblocks CI lint.
|
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)
WalkthroughPhysX runtime default URLs updated. PhysXPhysicsScene reuses persistent PxQueryFilterCallback instances with a per-call predicate slot for raycast/sweep/overlap to support reentrancy and skip initial overlaps. Tests updated to assert skip-initial-overlap and cover nested-filter ordering and exception recovery. ChangesPhysX Query Filter Callback Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2998 +/- ##
===========================================
- Coverage 78.14% 77.80% -0.34%
===========================================
Files 900 900
Lines 99255 99205 -50
Branches 10213 10177 -36
===========================================
- Hits 77563 77191 -372
- Misses 21521 21841 +320
- Partials 171 173 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/src/core/physics/PhysicsScene.test.ts (1)
573-817: ⚡ Quick winMake cleanup failure-safe for the new PhysX regression cases.
These tests use root names that the shared
afterEachdoes not clean up, so any assertion failure beforeroot.destroy()will leak colliders into later cases. Either use the shared"root"name again or wrap each body intry/finally.🤖 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 `@tests/src/core/physics/PhysicsScene.test.ts` around lines 573 - 817, The tests create per-case roots via root = scene.createRootEntity("<...>") but rely on a shared afterEach that only cleans up the root named "root", so assertion failures leak colliders; fix by making cleanup failure-safe: either use the shared name (createRootEntity("root")) for each test or wrap each test body that creates a root (e.g., the roots in the "boxCast skips initial overlap...", "sphereCast skips initial overlap...", "capsuleCast...", and the nested/throw recovery tests) in try/finally and call root.destroy() from finally to guarantee teardown even on assertion failures.
🤖 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.
Inline comments:
In `@packages/physics-physx/src/PhysXPhysicsScene.ts`:
- Around line 485-488: Allocate the PxSweepHit before mutating the sweep stack:
move the creation of the PxSweepHit instance (new
this._physXPhysics._physX.PxSweepHit()) to occur before pushing the onSweep
predicate onto this._onSweepStack (onSweepStack.push(onSweep)), or alternatively
wrap the push and following logic in a try/finally that always pops the
predicate; update the code around onSweepStack, onSweep, and PxSweepHit so that
an exception during allocation cannot leave a stale entry on _onSweepStack.
In `@tests/src/core/physics/PhysicsScene.test.ts`:
- Around line 751-762: Reformat the nativeScene.boxCast call site to satisfy
Prettier: rewrap the arguments and callback to follow the project's Prettier
rules (consistent indentation and line breaks) for the nativeScene.boxCast
invocation that uses sweepCenter, orientation, halfExtents, direction and the
inline callback that updates innerSweepCalls and innerSweepUuids; run Prettier
(or your editor's format command) on tests/src/core/physics/PhysicsScene.test.ts
to produce the properly wrapped/indented version so CI passes.
---
Nitpick comments:
In `@tests/src/core/physics/PhysicsScene.test.ts`:
- Around line 573-817: The tests create per-case roots via root =
scene.createRootEntity("<...>") but rely on a shared afterEach that only cleans
up the root named "root", so assertion failures leak colliders; fix by making
cleanup failure-safe: either use the shared name (createRootEntity("root")) for
each test or wrap each test body that creates a root (e.g., the roots in the
"boxCast skips initial overlap...", "sphereCast skips initial overlap...",
"capsuleCast...", and the nested/throw recovery tests) in try/finally and call
root.destroy() from finally to guarantee teardown even on assertion failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb46b6b8-0296-4a4e-a8f9-7078a274fd36
⛔ Files ignored due to path filters (4)
e2e/.dev/physx.release.simd.wasmis excluded by!**/*.wasme2e/.dev/physx.release.wasmis excluded by!**/*.wasmpackages/physics-physx/libs/physx.release.simd.wasmis excluded by!**/*.wasmpackages/physics-physx/libs/physx.release.wasmis excluded by!**/*.wasm
📒 Files selected for processing (7)
e2e/.dev/physx.release.jse2e/.dev/physx.release.simd.jspackages/physics-physx/libs/physx.release.jspackages/physics-physx/libs/physx.release.simd.jspackages/physics-physx/src/PhysXPhysics.tspackages/physics-physx/src/PhysXPhysicsScene.tstests/src/core/physics/PhysicsScene.test.ts
| nativeScene.boxCast( | ||
| sweepCenter, | ||
| orientation, | ||
| halfExtents, | ||
| direction, | ||
| 100, | ||
| (sweepUuid: number) => { | ||
| innerSweepCalls++; | ||
| innerSweepUuids.push(sweepUuid); | ||
| return false; // skip everything in inner sweep | ||
| } | ||
| ); |
There was a problem hiding this comment.
Run Prettier on this boxCast call site.
prettier/prettier is already failing on this block, so CI will stay red until the wrapping here is reformatted.
🧰 Tools
🪛 ESLint
[error] 751-757: Replace ⏎············sweepCenter,⏎············orientation,⏎············halfExtents,⏎············direction,⏎············100,⏎··········· with sweepCenter,·orientation,·halfExtents,·direction,·100,
(prettier/prettier)
[error] 758-758: Delete ··
(prettier/prettier)
[error] 759-759: Delete ··
(prettier/prettier)
[error] 760-760: Delete ··
(prettier/prettier)
[error] 761-762: Replace ············}⏎·········· with ··········}
(prettier/prettier)
🤖 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 `@tests/src/core/physics/PhysicsScene.test.ts` around lines 751 - 762, Reformat
the nativeScene.boxCast call site to satisfy Prettier: rewrap the arguments and
callback to follow the project's Prettier rules (consistent indentation and line
breaks) for the nativeScene.boxCast invocation that uses sweepCenter,
orientation, halfExtents, direction and the inline callback that updates
innerSweepCalls and innerSweepUuids; run Prettier (or your editor's format
command) on tests/src/core/physics/PhysicsScene.test.ts to produce the properly
wrapped/indented version so CI passes.
…aycast Make _sweepSingle structurally mirror raycast: - Promote pxSweepHit to a scene-level persistent PhysX object (allocated once in the constructor, released in destroy), matching how _pxRaycastHit is already handled. - Collapse the nested try/finally that existed only to dispose the per-call PxSweepHit; now a single try/finally guards onSweepStack.pop(). This removes one new+delete pair on every box/sphere/capsule cast (a real cost across the JS↔wasm boundary), and as a side effect closes the exception window where `new PxSweepHit()` could throw after onSweepStack had already been pushed — push now happens immediately before the only try block, leaving no path to leak a stale predicate. All 55 physics tests (48 baseline + 7 PR regressions) continue to pass.
…h prev-local Collapse the three persistent PhysX query filter callbacks (raycast, sweep, overlap) into a single `_pxQueryCallback`, and replace the three predicate stacks (`_onRaycastStack` / `_onSweepStack` / `_onOverlapStack`) with a single `_currentOnQuery` field guarded by a call-stack-local previous-value save in each query method. Why this is safe: - PhysX SDK guarantees `postFilter` is only invoked when the query's filter data carries `PxQueryFlag::ePOSTFILTER` (PxQueryFilterCallback / PxQueryFilterData doc-comments in PxQueryFiltering.h). Overlap queries use `_pxFilterData` which omits POST_FILTER, so the shared callback's postFilter is never called for overlaps even though the method exists. Raycast and sweep both use `_pxRaycastSweepFilterData` with POST_FILTER set, getting the initial-overlap-skip behavior they need. - Unity's native PhysX wrapper (`Modules/Physics/CastFilter.h`, `PhysicsQuery.cpp:OverlapSphereInternal`) follows the exact same pattern: a single `CastFilter` class implements both preFilter and postFilter and is shared by raycast, sweep, and overlap queries. - Reentrancy (e.g. raycast nested inside another raycast's onRaycast) is preserved via the prev-local save/restore in `try/finally`, recreating C++-style RAII without an explicit stack array. The existing nested-call and throw-recovery regression tests cover this path. Net result: -15 lines, one persistent callback instead of three, a single field instead of three arrays, and a faster preFilter hot path (`scene._currentOnQuery(idx)` vs `arr[arr.length - 1](idx)`). All 55 physics tests (48 baseline + 7 PR regressions including nested filter ordering, sweep-inside-raycast, and throw recovery) continue to pass.
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
本轮是第四轮增量审查,覆盖最新 commit 8fe796a6(persist PxSweepHit + align _sweepSingle with raycast)。
三轮历史遗留的所有问题均已关闭:
-
overlap preFilter 返回 BLOCK 可能提前终止多命中查询(P2) — 深入源码确认:
overlapMultiple在调用 PhysX 前强制追加eNO_BLOCKflag(ExtSceneQueryExt.cpp:156),preFilter 返回BLOCK在 overlap 查询中不会提前终止,行为正确。 -
两个 filterData 差异缺注释(P2) — 已修复。第 39–46 行有完整注释说明单一 callback 共用的安全性保证及 POST_FILTER 仅由 raycast/sweep filterData 激活的设计意图。
-
_pxSweepHit生命周期不对称(P3 简化建议) — 已修复,即本轮新增 commit 的核心改动。
新 commit 质量
perf(physics-physx): persist PxSweepHit and align _sweepSingle with raycast 改动干净:
- 将 per-call
new PxSweepHit() / .delete()提升为 constructor 分配 /destroy()释放,与_pxRaycastHit对称,消除每次 sweep 的 WASM boundary 往返开销。 - 嵌套 try/finally 合并为单层,逻辑更清晰。
- 所有 55 个测试继续通过(commit message 已确认)。
无新问题。可以合并。
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
所有历史问题均已关闭(前 4 轮):initial overlap 跳过、overlap preFilter BLOCK 问题(深入源码确认不影响多命中查询)、PxSweepHit 生命周期收紧、reentrancy 安全性、POST_FILTER 污染 overlap。
总结
三个独立物理修复方向全部正确:initial overlap 通过 POST_FILTER 回调过滤 distance <= 0 的 hit;query callback 对象持久化复用配合 reentrancy stack;sweep hit 生命周期收紧到 finally 块。回归测试覆盖完整(7 个新测试,含 initial overlap 4 类 + reentrancy 3 类)。
无新问题。LGTM,可合入。
| this._pxScene = pxPhysics.createScene(sceneDesc); | ||
| sceneDesc.delete(); | ||
|
|
||
| const scene = this; |
There was a problem hiding this comment.
const scene = this; 是冗余 — 箭头函数已经从构造器的 lexical scope capture 了 this,直接 this._currentOnQuery(index) 即可。
- const scene = this;
- this._pxQueryCallback = physX.PxQueryFilterCallback.implement({
- preFilter: (_filterData: any, index: number, _actor: any) =>
- scene._currentOnQuery(index) ? QueryHitType.BLOCK : QueryHitType.NONE,
+ this._pxQueryCallback = physX.PxQueryFilterCallback.implement({
+ preFilter: (_filterData: any, index: number, _actor: any) =>
+ this._currentOnQuery(index) ? QueryHitType.BLOCK : QueryHitType.NONE,看着像在绕什么 this 绑定坑,实际上没有。
There was a problem hiding this comment.
第一性原理核实正确:同文件 73-101 行的 triggerCallback 早就在 physX.PxSimulationEventCallback.implement() 里用 this._bufferContactEvent(...) 等直接调用,证明 emscripten .implement() 不破坏箭头函数 this。const scene = this 是孤例残留。已 push 修复(eb3a7e930)。
Arrow functions inherit `this` lexically, so the alias was unnecessary — same constructor's triggerCallback (lines 73-101) uses `this._xxx` directly inside arrow callbacks passed to physX.PxSimulationEventCallback.implement(), proving emscripten's .implement() doesn't rebind callback `this`. Reviewed by @cptbtptpbcptdtptp on galacean#2998.
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| initial overlap 跳过 | 已修复(POST_FILTER 过滤 distance <= 0) |
| overlap preFilter 返回 BLOCK 可能提前终止多命中查询 | 已关闭(深入 PhysX 源码确认 overlapMultiple 强制 eNO_BLOCK flag,不影响多命中) |
| query callback 持久化复用 + reentrancy | 已修复(_on*Stack 支持嵌套) |
| PxSweepHit 生命周期不对称 | 已修复(提升到 constructor 分配) |
| POST_FILTER 污染 overlap filterData | 已修复(拆出独立 _pxRaycastSweepFilterData) |
| 两个 filterData 差异缺注释 | 已修复 |
总结
三个独立物理修复:initial overlap 通过 POST_FILTER 回调过滤 distance <= 0 的 hit;query callback 对象持久化复用配合 reentrancy stack;PxSweepHit 生命周期收紧到 constructor/destroy 对称。回归测试覆盖完整(7 个新测试)。
前 4 轮 review 提出的所有问题均已修复或经作者/代码验证关闭。
无新问题。LGTM,可合入。
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
所有历史问题均已关闭(见前 4 轮 review 记录)。
总结
三个独立物理修复,方向全部正确:
- raycast/sweep 通过
POST_FILTER过滤distance <= 0的 hit 跳过 initial overlap,对应 Unity 的语义(起点在碰撞体内时不计为命中) - 持久化
_pxRaycastCallback/_pxSweepCallback/_pxOverlapCallback,配合_on*Stack支持 reentrant 查询 - 收紧
PxSweepHit生命周期,对齐PxRaycastHit的处理方式
overlap preFilter 返回 BLOCK 不影响多命中查询已通过深入 PhysX 源码确认(overlapMultiple 强制 eNO_BLOCK flag)。
无新问题。LGTM,可合入。
|
Follow-up 建议(不阻塞本 PR) 这次 PR 落地后,建议把 query filter 进一步推到 native,分两阶段: 阶段 1:内置 filter native 化(API 零改动)
阶段 2:暴露用户 callback(按需驱动)把 PhysicsScene.raycast(ray, distance, {
layerMask: Layer.Everything, // → native PxFilterData, fast path
preFilter?: (shape) => boolean, // 可选, JS callback
postFilter?: (hit) => boolean, // 可选, JS callback
hit?: HitResult
});没传 callback 时走阶段 1 的 native fast path;传了就回 JS callback 路径。底层本 PR 的 拆两个 PR 的理由
收益估算按 100 query/帧、broadphase 候选 ~20/query、narrowphase hit ~3/query、emval invoke ~300ns 估算:
主要价值是 preFilter 下推;postFilter native 化绝对收益小但改动也小(~10 行 + wasm patch),属于顺手做掉。 前置条件需要确认 PhysX wasm bindings 暴露程度:
如果 (2) 受限,阶段 1 的 postFilter native 化路线就走不通,只能保留 JS postFilter;但 preFilter 下推仍然能做。 设计参考:Unity |
…ysX scene queries) These tests were red on dev/2.0 before any of the recent physics work landed. Diagnosed and fixed at the root, no test-skipping or assertion weakening. **LitePhysicsMaterial — 1 failure** `ColliderShape Lite > clone` broke after R8 added `PhysicsMaterial._cloneTo → _syncNative()` which now calls every native setter on a freshly-cloned material. Lite's setters threw "Physics-lite don't support physics material", violating the no-op-with-log convention already used by `LiteColliderShape.setMaterial / setIsTrigger / setContactOffset`. Change all 5 setters to silent no-op + a one-time `console.log` warning, matching the existing convention. **PhysXPhysicsScene — 7 failures (raycast/boxCast/sphereCast/capsuleCast + overlapBoxAll/overlapSphereAll/overlapCapsuleAll)** Two underlying problems both surface in the JS filter-callback layer: 1. `_pxFilterData` had `POST_FILTER` flag enabled but `_overlapMultiple` reused it with a callback object that only defined `preFilter`. When PhysX tried to invoke `postFilter`, the wasm boundary errored with `Emval.toValue(...)[getStringOrSymbol(...)] is not a function`. 2. The 4 cast tests asserted the old "origin-inside-collider returns true with distance=0" behavior. The wasm side was already switched to the Unity-style "skip initial overlap" design (commit bdf5490 / PR #2998 on dev branch ahead of us), but the tests still encoded the rejected behavior. Mirror PR #2998's JS-side fix already merged on dev: - Split filter data into `_pxFilterData` (PRE only, for overlap) and `_pxRaycastSweepFilterData` (PRE + POST, for raycast/sweep). - Replace per-call `PxQueryFilterCallback.implement` with a single persistent `_pxQueryCallback` defining both `preFilter` and `postFilter`. User callbacks dispatch via a `_currentOnQuery` slot saved/restored around each native call for reentrancy safety. - Cache the `PxSweepHit` instance instead of creating per-call. - Add `QueryHitType` enum. Update 4 stale test assertions in PhysicsScene.test.ts to match the already-shipped "skip initial overlap" wasm contract. Use strictly-inside origin (2.9,2.9,2.9) for the raycast case to avoid the corner-tolerance flake that PR \#2998 already addressed. Verification: 11 physics test files, 213/213 passing locally.
摘要
物理 raycast / sweep 修复。与 #2999 一起从原 #2984/#2983 抽离出来,分别承担物理 / 动画+加载器修复,两个 PR 零文件重叠,独立合入。
修复内容
1. raycast / sweep 跳过 initial overlap (
fix(physics-physx))PhysX raycast / sweep 默认会把「ray 起点已在 collider 内部」当作 hit 返回 distance=0,与 Unity / 大部分引擎语义相反(应当跳过 initial overlap)。修复:
POST_FILTERflag,在 query callback 里过滤掉distance <= 0的 initial-overlap hit_pxRaycastSweepFilterData(仅 raycast/sweep 启用 POST_FILTER);overlap 仍用原_pxFilterData,避免 wasm 调用不存在的 postFilter callback2. 复用 PhysX query callbacks (
perf(physics-physx))每次 raycast/sweep/overlap 调用都新建
PxRaycastCallback/PxSweepCallback/PxOverlapCallback浪费跨 wasm 边界开销。改为持久化复用 + per-type reentrancy stack,hit 数据每次重置。3. sweep hit 生命周期收紧 (
refactor(physics-physx))QueryHitTypeenum 替代 raycast/sweep 内的 magic number 0/2(NONE/BLOCK)PxSweepHit.delete()没被调到的泄漏,移到外层 finally4. PhysX wasm 二进制 + CDN URL 更新
packages/physics-physx/src/PhysXPhysics.ts默认 CDN URL 指向包含 fix 的新版 PhysX wasm。packages/physics-physx/libs/与e2e/.dev/下的.js/.wasm同步更新。测试
tests/src/core/physics/PhysicsScene.test.ts新增 7 个回归测试:Initial overlap 跳过(4 个)
raycast skips initial overlap when ray origin is inside a colliderboxCast skips initial overlap and hits far collider beyondsphereCast skips initial overlap and hits far collider beyondcapsuleCast skips initial overlap and hits far collider beyondReentrancy / 异常恢复(3 个,覆盖 query callback 复用的并发安全)
raycast nested inside another raycast's onRaycast keeps stack orderingsweep nested inside raycast's onRaycast uses independent filter stacksraycast callback throwing leaves the filter stack clean for subsequent callsPhysicsScene.test.ts 全部 55 个测试通过(48 baseline + 7 新增)。
变更范围
packages/physics-physx/src/PhysXPhysics.ts(4 行,CDN URL)packages/physics-physx/src/PhysXPhysicsScene.ts(173 行)packages/physics-physx/libs/physx.release.{js,wasm,simd.js,simd.wasm}(PhysX wasm 二进制)e2e/.dev/physx.release.*(同步 e2e wasm)tests/src/core/physics/PhysicsScene.test.ts(310 行新测试)零文件触及动画 / loader / entity / shader。
Summary by CodeRabbit
Bug Fixes
Improvements
New Features
Tests