feat: support source-v2 migration runtime and physics compatibility#3066
Draft
luzhuang wants to merge 15 commits into
Draft
feat: support source-v2 migration runtime and physics compatibility#3066luzhuang wants to merge 15 commits into
luzhuang wants to merge 15 commits into
Conversation
WalkthroughThis PR updates physics synchronization and native resource handling, adds particle collection setters, exposes canonical glTF scene selection, normalizes resource cache lookup, registers engine math classes, preserves transform state during replacement, and expands regression coverage across core, loader, and physics behavior. ChangesCore state and particle serialization
Physics behavior and native integration
Loader, resources, and runtime registration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/particle/modules/ParticleCurve.ts (1)
90-96: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
setKeysleaks callbacks and lacks pre-validation, unlikeParticleGradient.setKeys.Two issues compared to the
ParticleGradient.setKeyspattern:
Callback leak: Old
CurveKeyobjects are cleared from the array without unregistering their_updateDispatchcallback.removeKey(line 82) properly calls_unRegisterOnValueChanged, butsetKeysdoes not. If old keys are shared or reused, their value changes will trigger stale dispatches on this curve.No input length pre-validation: If called with >4 keys,
addKeythrows on the 5th key — but the old keys are already cleared, leaving the curve in a broken state with data loss.ParticleGradient.setKeysvalidateslength > 4before clearing.🔧 Proposed fix: unregister old callbacks and pre-validate length
setKeys(keys: ReadonlyArray<CurveKey>): void { + if (keys.length > 4) { + throw new Error("Curve can only have 4 keys"); + } + + const currentKeys = this._keys; + for (let i = 0, n = currentKeys.length; i < n; i++) { + currentKeys[i]._unRegisterOnValueChanged(this._updateDispatch); + } this._keys.length = 0; for (let i = 0, n = keys.length; i < n; i++) { this.addKey(keys[i]); } this._typeArrayDirty = true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/particle/modules/ParticleCurve.ts` around lines 90 - 96, Update ParticleCurve.setKeys to reject inputs longer than four keys before mutating the existing collection, preserving the current keys when validation fails. On valid input, unregister each existing key’s _updateDispatch callback before clearing the array, then add the replacement keys and mark _typeArrayDirty as currently done.
🧹 Nitpick comments (2)
packages/galacean/src/index.ts (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate math registration loop.
MathObjectsis registered once at Lines 18–20 and then immediately registered again at Lines 22–24. Remove the second loop to avoid redundant startup work and potential duplicate-registration side effects.Proposed cleanup
-for (let key in MathObjects) { - Loader.registerClass(key, MathObjects[key]); -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/galacean/src/index.ts` around lines 22 - 24, Remove the duplicate for-in registration loop over MathObjects following the existing registration block, leaving the initial MathObjects registration through Loader.registerClass unchanged.tests/src/loader/GLTFLoader.test.ts (1)
605-617: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the uncached sub-asset path.
Because the root asset is loaded first, the second request is resolved through the cache and
glTFResource.scenes[0]; it does not exercise the changedGLTFParserContextcallback forscenes[0]. Query the sub-asset first, or use a fresh manager/fixture, so this test verifies pending sub-asset resolution as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/loader/GLTFLoader.test.ts` around lines 605 - 617, Update the test around the “resolves scene sub-assets by the canonical glTF schema key” case so the queried scenes[0] sub-asset loads before the root GLTF resource, or use a fresh resource manager/fixture. Ensure the assertion still compares the resolved scene to glTFResource.scenes[0] while exercising uncached pending sub-asset resolution through GLTFParserContext.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/physics/PhysicsScene.ts`:
- Around line 855-883: Update _hasCollisionEventConsumers to skip null entries
when reading scripts._elements, matching the element guard used by
DisorderedArray.forEach and _dispatchEvents. Check each script before accessing
onCollisionEnter, onCollisionExit, or onCollisionStay, while preserving the
existing consumer cache behavior for valid scripts.
In `@packages/core/src/physics/shape/MeshColliderShape.ts`:
- Around line 26-33: Update the cookingFlags setter and its native mesh-update
path around _updateNativeShapeData so exceptions from
PhysXMeshColliderShape.setMeshData are handled transactionally, not only false
returns. Track whether the native update succeeded, and in a finally block
restore the previous cooking flags plus the prior mesh, buffers, and reference
counts whenever it did not; preserve the successful update path unchanged.
In
`@packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts`:
- Around line 293-317: After resolving the resource in the promise within
HierarchyParser’s asset-loading flow, validate glTFSceneSelection.sceneIndex
against the resolved GLTFResource’s scenes collection before calling
instantiateSceneRoot. Reject out-of-range indices with a clear error identifying
the requested scene key and asset reference, while preserving the existing
prefab handling and valid-index instantiation behavior.
In `@packages/physics-physx/src/PhysXPhysics.ts`:
- Around line 60-88: Validate resolvedOptions?.tolerancesScale for positive
finite length and speed in the PhysXPhysics constructor before assigning
_tolerancesScaleOptions or calling _updateScaledDefaults. Reuse the existing
validation behavior from _applyTolerancesScale, and throw immediately for
invalid values so initialization cannot proceed with invalid defaults.
In `@tests/src/core/physics/MeshColliderShape.test.ts`:
- Line 311: Remove the `_isShapeAttached` assertions from the MeshColliderShape
tests, including the checks on clonedShape and the other indicated cases. Keep
the adjacent native-shape count assertions, which already validate attachment
ownership.
In `@tests/src/core/physics/PhysicsScene.test.ts`:
- Around line 99-101: Update getLastContactEventDemandCall to distinguish an
empty calls array from a native call returning false; do not use false as the
missing-call fallback. Make the helper expose or throw on the missing-call case
so disable-demand tests fail when setContactEventEnabled was never invoked,
while preserving false for an actual recorded call.
In `@tests/src/core/Transform.test.ts`:
- Around line 119-122: Update the rotation assertion in the Transform test to
use an approximate comparison with an appropriate tolerance instead of exact
deep inclusion, while leaving the position and scale assertions unchanged.
- Around line 128-130: Update the second rotation assertion in the transform
test to use approximate floating-point comparison for the y value, matching the
tolerance approach used by the earlier rotation assertion, while preserving the
expected rotation of x: 0, y: 90, z: 0.
---
Outside diff comments:
In `@packages/core/src/particle/modules/ParticleCurve.ts`:
- Around line 90-96: Update ParticleCurve.setKeys to reject inputs longer than
four keys before mutating the existing collection, preserving the current keys
when validation fails. On valid input, unregister each existing key’s
_updateDispatch callback before clearing the array, then add the replacement
keys and mark _typeArrayDirty as currently done.
---
Nitpick comments:
In `@packages/galacean/src/index.ts`:
- Around line 22-24: Remove the duplicate for-in registration loop over
MathObjects following the existing registration block, leaving the initial
MathObjects registration through Loader.registerClass unchanged.
In `@tests/src/loader/GLTFLoader.test.ts`:
- Around line 605-617: Update the test around the “resolves scene sub-assets by
the canonical glTF schema key” case so the queried scenes[0] sub-asset loads
before the root GLTF resource, or use a fresh resource manager/fixture. Ensure
the assertion still compares the resolved scene to glTFResource.scenes[0] while
exercising uncached pending sub-asset resolution through GLTFParserContext.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f46c8a75-16be-493c-8c80-88d2aaff1e24
⛔ Files ignored due to path filters (1)
packages/shader/src/ShaderLibrary/Skin/Skin.glslis excluded by!**/*.glsl
📒 Files selected for processing (40)
.gitignorepackages/core/src/Entity.tspackages/core/src/Script.tspackages/core/src/asset/ResourceManager.tspackages/core/src/particle/modules/EmissionModule.tspackages/core/src/particle/modules/ParticleCurve.tspackages/core/src/particle/modules/ParticleGradient.tspackages/core/src/physics/CharacterController.tspackages/core/src/physics/Collider.tspackages/core/src/physics/Collision.tspackages/core/src/physics/DynamicCollider.tspackages/core/src/physics/PhysicsMaterial.tspackages/core/src/physics/PhysicsScene.tspackages/core/src/physics/index.tspackages/core/src/physics/shape/ColliderShape.tspackages/core/src/physics/shape/MeshColliderShape.tspackages/design/src/physics/IPhysics.tspackages/design/src/physics/IPhysicsScene.tspackages/galacean/src/index.tspackages/loader/src/gltf/GLTFResource.tspackages/loader/src/gltf/parser/GLTFParserContext.tspackages/loader/src/resource-deserialize/resources/parser/HierarchyParser.tspackages/physics-physx/src/PhysXCharacterController.tspackages/physics-physx/src/PhysXDynamicCollider.tspackages/physics-physx/src/PhysXPhysics.tspackages/physics-physx/src/PhysXPhysicsScene.tspackages/physics-physx/src/index.tspackages/physics-physx/src/shape/PhysXColliderShape.tspackages/physics-physx/src/shape/PhysXMeshColliderShape.tstests/src/core/Transform.test.tstests/src/core/particle/Burst.test.tstests/src/core/particle/ParticleSerialization.test.tstests/src/core/physics/Collision.test.tstests/src/core/physics/DynamicCollider.test.tstests/src/core/physics/MeshColliderShape.test.tstests/src/core/physics/PhysicsMaterial.test.tstests/src/core/physics/PhysicsScene.test.tstests/src/core/resource/ResourceManager.test.tstests/src/loader/GLTFLoader.test.tstests/src/loader/SceneFormatV2.test.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
luzhuang
force-pushed
the
feat/migrate-2.0
branch
from
3e51064 to
4252f17
Compare
luzhuang
changed the title
Contributor
Author
|
Review feedback is addressed in
Two suggestions were intentionally not applied: Validated with |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3066 +/- ##
===========================================
+ Coverage 79.61% 79.72% +0.10%
===========================================
Files 904 904
Lines 101208 101735 +527
Branches 11404 11518 +114
===========================================
+ Hits 80579 81106 +527
- Misses 20445 20448 +3
+ Partials 184 181 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
GuoLei1990
approved these changes
Jul 14, 2026
GuoLei1990
left a comment
GuoLei1990
left a comment
Member
There was a problem hiding this comment.
总结
第二轮审查（tip 4252f17ab「fix: resolve migration runtime review findings」）。该 commit 是精准的 review-response，逐条闭环了首轮的全部阻塞项，且每个 correctness 修复都配了反向证伪测试。CI 全绿（build ×3 / codecov 真 vitest 246/246 / e2e 4/4 / lint）。首轮的 request-changes 门控解除。
已闭环问题清单
- [P1]
fix:前缀 + 多正交域混装 — 已修复：标题改为feat:，且移除了整套RENDERER_USE_BAKED_SKINNINGbaked-skinning shader 路径（Skin.glsl回归单一getJointMatrix），把无关的蒙皮新特性从物理迁移 PR 中剥离。 - [P2]
packages/galacean/src/index.tsMathObjects 重复注册 — 已修复：删除第二个for...in循环。 - [P2] 越界 scene index 崩溃 — 已修复：
GLTFResource.instantiateSceneRoot增加Number.isSafeInteger + <0 + >=length校验抛RangeError（含 url + scene 数），并对_defaultSceneRoot为空补Error。收口在单一入口，所有 caller（含 HierarchyParser）统一获得清晰报错。配GLTFLoader.test.ts反向测试断言越界抛out of range。 - [P1] MeshColliderShape 事务性回滚（throw 路径） — 已修复：
cookingFlags与mesh两个 setter 均改try { updated = _updateNativeShapeData() } finally { if(!updated) revert }，false返回与 native throw 两条路径都回滚。测试rolls back cached state when native recooking throws用 spy 抛异常，断言cookingFlags/mesh/ refCount（originalMesh=1replacementMesh=0）三者全回滚 — revert 修复即红。 - [P3→已修] PhysXPhysics tolerances 校验 — 校验提前到构造期（
_validateTolerancesScale），非法 length/speed 在new PhysXPhysics(...)即抛，报错点更早。配DynamicCollider.test.tsrejects invalid tolerancesScale before initialization。 - [Correction] PhysicsScene null guard — 作者说明「
_syncContactEventDemand在 script 迭代后执行、DisorderedArray无 deferred null 空洞」，与我首轮结论一致，确认不可达 — 已关闭，不再提。 - CI type error（ReflectionParser
$args） —constructorArgs = $args ?? []后统一Array.isArray校验，与旧undefined→[]语义等价。 - 粒子集合替换原子性 —
ParticleCurve.setKeys/ParticleGradient.setKeys先[...keys]快照输入再清空，消除「传入自身内部数组 → clear 把输入也清掉」的别名 bug；旧 key 先_unRegisterOnValueChanged/_onValueChanged=null解绑 stale callback，新 key 重新注册。配ParticleSerialization.test.ts断言 stale key 变更不再触发 dispatch（original.value=4 → changes===0）+setKeys(自身 keys)幂等 + 超 4 key 抛错后原状保留。 - Transform 精度测试 —
deep.include精确等值改approximately(…,1e-6)，消除浮点抖动 flaky。 - contact-demand 测试 —
getLastContactEventDemandCall返回undefined而非强制false，区分「native 调用从未发生」与「真的传了false」；并从共享activeScene改用独立new Scene，测试隔离更干净。
额外核对
_onPhysicsUpdate每帧重试机制整体移除（Collider._onUpdate的 per-shape loop +ColliderShape基类 no-op +MeshColliderShapeoverride）— 方向正确。cooking 失败时同一输入每帧重试不会有不同结果，这是纯物理 update 热路径上的死重试，删除是净收益而非回归；与该机制在 #3042 差点被判 P1 的判断一致。_isShapeAttached确为ColliderShape.ts:20的继承字段，供 attach/detach 归属使用，作者保留断言的说明成立。
结论
approve：首轮全部阻塞项已闭环，修复落点正确（事务性回滚在 setter 层、bounds 校验收口在单一入口、别名快照在 setKeys 内部），correctness 类均配反向证伪测试，CI 全绿。无新问题。
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/physics-physx/src/shape/PhysXMeshColliderShape.ts (1)
103-106: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent memory leak by deleting
_pxGeometryduring destruction.
this._pxGeometryis created dynamically (e.g., viacreateConvexMeshGeometry) and manually deleted during swaps insetMeshDataand_updateGeometry. However, it is not being deleted when the shape is destroyed, which can lead to a memory leak in the Emscripten heap.🛠️ Proposed fix to clean up geometry
override destroy(): void { this._pxMesh?.release(); + this._pxGeometry?.delete(); super.destroy(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts` around lines 103 - 106, Update the destroy() method to delete the dynamically allocated _pxGeometry before completing destruction, while preserving the existing _pxMesh release and super.destroy() calls. Ensure cleanup is safe when _pxGeometry is absent and matches the ownership handling used by setMeshData and _updateGeometry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts`:
- Around line 103-106: Update the destroy() method to delete the dynamically
allocated _pxGeometry before completing destruction, while preserving the
existing _pxMesh release and super.destroy() calls. Ensure cleanup is safe when
_pxGeometry is absent and matches the ownership handling used by setMeshData and
_updateGeometry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 18fa3b39-05ff-4026-97fd-e67d0eea3b82
📒 Files selected for processing (42)
.gitignorepackages/core/src/Entity.tspackages/core/src/Script.tspackages/core/src/asset/ResourceManager.tspackages/core/src/particle/modules/EmissionModule.tspackages/core/src/particle/modules/ParticleCurve.tspackages/core/src/particle/modules/ParticleGradient.tspackages/core/src/physics/CharacterController.tspackages/core/src/physics/Collider.tspackages/core/src/physics/Collision.tspackages/core/src/physics/DynamicCollider.tspackages/core/src/physics/PhysicsMaterial.tspackages/core/src/physics/PhysicsScene.tspackages/core/src/physics/index.tspackages/core/src/physics/shape/ColliderShape.tspackages/core/src/physics/shape/MeshColliderShape.tspackages/design/src/physics/IPhysics.tspackages/design/src/physics/IPhysicsScene.tspackages/galacean/src/index.tspackages/loader/src/gltf/GLTFResource.tspackages/loader/src/gltf/parser/GLTFParserContext.tspackages/loader/src/resource-deserialize/resources/parser/HierarchyParser.tspackages/loader/src/resource-deserialize/resources/parser/ReflectionParser.tspackages/loader/src/schema/CommonSchema.tspackages/physics-physx/src/PhysXCharacterController.tspackages/physics-physx/src/PhysXDynamicCollider.tspackages/physics-physx/src/PhysXPhysics.tspackages/physics-physx/src/PhysXPhysicsScene.tspackages/physics-physx/src/index.tspackages/physics-physx/src/shape/PhysXColliderShape.tspackages/physics-physx/src/shape/PhysXMeshColliderShape.tstests/src/core/Transform.test.tstests/src/core/particle/Burst.test.tstests/src/core/particle/ParticleSerialization.test.tstests/src/core/physics/Collision.test.tstests/src/core/physics/DynamicCollider.test.tstests/src/core/physics/MeshColliderShape.test.tstests/src/core/physics/PhysicsMaterial.test.tstests/src/core/physics/PhysicsScene.test.tstests/src/core/resource/ResourceManager.test.tstests/src/loader/GLTFLoader.test.tstests/src/loader/SceneFormatV2.test.ts
🚧 Files skipped from review as they are similar to previous changes (33)
- packages/design/src/physics/IPhysics.ts
- packages/physics-physx/src/index.ts
- packages/core/src/physics/index.ts
- tests/src/core/particle/Burst.test.ts
- tests/src/core/resource/ResourceManager.test.ts
- packages/core/src/asset/ResourceManager.ts
- packages/physics-physx/src/PhysXPhysicsScene.ts
- packages/core/src/Script.ts
- tests/src/core/particle/ParticleSerialization.test.ts
- packages/core/src/Entity.ts
- packages/physics-physx/src/PhysXCharacterController.ts
- packages/core/src/particle/modules/EmissionModule.ts
- packages/core/src/physics/Collision.ts
- packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts
- packages/physics-physx/src/shape/PhysXColliderShape.ts
- packages/core/src/particle/modules/ParticleCurve.ts
- packages/loader/src/gltf/parser/GLTFParserContext.ts
- tests/src/core/physics/MeshColliderShape.test.ts
- tests/src/core/physics/Collision.test.ts
- tests/src/core/Transform.test.ts
- packages/core/src/physics/PhysicsMaterial.ts
- .gitignore
- tests/src/core/physics/PhysicsMaterial.test.ts
- packages/core/src/physics/PhysicsScene.ts
- packages/core/src/physics/Collider.ts
- tests/src/core/physics/DynamicCollider.test.ts
- packages/core/src/physics/CharacterController.ts
- packages/core/src/particle/modules/ParticleGradient.ts
- tests/src/core/physics/PhysicsScene.test.ts
- packages/physics-physx/src/PhysXPhysics.ts
- packages/core/src/physics/shape/MeshColliderShape.ts
- packages/physics-physx/src/PhysXDynamicCollider.ts
- packages/core/src/physics/DynamicCollider.ts
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
2 participants
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.
Summary
Scope
This PR now contains only the physics and glTF compatibility required by migrated Engine 2.0 projects. Generic runtime constructed values and nested calls were moved to #3067; Editor source-v2 compilation is in galacean/editor#3793. Particle Core collection setters and global Math class registration were removed.
Validation
pnpm buildpnpm lint(0 errors)下落2048and保卫萝卜built against Engine 2.0.0-alpha.38 with runtime component validation