feat(physics): 添加 MeshColliderShape 支持 #2880
Conversation
- 新增 MeshColliderShape 类，支持凸包和三角网格碰撞体 - 新增 IMeshColliderShape 接口定义 - PhysXPhysics 中实现 createMeshColliderShape 方法 - 初始化 PxCooking 用于网格碰撞体的烘焙 - 更新 PhysX WASM 文件以支持网格碰撞体 - 修复 Collider 添加 shape 时设置世界缩放 - 添加 MeshColliderShape 单元测试和 E2E 测试
- 重写 physx-mesh-collider 测试，使用 glTF 模型展示网格碰撞 - 新增 physx-mesh-collider-data 测试，展示 setMeshData API - 添加 CCD 支持防止高速物体穿透薄网格 - 更新 PhysX WASM 支持 setCookingMeshPreprocessParams - e2e 测试使用本地 physx 文件而非 CDN - 删除 engine-mcp 目录
- 删除 C++ 源码中的调试日志 - 重新编译并上传 physx wasm/js 到 CDN - 更新 PhysXPhysics.ts 中的默认 CDN URL - e2e case 改回使用默认 CDN
WalkthroughAdds mesh collider support across engine layers: new MeshColliderShape and PhysXMeshColliderShape, PhysX cooking integration and flags, runtime guards for DynamicCollider/kinematic usage, E2E and unit tests, and related interface and export updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant MeshShape as MeshColliderShape
participant Core as Engine Core / Native API
participant PhysX as PhysXPhysics
participant PxShape as PhysXMeshColliderShape
participant Native as Native PhysX
Client->>MeshShape: construct(isConvex)
Client->>MeshShape: setMesh / setMeshData(vertices, indices)
MeshShape->>MeshShape: _extractMeshData()
MeshShape->>Core: request native mesh shape
Core->>PhysX: createMeshColliderShape(...)
PhysX->>PhysX: ensure cooking resources (_pxCooking)
PhysX->>PxShape: instantiate PhysXMeshColliderShape(...)
PxShape->>Native: createTriMesh() / createConvexMesh()
Native-->>PxShape: PxMesh / PxGeometry
PxShape-->>PhysX: created shape
PhysX-->>Core: native shape reference
Core-->>MeshShape: native shape bound
Client->>MeshShape: setDoubleSided / setTightBounds
MeshShape->>PxShape: update geometry flags
PxShape->>Native: update geometry / upload buffers
Client->>MeshShape: destroy()
PxShape->>Native: release native resources
sequenceDiagram
participant Client as Client Code
participant DC as DynamicCollider
participant Shape as MeshColliderShape
Client->>DC: set isKinematic = false
DC->>DC: inspect attached shapes
alt non-convex triangle present
DC->>Client: log error, reject change
else
DC->>DC: apply change, _syncNative()
end
Client->>DC: addShape(meshShape)
alt triangle mesh + non-kinematic
DC->>Client: log error, skip add
else
DC->>Shape: add to native collider
end
Client->>DC: move(position)
alt isKinematic == false
DC->>Client: log warning, no-op
else
DC->>Core: apply native move
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
🤖 Augment PR SummarySummary: This PR introduces PhysX mesh collider support (triangle mesh + convex mesh) and adds E2E/test coverage for the new capability. Changes:
Technical Notes: Triangle meshes are intended for 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
|
||
| protected _addNativeShape(shape: ColliderShape): void { | ||
| shape._collider = this; | ||
| shape._nativeShape.setWorldScale(this.entity.transform.lossyWorldScale); |
There was a problem hiding this comment.
shape._nativeShape is assumed to be non-null here, but MeshColliderShape sets _nativeShape lazily (null until mesh data is provided), so adding such a shape too early would throw at runtime.
🤖 Was this useful? React with 👍 or 👎
| ); | ||
| } else { | ||
| // Create new shape | ||
| this._nativeShape = Engine._nativePhysics.createMeshColliderShape( |
There was a problem hiding this comment.
When the native shape is created here, cached properties set before creation (e.g. doubleSided/tightBounds, and potentially other _nativeShape-backed fields) aren’t applied unless they’re set again after creation, which can lead to surprising defaults.
🤖 Was this useful? React with 👍 or 👎
| * Destroy PhysXPhysics. | ||
| */ | ||
| destroy(): void { | ||
| this._pxCooking.release(); |
There was a problem hiding this comment.
destroy() unconditionally calls this._pxCooking.release() / this._pxCookingParams.delete(), which will throw if destroy() is called before initialization completes or if initialization fails partway.
🤖 Was this useful? React with 👍 or 👎
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2880 +/- ##
===========================================
+ Coverage 78.96% 82.72% +3.75%
===========================================
Files 857 794 -63
Lines 93517 89853 -3664
Branches 9378 9402 +24
===========================================
+ Hits 73846 74331 +485
+ Misses 19522 15438 -4084
+ Partials 149 84 -65
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.gitignore:
- Around line 50-51: The .gitignore contains unrelated patterns ".serena" and
"docs/plans"; either remove them or clarify intent: if ".serena" is a local dev
artifact, remove it from the commit and add a short comment in the PR or keep it
but move to a separate commit labeling it as local; if "docs/plans" is not
present or planned, remove that entry or add the directory in a separate PR; if
you intend these to match only at the repo root, prefix them with "/" (i.e.,
"/.serena" and "/docs/plans") so they don't match subdirectories—update the file
accordingly and add a brief PR description explaining the change.
In `@packages/core/src/physics/shape/MeshColliderShape.ts`:
- Around line 242-261: The static analyzer flagged formatting in the
MeshColliderShape update/create block; reformat the if/else block around
this._nativeShape so it matches project style (consistent spacing, indentation
and comment placement) — specifically tidy the cast and method call
(<IMeshColliderShape>this._nativeShape).setMeshData(...), align the create call
to Engine._nativePhysics.createMeshColliderShape(...) and ensure
Engine._physicalObjectsMap[this._id] = this; follows the same indentation and
trailing comma conventions; you can fix by applying the repository formatter or
manually adjusting spacing around parentheses/comments in the MeshColliderShape
update/create block.
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts`:
- Around line 86-92: setWorldScale currently calls _updateMeshScale
unconditionally which can run before mesh data exists and cause errors; guard
the call by checking that the mesh is created (e.g., that _pxMesh is not
null/undefined or that _createMeshAndShape has already run) and only call
_updateMeshScale when the mesh exists. Locate the override setWorldScale(scale:
Vector3) and add a null/undefined check for this._pxMesh (or a boolean flag set
when _createMeshAndShape completes) before invoking this._updateMeshScale to
avoid creating geometry with a null mesh.
- Around line 194-245: The mesh creation can return null and cause leaked
allocations; update _createMeshGeometry (and similarly _createMeshAndShape) to
ensure verticesPtr and indicesPtr are always freed if mesh creation fails —
either wrap the allocation/creation block in try/finally that calls
physX._free(verticesPtr) and physX._free(indicesPtr) (when set), or immediately
check the result of cooking.createConvexMesh / cooking.createTriMesh (and
this._pxMesh) and free allocated buffers before throwing an Error like "Failed
to create mesh"; ensure you reference and free the same verticesPtr and
indicesPtr variables and only call physX.createConvexMeshGeometry /
physX.createTriMeshGeometry after verifying this._pxMesh is non-null.
🧹 Nitpick comments (8)
tests/src/core/physics/PhysXDowngrade.test.ts (1)
4-33: Consider adding cleanup inafterAllto release PhysX resources.The test initializes PhysXPhysics but doesn't call
physics.destroy()after tests complete. While this may not cause issues in isolated test runs, it's good practice to clean up resources.Suggested cleanup
describe("PhysX Downgrade Mode", () => { let physics: PhysXPhysics; beforeAll(async () => { physics = new PhysXPhysics(PhysXRuntimeMode.JavaScript); await physics.initialize(); }, 30000); + afterAll(() => { + physics?.destroy(); + }); + it("should load PhysX in JavaScript mode", () => {e2e/case/physx-mesh-collider-data.ts (1)
26-65: Consider extracting shared mesh generation logic to reduce duplication.The terrain mesh generation (vertices, indices, normals) is duplicated between
createTerrainMesh(lines 26-65) for visuals andcreateWavyTerrain(lines 75-94) for physics. This could lead to subtle inconsistencies if one is modified without the other.♻️ Suggested refactor
Extract shared vertex/index generation into a helper:
function generateTerrainData(gridSize: number, scale: number) { const vertices: number[] = []; const indices: number[] = []; for (let z = 0; z <= gridSize; z++) { for (let x = 0; x <= gridSize; x++) { const px = (x - gridSize / 2) * scale; const pz = (z - gridSize / 2) * scale; const py = Math.sin(x * 0.5) * Math.cos(z * 0.5) * 1.5; vertices.push(px, py, pz); } } for (let z = 0; z < gridSize; z++) { for (let x = 0; x < gridSize; x++) { const i = z * (gridSize + 1) + x; indices.push(i, i + gridSize + 1, i + 1); indices.push(i + 1, i + gridSize + 1, i + gridSize + 2); } } return { vertices, indices }; }Also applies to: 68-115
tests/src/core/physics/MeshColliderShape.test.ts (2)
30-38: Consider adding engine cleanup in afterAll.The engine is created in
beforeAllbut never destroyed. While this may not cause immediate issues in test environments, it's good practice to clean up resources.♻️ Suggested addition
afterAll(() => { engine.destroy(); });
40-48: Prototype mutation for spies could affect test isolation.Mutating
CollisionScript.prototypemethods inbeforeEachworks but could leak between test files if the test runner doesn't properly isolate modules. Consider usingvi.spyOnon specific instances instead.e2e/case/physx-mesh-collider.ts (2)
169-181: Fallback bounds handling is defensive but could mask issues.The fallback to hardcoded defaults when bounds are invalid silently handles missing mesh data. Consider logging a warning when falling back so issues are easier to diagnose.
♻️ Suggested improvement
// Use defaults if bounds are invalid (no mesh data found) const validBounds = isFinite(potWidth) && isFinite(potHeight) && potWidth > 0; + if (!validBounds) { + console.warn("Pot bounds invalid, using defaults. Check if mesh data is accessible."); + } const actualPotWidth = validBounds ? potWidth : 0.24;
172-174: Consider removing debug console.log statements for E2E tests.The debug log statements at lines 172-174 and 224 are useful during development but may add noise to E2E test output. Consider removing or guarding them behind a debug flag.
Also applies to: 224-224
packages/core/src/physics/shape/MeshColliderShape.ts (2)
79-86: Consider accessing parent class properties through protected accessors.Using
(this as any)._contactOffsetand similar patterns to access parent class internals is fragile. Consider exposing protected accessors in the baseColliderShapeclass or using a different approach.Also applies to: 91-104, 109-119
166-173: Consider supporting BufferMesh in addition to ModelMesh.The
setMeshmethod only supportsModelMesh, logging a warning for other mesh types. IfBufferMeshsupport is planned, consider documenting this as a future enhancement or throwing a more descriptive error.
| .serena | ||
| docs/plans |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the .gitignore file around lines 50-51 to verify the changes
cat -n .gitignore | sed -n '40,60p'Repository: galacean/engine
Length of output: 310
🏁 Script executed:
# Search for any references to .serena and docs/plans in the repository
rg -i "serena" --type-list | head -20
rg -i "docs/plans"Repository: galacean/engine
Length of output: 448
🏁 Script executed:
# Check if there are any directories or files matching these patterns
fd -H "\.serena"
fd -H "docs/plans"Repository: galacean/engine
Length of output: 41
🏁 Script executed:
# Look for any comments or context in .gitignore about these entries
cat -n .gitignore | grep -A 5 -B 5 "serena\|docs/plans"Repository: galacean/engine
Length of output: 208
🏁 Script executed:
# Search for any references to "serena" in the codebase
rg "serena" -iRepository: galacean/engine
Length of output: 41
🏁 Script executed:
# Check if docs/plans directory exists or is referenced
find . -name "plans" -type d 2>/dev/null
ls -la docs/ 2>/dev/null | head -20Repository: galacean/engine
Length of output: 273
🏁 Script executed:
# Look for any documentation or comments about .serena
rg "\.serena" -iRepository: galacean/engine
Length of output: 41
🏁 Script executed:
# Check the git history of .gitignore to understand when these were added
git log --oneline .gitignore | head -10
git diff HEAD~1 .gitignore 2>/dev/null || git show HEAD:.gitignoreRepository: galacean/engine
Length of output: 831
🏁 Script executed:
# Double-check if .serena or docs/plans were recently created/changed
git status --short 2>/dev/null | grep -E "serena|docs/plans" || echo "No matching files in git status"Repository: galacean/engine
Length of output: 90
🏁 Script executed:
# Check if there's any PR metadata or documentation about why these were added
cat README.md 2>/dev/null | grep -i "serena\|docs/plans" || echo "Not found in README"Repository: galacean/engine
Length of output: 78
Clarify or remove unrelated .gitignore entries.
These patterns appear unrelated to the MeshCollider feature in this PR. Additionally, .serena has no references in the codebase and docs/plans directory does not exist in the repository.
Please clarify:
- What is
.serena? Is this a local development artifact that should not be committed? - Is
docs/plansintended for future use, or should this be removed or added in a separate commit?
If these patterns should only match at the repository root, prefix them with /:
Suggested fix
-.serena
-docs/plans
+/.serena
+/docs/plansWithout the leading /, these patterns match anywhere in the directory tree (e.g., packages/foo/docs/plans).
📝 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.
| .serena | |
| docs/plans | |
| /.serena | |
| /docs/plans |
🤖 Prompt for AI Agents
In @.gitignore around lines 50 - 51, The .gitignore contains unrelated patterns
".serena" and "docs/plans"; either remove them or clarify intent: if ".serena"
is a local dev artifact, remove it from the commit and add a short comment in
the PR or keep it but move to a separate commit labeling it as local; if
"docs/plans" is not present or planned, remove that entry or add the directory
in a separate PR; if you intend these to match only at the repo root, prefix
them with "/" (i.e., "/.serena" and "/docs/plans") so they don't match
subdirectories—update the file accordingly and add a brief PR description
explaining the change.
| /** | ||
| * {@inheritDoc IColliderShape.setWorldScale } | ||
| */ | ||
| override setWorldScale(scale: Vector3): void { | ||
| super.setWorldScale(scale); | ||
| this._updateMeshScale(); | ||
| } |
There was a problem hiding this comment.
Add null check before _updateMeshScale in setWorldScale.
If setWorldScale is called before mesh data is set (before _createMeshAndShape), _updateMeshScale will try to create geometry with a null _pxMesh, potentially causing errors.
🐛 Proposed fix
override setWorldScale(scale: Vector3): void {
super.setWorldScale(scale);
- this._updateMeshScale();
+ if (this._pxMesh) {
+ this._updateMeshScale();
+ }
}📝 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.
| /** | |
| * {@inheritDoc IColliderShape.setWorldScale } | |
| */ | |
| override setWorldScale(scale: Vector3): void { | |
| super.setWorldScale(scale); | |
| this._updateMeshScale(); | |
| } | |
| /** | |
| * {`@inheritDoc` IColliderShape.setWorldScale } | |
| */ | |
| override setWorldScale(scale: Vector3): void { | |
| super.setWorldScale(scale); | |
| if (this._pxMesh) { | |
| this._updateMeshScale(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts` around lines 86 -
92, setWorldScale currently calls _updateMeshScale unconditionally which can run
before mesh data exists and cause errors; guard the call by checking that the
mesh is created (e.g., that _pxMesh is not null/undefined or that
_createMeshAndShape has already run) and only call _updateMeshScale when the
mesh exists. Locate the override setWorldScale(scale: Vector3) and add a
null/undefined check for this._pxMesh (or a boolean flag set when
_createMeshAndShape completes) before invoking this._updateMeshScale to avoid
creating geometry with a null mesh.
- ColliderShape: setter 使用可选链处理 null - MeshColliderShape: 删除冗余 override 和回调重绑定 - PhysXMeshColliderShape: 提取内存分配公共方法 减少 122 行重复代码
- Collider._addNativeShape 中 setWorldScale 添加可选链保护 - 补充运行时动态添加 shape 缩放的单元测试
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/src/core/physics/MeshColliderShape.test.ts`:
- Around line 25-48: Add an afterAll teardown that stops and disposes the engine
started in beforeAll to avoid leaking GL/PhysX resources: afterAll should
reference the same engine variable and call the appropriate shutdown/dispose API
(e.g., await engine.destroy() or engine.stop()/engine.destroy() if destroy is
async) after the tests complete, and null out engine/root/physicsScene as needed
to release references; place this afterAll alongside the existing beforeAll and
beforeEach blocks so engine.run() is properly shut down.
| describe("MeshColliderShape PhysX", () => { | ||
| let engine: WebGLEngine; | ||
| let root: Entity; | ||
| let physicsScene: any; | ||
|
|
||
| beforeAll(async () => { | ||
| engine = await WebGLEngine.create({ canvas: document.createElement("canvas"), physics: new PhysXPhysics() }); | ||
| engine.run(); | ||
|
|
||
| const scene = engine.sceneManager.activeScene; | ||
| physicsScene = scene.physics; | ||
| physicsScene.gravity = new Vector3(0, -9.81, 0); | ||
| root = scene.createRootEntity("root"); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| // Reset collision script spies | ||
| CollisionScript.prototype.onCollisionEnter = vi.fn(); | ||
| CollisionScript.prototype.onCollisionStay = vi.fn(); | ||
| CollisionScript.prototype.onCollisionExit = vi.fn(); | ||
| CollisionScript.prototype.onTriggerEnter = vi.fn(); | ||
| CollisionScript.prototype.onTriggerStay = vi.fn(); | ||
| CollisionScript.prototype.onTriggerExit = vi.fn(); | ||
| }); |
There was a problem hiding this comment.
Add teardown for engine lifecycle.
engine.run() starts a render/physics loop and allocates WebGL/PhysX resources. Without an afterAll cleanup, this can leak handles across the test suite.
🧹 Proposed teardown addition
-import { describe, beforeAll, beforeEach, expect, it, vi } from "vitest";
+import { describe, beforeAll, beforeEach, afterAll, expect, it, vi } from "vitest";
@@
beforeAll(async () => {
engine = await WebGLEngine.create({ canvas: document.createElement("canvas"), physics: new PhysXPhysics() });
engine.run();
@@
root = scene.createRootEntity("root");
});
+
+ afterAll(() => {
+ engine?.destroy?.();
+ });🤖 Prompt for AI Agents
In `@tests/src/core/physics/MeshColliderShape.test.ts` around lines 25 - 48, Add
an afterAll teardown that stops and disposes the engine started in beforeAll to
avoid leaking GL/PhysX resources: afterAll should reference the same engine
variable and call the appropriate shutdown/dispose API (e.g., await
engine.destroy() or engine.stop()/engine.destroy() if destroy is async) after
the tests complete, and null out engine/root/physicsScene as needed to release
references; place this afterAll alongside the existing beforeAll and beforeEach
blocks so engine.run() is properly shut down.
- move() 内部调用 PhysX 的 setKinematicTarget()，专为 kinematic 刚体设计 - 在非 kinematic 刚体上调用会导致 PhysX 隐式转换为 kinematic 模式 - 添加 isKinematic 检查，非 kinematic 时打印警告并返回 Closes galacean#2879
luzhuang
changed the title
- 修复 MeshColliderShape.ts prettier 格式问题 - 调大 MeshCollider e2e 测试的 diffPercentage 阈值 (0 -> 0.02)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts`:
- Around line 46-61: setMeshData currently releases the existing mesh via
_releaseMesh() before calling _createMesh(), which can leave _pxShape
referencing a null _pxGeometry if _createMesh() throws; change the flow so you
validate inputs or build the new geometry before dropping the old one: either
(A) perform pre-checks that _createMesh() would require (e.g., if !isConvex
ensure indices is non-null and index type valid) and only call _releaseMesh()
after validation passes, or (B) allocate/create the new mesh/geometry in a
temporary variable (invoke the logic inside _createMesh() but return the new
geometry or wrap it) and only call _releaseMesh() and swap in the new geometry
and call _pxShape.setGeometry(_pxGeometry) after creation succeeds; reference
setMeshData, _releaseMesh, _createMesh, _pxGeometry and _pxShape when making the
change.
♻️ Duplicate comments (3)
packages/physics-physx/src/shape/PhysXMeshColliderShape.ts (3)
86-89: Add null check before_updateGeometryinsetWorldScale.Unlike
setDoubleSidedandsetTightBounds, this method calls_updateGeometry()unconditionally. IfsetWorldScaleis called before mesh data is set or afterdestroy(),_updateGeometrywill attempt to create geometry with a null_pxMesh.🐛 Proposed fix
override setWorldScale(scale: Vector3): void { super.setWorldScale(scale); - this._updateGeometry(); + if (this._pxMesh) { + this._updateGeometry(); + } }
136-171: Memory leak risk if mesh creation fails.If
cooking.createConvexMesh()orcooking.createTriMesh()fails and returns null,_pxMeshis null. The subsequentphysX.createConvexMeshGeometry()/physX.createTriMeshGeometry()may throw or behave unexpectedly, and if an exception occurs beforephysX._free(verticesPtr)is reached, memory will leak.Use try-finally or check for null immediately after mesh creation.
🐛 Proposed fix with try-finally
private _createMesh(): void { const physX = this._physXPhysics._physX; const physics = this._physXPhysics._pxPhysics; const cooking = this._physXPhysics._pxCooking; const verticesPtr = this._allocateVertices(); + let indicesPtr: number | null = null; + try { if (this._isConvex) { this._pxMesh = cooking.createConvexMesh(verticesPtr, this._vertexCount, physics); + if (!this._pxMesh) { + throw new Error("Failed to cook convex mesh"); + } this._pxGeometry = physX.createConvexMeshGeometry( this._pxMesh, this._worldScale.x, this._worldScale.y, this._worldScale.z, this._tightBounds ? TIGHT_BOUNDS_FLAG : 0 ); } else { if (!this._indices) { - physX._free(verticesPtr); throw new Error("Triangle mesh requires indices"); } - const { ptr: indicesPtr, isU16, triangleCount } = this._allocateIndices(); + const { ptr, isU16, triangleCount } = this._allocateIndices(); + indicesPtr = ptr; this._pxMesh = cooking.createTriMesh(verticesPtr, this._vertexCount, indicesPtr, triangleCount, isU16, physics); + if (!this._pxMesh) { + throw new Error("Failed to cook triangle mesh"); + } this._pxGeometry = physX.createTriMeshGeometry( this._pxMesh, this._worldScale.x, this._worldScale.y, this._worldScale.z, this._doubleSided ? DOUBLE_SIDED_FLAG : 0 ); - physX._free(indicesPtr); } - - physX._free(verticesPtr); + } finally { + physX._free(verticesPtr); + if (indicesPtr !== null) { + physX._free(indicesPtr); + } + } }
99-134: Missing null check after mesh creation.After
_createMesh(),_pxMeshcould be null ifcooking.createConvexMesh()orcooking.createTriMesh()fails. The subsequentphysX.createConvexMeshShape()orphysX.createTriMeshShape()would receive a null mesh, causing undefined behavior.🐛 Proposed fix
private _createMeshAndShape(material: PhysXPhysicsMaterial, uniqueID: number): void { const physX = this._physXPhysics._physX; const physics = this._physXPhysics._pxPhysics; const shapeFlags = ShapeFlag.SCENE_QUERY_SHAPE | ShapeFlag.SIMULATION_SHAPE; this._createMesh(); + if (!this._pxMesh) { + throw new Error("Failed to create mesh collider: mesh cooking failed"); + } // Create shape with material if (this._isConvex) {
🧹 Nitpick comments (1)
packages/physics-physx/src/shape/PhysXMeshColliderShape.ts (1)
24-41: Consider validating vertex data consistency.The constructor accepts
verticesandvertexCountseparately without validating thatvertices.length >= vertexCount * 3. If mismatched,_allocateVerticeswill copy incorrect data (or potentially read out of bounds when setting the view).💡 Suggested validation
constructor( physXPhysics: PhysXPhysics, uniqueID: number, vertices: Float32Array, vertexCount: number, indices: Uint16Array | Uint32Array | null, isConvex: boolean, material: PhysXPhysicsMaterial ) { super(physXPhysics); + if (vertices.length < vertexCount * 3) { + throw new Error("Vertices array length must be at least vertexCount * 3"); + } this._isConvex = isConvex; this._vertices = vertices;
luzhuang
changed the title
- 直接读取 vertex buffer 数据，跳过 getPositions() 的 Vector3[] 中间层 - 紧凑布局 (stride=12, offset=0) 使用 Float32Array.set() 直接复制 - 交错布局使用单次遍历逐顶点复制 - 复用 _vertices 数组减少内存分配
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/physics/shape/MeshColliderShape.ts`:
- Around line 159-177: Add a defensive alignment check before creating the
Float32Array view in MeshColliderShape: validate that bufferData.byteOffset % 4
=== 0 and byteStride % 4 === 0 (and byteOffset % 4 if separate) to avoid
RangeError when constructing sourceData; if the check fails, emit a warning
(e.g., via console.warn or processLogger.warn) and bail out or return early from
the method to avoid further unsafe divisions, and only then create sourceData
and compute floatStride/floatOffset = byteStride/4 and byteOffset/4 safely;
reference the sourceData variable and the byteStride/byteOffset calculations
around the current Float32Array construction and the loop that computes
floatStride/floatOffset.
♻️ Duplicate comments (1)
packages/core/src/physics/shape/MeshColliderShape.ts (1)
49-68: CacheddoubleSided/tightBoundsnot applied when native shape is created.If these properties are set before mesh data is provided, the native shape is created with defaults rather than the cached values. The
_updateNativeMeshmethod should apply these after creating the native shape.🔧 Proposed fix
In
_updateNativeMesh(), after creating the native shape, apply the cached properties:this._nativeShape = Engine._nativePhysics.createMeshColliderShape( this._id, this._vertices, vertexCount, this._indices, this._isConvex, this._material._nativeMaterial ); Engine._physicalObjectsMap[this._id] = this; + // Apply cached properties + (<IMeshColliderShape>this._nativeShape).setDoubleSided(this._doubleSided); + (<IMeshColliderShape>this._nativeShape).setTightBounds(this._tightBounds); }
| // 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 | ||
| const floatStride = byteStride / 4; | ||
| const floatOffset = byteOffset / 4; | ||
| for (let i = 0; i < vertexCount; i++) { | ||
| const srcIdx = i * floatStride + floatOffset; | ||
| const dstIdx = i * 3; | ||
| this._vertices[dstIdx] = sourceData[srcIdx]; | ||
| this._vertices[dstIdx + 1] = sourceData[srcIdx + 1]; | ||
| this._vertices[dstIdx + 2] = sourceData[srcIdx + 2]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Float32Array TypedArray byteOffset alignment requirement JavaScript RangeError
💡 Result:
Short answer: When you create a typed-array view on an ArrayBuffer, the byteOffset must be a multiple of the view's BYTES_PER_ELEMENT. For Float32Array BYTES_PER_ELEMENT === 4, so byteOffset must be divisible by 4 — otherwise a RangeError is thrown. Also the viewed range must fit: byteOffset + length * BYTES_PER_ELEMENT <= buffer.byteLength. [1][2]
Example:
const buf = new ArrayBuffer(8);
new Float32Array(buf, 1); // RangeError (offset not a multiple of 4)
new Float32Array(buf, 4); // OK (offset is multiple of 4)
Sources: MDN TypedArray (alignment and RangeError) and MDN Float32Array (BYTES_PER_ELEMENT = 4). [1] [2]
Sources
[1] MDN: TypedArray — byteOffset/byteLength alignment and RangeError.
[2] MDN: Float32Array — BYTES_PER_ELEMENT = 4.
🏁 Script executed:
cd packages/core/src/physics/shape && cat -n MeshColliderShape.ts | sed -n '126,192p'Repository: galacean/engine
Length of output: 2835
🏁 Script executed:
rg "byteOffset.*align|align.*byteOffset|BYTES_PER_ELEMENT|RangeError.*TypedArray" packages/core/src/physics/ --type ts -A 3 -B 3Repository: galacean/engine
Length of output: 41
🏁 Script executed:
fd -t f "buffer|Buffer" packages/core/src/graphic* --type f -name "*.ts" | head -5 | xargs rg "stride|offset" -lRepository: galacean/engine
Length of output: 6131
🏁 Script executed:
cat -n packages/core/src/graphic/Buffer.ts | head -100Repository: galacean/engine
Length of output: 3258
🏁 Script executed:
cat -n packages/core/src/graphic/Buffer.ts | sed -n '100,200p'Repository: galacean/engine
Length of output: 4096
Add defensive alignment checks for Float32Array view creation.
Line 160 creates a Float32Array view from bufferData.buffer using bufferData.byteOffset. The Float32Array constructor requires the byte offset to be aligned to 4-byte boundaries (BYTES_PER_ELEMENT for Float32Array); otherwise it throws a RangeError. Additionally, lines 168-169 assume byteStride and byteOffset are multiples of 4 when dividing by 4.
While vertex buffers are typically 4-byte aligned in practice, add explicit validation or document this requirement to prevent edge cases:
if (bufferData.byteOffset % 4 !== 0 || byteStride % 4 !== 0) {
console.warn("MeshColliderShape: Buffer offsets must be 4-byte aligned");
return;
}
🤖 Prompt for AI Agents
In `@packages/core/src/physics/shape/MeshColliderShape.ts` around lines 159 - 177,
Add a defensive alignment check before creating the Float32Array view in
MeshColliderShape: validate that bufferData.byteOffset % 4 === 0 and byteStride
% 4 === 0 (and byteOffset % 4 if separate) to avoid RangeError when constructing
sourceData; if the check fails, emit a warning (e.g., via console.warn or
processLogger.warn) and bail out or return early from the method to avoid
further unsafe divisions, and only then create sourceData and compute
floatStride/floatOffset = byteStride/4 and byteOffset/4 safely; reference the
sourceData variable and the byteStride/byteOffset calculations around the
current Float32Array construction and the loop that computes
floatStride/floatOffset.
1. MeshColliderShape: 创建 native shape 后同步 doubleSided 和 tightBounds 属性 2. PhysXDynamicCollider: 修复 ContinuousDynamic 模式下 eENABLE_CCD 应为 true 3. PhysXDynamicCollider: 各碰撞检测模式切换时正确清除其他模式的标志位 4. 添加对应的单元测试验证修复
MeshColliderShape: - Fix: indices missing didn't return false - Fix: setMesh after addShape didn't add native shape to collider - Add super._syncNative() to sync base class properties - Extract _extractIndices method for code reuse - Add comment for PhysX Uint16/Uint32 indices - Add isConvex doc noting setMesh must be called after changing PhysXPhysics: - Remove unused buildTriangleAdjacencies setting
- Add try/finally to ensure WASM memory (_malloc) is freed on failure - Add null check for _pxMesh after cooking (returns null on failure) - Make setMeshData atomic: create new mesh before releasing old one - Release JS memory (_vertices/_indices) after copying to WASM
When switching from kinematic to dynamic, resync the collision detection mode to ensure CCD flags are properly restored to the user's setting.
- Block adding triangle mesh to non-kinematic DynamicCollider - Block switching isKinematic to false when triangle mesh is attached - Skip mass/inertia calculation for kinematic bodies (not used per PhysX doc) - Remove unused buildTriangleAdjacencies from setCookingParams
…to dynamic - Add mass/inertia recalculation when isKinematic changes from true to false - Improve _pxCookingParams documentation
| } | ||
| } | ||
|
|
||
| @ignoreClone |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/src/core/physics/MeshColliderShape.test.ts`:
- Around line 459-467: The vertices Float32Array in MeshColliderShape.test.ts
violates the formatter because coordinates must be split per vertex; update the
vertices declaration used with meshShape.setMeshData so each vertex's three
components are on their own line (e.g., one line per v0/v1/v2/v3) and keep the
inline comments (// v0, etc.) as needed; ensure the array remains a Float32Array
and that indices and meshShape.setMeshData calls are unchanged.
♻️ Duplicate comments (4)
packages/physics-physx/src/PhysXPhysics.ts (1)
147-156: Add null guards indestroy()for cooking resources.The
destroy()method unconditionally callsrelease()anddelete()on cooking resources, which will throw ifdestroy()is called before initialization completes or if initialization fails partway.🐛 Proposed fix
destroy(): void { - this._pxCooking.release(); - this._pxCookingParams.delete(); + this._pxCooking?.release(); + this._pxCookingParams?.delete(); this._physX.PxCloseExtensions(); this._pxPhysics.release(); this._pxFoundation.release();packages/core/src/physics/shape/MeshColliderShape.ts (1)
164-181: Consider adding alignment validation for buffer offsets.The
Float32Arrayconstructor requiresbyteOffsetto be 4-byte aligned. While vertex buffers are typically aligned in practice, adding defensive validation would prevent crypticRangeErrorexceptions in edge cases.♻️ Suggested validation
// Create Float32Array view to read source data + if (bufferData.byteOffset % 4 !== 0) { + console.warn("MeshColliderShape: Buffer byteOffset is not 4-byte aligned"); + return false; + } 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 { + if (byteStride % 4 !== 0 || byteOffset % 4 !== 0) { + console.warn("MeshColliderShape: Stride/offset must be 4-byte aligned for interleaved data"); + return false; + } // Interleaved: copy per vertex with optimized indexingpackages/physics-physx/src/shape/PhysXMeshColliderShape.ts (1)
96-102: Add null check before_updateGeometryinsetWorldScale.If
setWorldScaleis called before mesh data is set (which can happen duringCollider._addNativeShape),_updateGeometrywill attempt to create geometry with a null_pxMesh, causing errors.🐛 Proposed fix
override setWorldScale(scale: Vector3): void { super.setWorldScale(scale); - this._updateGeometry(); + if (this._pxMesh) { + this._updateGeometry(); + } }tests/src/core/physics/MeshColliderShape.test.ts (1)
15-39: Add engine teardown to avoid leaking WebGL/PhysX across tests.
engine.run()starts a render/physics loop; without anafterAllcleanup, resources can persist across the suite.🧹 Proposed teardown addition
-import { describe, beforeAll, beforeEach, expect, it, vi } from "vitest"; +import { describe, beforeAll, beforeEach, afterAll, expect, it, vi } from "vitest"; @@ beforeAll(async () => { engine = await WebGLEngine.create({ canvas: document.createElement("canvas"), physics: new PhysXPhysics() }); engine.run(); @@ root = scene.createRootEntity("root"); }); + + afterAll(() => { + engine?.destroy?.(); + });#!/bin/bash # Locate WebGLEngine and its shutdown API to confirm the right method name. files=$(rg -l "class WebGLEngine" -g '*.ts') for f in $files; do echo "== $f ==" rg -n "destroy\\(|stop\\(" "$f" done
🧹 Nitpick comments (3)
e2e/case/physx-mesh-collider-sphere-container.ts (1)
85-88: Guardmove()to kinematic colliders.Prevents warning spam/no-ops if any non‑kinematic colliders are included.
♻️ Proposed change
for (const collider of this._colliders) { - collider.move(this._tempPosition, this._tempQuaternion); + if (!collider.isKinematic) continue; + collider.move(this._tempPosition, this._tempQuaternion); }packages/core/src/physics/DynamicCollider.ts (1)
434-462: LGTM - Ensures kinematic actors participate in collision detection.The implementation correctly addresses the PhysX requirement that kinematic actors must have
setKinematicTargetcalled every frame to participate in collision detection. Without the auto-move in_onUpdate, stationary kinematic bodies would stop detecting collisions.Consider translating the Chinese comments to English for consistency with the rest of the codebase:
- Line 436-437: "Kinematic actor needs setKinematicTarget called every frame to participate in collision detection. If user didn't call move(), auto-call to maintain current position."
- Line 448: "Reset flag"
tests/src/core/physics/DynamicCollider.test.ts (1)
428-435: LGTM - Comprehensive CCD flag assertions.The test now correctly verifies that
ContinuousDynamicmode enables botheENABLE_CCDandeENABLE_CCD_FRICTIONflags, matching the implementation changes inPhysXDynamicCollider.setCollisionDetectionMode.Consider translating the Chinese comment (line 429) to English for consistency: "ContinuousDynamic mode requires both CCD and CCD_FRICTION enabled"
- Change error message from "Triangle mesh" to "triangle mesh" for consistency - Update test to match the error message
- Remove _kinematicTargetSet flag - Remove auto move() call in _onUpdate - Fix PhysicsScene collision tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/physics/DynamicCollider.ts`:
- Around line 271-295: When changing a MeshColliderShape from convex to
non-convex (in the isConvex setter) or when switching mesh via
setMesh/setMeshData, validate the parent collider: if this._collider is non-null
and is a DynamicCollider and that DynamicCollider is non-kinematic (isKinematic
=== false), disallow the transition (log/throw and return) because triangle
meshes are not supported on non-kinematic DynamicCollider; update the
MeshColliderShape.isConvex setter and both setMesh/setMeshData to perform this
check before mutating isConvex/mesh so you cannot toggle to a triangle-mesh
while attached to a dynamic (non-kinematic) collider — mirror the same
restriction enforced by DynamicCollider.addShape and the isKinematic setter.
♻️ Duplicate comments (2)
tests/src/core/physics/MeshColliderShape.test.ts (2)
31-39: Add teardown for engine lifecycle to prevent resource leaks.
engine.run()is started but never stopped/destroyed after the suite finishes. This can leak WebGL/PhysX resources across tests.🧹 Proposed teardown
-import { describe, beforeAll, beforeEach, expect, it, vi } from "vitest"; +import { describe, beforeAll, beforeEach, afterAll, expect, it, vi } from "vitest"; @@ beforeAll(async () => { engine = await WebGLEngine.create({ canvas: document.createElement("canvas"), physics: new PhysXPhysics() }); engine.run(); @@ root = scene.createRootEntity("root"); }); + + afterAll(() => { + engine?.destroy?.(); + });
458-464: Fix formatter violation in the vertices array.Prettier expects vertex components to be split across lines here.
🎨 Formatting fix
- const vertices = new Float32Array([ - -5, 0, -5, // v0 - 5, 0, -5, // v1 - -5, 0, 5, // v2 - 5, 0, 5 // v3 - ]); + const vertices = new Float32Array([ + -5, + 0, + -5, // v0 + 5, + 0, + -5, // v1 + -5, + 0, + 5, // v2 + 5, + 0, + 5 // v3 + ]);
| set isKinematic(value: boolean) { | ||
| if (this._isKinematic !== value) { | ||
| // Block switching to non-kinematic if triangle mesh is attached | ||
| if (!value) { | ||
| 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."); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this._isKinematic = value; | ||
| (<IDynamicCollider>this._nativeCollider).setIsKinematic(value); | ||
|
|
||
| // Resync when switching back to dynamic | ||
| if (!value) { | ||
| (<IDynamicCollider>this._nativeCollider).setCollisionDetectionMode(this._collisionDetectionMode); | ||
| // Recalculate mass/inertia that was skipped in kinematic mode | ||
| if (this._automaticCenterOfMass || this._automaticInertiaTensor) { | ||
| this._setMassAndUpdateInertia(); | ||
| } | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
# Search for MeshColliderShape.isConvex setter and related validation
rg -n -C3 "set isConvex|isConvex" packages/core/src/physics/shape/MeshColliderShape.tsRepository: galacean/engine
Repository: galacean/engine
Exit code: 0
stdout:
9-/**
10- * Physical collider shape for mesh.
11- * `@remarks`
12: * - Triangle mesh (isConvex=false) works with StaticCollider or kinematic DynamicCollider
13: * - Convex mesh (isConvex=true) works with both StaticCollider and DynamicCollider
14- * `@see` https://nvidia-omniverse.github.io/PhysX/physx/5.4.1/docs/Geometry.html#triangle-meshes
15- */
16-export class MeshColliderShape extends ColliderShape {
17: private _isConvex: boolean = false;
18- private _vertices: Float32Array = null;
19- private _indices: Uint16Array | Uint32Array | null = null;
20- private _doubleSided: boolean = false;
--
28- * - Triangle mesh: Works with StaticCollider or kinematic DynamicCollider, requires indices
29- * - After changing this property, you must call {`@link` setMesh} or {`@link` setMeshData} again to apply the change
30- */
31: get isConvex(): boolean {
32: return this._isConvex;
33- }
34-
35: set isConvex(value: boolean) {
36: if (this._isConvex !== value) {
37: this._isConvex = value;
38- if (this._vertices && this._nativeShape) {
39- this._updateNativeMesh();
40- }
--
73-
74- /**
75- * Create a MeshColliderShape.
76: * `@param` isConvex - Whether to use convex mesh mode (default: false)
77- */
78: constructor(isConvex: boolean = false) {
79- super();
80: this._isConvex = isConvex;
81- // Native shape is created lazily when mesh data is set
82- this._nativeShape = null;
83- }
--
181- }
182-
183- // Extract indices for triangle mesh
184: if (!this._isConvex) {
185- this._extractIndices(mesh);
186- if (!this._indices) {
187- console.warn("MeshColliderShape: Triangle mesh requires indices");
--
222- const vertexCount = this._vertices.length / 3;
223-
224- // Validate triangle mesh has indices
225: if (!this._isConvex && !this._indices) {
226- console.warn("MeshColliderShape: Triangle mesh requires indices, skipping update");
227- return;
228- }
229-
230- if (this._nativeShape) {
231- // Update existing shape
232: (<IMeshColliderShape>this._nativeShape).setMeshData(this._vertices, vertexCount, this._indices, this._isConvex);
233- } else {
234- // Create new shape
235- this._nativeShape = Engine._nativePhysics.createMeshColliderShape(
--
237- this._vertices,
238- vertexCount,
239- this._indices,
240: this._isConvex,
241- this._material._nativeMaterial
242- );
243- Engine._physicalObjectsMap[this._id] = this;
Script executed:
# Check for triangle mesh and MeshColliderShape references in the physics module
rg -n "MeshColliderShape|triangle mesh" packages/core/src/physics --type tsRepository: galacean/engine
Repository: galacean/engine
Exit code: 0
stdout:
packages/core/src/physics/DynamicCollider.ts:9:import { MeshColliderShape } from "./shape/MeshColliderShape";
packages/core/src/physics/DynamicCollider.ts:273: // Block switching to non-kinematic if triangle mesh is attached
packages/core/src/physics/DynamicCollider.ts:278: if (shape instanceof MeshColliderShape && !shape.isConvex) {
packages/core/src/physics/DynamicCollider.ts:279: console.error("DynamicCollider: Cannot set isKinematic=false when triangle mesh is attached.");
packages/core/src/physics/DynamicCollider.ts:422: if (shape instanceof MeshColliderShape && !shape.isConvex && !this._isKinematic) {
packages/core/src/physics/DynamicCollider.ts:423: console.error("DynamicCollider: triangle mesh is not supported on non-kinematic DynamicCollider.");
packages/core/src/physics/shape/index.ts:6:export { MeshColliderShape } from "./MeshColliderShape";
packages/core/src/physics/shape/MeshColliderShape.ts:1:import { IMeshColliderShape } from "@galacean/engine-design";
packages/core/src/physics/shape/MeshColliderShape.ts:16:export class MeshColliderShape extends ColliderShape {
packages/core/src/physics/shape/MeshColliderShape.ts:45: * Whether the triangle mesh should be double-sided for collision detection.
packages/core/src/physics/shape/MeshColliderShape.ts:46: * `@remarks` Only applies to triangle mesh (non-convex).
packages/core/src/physics/shape/MeshColliderShape.ts:55: (<IMeshColliderShape>this._nativeShape)?.setDoubleSided(value);
packages/core/src/physics/shape/MeshColliderShape.ts:70: (<IMeshColliderShape>this._nativeShape)?.setTightBounds(value);
packages/core/src/physics/shape/MeshColliderShape.ts:75: * Create a MeshColliderShape.
packages/core/src/physics/shape/MeshColliderShape.ts:88: * `@param` indices - Triangle indices (required for triangle mesh, optional for convex)
packages/core/src/physics/shape/MeshColliderShape.ts:107: console.warn("MeshColliderShape: Only ModelMesh is supported");
packages/core/src/physics/shape/MeshColliderShape.ts:137: console.warn("MeshColliderShape: Mesh has no position attribute");
packages/core/src/physics/shape/MeshColliderShape.ts:145: console.warn("MeshColliderShape: Position buffer not found");
packages/core/src/physics/shape/MeshColliderShape.ts:150: console.warn("MeshColliderShape: Buffer is not readable");
packages/core/src/physics/shape/MeshColliderShape.ts:183: // Extract indices for triangle mesh
packages/core/src/physics/shape/MeshColliderShape.ts:187: console.warn("MeshColliderShape: Triangle mesh requires indices");
packages/core/src/physics/shape/MeshColliderShape.ts:224: // Validate triangle mesh has indices
packages/core/src/physics/shape/MeshColliderShape.ts:226: console.warn("MeshColliderShape: Triangle mesh requires indices, skipping update");
packages/core/src/physics/shape/MeshColliderShape.ts:232: (<IMeshColliderShape>this._nativeShape).setMeshData(this._vertices, vertexCount, this._indices, this._isConvex);
packages/core/src/physics/shape/MeshColliderShape.ts:235: this._nativeShape = Engine._nativePhysics.createMeshColliderShape(
packages/core/src/physics/shape/MeshColliderShape.ts:246: (<IMeshColliderShape>this._nativeShape).setDoubleSided(this._doubleSided);
packages/core/src/physics/shape/MeshColliderShape.ts:247: (<IMeshColliderShape>this._nativeShape).setTightBounds(this._tightBounds);
Script executed:
# Locate MeshColliderShape.ts file
fd "MeshColliderShape" packages/core/src/physicsRepository: galacean/engine
Repository: galacean/engine
Exit code: 0
stdout:
packages/core/src/physics/shape/MeshColliderShape.ts
Script executed:
# Check setMesh and setMeshData implementations
rg -n -A10 "setMesh\(|setMeshData\(" packages/core/src/physics/shape/MeshColliderShape.tsRepository: galacean/engine
Repository: galacean/engine
Exit code: 0
stdout:
90: setMeshData(vertices: Float32Array, indices?: Uint16Array | Uint32Array): void {
91- this._vertices = vertices;
92- this._indices = indices || null;
93- this._updateNativeMesh();
94- }
95-
96- /**
97- * Set mesh data from a Mesh object.
98- * `@param` mesh - The mesh to extract vertex and index data from
99- * `@remarks` The mesh must have accessible data (not released after upload)
100- */
101: setMesh(mesh: Mesh): void {
102- if (mesh instanceof ModelMesh) {
103- if (this._extractMeshData(mesh)) {
104- this._updateNativeMesh();
105- }
106- } else {
107- console.warn("MeshColliderShape: Only ModelMesh is supported");
108- }
109- }
110-
111- protected override _syncNative(): void {
--
232: (<IMeshColliderShape>this._nativeShape).setMeshData(this._vertices, vertexCount, this._indices, this._isConvex);
233- } else {
234- // Create new shape
235- this._nativeShape = Engine._nativePhysics.createMeshColliderShape(
236- this._id,
237- this._vertices,
238- vertexCount,
239- this._indices,
240- this._isConvex,
241- this._material._nativeMaterial
242- );
Script executed:
# Check if there's any shape attachment tracking in ColliderShape or DynamicCollider
rg -n "_shapes|_collider|parent" packages/core/src/physics/shape/ColliderShape.ts packages/core/src/physics/Collider.tsRepository: galacean/engine
Repository: galacean/engine
Exit code: 0
stdout:
packages/core/src/physics/Collider.ts:28: protected _shapes: ColliderShape[] = [];
packages/core/src/physics/Collider.ts:35: return this._shapes;
packages/core/src/physics/Collider.ts:71: const oldCollider = shape._collider;
packages/core/src/physics/Collider.ts:76: this._shapes.push(shape);
packages/core/src/physics/Collider.ts:87: const index = this._shapes.indexOf(shape);
packages/core/src/physics/Collider.ts:89: this._shapes.splice(index, 1);
packages/core/src/physics/Collider.ts:99: const shapes = this._shapes;
packages/core/src/physics/Collider.ts:173: const shapes = this._shapes;
packages/core/src/physics/Collider.ts:184: shape._collider = this;
packages/core/src/physics/Collider.ts:190: shape._collider = null;
packages/core/src/physics/shape/ColliderShape.ts:18: _collider: Collider;
packages/core/src/physics/shape/ColliderShape.ts:44: return this._collider;
packages/core/src/physics/shape/ColliderShape.ts:148: const collider = this._collider;
packages/core/src/physics/shape/ColliderShape.ts:188: this._collider?._handleShapesChanged(ColliderShapeChangeFlag.Property);
packages/core/src/physics/shape/ColliderShape.ts:194: this._collider?._handleShapesChanged(ColliderShapeChangeFlag.Property);
packages/core/src/physics/shape/ColliderShape.ts:200: this._collider?._handleShapesChanged(ColliderShapeChangeFlag.Property);
Add validation to prevent post-attachment isConvex toggles that would violate collider constraints.
The isConvex setter in MeshColliderShape (and setMesh/setMeshData methods) can switch a shape between convex and triangle-mesh modes without validating the parent collider's state. This bypasses the triangle-mesh restrictions enforced in DynamicCollider.addShape() and isKinematic setter.
Scenario: Add MeshColliderShape(isConvex=true) to a non-kinematic DynamicCollider (passes check), then toggle shape.isConvex = false—the shape becomes a non-supported triangle mesh on a non-kinematic collider.
Since ColliderShape tracks its parent via the _collider property, the setter should validate that if switching to non-convex mode (isConvex=false), the parent collider is either kinematic or null (not attached).
Also applies to: 421-427
🤖 Prompt for AI Agents
In `@packages/core/src/physics/DynamicCollider.ts` around lines 271 - 295, When
changing a MeshColliderShape from convex to non-convex (in the isConvex setter)
or when switching mesh via setMesh/setMeshData, validate the parent collider: if
this._collider is non-null and is a DynamicCollider and that DynamicCollider is
non-kinematic (isKinematic === false), disallow the transition (log/throw and
return) because triangle meshes are not supported on non-kinematic
DynamicCollider; update the MeshColliderShape.isConvex setter and both
setMesh/setMeshData to perform this check before mutating isConvex/mesh so you
cannot toggle to a triangle-mesh while attached to a dynamic (non-kinematic)
collider — mirror the same restriction enforced by DynamicCollider.addShape and
the isKinematic setter.
Remove e2e test files without baseline images: - physx-mesh-collider-kinematic-bug.ts - physx-mesh-collider-sphere-container.backup.ts - physx-mesh-collider-sphere-container.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/physics/shape/MeshColliderShape.ts`:
- Around line 217-229: Validate that this._vertices length is divisible by 3
and, when this._indices exists, its length is divisible by 3 before computing
vertexCount or using triangle indices in _updateNativeMesh (and consider similar
checks in setMeshData); if lengths are invalid, log a clear warning mentioning
MeshColliderShape and the offending array (vertices or indices) and return early
to avoid fractional vertexCount or incomplete triangles. Ensure checks reference
_updateNativeMesh, setMeshData, this._vertices, this._indices, and vertexCount
so reviewers can find the guarded logic quickly.
♻️ Duplicate comments (1)
packages/core/src/physics/shape/MeshColliderShape.ts (1)
155-176: Guard against unaligned buffer offsets before creating the Float32Array view.
Float32Array(buffer, byteOffset, length)requires 4‑byte alignment; the current code also dividesbyteStride/byteOffsetby 4 without validation. A quick guard avoids RangeError and misreads.🛡️ Proposed fix
const vertexCount = mesh.vertexCount; const byteOffset = vertexElement.offset; const byteStride = bufferBinding.stride; const bufferData = buffer.data; + if (bufferData.byteOffset % 4 !== 0 || byteOffset % 4 !== 0 || byteStride % 4 !== 0) { + console.warn("MeshColliderShape: buffer offset/stride must be 4-byte aligned"); + return false; + } // Reuse or create Float32Array if (!this._vertices || this._vertices.length !== vertexCount * 3) { this._vertices = new Float32Array(vertexCount * 3); }JavaScript Float32Array byteOffset alignment requirement RangeError
| private _updateNativeMesh(): void { | ||
| if (!this._vertices || this._vertices.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const vertexCount = this._vertices.length / 3; | ||
|
|
||
| // Validate triangle mesh has indices | ||
| if (!this._isConvex && !this._indices) { | ||
| console.warn("MeshColliderShape: Triangle mesh requires indices, skipping update"); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Validate vertex/indices lengths to avoid fractional vertex counts and invalid triangles.
setMeshData is public; if the caller passes malformed arrays, vertexCount becomes fractional and triangle indices can be incomplete. Add a small guard and warn early.
🧯 Proposed fix
- const vertexCount = this._vertices.length / 3;
+ if (this._vertices.length % 3 !== 0) {
+ console.warn("MeshColliderShape: vertices length must be a multiple of 3");
+ return;
+ }
+ const vertexCount = this._vertices.length / 3;
// Validate triangle mesh has indices
if (!this._isConvex && !this._indices) {
console.warn("MeshColliderShape: Triangle mesh requires indices, skipping update");
return;
}
+ if (!this._isConvex && this._indices.length % 3 !== 0) {
+ console.warn("MeshColliderShape: indices length must be a multiple of 3");
+ return;
+ }🤖 Prompt for AI Agents
In `@packages/core/src/physics/shape/MeshColliderShape.ts` around lines 217 - 229,
Validate that this._vertices length is divisible by 3 and, when this._indices
exists, its length is divisible by 3 before computing vertexCount or using
triangle indices in _updateNativeMesh (and consider similar checks in
setMeshData); if lengths are invalid, log a clear warning mentioning
MeshColliderShape and the offending array (vertices or indices) and return early
to avoid fractional vertexCount or incomplete triangles. Ensure checks reference
_updateNativeMesh, setMeshData, this._vertices, this._indices, and vertexCount
so reviewers can find the guarded logic quickly.
2 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Feature: MeshColliderShape 支持
Bug Fix: 物理碰撞相关修复
What is the current behavior?
DynamicCollider.move()在isKinematic = false时会导致碰撞失效 (isKinematic 为 false 时调用move() 后物理碰撞会失效 #2879)What is the new behavior?
新功能：MeshColliderShape
setMeshData(vertices, indices)API 设置网格数据Bug 修复
Collider._addNativeShape()中添加setWorldScale调用，确保动态添加的 shape 正确应用 transform 缩放isKinematic检查，非 kinematic 刚体调用move()时打印警告并返回Does this PR introduce a breaking change?
No
Commits
d1600019d47d84c85704cf663c0ac0b567cd49bdb2Files Changed
Core API
packages/core/src/physics/shape/MeshColliderShape.ts- 新增packages/core/src/physics/Collider.ts- 修复 addShape 缩放问题packages/core/src/physics/DynamicCollider.ts- 修复 move() 问题PhysX Implementation
packages/physics-physx/src/shape/PhysXMeshColliderShape.ts- 新增packages/physics-physx/libs/physx.release.*- 更新 PhysX 库Tests
tests/src/core/physics/MeshColliderShape.test.ts- 单元测试e2e/case/physx-mesh-collider*.ts- E2E 测试Fixes #2879
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.