Refine MeshColliderShape and add WebAssembly SIMD runtime#2892
Open
GuoLei1990 wants to merge 25 commits intogalacean:dev/2.0from
Open
Refine MeshColliderShape and add WebAssembly SIMD runtime#2892GuoLei1990 wants to merge 25 commits intogalacean:dev/2.0from
GuoLei1990 wants to merge 25 commits intogalacean:dev/2.0from
Conversation
…thod in MeshColliderShape
…st related methods
…IMD support - Added support for WebAssembly SIMD mode in PhysXPhysics. - Updated constructor to accept URLs for SIMD mode. - Refactored runtime mode detection to prioritize SIMD if supported. - Removed JavaScript downgrade mode as a fallback option. - Updated PhysXRuntimeMode enum to include WebAssemblySIMD. - Removed obsolete tests related to JavaScript mode.
|
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:
WalkthroughRefactors mesh collider APIs to use ModelMesh/Vector3[] positions with cookingFlags; PhysX runtime now prioritizes WebAssembly SIMD via simdModeUrl and removes JS downgrade; enums, interfaces, PhysX implementation, E2E tests, and unit tests updated; added defensive null-checks around native shapes. Changes
Sequence DiagramsequenceDiagram
actor Client
participant MeshColliderShape
participant PhysXMeshColliderShape
participant PhysXEngine
Client->>MeshColliderShape: set mesh (ModelMesh)
MeshColliderShape->>MeshColliderShape: extract positions & indices
MeshColliderShape->>PhysXMeshColliderShape: request native mesh update (positions, indices, isConvex, cookingFlags)
PhysXMeshColliderShape->>PhysXEngine: allocate buffers & cook mesh with cookingFlags
PhysXEngine-->>PhysXMeshColliderShape: return cooked pxMesh
PhysXMeshColliderShape->>PhysXEngine: create pxShape from pxMesh
PhysXEngine-->>PhysXMeshColliderShape: return pxShape
PhysXMeshColliderShape->>MeshColliderShape: attach native shape
MeshColliderShape->>Client: collider updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/core/src/physics/shape/ColliderShape.ts (1)
183-192:⚠️ Potential issue | 🟠 Major
getClosestPointat Line 154 lacks the same null guard for_nativeShape.
_syncNativenow guards against a null_nativeShape, acknowledging it can legitimately be null (e.g., before a mesh is assigned toMeshColliderShape). However,getClosestPoint(Line 154) directly dereferencesthis._nativeShape.pointDistance(point)without a null check, which will throw a runtime error if called on a shape whose native counterpart hasn't been created yet.Proposed fix
getClosestPoint(point: Vector3, outClosestPoint: Vector3): number { const collider = this._collider; if (collider.enabled === false || collider.entity._isActiveInHierarchy === false) { console.warn("The collider is not active in scene."); return -1; } + if (!this._nativeShape) { + console.warn("The native shape is not available."); + return -1; + } + const res = this._nativeShape.pointDistance(point);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/physics/shape/ColliderShape.ts` around lines 183 - 192, The getClosestPoint method dereferences this._nativeShape without the null guard used in _syncNative, causing a runtime error when the native shape isn't created (e.g., MeshColliderShape before mesh assignment); add a guard at the start of getClosestPoint to check if this._nativeShape exists and, if not, return a safe sentinel (e.g., null or an appropriate "no result" value used in your codebase) instead of calling this._nativeShape.pointDistance(point); ensure the method references getClosestPoint and _syncNative so the fix mirrors the established null-check pattern.tests/src/core/physics/MeshColliderShape.test.ts (4)
506-527:⚠️ Potential issue | 🔴 CriticalTest assertions for error messages no longer match the actual messages.
Multiple tests assert against strings that were changed in this PR:
- Line 522: Expects
stringContaining("triangle mesh"), butDynamicCollider.addShape(line 422) now says"Non-convex MeshColliderShape is not supported...".- Line 547: Same mismatch — expects
"triangle mesh"but theisKinematicsetter (line 279) says"non-convex MeshColliderShape".These tests will fail at runtime.
Proposed fix
- expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("triangle mesh")); + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("Non-convex MeshColliderShape"));(Line 522)
- expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("triangle mesh")); + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("non-convex MeshColliderShape"));(Line 547)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/core/physics/MeshColliderShape.test.ts` around lines 506 - 527, Update the failing tests to match the new error text emitted by DynamicCollider.addShape and the isKinematic setter: change expectations that look for "triangle mesh" to assert on the new message fragment (e.g., "Non-convex MeshColliderShape" or "non-convex MeshColliderShape") so they reference the actual strings emitted by DynamicCollider.addShape and MeshColliderShape.isKinematic; update the assertions in MeshColliderShape.test.ts that reference DynamicCollider.addShape and the isKinematic setter to use expect.stringContaining with the exact new fragment and restore the error spy as before.
458-503:⚠️ Potential issue | 🔴 CriticalWarning message assertion mismatch: "position attribute" vs "position data".
Line 494 expects
"MeshColliderShape: Mesh has no position attribute"butMeshColliderShape.tsline 98 emits"MeshColliderShape: Mesh has no position data".Proposed fix
- expect(warnSpy).toHaveBeenCalledWith("MeshColliderShape: Mesh has no position attribute"); + expect(warnSpy).toHaveBeenCalledWith("MeshColliderShape: Mesh has no position data");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/core/physics/MeshColliderShape.test.ts` around lines 458 - 503, The test's expected warning string is out of sync with MeshColliderShape.ts; update the assertion in MeshColliderShape.test.ts to match the actual message emitted by MeshColliderShape (change expect(warnSpy).toHaveBeenCalledWith("MeshColliderShape: Mesh has no position attribute") to expect the message "MeshColliderShape: Mesh has no position data") so the warnSpy assertion matches the warning emitted by the MeshColliderShape class.
599-613:⚠️ Potential issue | 🔴 Critical
move()warning test assertion doesn't match the actual warning message.Line 609 expects:
"DynamicCollider.move() should only be called when isKinematic is true."But
DynamicCollider.tsline 389 emits:
"DynamicCollider: move() is only supported when isKinematic is true."Proposed fix
- expect(warnSpy).toHaveBeenCalledWith("DynamicCollider.move() should only be called when isKinematic is true."); + expect(warnSpy).toHaveBeenCalledWith("DynamicCollider: move() is only supported when isKinematic is true.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/core/physics/MeshColliderShape.test.ts` around lines 599 - 613, The test's expected warning text is outdated; update the assertion in the DynamicCollider.move() test so the expect(warnSpy).toHaveBeenCalledWith(...) matches the actual message emitted by DynamicCollider.move() ("DynamicCollider: move() is only supported when isKinematic is true."); locate the test that spies on console.warn (warnSpy) and replace the current expected string with the exact string produced in DynamicCollider.ts for move() so the test assertion matches the implementation.
433-456:⚠️ Potential issue | 🔴 CriticalTest expects a warning message that doesn't exist in the source code.
Line 451 asserts
"MeshColliderShape: Only ModelMesh is supported", but theset meshsetter inMeshColliderShape.ts(lines 65-72) has noinstanceofcheck — it directly passes the value to_extractMeshData, which callsmesh.getPositions(). Passing{ notAModelMesh: true }would throw aTypeErrorrather than emitting a warning.Either the
set meshsetter needs aninstanceof ModelMeshguard that emits this warning, or the test expectation needs updating.#!/bin/bash # Verify if there's an instanceof ModelMesh check in MeshColliderShape.ts rg -n "instanceof.*ModelMesh\|Only ModelMesh" --type=ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/core/physics/MeshColliderShape.test.ts` around lines 433 - 456, The test fails because MeshColliderShape's mesh setter directly calls _extractMeshData and will throw when given a non-ModelMesh; update the MeshColliderShape.set mesh setter to guard the input with an instanceof ModelMesh check (or equivalent type check) and, if it is not a ModelMesh, call console.warn("MeshColliderShape: Only ModelMesh is supported") and return early without calling _extractMeshData; ensure you reference MeshColliderShape, its mesh setter, and _extractMeshData so the change prevents the TypeError and satisfies the test expectation.packages/core/src/physics/DynamicCollider.ts (1)
267-296:⚠️ Potential issue | 🟡 MinorFormatting: lint failure on line 279.
The long string literal on line 279 triggers the prettier lint failure. Wrap the line per the formatter's recommendation.
- console.error("DynamicCollider: Cannot set isKinematic=false when non-convex MeshColliderShape is attached."); + console.error( + "DynamicCollider: Cannot set isKinematic=false when non-convex MeshColliderShape is attached." + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/physics/DynamicCollider.ts` around lines 267 - 296, The console.error call in the isKinematic setter contains a single long string literal that fails the formatter; locate the setter for isKinematic (method name isKinematic, references this._shapes and MeshColliderShape) and break the long error message into multiple lines so it conforms to prettier (for example use a template literal with an explicit newline or concatenate shorter strings, or split into two arguments to console.error) while keeping the exact message content and the early return behavior unchanged.
🧹 Nitpick comments (3)
packages/physics-physx/src/PhysXPhysics.ts (1)
307-311: Commented-outsetCookingMidphaseType— add a TODO or issue reference.Line 309 has the BVH34 midphase call commented out with no indication of when/why it should be enabled. The comment on lines 307-308 describes the intent but doesn't explain why it's disabled. Add a
// TODO:with a tracking issue to avoid this becoming permanently dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/physics-physx/src/PhysXPhysics.ts` around lines 307 - 311, Add a TODO with a tracking issue reference explaining why the BVH34 midphase call (physX.setCookingMidphaseType(cookingParams, 1)) is commented out and when it should be re-enabled: locate the block that checks PhysXRuntimeMode.WebAssemblySIMD (this._runTimeMode === PhysXRuntimeMode.WebAssemblySIMD) near the physX.PxCreateCooking call and replace the bare commented call with a short TODO comment referencing an issue ID or URL (e.g., TODO: enable eBVH34 via physX.setCookingMidphaseType when SIMD support is verified - issue `#1234`) so the intent and action item are tracked.packages/physics-physx/src/shape/PhysXMeshColliderShape.ts (2)
87-142: Shared cooking params are mutated per-shape — possible interference.
_createMeshmodifies the shared_pxCookingParams(lines 96-104) on every call. Since cooking params are owned byPhysXPhysicsand shared across all mesh collider shapes, each_createMeshcall overwrites the global state. While JS is single-threaded, this means the global cooking params always reflect the last shape's flags, which could be confusing and fragile if future code expects them to retain their initial values.Consider documenting this intentional side-effect or, better, resetting the params to their original state after cooking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts` around lines 87 - 142, _createMesh currently mutates the shared PhysX cooking params (_pxCookingParams) via physX.setCookingMeshPreprocessParams and cooking.setParams which overwrites global state for all shapes; to fix, capture a copy of the original cooking params at start of _createMesh, apply the per-shape preprocessFlags and call physX.setCookingMeshPreprocessParams / cooking.setParams as you do now, then after cooking (before every return) restore the original params by calling physX.setCookingMeshPreprocessParams(originalParams, originalPreprocess) and cooking.setParams(originalParams) so other shapes aren't affected; use the existing symbols (_createMesh, _pxCookingParams, physX.setCookingMeshPreprocessParams, cooking.setParams) to locate and implement the save/restore logic and ensure restoration happens both on success and on early error paths (e.g., before returning false after _logConvexCookingError or _logTriMeshCookingError).
33-47: Constructor: geometry is created in_createMeshbut immediately superseded by the shape factory.
_createMeshstores a new_pxGeometry(lines 137-139), but the constructor then callscreateConvexMeshShape/createTriMeshShape(lines 39-41) which creates a shape with its own embedded geometry. The_pxGeometryfrom_createMeshis not used until a subsequentsetMeshDataor_updateGeometrycall. This means the initial_pxGeometryallocation is wasted.Consider either skipping geometry creation in
_createMeshwhen called from the constructor (e.g., a flag parameter), or restructuring so the constructor uses_pxGeometrydirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts` around lines 33 - 47, The constructor is creating a shape via createConvexMeshShape/createTriMeshShape while _createMesh always allocates a new _pxGeometry that isn’t used initially; change _createMesh to accept a boolean (e.g., skipCreateGeometry) or an options object so the constructor can call _createMesh(skipCreateGeometry=true) to avoid allocating _pxGeometry unnecessarily, or alternatively make the constructor call _createMesh to produce and return the _pxGeometry and then pass that geometry into createConvexMeshShape/createTriMeshShape so the initially created geometry is reused; update callers of _createMesh and ensure setMeshData/_updateGeometry behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/physics/DynamicCollider.ts`:
- Around line 274-282: The test failures are caused by changed error/warning
text in DynamicCollider and MeshColliderShape; revert or adjust the messages to
match test expectations: in DynamicCollider (the block checking
MeshColliderShape and the move() guard) restore the error text to include
"triangle mesh" (matching existing MeshColliderShape tests) and change the
move() warning to "DynamicCollider.move() should only be called when isKinematic
is true."; update the string passed to console.error/console.warn in the methods
where MeshColliderShape.nonConvex is checked and where move() checks isKinematic
(refer to the DynamicCollider class and MeshColliderShape type usages) so that
existing tests continue to match.
In `@packages/core/src/physics/shape/MeshColliderShape.ts`:
- Around line 95-115: The _extractMeshData method in MeshColliderShape currently
calls mesh.getPositions() and mesh.getIndices() without verifying the input
type, which causes a TypeError for non-ModelMesh inputs; update
MeshColliderShape._extractMeshData to first check that the passed object is an
instance of ModelMesh (or otherwise has the expected shape) and, if not, emit
the existing warning "MeshColliderShape: Only ModelMesh is supported" and return
false; ensure you use the ModelMesh type guard before calling
getPositions()/getIndices() so the subsequent logic (setting this._positions,
this._indices and the convex/non-convex branch) only runs for valid ModelMesh
instances.
- Around line 65-72: The mesh setter in MeshColliderShape currently calls
this._extractMeshData(value) even when value is null, which causes
_extractMeshData to dereference null (e.g., mesh.getPositions()) and throw;
modify the mesh setter to early-handle null: if value is null (or falsy) clear
this._mesh, clear any stored mesh data/state used by MeshColliderShape, and call
this._updateNativeMesh() (or the existing clear/update routine) as appropriate
instead of calling _extractMeshData; ensure references to
MeshColliderShape.mesh, _extractMeshData, and _updateNativeMesh are updated so
null assignments do not call methods on a null mesh.
In `@packages/design/src/physics/shape/IMeshColliderShape.ts`:
- Around line 9-20: Update the JSDoc for the setMeshData method to match the
TypeScript signature: change the `@param` description for indices to include
Uint8Array as well as Uint16Array and Uint32Array (or state "Uint8Array |
Uint16Array | Uint32Array | null") so the comment aligns with the indices
parameter type in IMeshColliderShape.setMeshData; keep references to positions,
indices, isConvex, and cookingFlags intact.
In `@packages/physics-physx/src/enum/PhysXRuntimeMode.ts`:
- Around line 4-11: Document that the enum member PhysXRuntimeMode.JavaScript
was removed from PhysXRuntimeMode and is a breaking change for external
consumers; update the 2.0.0-alpha.2 pre-release notes or changelog to explicitly
mention the removal, the migration path (use Auto, WebAssembly, or
WebAssemblySIMD), and the rationale for switching to WebAssembly-only modes so
downstream projects can adjust their code.
In `@packages/physics-physx/src/PhysXPhysics.ts`:
- Around line 96-100: The code is using SystemInfo as any to call the internal
method _detectSIMDSupported(); change the API to make this a public method
(rename to detectSIMDSupported or remove the leading underscore) on the
SystemInfo class/interface, update its JSDoc to state it checks WebAssembly SIMD
support, and then replace all casted/ignored calls (e.g., in PhysXPhysics.ts and
MeshoptDecoder.ts) to call SystemInfo.detectSIMDSupported() directly and remove
the any/@ts-ignore workarounds so callers use the typed public API (ensure the
method is exported in the SystemInfo declaration and its signature is updated
where implemented).
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts`:
- Around line 52-69: The setMeshData method currently releases this._pxMesh and
this._pxGeometry before calling this._createMesh, leaving this._pxShape pointing
to freed geometry if creation fails; change the flow so you first attempt to
create the new mesh (call this._createMesh) without releasing the old resources,
and only on success release the old this._pxMesh/this._pxGeometry and assign the
new ones and update this._pxShape geometry; if this._createMesh returns false,
keep the old this._pxMesh/this._pxGeometry and do not call
this._pxShape.setGeometry (or alternatively disable/detach this._pxShape),
ensuring this._isConvex is only updated after successful creation to avoid
inconsistent state.
---
Outside diff comments:
In `@packages/core/src/physics/DynamicCollider.ts`:
- Around line 267-296: The console.error call in the isKinematic setter contains
a single long string literal that fails the formatter; locate the setter for
isKinematic (method name isKinematic, references this._shapes and
MeshColliderShape) and break the long error message into multiple lines so it
conforms to prettier (for example use a template literal with an explicit
newline or concatenate shorter strings, or split into two arguments to
console.error) while keeping the exact message content and the early return
behavior unchanged.
In `@packages/core/src/physics/shape/ColliderShape.ts`:
- Around line 183-192: The getClosestPoint method dereferences this._nativeShape
without the null guard used in _syncNative, causing a runtime error when the
native shape isn't created (e.g., MeshColliderShape before mesh assignment); add
a guard at the start of getClosestPoint to check if this._nativeShape exists
and, if not, return a safe sentinel (e.g., null or an appropriate "no result"
value used in your codebase) instead of calling
this._nativeShape.pointDistance(point); ensure the method references
getClosestPoint and _syncNative so the fix mirrors the established null-check
pattern.
In `@tests/src/core/physics/MeshColliderShape.test.ts`:
- Around line 506-527: Update the failing tests to match the new error text
emitted by DynamicCollider.addShape and the isKinematic setter: change
expectations that look for "triangle mesh" to assert on the new message fragment
(e.g., "Non-convex MeshColliderShape" or "non-convex MeshColliderShape") so they
reference the actual strings emitted by DynamicCollider.addShape and
MeshColliderShape.isKinematic; update the assertions in
MeshColliderShape.test.ts that reference DynamicCollider.addShape and the
isKinematic setter to use expect.stringContaining with the exact new fragment
and restore the error spy as before.
- Around line 458-503: The test's expected warning string is out of sync with
MeshColliderShape.ts; update the assertion in MeshColliderShape.test.ts to match
the actual message emitted by MeshColliderShape (change
expect(warnSpy).toHaveBeenCalledWith("MeshColliderShape: Mesh has no position
attribute") to expect the message "MeshColliderShape: Mesh has no position
data") so the warnSpy assertion matches the warning emitted by the
MeshColliderShape class.
- Around line 599-613: The test's expected warning text is outdated; update the
assertion in the DynamicCollider.move() test so the
expect(warnSpy).toHaveBeenCalledWith(...) matches the actual message emitted by
DynamicCollider.move() ("DynamicCollider: move() is only supported when
isKinematic is true."); locate the test that spies on console.warn (warnSpy) and
replace the current expected string with the exact string produced in
DynamicCollider.ts for move() so the test assertion matches the implementation.
- Around line 433-456: The test fails because MeshColliderShape's mesh setter
directly calls _extractMeshData and will throw when given a non-ModelMesh;
update the MeshColliderShape.set mesh setter to guard the input with an
instanceof ModelMesh check (or equivalent type check) and, if it is not a
ModelMesh, call console.warn("MeshColliderShape: Only ModelMesh is supported")
and return early without calling _extractMeshData; ensure you reference
MeshColliderShape, its mesh setter, and _extractMeshData so the change prevents
the TypeError and satisfies the test expectation.
---
Nitpick comments:
In `@packages/physics-physx/src/PhysXPhysics.ts`:
- Around line 307-311: Add a TODO with a tracking issue reference explaining why
the BVH34 midphase call (physX.setCookingMidphaseType(cookingParams, 1)) is
commented out and when it should be re-enabled: locate the block that checks
PhysXRuntimeMode.WebAssemblySIMD (this._runTimeMode ===
PhysXRuntimeMode.WebAssemblySIMD) near the physX.PxCreateCooking call and
replace the bare commented call with a short TODO comment referencing an issue
ID or URL (e.g., TODO: enable eBVH34 via physX.setCookingMidphaseType when SIMD
support is verified - issue `#1234`) so the intent and action item are tracked.
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts`:
- Around line 87-142: _createMesh currently mutates the shared PhysX cooking
params (_pxCookingParams) via physX.setCookingMeshPreprocessParams and
cooking.setParams which overwrites global state for all shapes; to fix, capture
a copy of the original cooking params at start of _createMesh, apply the
per-shape preprocessFlags and call physX.setCookingMeshPreprocessParams /
cooking.setParams as you do now, then after cooking (before every return)
restore the original params by calling
physX.setCookingMeshPreprocessParams(originalParams, originalPreprocess) and
cooking.setParams(originalParams) so other shapes aren't affected; use the
existing symbols (_createMesh, _pxCookingParams,
physX.setCookingMeshPreprocessParams, cooking.setParams) to locate and implement
the save/restore logic and ensure restoration happens both on success and on
early error paths (e.g., before returning false after _logConvexCookingError or
_logTriMeshCookingError).
- Around line 33-47: The constructor is creating a shape via
createConvexMeshShape/createTriMeshShape while _createMesh always allocates a
new _pxGeometry that isn’t used initially; change _createMesh to accept a
boolean (e.g., skipCreateGeometry) or an options object so the constructor can
call _createMesh(skipCreateGeometry=true) to avoid allocating _pxGeometry
unnecessarily, or alternatively make the constructor call _createMesh to produce
and return the _pxGeometry and then pass that geometry into
createConvexMeshShape/createTriMeshShape so the initially created geometry is
reused; update callers of _createMesh and ensure setMeshData/_updateGeometry
behavior remains unchanged.
Comment on lines
274
to
282
| // Block switching to non-kinematic if non-convex MeshColliderShape is attached | ||
| const shapes = this._shapes; | ||
| for (let i = 0, n = shapes.length; i < n; i++) { | ||
| const shape = shapes[i]; | ||
| if (shape instanceof MeshColliderShape && !shape.isConvex) { | ||
| console.error("DynamicCollider: Cannot set isKinematic=false when triangle mesh is attached."); | ||
| console.error("DynamicCollider: Cannot set isKinematic=false when non-convex MeshColliderShape is attached."); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Error message change will break two existing tests.
The error message on line 279 now says "non-convex MeshColliderShape" and line 422 says "Non-convex MeshColliderShape". However, the tests in MeshColliderShape.test.ts at lines 522 and 547 assert with expect.stringContaining("triangle mesh"), which will no longer match.
Similarly, line 389's warning "DynamicCollider: move() is only supported when isKinematic is true." doesn't match the test assertion at line 609 which expects "DynamicCollider.move() should only be called when isKinematic is true.".
Either update the test assertions to match the new messages, or align the messages with what the tests expect.
🧰 Tools
🪛 ESLint
[error] 279-279: Replace "DynamicCollider:·Cannot·set·isKinematic=false·when·non-convex·MeshColliderShape·is·attached." with ⏎··············"DynamicCollider:·Cannot·set·isKinematic=false·when·non-convex·MeshColliderShape·is·attached."⏎············
(prettier/prettier)
🪛 GitHub Check: lint
[failure] 279-279:
Replace "DynamicCollider:·Cannot·set·isKinematic=false·when·non-convex·MeshColliderShape·is·attached." with ⏎··············"DynamicCollider:·Cannot·set·isKinematic=false·when·non-convex·MeshColliderShape·is·attached."⏎············
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/physics/DynamicCollider.ts` around lines 274 - 282, The
test failures are caused by changed error/warning text in DynamicCollider and
MeshColliderShape; revert or adjust the messages to match test expectations: in
DynamicCollider (the block checking MeshColliderShape and the move() guard)
restore the error text to include "triangle mesh" (matching existing
MeshColliderShape tests) and change the move() warning to
"DynamicCollider.move() should only be called when isKinematic is true."; update
the string passed to console.error/console.warn in the methods where
MeshColliderShape.nonConvex is checked and where move() checks isKinematic
(refer to the DynamicCollider class and MeshColliderShape type usages) so that
existing tests continue to match.
Comment on lines
95
to
115
| private _extractMeshData(mesh: ModelMesh): boolean { | ||
| // @ts-ignore: Access internal property for performance optimization | ||
| const primitive = mesh._primitive; | ||
| const vertexElement = primitive._vertexElementMap?.[VertexAttribute.Position]; | ||
|
|
||
| if (!vertexElement) { | ||
| console.warn("MeshColliderShape: Mesh has no position attribute"); | ||
| return false; | ||
| } | ||
|
|
||
| const bufferBinding = primitive.vertexBufferBindings[vertexElement.bindingIndex]; | ||
| const buffer = bufferBinding?.buffer; | ||
|
|
||
| if (!buffer) { | ||
| console.warn("MeshColliderShape: Position buffer not found"); | ||
| return false; | ||
| } | ||
|
|
||
| if (!buffer.readable) { | ||
| console.warn("MeshColliderShape: Buffer is not readable"); | ||
| const positions = mesh.getPositions(); | ||
| if (!positions || positions.length === 0) { | ||
| console.warn("MeshColliderShape: Mesh has no position data"); | ||
| return false; | ||
| } | ||
|
|
||
| const vertexCount = mesh.vertexCount; | ||
| const byteOffset = vertexElement.offset; | ||
| const byteStride = bufferBinding.stride; | ||
| const bufferData = buffer.data; | ||
|
|
||
| // Reuse or create Float32Array | ||
| if (!this._vertices || this._vertices.length !== vertexCount * 3) { | ||
| this._vertices = new Float32Array(vertexCount * 3); | ||
| } | ||
|
|
||
| // Create Float32Array view to read source data | ||
| const sourceData = new Float32Array(bufferData.buffer, bufferData.byteOffset, bufferData.byteLength / 4); | ||
|
|
||
| // Choose optimal copy method based on stride | ||
| if (byteStride === 12 && byteOffset === 0) { | ||
| // Tightly packed: direct copy | ||
| this._vertices.set(sourceData.subarray(0, vertexCount * 3)); | ||
| } else { | ||
| // Interleaved: copy per vertex with optimized indexing | ||
| const floatStride = byteStride / 4; | ||
| const floatOffset = byteOffset / 4; | ||
| const vertices = this._vertices; | ||
| for (let i = 0, srcIdx = floatOffset, dstIdx = 0; i < vertexCount; i++, srcIdx += floatStride, dstIdx += 3) { | ||
| vertices[dstIdx] = sourceData[srcIdx]; | ||
| vertices[dstIdx + 1] = sourceData[srcIdx + 1]; | ||
| vertices[dstIdx + 2] = sourceData[srcIdx + 2]; | ||
| } | ||
| } | ||
| this._positions = positions; | ||
| this._indices = null; | ||
|
|
||
| // Extract indices for triangle mesh | ||
| if (!this._isConvex) { | ||
| this._extractIndices(mesh); | ||
| if (!this._indices) { | ||
| const indices = mesh.getIndices(); | ||
| if (!indices) { | ||
| console.warn("MeshColliderShape: Triangle mesh requires indices"); | ||
| return false; | ||
| } | ||
| this._indices = indices; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
No instanceof ModelMesh guard before calling getPositions().
The test at line 448-451 of MeshColliderShape.test.ts passes a fake object ({ notAModelMesh: true }) and expects a warning "MeshColliderShape: Only ModelMesh is supported". However, _extractMeshData directly calls mesh.getPositions() without an instanceof check, which would throw a TypeError on non-ModelMesh inputs.
Add a runtime check if you want to support graceful handling of invalid inputs:
private _extractMeshData(mesh: ModelMesh): boolean {
+ if (!(mesh instanceof ModelMesh)) {
+ console.warn("MeshColliderShape: Only ModelMesh is supported");
+ return false;
+ }
const positions = mesh.getPositions();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/physics/shape/MeshColliderShape.ts` around lines 95 - 115,
The _extractMeshData method in MeshColliderShape currently calls
mesh.getPositions() and mesh.getIndices() without verifying the input type,
which causes a TypeError for non-ModelMesh inputs; update
MeshColliderShape._extractMeshData to first check that the passed object is an
instance of ModelMesh (or otherwise has the expected shape) and, if not, emit
the existing warning "MeshColliderShape: Only ModelMesh is supported" and return
false; ensure you use the ModelMesh type guard before calling
getPositions()/getIndices() so the subsequent logic (setting this._positions,
this._indices and the convex/non-convex branch) only runs for valid ModelMesh
instances.
Comment on lines
9
to
20
| * Set mesh data for this collider shape. | ||
| * @param vertices - The vertex positions array (Float32Array, 3 floats per vertex) | ||
| * @param vertexCount - Number of vertices | ||
| * @param positions - Vertex positions | ||
| * @param indices - The index array (Uint16Array or Uint32Array), required for triangle mesh | ||
| * @param isConvex - Whether to use convex mesh (true) or triangle mesh (false) | ||
| * @param cookingFlags - Cooking flags | ||
| */ | ||
| setMeshData( | ||
| vertices: Float32Array, | ||
| vertexCount: number, | ||
| indices: Uint16Array | Uint32Array | null, | ||
| isConvex: boolean | ||
| positions: Vector3[], | ||
| indices: Uint8Array | Uint16Array | Uint32Array | null, | ||
| isConvex: boolean, | ||
| cookingFlags: number | ||
| ): void; |
There was a problem hiding this comment.
JSDoc for indices parameter is inconsistent with the type signature.
Line 11 says "Uint16Array or Uint32Array" but the type on line 17 also includes Uint8Array. Update the doc to match.
- * `@param` indices - The index array (Uint16Array or Uint32Array), required for triangle mesh
+ * `@param` indices - The index array (Uint8Array, Uint16Array or Uint32Array), required for triangle mesh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * Set mesh data for this collider shape. | |
| * @param vertices - The vertex positions array (Float32Array, 3 floats per vertex) | |
| * @param vertexCount - Number of vertices | |
| * @param positions - Vertex positions | |
| * @param indices - The index array (Uint16Array or Uint32Array), required for triangle mesh | |
| * @param isConvex - Whether to use convex mesh (true) or triangle mesh (false) | |
| * @param cookingFlags - Cooking flags | |
| */ | |
| setMeshData( | |
| vertices: Float32Array, | |
| vertexCount: number, | |
| indices: Uint16Array | Uint32Array | null, | |
| isConvex: boolean | |
| positions: Vector3[], | |
| indices: Uint8Array | Uint16Array | Uint32Array | null, | |
| isConvex: boolean, | |
| cookingFlags: number | |
| ): void; | |
| * Set mesh data for this collider shape. | |
| * `@param` positions - Vertex positions | |
| * `@param` indices - The index array (Uint8Array, Uint16Array or Uint32Array), required for triangle mesh | |
| * `@param` isConvex - Whether to use convex mesh (true) or triangle mesh (false) | |
| * `@param` cookingFlags - Cooking flags | |
| */ | |
| setMeshData( | |
| positions: Vector3[], | |
| indices: Uint8Array | Uint16Array | Uint32Array | null, | |
| isConvex: boolean, | |
| cookingFlags: number | |
| ): void; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/design/src/physics/shape/IMeshColliderShape.ts` around lines 9 - 20,
Update the JSDoc for the setMeshData method to match the TypeScript signature:
change the `@param` description for indices to include Uint8Array as well as
Uint16Array and Uint32Array (or state "Uint8Array | Uint16Array | Uint32Array |
null") so the comment aligns with the indices parameter type in
IMeshColliderShape.setMeshData; keep references to positions, indices, isConvex,
and cookingFlags intact.
Comment on lines
4
to
11
| export enum PhysXRuntimeMode { | ||
| /** Use webAssembly mode first, if WebAssembly mode is not supported, roll back to JavaScript mode. */ | ||
| /** Use WebAssembly SIMD mode first, then WebAssembly as fallback. */ | ||
| Auto, | ||
| /** WebAssembly mode. */ | ||
| WebAssembly, | ||
| /** JavaScript mode. */ | ||
| JavaScript | ||
| /** WebAssembly SIMD mode. */ | ||
| WebAssemblySIMD | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any remaining references to PhysXRuntimeMode.JavaScript or javaScriptModeUrl in the codebase
echo "=== References to PhysXRuntimeMode.JavaScript ==="
rg -n 'PhysXRuntimeMode\.JavaScript' --type=ts
echo ""
echo "=== References to javaScriptModeUrl ==="
rg -n 'javaScriptModeUrl' --type=ts
echo ""
echo "=== Check for any JavaScript mode references more broadly ==="
rg -n 'JavaScript' packages/physics-physx/src/ --type=ts | head -30Repository: galacean/engine
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Search for changelog and migration documentation
echo "=== Searching for CHANGELOG files ==="
find . -iname "*changelog*" -o -iname "*change*" -o -iname "*migration*" | grep -v node_modules | head -20
echo ""
echo "=== Check package.json for version info ==="
cat packages/physics-physx/package.json | grep -A 2 -B 2 '"version"'
echo ""
echo "=== Search for breaking change notices in docs ==="
rg -i 'breaking\s+change' --type=ts --type=md | head -20
echo ""
echo "=== Check git log for this file (recent commits) ==="
git log --oneline -20 packages/physics-physx/src/enum/PhysXRuntimeMode.ts 2>/dev/null || echo "Git history not available"
echo ""
echo "=== Look for all enum usages in the codebase ==="
rg 'PhysXRuntimeMode' --type=ts | grep -v 'node_modules' | head -30Repository: galacean/engine
Length of output: 2162
Document breaking change in pre-release notes for external consumers.
Removing PhysXRuntimeMode.JavaScript is a breaking change. While all internal usages have been properly migrated, external consumers relying on this enum member will encounter compilation errors. Since this is 2.0.0-alpha.2, breaking changes are expected, but consider adding a migration note to the pre-release documentation or changelog explaining the shift to WebAssembly-only modes and the rationale for dropping JavaScript mode support.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/physics-physx/src/enum/PhysXRuntimeMode.ts` around lines 4 - 11,
Document that the enum member PhysXRuntimeMode.JavaScript was removed from
PhysXRuntimeMode and is a breaking change for external consumers; update the
2.0.0-alpha.2 pre-release notes or changelog to explicitly mention the removal,
the migration path (use Auto, WebAssembly, or WebAssemblySIMD), and the
rationale for switching to WebAssembly-only modes so downstream projects can
adjust their code.
Comment on lines
96
to
100
| if (runtimeMode == PhysXRuntimeMode.Auto) { | ||
| const supported = (() => { | ||
| try { | ||
| if (typeof WebAssembly === "object" && typeof WebAssembly.instantiate === "function") { | ||
| const wasmModule = new WebAssembly.Module(Uint8Array.of(0x0, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00)); | ||
| if (wasmModule instanceof WebAssembly.Module) | ||
| return new WebAssembly.Instance(wasmModule) instanceof WebAssembly.Instance; | ||
| } | ||
| } catch (e) {} | ||
| return false; | ||
| })(); | ||
| if (supported) { | ||
| runtimeMode = PhysXRuntimeMode.WebAssembly; | ||
| } else { | ||
| runtimeMode = PhysXRuntimeMode.JavaScript; | ||
| } | ||
| runtimeMode = (SystemInfo as any)._detectSIMDSupported() | ||
| ? PhysXRuntimeMode.WebAssemblySIMD | ||
| : PhysXRuntimeMode.WebAssembly; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "_detectSIMDSupported" --type=ts -C3Repository: galacean/engine
Length of output: 1603
🏁 Script executed:
sed -n '80,110p' packages/core/src/SystemInfo.tsRepository: galacean/engine
Length of output: 964
🏁 Script executed:
head -100 packages/core/src/SystemInfo.ts | cat -nRepository: galacean/engine
Length of output: 4063
Remove the any cast by exposing _detectSIMDSupported() as a public API in SystemInfo.
The method is marked @internal but is already used across the codebase (found in MeshoptDecoder.ts and PhysXPhysics.ts). Multiple callers accessing an internal API via any cast indicates this should be a public, documented method. The current approach forces workarounds like @ts-ignore comments and type casts, which obscures intent. Promote _detectSIMDSupported() to public and update its JSDoc to clarify that it checks SIMD support in WebAssembly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/physics-physx/src/PhysXPhysics.ts` around lines 96 - 100, The code
is using SystemInfo as any to call the internal method _detectSIMDSupported();
change the API to make this a public method (rename to detectSIMDSupported or
remove the leading underscore) on the SystemInfo class/interface, update its
JSDoc to state it checks WebAssembly SIMD support, and then replace all
casted/ignored calls (e.g., in PhysXPhysics.ts and MeshoptDecoder.ts) to call
SystemInfo.detectSIMDSupported() directly and remove the any/@ts-ignore
workarounds so callers use the typed public API (ensure the method is exported
in the SystemInfo declaration and its signature is updated where implemented).
Comment on lines
52
to
69
| setMeshData( | ||
| vertices: Float32Array, | ||
| vertexCount: number, | ||
| indices: Uint16Array | Uint32Array | null, | ||
| isConvex: boolean | ||
| positions: Vector3[], | ||
| indices: Uint8Array | Uint16Array | Uint32Array | null, | ||
| isConvex: boolean, | ||
| cookingFlags: number | ||
| ): void { | ||
| // Save old resources | ||
| const oldMesh = this._pxMesh; | ||
| const oldGeometry = this._pxGeometry; | ||
|
|
||
| // Update data and create new mesh (may throw on failure) | ||
| this._pxMesh?.release(); | ||
| this._pxGeometry?.delete(); | ||
| this._pxMesh = null; | ||
| this._pxGeometry = null; | ||
| this._isConvex = isConvex; | ||
| this._vertices = vertices; | ||
| this._vertexCount = vertexCount; | ||
| this._indices = indices; | ||
|
|
||
| this._createMesh(); | ||
| this._pxShape.setGeometry(this._pxGeometry); | ||
|
|
||
| // Release old resources only after successful creation | ||
| if (oldMesh) { | ||
| oldMesh.release(); | ||
| } | ||
| if (oldGeometry) { | ||
| oldGeometry.delete(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc IMeshColliderShape.setDoubleSided } | ||
| */ | ||
| setDoubleSided(value: boolean): void { | ||
| this._doubleSided = value; | ||
| if (!this._isConvex && this._pxMesh) { | ||
| this._updateGeometry(); | ||
| if (!this._createMesh(positions, indices, cookingFlags)) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc IMeshColliderShape.setTightBounds } | ||
| */ | ||
| setTightBounds(value: boolean): void { | ||
| this._tightBounds = value; | ||
| if (this._isConvex && this._pxMesh) { | ||
| this._updateGeometry(); | ||
| } | ||
| this._pxShape.setGeometry(this._pxGeometry); | ||
| } |
There was a problem hiding this comment.
setMeshData leaves shape in inconsistent state if _createMesh fails.
Lines 58-59 release the old _pxMesh and _pxGeometry before attempting to create new ones. If _createMesh fails (returns false), _pxShape still exists but its underlying geometry has been deleted. Subsequent use of the shape (e.g., physics queries, simulation) will reference freed memory.
Either defer releasing old resources until after the new mesh is confirmed created, or detach/disable the shape on failure.
Proposed fix: defer release of old resources
setMeshData(
positions: Vector3[],
indices: Uint8Array | Uint16Array | Uint32Array | null,
isConvex: boolean,
cookingFlags: number
): void {
+ const oldMesh = this._pxMesh;
+ const oldGeometry = this._pxGeometry;
+ this._pxMesh = null;
+ this._pxGeometry = null;
- this._pxMesh?.release();
- this._pxGeometry?.delete();
- this._pxMesh = null;
- this._pxGeometry = null;
this._isConvex = isConvex;
if (!this._createMesh(positions, indices, cookingFlags)) {
+ // Restore old resources if new mesh creation fails
+ this._pxMesh = oldMesh;
+ this._pxGeometry = oldGeometry;
return;
}
+ oldMesh?.release();
+ oldGeometry?.delete();
this._pxShape.setGeometry(this._pxGeometry);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| setMeshData( | |
| vertices: Float32Array, | |
| vertexCount: number, | |
| indices: Uint16Array | Uint32Array | null, | |
| isConvex: boolean | |
| positions: Vector3[], | |
| indices: Uint8Array | Uint16Array | Uint32Array | null, | |
| isConvex: boolean, | |
| cookingFlags: number | |
| ): void { | |
| // Save old resources | |
| const oldMesh = this._pxMesh; | |
| const oldGeometry = this._pxGeometry; | |
| // Update data and create new mesh (may throw on failure) | |
| this._pxMesh?.release(); | |
| this._pxGeometry?.delete(); | |
| this._pxMesh = null; | |
| this._pxGeometry = null; | |
| this._isConvex = isConvex; | |
| this._vertices = vertices; | |
| this._vertexCount = vertexCount; | |
| this._indices = indices; | |
| this._createMesh(); | |
| this._pxShape.setGeometry(this._pxGeometry); | |
| // Release old resources only after successful creation | |
| if (oldMesh) { | |
| oldMesh.release(); | |
| } | |
| if (oldGeometry) { | |
| oldGeometry.delete(); | |
| } | |
| } | |
| /** | |
| * {@inheritDoc IMeshColliderShape.setDoubleSided } | |
| */ | |
| setDoubleSided(value: boolean): void { | |
| this._doubleSided = value; | |
| if (!this._isConvex && this._pxMesh) { | |
| this._updateGeometry(); | |
| if (!this._createMesh(positions, indices, cookingFlags)) { | |
| return; | |
| } | |
| } | |
| /** | |
| * {@inheritDoc IMeshColliderShape.setTightBounds } | |
| */ | |
| setTightBounds(value: boolean): void { | |
| this._tightBounds = value; | |
| if (this._isConvex && this._pxMesh) { | |
| this._updateGeometry(); | |
| } | |
| this._pxShape.setGeometry(this._pxGeometry); | |
| } | |
| setMeshData( | |
| positions: Vector3[], | |
| indices: Uint8Array | Uint16Array | Uint32Array | null, | |
| isConvex: boolean, | |
| cookingFlags: number | |
| ): void { | |
| const oldMesh = this._pxMesh; | |
| const oldGeometry = this._pxGeometry; | |
| this._pxMesh = null; | |
| this._pxGeometry = null; | |
| this._isConvex = isConvex; | |
| if (!this._createMesh(positions, indices, cookingFlags)) { | |
| // Restore old resources if new mesh creation fails | |
| this._pxMesh = oldMesh; | |
| this._pxGeometry = oldGeometry; | |
| return; | |
| } | |
| oldMesh?.release(); | |
| oldGeometry?.delete(); | |
| this._pxShape.setGeometry(this._pxGeometry); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts` around lines 52 -
69, The setMeshData method currently releases this._pxMesh and this._pxGeometry
before calling this._createMesh, leaving this._pxShape pointing to freed
geometry if creation fails; change the flow so you first attempt to create the
new mesh (call this._createMesh) without releasing the old resources, and only
on success release the old this._pxMesh/this._pxGeometry and assign the new ones
and update this._pxShape geometry; if this._createMesh returns false, keep the
old this._pxMesh/this._pxGeometry and do not call this._pxShape.setGeometry (or
alternatively disable/detach this._pxShape), ensuring this._isConvex is only
updated after successful creation to avoid inconsistent state.
GuoLei1990
changed the title
GuoLei1990
changed the title
GuoLei1990
changed the title
GuoLei1990
added
enhancement
GuoLei1990
changed the title
GuoLei1990
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/physics-physx/src/shape/PhysXMeshColliderShape.ts (1)
94-115: Modifying sharedcookingParamsis safe but fragile.
_pxCookingParamsis a singleton on thePhysXPhysicsinstance. Each call to_createMeshmutates it and callscooking.setParams(cookingParams). This is correct in single-threaded JS, but note that after this call the global cooking state reflects the last shape's flags — not the original defaults. If any other code path reads_pxCookingParamsexpecting the initial configuration, it will see stale values.This is fine for now since all mesh cooking routes through
_createMesh, but worth a brief comment to flag the intentional side-effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts` around lines 94 - 115, The code mutates the shared _pxCookingParams singleton on the PhysXPhysics instance inside _createMesh (via physX.setCookingMeshPreprocessParams and cooking.setParams), which intentionally changes global cooking state and can lead to stale values elsewhere; add a brief inline comment immediately before the mutation (referencing _createMesh, _pxCookingParams, physX.setCookingMeshPreprocessParams and cooking.setParams) stating that this side-effect is intentional, that _pxCookingParams is shared and will be mutated for each mesh cook, and warning future maintainers that changes affect global cooking state so they should avoid relying on original defaults or create clones if isolation is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts`:
- Around line 136-143: PhysXMeshColliderShape passes indices.length / 3 into
cooking.createTriMesh which can be non-integer if indices.length isn't divisible
by 3; update the code around the call that constructs this._pxMesh (the
cooking.createTriMesh invocation using verticesPtr and indicesPtr) to validate
that indices.length % 3 === 0 and either throw/log a clear error if not, or use
Math.floor(indices.length / 3) after logging a warning, ensuring the triangle
count passed to createTriMesh is an integer.
---
Duplicate comments:
In `@packages/core/src/physics/DynamicCollider.ts`:
- Around line 422-428: Tests that assert on the addShape error need to be
updated to match the new error text in DynamicCollider.addShape — locate
references to DynamicCollider.addShape or assertions checking for "triangle
mesh" or previous phrasing and change them to expect the new message
"DynamicCollider: Non-convex MeshColliderShape is not supported on non-kinematic
DynamicCollider." (or relax to a regex like /Non[- ]convex.*MeshColliderShape/);
update any expect(...).toThrow/console error spies accordingly so tests assert
the new message or the looser pattern.
- Around line 389-395: The warning text in DynamicCollider.move was changed;
update the tests that assert this warning (e.g., in MeshColliderShape.test.ts)
to expect the exact new string "DynamicCollider: move() is only supported when
isKinematic is true." Locate assertions that spy or mock console.warn (or use
toHaveBeenCalledWith) and replace the expected message to match the new text;
ensure the test still sets _isKinematic false and restores any console
spies/mocks after the test.
In `@packages/core/src/physics/shape/MeshColliderShape.ts`:
- Around line 95-100: In _extractMeshData, add an explicit guard to check that
the mesh is an instance of ModelMesh before calling mesh.getPositions(); if not,
emit the expected warning ("Only ModelMesh is supported" or similar) and return
false. Keep the existing null/empty positions check and early return if
positions are missing, but move the instanceof check to run first so TypeError
is avoided when non-ModelMesh objects are passed to _extractMeshData.
- Around line 65-72: The setter mesh assigns this._mesh then calls
this._extractMeshData(value) which dereferences value and crashes when value is
null/undefined; guard against null by checking value (or this._mesh) before
calling _extractMeshData and only call _updateNativeMesh when _extractMeshData
succeeded; update the mesh setter (referencing the mesh setter,
_extractMeshData, and _updateNativeMesh) to return early or skip extraction when
value is null/undefined and handle clearing native mesh state if needed.
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts`:
- Around line 59-76: In setMeshData, avoid releasing
this._pxMesh/this._pxGeometry before creating the replacement: change
setMeshData to call _createMesh in a way that returns the newly created
mesh/geometry (or have _createMesh write to temp variables instead of assigning
to this._pxMesh/this._pxGeometry), verify creation succeeded, then atomically
set this._pxMesh/this._pxGeometry to the new objects, call
this._pxShape.setGeometry(this._pxGeometry), and only after successful
assignment release the old mesh/geometry; update _createMesh signature/behavior
accordingly so old and new objects can coexist briefly and no use-after-free
occurs.
---
Nitpick comments:
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts`:
- Around line 94-115: The code mutates the shared _pxCookingParams singleton on
the PhysXPhysics instance inside _createMesh (via
physX.setCookingMeshPreprocessParams and cooking.setParams), which intentionally
changes global cooking state and can lead to stale values elsewhere; add a brief
inline comment immediately before the mutation (referencing _createMesh,
_pxCookingParams, physX.setCookingMeshPreprocessParams and cooking.setParams)
stating that this side-effect is intentional, that _pxCookingParams is shared
and will be mutated for each mesh cook, and warning future maintainers that
changes affect global cooking state so they should avoid relying on original
defaults or create clones if isolation is needed.
Comment on lines
+136
to
143
| this._pxMesh = cooking.createTriMesh( | ||
| verticesPtr, | ||
| positions.length, | ||
| indicesPtr, | ||
| indices.length / 3, | ||
| !isU32, | ||
| physics | ||
| ); |
There was a problem hiding this comment.
indices.length / 3 may produce a non-integer triangle count.
If indices.length is not divisible by 3, this passes a fractional value to the native createTriMesh, which may silently truncate or produce undefined behavior depending on the PhysX binding. Consider adding a validation or using Math.floor.
Proposed fix
this._pxMesh = cooking.createTriMesh(
verticesPtr,
positions.length,
indicesPtr,
- indices.length / 3,
+ (indices.length / 3) | 0,
!isU32,
physics
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this._pxMesh = cooking.createTriMesh( | |
| verticesPtr, | |
| positions.length, | |
| indicesPtr, | |
| indices.length / 3, | |
| !isU32, | |
| physics | |
| ); | |
| this._pxMesh = cooking.createTriMesh( | |
| verticesPtr, | |
| positions.length, | |
| indicesPtr, | |
| (indices.length / 3) | 0, | |
| !isU32, | |
| physics | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts` around lines 136
- 143, PhysXMeshColliderShape passes indices.length / 3 into
cooking.createTriMesh which can be non-integer if indices.length isn't divisible
by 3; update the code around the call that constructs this._pxMesh (the
cooking.createTriMesh invocation using verticesPtr and indicesPtr) to validate
that indices.length % 3 === 0 and either throw/log a clear error if not, or use
Math.floor(indices.length / 3) after logging a warning, ensuring the triangle
count passed to createTriMesh is an integer.
…ignment in MeshColliderShape
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/case/physx-mesh-collider.ts`:
- Line 161: The line declaring "let collider =
meshEntity.getComponent(StaticCollider);" is over-indented by 10 extra spaces;
remove the extra leading spaces so its indentation matches the surrounding block
(align with the other statements in the same scope where meshEntity and
StaticCollider are used) to satisfy ESLint's "Delete '··········'".
…RLs for SIMD and WASM
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/case/physx-collision-group.ts (1)
74-78: Same initialization pattern — LGTM.Matches the updated constructor API. Consider extracting the shared
PhysXPhysicsconfiguration (runtime mode + URLs) into a common E2E helper to avoid repeating the same boilerplate in every test file. This would also give you a single place to update when the CDN URLs are finalized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/case/physx-collision-group.ts` around lines 74 - 78, Extract the repeated PhysX initialization into a shared E2E helper: create a helper function (e.g., getDefaultPhysXConfig or createPhysXInstance) that returns or instantiates PhysXPhysics with PhysXRuntimeMode.Auto and the simdModeUrl/wasmModeUrl values, then replace inline uses in test files (the call site using new PhysXPhysics(...) and WebGLEngine.create({ canvas: "canvas", physics })) to call the helper; ensure the helper is exported from a common test utilities module so the CDN URL literals are centralized for easy updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/case/physx-collision-group.ts`:
- Around line 74-78: Extract the repeated PhysX initialization into a shared E2E
helper: create a helper function (e.g., getDefaultPhysXConfig or
createPhysXInstance) that returns or instantiates PhysXPhysics with
PhysXRuntimeMode.Auto and the simdModeUrl/wasmModeUrl values, then replace
inline uses in test files (the call site using new PhysXPhysics(...) and
WebGLEngine.create({ canvas: "canvas", physics })) to call the helper; ensure
the helper is exported from a common test utilities module so the CDN URL
literals are centralized for easy updates.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Code review and refinement of #2880, plus WebAssembly SIMD runtime support.
MeshColliderShape refinements (vs #2880)
setMesh()+setMeshData()with singlemeshproperty setter, useVector3[]fromgetPositions()instead of manualFloat32Arraybuffer extractiondoubleSided/tightBoundspublic properties, add per-shapecookingFlags(MeshColliderShapeCookingFlag) replacing globalsetCookingParams()APIisConvexsetter triggers re-cooking at runtimecreateMeshColliderShape()returnsnullon cooking failure with detailed error codesCollisionDetectionMode— each mode explicitly resets unrelated CCD flagsWebAssembly SIMD runtime
PhysXRuntimeMode.JavaScriptwithWebAssemblySIMD,Autoprefers SIMD WASM viaSystemInfo._detectSIMDSupported()physx.release.wasm, now correctly referencesphysx.release.simd.wasmviasedpost-processing inbuild.shBreaking changes
PhysXRuntimeMode.JavaScriptremovedPhysXRuntimeUrls.javaScriptModeUrlremovedDependencies
Note
E2E tests temporarily use local PhysX library paths (
../physx.release.simd.js,../physx.release.js) instead of CDN URLs, pending CDN upload of the new SIMD WASM build.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores