fix: align source-v2 and physics migration semantics#3066
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 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
| private _hasCollisionEventConsumers(): boolean { | ||
| if (!this._collisionEventConsumersDirty) { | ||
| return this._hasCollisionEventConsumersCache; | ||
| } | ||
|
|
||
| const { _elements: colliders } = this._colliders; | ||
| const { onCollisionEnter, onCollisionExit, onCollisionStay } = Script.prototype; | ||
|
|
||
| for (let i = this._colliders.length - 1; i >= 0; --i) { | ||
| const scripts = colliders[i].entity._scripts; | ||
| const scriptElements = scripts._elements; | ||
| for (let j = scripts.length - 1; j >= 0; --j) { | ||
| const script = scriptElements[j]; | ||
| if ( | ||
| script.onCollisionEnter !== onCollisionEnter || | ||
| script.onCollisionExit !== onCollisionExit || | ||
| script.onCollisionStay !== onCollisionStay | ||
| ) { | ||
| this._collisionEventConsumersDirty = false; | ||
| this._hasCollisionEventConsumersCache = true; | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this._collisionEventConsumersDirty = false; | ||
| this._hasCollisionEventConsumersCache = false; | ||
| return this._hasCollisionEventConsumersCache; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a null guard when scanning entity._scripts for collision consumers.
_hasCollisionEventConsumers indexes scriptElements[j] directly, without the element && guard that DisorderedArray.forEach (and this same file's _dispatchEvents) applies to the identical entity._scripts collection. If a script slot is temporarily null (e.g. mid-loop removal from a script callback), this will throw on script.onCollisionEnter.
🛡️ Proposed fix
for (let j = scripts.length - 1; j >= 0; --j) {
const script = scriptElements[j];
if (
+ script &&
(script.onCollisionEnter !== onCollisionEnter ||
script.onCollisionExit !== onCollisionExit ||
- script.onCollisionStay !== onCollisionStay
+ script.onCollisionStay !== onCollisionStay)
) {📝 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.
| private _hasCollisionEventConsumers(): boolean { | |
| if (!this._collisionEventConsumersDirty) { | |
| return this._hasCollisionEventConsumersCache; | |
| } | |
| const { _elements: colliders } = this._colliders; | |
| const { onCollisionEnter, onCollisionExit, onCollisionStay } = Script.prototype; | |
| for (let i = this._colliders.length - 1; i >= 0; --i) { | |
| const scripts = colliders[i].entity._scripts; | |
| const scriptElements = scripts._elements; | |
| for (let j = scripts.length - 1; j >= 0; --j) { | |
| const script = scriptElements[j]; | |
| if ( | |
| script.onCollisionEnter !== onCollisionEnter || | |
| script.onCollisionExit !== onCollisionExit || | |
| script.onCollisionStay !== onCollisionStay | |
| ) { | |
| this._collisionEventConsumersDirty = false; | |
| this._hasCollisionEventConsumersCache = true; | |
| return true; | |
| } | |
| } | |
| } | |
| this._collisionEventConsumersDirty = false; | |
| this._hasCollisionEventConsumersCache = false; | |
| return this._hasCollisionEventConsumersCache; | |
| } | |
| private _hasCollisionEventConsumers(): boolean { | |
| if (!this._collisionEventConsumersDirty) { | |
| return this._hasCollisionEventConsumersCache; | |
| } | |
| const { _elements: colliders } = this._colliders; | |
| const { onCollisionEnter, onCollisionExit, onCollisionStay } = Script.prototype; | |
| for (let i = this._colliders.length - 1; i >= 0; --i) { | |
| const scripts = colliders[i].entity._scripts; | |
| const scriptElements = scripts._elements; | |
| for (let j = scripts.length - 1; j >= 0; --j) { | |
| const script = scriptElements[j]; | |
| if ( | |
| script && | |
| (script.onCollisionEnter !== onCollisionEnter || | |
| script.onCollisionExit !== onCollisionExit || | |
| script.onCollisionStay !== onCollisionStay) | |
| ) { | |
| this._collisionEventConsumersDirty = false; | |
| this._hasCollisionEventConsumersCache = true; | |
| return true; | |
| } | |
| } | |
| } | |
| this._collisionEventConsumersDirty = false; | |
| this._hasCollisionEventConsumersCache = false; | |
| return this._hasCollisionEventConsumersCache; | |
| } |
🤖 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/physics/PhysicsScene.ts` around lines 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.
| set cookingFlags(value: MeshColliderShapeCookingFlag) { | ||
| if (this._cookingFlags !== value) { | ||
| const previousValue = this._cookingFlags; | ||
| this._cookingFlags = value; | ||
| if (this._nativeShape) { | ||
| this._updateNativeShapeData(); | ||
| if (!this._updateNativeShapeData()) { | ||
| this._cookingFlags = previousValue; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Roll back state when native recooking throws.
The rollback only handles a false result. PhysXMeshColliderShape.setMeshData() can throw after preserving its old geometry, leaving these cached flags, mesh buffers, and reference counts committed while the native shape still uses the old mesh. Use try/finally so both failure paths restore state.
Proposed transactional handling
- if (this._nativeShape) {
- if (!this._updateNativeShapeData()) {
+ if (this._nativeShape) {
+ let updated = false;
+ try {
+ updated = this._updateNativeShapeData();
+ } finally {
+ if (!updated) {
this._cookingFlags = previousValue;
+ }
}Apply the same pattern around the mesh update and restore its mesh, buffers, and reference counts in the finally block when updated remains false.
Also applies to: 74-91
🤖 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/physics/shape/MeshColliderShape.ts` around lines 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.
| let glTFSceneSelection: GLTFSceneSelection | undefined; | ||
| try { | ||
| refItem = resolveRefItem(this.data.refs, instance.asset, "HierarchyParser", "instance.asset"); | ||
| glTFSceneSelection = parseGLTFSceneSelection(refItem.key); | ||
| } catch (error) { | ||
| return Promise.reject(error); | ||
| } | ||
|
|
||
| const resourceRef = glTFSceneSelection ? { url: refItem.url } : refItem; | ||
| return ( | ||
| engine.resourceManager | ||
| // @ts-ignore | ||
| .getResourceByRef<Entity>(refItem) | ||
| .then((prefabResource: PrefabResource | GLTFResource) => { | ||
| const entity = | ||
| prefabResource instanceof PrefabResource | ||
| ? prefabResource.instantiate() | ||
| : prefabResource.instantiateSceneRoot(); | ||
| .getResourceByRef<PrefabResource | GLTFResource>(resourceRef) | ||
| .then((resource: PrefabResource | GLTFResource) => { | ||
| let entity: Entity; | ||
| if (resource instanceof PrefabResource) { | ||
| if (glTFSceneSelection) { | ||
| throw new Error(`HierarchyParser: glTF scene key "${refItem.key}" resolved to a prefab resource.`); | ||
| } | ||
| entity = resource.instantiate(); | ||
| } else if (resource instanceof GLTFResource) { | ||
| entity = resource.instantiateSceneRoot(glTFSceneSelection?.sceneIndex); | ||
| } else { | ||
| throw new Error(`HierarchyParser: instance asset "${refItem.url}" is not a prefab or glTF resource.`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject out-of-range scene indices explicitly.
parseGLTFSceneSelection validates only the index format. A valid-looking scenes[n] that exceeds resource.scenes.length reaches instantiateSceneRoot, whose direct .clone() call then fails with an opaque TypeError. Validate the index after resolving the GLTFResource and report the invalid reference clearly.
Proposed validation
} else if (resource instanceof GLTFResource) {
+ const sceneIndex = glTFSceneSelection?.sceneIndex;
+ if (sceneIndex !== undefined && sceneIndex >= resource.scenes.length) {
+ throw new Error(`HierarchyParser: glTF scene index ${sceneIndex} is out of range.`);
+ }
- entity = resource.instantiateSceneRoot(glTFSceneSelection?.sceneIndex);
+ entity = resource.instantiateSceneRoot(sceneIndex);📝 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.
| let glTFSceneSelection: GLTFSceneSelection | undefined; | |
| try { | |
| refItem = resolveRefItem(this.data.refs, instance.asset, "HierarchyParser", "instance.asset"); | |
| glTFSceneSelection = parseGLTFSceneSelection(refItem.key); | |
| } catch (error) { | |
| return Promise.reject(error); | |
| } | |
| const resourceRef = glTFSceneSelection ? { url: refItem.url } : refItem; | |
| return ( | |
| engine.resourceManager | |
| // @ts-ignore | |
| .getResourceByRef<Entity>(refItem) | |
| .then((prefabResource: PrefabResource | GLTFResource) => { | |
| const entity = | |
| prefabResource instanceof PrefabResource | |
| ? prefabResource.instantiate() | |
| : prefabResource.instantiateSceneRoot(); | |
| .getResourceByRef<PrefabResource | GLTFResource>(resourceRef) | |
| .then((resource: PrefabResource | GLTFResource) => { | |
| let entity: Entity; | |
| if (resource instanceof PrefabResource) { | |
| if (glTFSceneSelection) { | |
| throw new Error(`HierarchyParser: glTF scene key "${refItem.key}" resolved to a prefab resource.`); | |
| } | |
| entity = resource.instantiate(); | |
| } else if (resource instanceof GLTFResource) { | |
| entity = resource.instantiateSceneRoot(glTFSceneSelection?.sceneIndex); | |
| } else { | |
| throw new Error(`HierarchyParser: instance asset "${refItem.url}" is not a prefab or glTF resource.`); | |
| } | |
| let glTFSceneSelection: GLTFSceneSelection | undefined; | |
| try { | |
| refItem = resolveRefItem(this.data.refs, instance.asset, "HierarchyParser", "instance.asset"); | |
| glTFSceneSelection = parseGLTFSceneSelection(refItem.key); | |
| } catch (error) { | |
| return Promise.reject(error); | |
| } | |
| const resourceRef = glTFSceneSelection ? { url: refItem.url } : refItem; | |
| return ( | |
| engine.resourceManager | |
| // `@ts-ignore` | |
| .getResourceByRef<PrefabResource | GLTFResource>(resourceRef) | |
| .then((resource: PrefabResource | GLTFResource) => { | |
| let entity: Entity; | |
| if (resource instanceof PrefabResource) { | |
| if (glTFSceneSelection) { | |
| throw new Error(`HierarchyParser: glTF scene key "${refItem.key}" resolved to a prefab resource.`); | |
| } | |
| entity = resource.instantiate(); | |
| } else if (resource instanceof GLTFResource) { | |
| const sceneIndex = glTFSceneSelection?.sceneIndex; | |
| if (sceneIndex !== undefined && sceneIndex >= resource.scenes.length) { | |
| throw new Error(`HierarchyParser: glTF scene index ${sceneIndex} is out of range.`); | |
| } | |
| entity = resource.instantiateSceneRoot(sceneIndex); | |
| } else { | |
| throw new Error(`HierarchyParser: instance asset "${refItem.url}" is not a prefab or glTF resource.`); | |
| } |
🤖 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/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts`
around lines 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.
| private _tolerancesScaleOptions: PhysXTolerancesScale | undefined; | ||
| private _defaultContactOffset = 0.02; | ||
| private _defaultSleepThreshold = 5e-3; | ||
|
|
||
| /** | ||
| * Create a PhysXPhysics instance. | ||
| * @param runtimeMode - Runtime mode, `Auto` prefers WebAssembly SIMD if supported @see {@link PhysXRuntimeMode} | ||
| * @param runtimeUrls - Manually specify the runtime URLs | ||
| * @param options - PhysX options. | ||
| */ | ||
| constructor(runtimeMode: PhysXRuntimeMode = PhysXRuntimeMode.Auto, runtimeUrls?: PhysXRuntimeUrls) { | ||
| constructor(runtimeMode?: PhysXRuntimeMode, runtimeUrls?: PhysXRuntimeUrls, options?: PhysXPhysicsOptions); | ||
| constructor(options?: PhysXPhysicsOptions); | ||
| constructor( | ||
| runtimeModeOrOptions: PhysXRuntimeMode | PhysXPhysicsOptions = PhysXRuntimeMode.Auto, | ||
| runtimeUrls?: PhysXRuntimeUrls, | ||
| options?: PhysXPhysicsOptions | ||
| ) { | ||
| const isOptionsObject = typeof runtimeModeOrOptions === "object"; | ||
| const runtimeMode = isOptionsObject ? PhysXRuntimeMode.Auto : (runtimeModeOrOptions ?? PhysXRuntimeMode.Auto); | ||
| const resolvedOptions = isOptionsObject ? runtimeModeOrOptions : options; | ||
| this._runTimeMode = runtimeMode; | ||
| this._wasmSIMDModeUrl = | ||
| runtimeUrls?.wasmSIMDModeUrl ?? | ||
| "https://mdn.alipayobjects.com/rms/uri/file/as/apwallet/1781696156399/suyi/physx.release.simd.js"; | ||
| this._wasmModeUrl = | ||
| runtimeUrls?.wasmModeUrl ?? | ||
| "https://mdn.alipayobjects.com/rms/uri/file/as/apwallet/1781696156399/suyi/physx.release.js"; | ||
| this._tolerancesScaleOptions = resolvedOptions?.tolerancesScale; | ||
| this._updateScaledDefaults(this._tolerancesScaleOptions?.length ?? 1, this._tolerancesScaleOptions?.speed ?? 10); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect relevant sections with line numbers.
wc -l packages/physics-physx/src/PhysXPhysics.ts
sed -n '1,220p' packages/physics-physx/src/PhysXPhysics.ts | cat -n
printf '\n---\n'
sed -n '300,390p' packages/physics-physx/src/PhysXPhysics.ts | cat -n
printf '\n---\n'
sed -n '390,470p' packages/physics-physx/src/PhysXPhysics.ts | cat -n
# Find tests/usages related to tolerancesScale and initialize state.
printf '\n=== SEARCH ===\n'
rg -n "tolerancesScale|_initializeState|initialize\(\)|_applyTolerancesScale|_assertPositiveFinite|getDefaultContactOffset|getDefaultSleepThreshold" packages/physics-physx -SRepository: galacean/engine
Length of output: 16137
Validate tolerancesScale before caching scaled defaults
_updateScaledDefaults() runs in the constructor with raw tolerancesScale values, but the positive-finite check only happens later in _applyTolerancesScale(). Invalid input can leak bogus defaults before initialization, and a failed initialize() leaves _initializeState stuck in Initializing. Move the validation into the constructor so misconfiguration fails fast.
🤖 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/PhysXPhysics.ts` around lines 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.
| expect(clonedCollider.shapes).toHaveLength(1); | ||
| expect(clonedShape.mesh).toBe(mesh); | ||
| expect((clonedShape as any)._nativeShape).toBeTruthy(); | ||
| expect((clonedShape as any)._isShapeAttached).toBe(true); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove assertions for the deleted _isShapeAttached field.
MeshColliderShape no longer defines this flag, so each expression evaluates to undefined and fails both toBe(true) and toBe(false). The adjacent native-shape count assertions already validate centralized attachment ownership.
Proposed fix
- expect((clonedShape as any)._isShapeAttached).toBe(true);
- expect((clonedShape as any)._isShapeAttached).toBe(false);
- expect((clonedShape as any)._isShapeAttached).toBe(true);
- expect((clonedShape as any)._isShapeAttached).toBe(false);Also applies to: 336-336, 343-343, 377-377
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/core/physics/MeshColliderShape.test.ts` at 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.
| function getLastContactEventDemandCall(calls: boolean[]): boolean { | ||
| return calls[calls.length - 1] ?? false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not treat a missing native call as false.
Returning false for an empty call list lets the disable-demand tests pass even when setContactEventEnabled was never invoked.
Proposed fix
-function getLastContactEventDemandCall(calls: boolean[]): boolean {
- return calls[calls.length - 1] ?? false;
+function getLastContactEventDemandCall(calls: boolean[]): boolean | undefined {
+ return calls[calls.length - 1];
}📝 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.
| function getLastContactEventDemandCall(calls: boolean[]): boolean { | |
| return calls[calls.length - 1] ?? false; | |
| } | |
| function getLastContactEventDemandCall(calls: boolean[]): boolean | undefined { | |
| return calls[calls.length - 1]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/core/physics/PhysicsScene.test.ts` around lines 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.
| expect(entity0.transform.position).to.deep.include({ x: 1, y: 2, z: 3 }); | ||
| expect(entity0.transform.rotation).to.deep.include({ x: 0, y: 45, z: 0 }); | ||
| expect(entity0.transform.scale).to.deep.include({ x: 1, y: 2, z: 3 }); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix floating-point precision assertion for rotation values.
CI shows y is 45.00000000000001 instead of 45 because rotation goes through euler → quaternion → euler conversion in _setTransform, which introduces floating-point error. deep.include performs exact comparison and fails.
🔧 Proposed fix: use approximate comparison for rotation
expect(entity0.transform.position).to.deep.include({ x: 1, y: 2, z: 3 });
- expect(entity0.transform.rotation).to.deep.include({ x: 0, y: 45, z: 0 });
+ expect(entity0.transform.rotation.x).to.be.approximately(0, 1e-6);
+ expect(entity0.transform.rotation.y).to.be.approximately(45, 1e-6);
+ expect(entity0.transform.rotation.z).to.be.approximately(0, 1e-6);
expect(entity0.transform.scale).to.deep.include({ x: 1, y: 2, z: 3 });📝 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.
| expect(entity0.transform.position).to.deep.include({ x: 1, y: 2, z: 3 }); | |
| expect(entity0.transform.rotation).to.deep.include({ x: 0, y: 45, z: 0 }); | |
| expect(entity0.transform.scale).to.deep.include({ x: 1, y: 2, z: 3 }); | |
| expect(entity0.transform.position).to.deep.include({ x: 1, y: 2, z: 3 }); | |
| expect(entity0.transform.rotation.x).to.be.approximately(0, 1e-6); | |
| expect(entity0.transform.rotation.y).to.be.approximately(45, 1e-6); | |
| expect(entity0.transform.rotation.z).to.be.approximately(0, 1e-6); | |
| expect(entity0.transform.scale).to.deep.include({ x: 1, y: 2, z: 3 }); |
🧰 Tools
🪛 GitHub Check: codecov
[failure] 120-120: src/core/Transform.test.ts > Transform test > Subclasses of Transform
AssertionError: expected Vector3{ …(4) } to have deep property 'y' of 45, but got 45.00000000000001
- Expected
- Received
- 45
- 45.00000000000001
❯ src/core/Transform.test.ts:120:47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/core/Transform.test.ts` around lines 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.
Source: Linters/SAST tools
| expect(entity1.transform.position).to.deep.include({ x: 4, y: 5, z: 6 }); | ||
| expect(entity1.transform.rotation).to.deep.include({ x: 0, y: 90, z: 0 }); | ||
| expect(entity1.transform.scale).to.deep.include({ x: 4, y: 5, z: 6 }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Same floating-point precision issue applies to the second rotation assertion.
Line 130 will fail for the same reason as line 120 — y: 90 will likely be 90.00000000000001 after quaternion conversion.
🔧 Proposed fix: use approximate comparison for rotation
expect(entity1.transform.position).to.deep.include({ x: 4, y: 5, z: 6 });
- expect(entity1.transform.rotation).to.deep.include({ x: 0, y: 90, z: 0 });
+ expect(entity1.transform.rotation.x).to.be.approximately(0, 1e-6);
+ expect(entity1.transform.rotation.y).to.be.approximately(90, 1e-6);
+ expect(entity1.transform.rotation.z).to.be.approximately(0, 1e-6);
expect(entity1.transform.scale).to.deep.include({ x: 4, y: 5, z: 6 });📝 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.
| expect(entity1.transform.position).to.deep.include({ x: 4, y: 5, z: 6 }); | |
| expect(entity1.transform.rotation).to.deep.include({ x: 0, y: 90, z: 0 }); | |
| expect(entity1.transform.scale).to.deep.include({ x: 4, y: 5, z: 6 }); | |
| expect(entity1.transform.position).to.deep.include({ x: 4, y: 5, z: 6 }); | |
| expect(entity1.transform.rotation.x).to.be.approximately(0, 1e-6); | |
| expect(entity1.transform.rotation.y).to.be.approximately(90, 1e-6); | |
| expect(entity1.transform.rotation.z).to.be.approximately(0, 1e-6); | |
| expect(entity1.transform.scale).to.deep.include({ x: 4, y: 5, z: 6 }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/core/Transform.test.ts` around lines 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.
Source: Linters/SAST tools
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
首次审查。PR 体量很大（41 文件），实际内容远超标题的 "align source-v2 and physics migration semantics"：物理硬化（collider/dynamic collider/physics material/mesh collider/physics scene）+ 可写粒子集合（ParticleCurve/Gradient/Emission 的 set 入口）+ glTF scene 子资产（scenes[n] 查询）+ 可配置物理 tolerances + Skin.glsl 里一整套 baked skinning 新路径（RENDERER_USE_BAKED_SKINNING）。CodeRabbit 已就物理/loader/测试逐行提了 inline，我不重复，只做两件事：补 CodeRabbit 漏掉的问题、校正被高估的几条，并提 PR 元信息层面的结构问题。
问题
[P1] PR 元信息：fix: 严重低估变更范围，多个正交功能域混装
标题挂 fix: 但本 PR 至少包含 4 个独立新功能域：
Skin.glsl新增RENDERER_USE_BAKED_SKINNINGbaked-skinning 采样路径（全新蒙皮特性，与"migration semantics"无关）- ParticleCurve/ParticleGradient/EmissionModule 新增
set keys/ 可写集合入口（新 API） - GLTFResource
scenes[n]子资产查询 +instantiateSceneRoot(sceneIndex)（新 API） - PhysXPhysics
tolerancesScale可配置（新 API）
fix: 前缀会误导 changelog 生成与回滚决策；一个 PR 里 4+ 正交域混装也让 review 注意力稀释、回滚粒度过粗（revert 一个丢全部）。建议：至少把 baked-skinning shader、glTF 子资产、粒子可写集合从"物理/migration 修复"里拆出，或把前缀改为体现主体的 feat:。这条只提一次。
[P2] packages/galacean/src/index.ts:18-24 — MathObjects 被重复注册两遍
新增代码里有两个内容完全相同的循环：
for (const key in MathObjects) { Loader.registerClass(key, MathObjects[key]); }
for (let key in MathObjects) { Loader.registerClass(key, MathObjects[key]); }明显是复制粘贴残留。运行时幂等无害（重复注册同 key 同类），但是死重复，应删掉第二个循环。（CodeRabbit 未覆盖此处。）
[P2] HierarchyParser → GLTFResource.instantiateSceneRoot 越界 scene index 崩溃（与 CodeRabbit HierarchyParser:317 同源，赞同）
GLTFResource.ts:62 this._sceneRoots[sceneIndex] 对越界 index 返回 undefined，:63 sceneRoot.clone() 直接 undefined.clone() 崩。sceneIndex 来自 parseGLTFSceneSelection(refItem.key) 解析序列化场景文件里的 ?q=scenes[N]，属系统边界的外部/序列化数据——迁移数据引用了不存在的 scene 应尽早报清晰错误，而非 cryptic 崩溃。CodeRabbit 已提，赞同，建议加 bounds 校验 + 描述性 error。
校正 CodeRabbit 几条（供作者判断优先级）
MeshColliderShape.ts:33事务性回滚（CodeRabbit 标 Major）— 属实，赞同。setter 已处理false返回路径（:32revert），但PhysXMeshColliderShape.setMeshData（:76-80setGeometrytry/catch 后throw error）确实会抛异常，抛出时跳过:32的 revert，_cookingFlags留在失败值、native 仍用旧 geometry → 状态不一致。CodeRabbit 的try/finally修法方向正确（这里 catch 的是 nativesetGeometry真会抛的异常，非防御性伪兜底）。PhysicsScene.ts:883null guard（CodeRabbit 标 Minor）— 现有调用流下不可达：_hasCollisionEventConsumers唯一调用点_syncContactEventDemand（_updatesubstep 顶层:664）在所有 script forEach 返回后、native step 前执行，此刻各 entity_scripts的 DisorderedArray_loopCounter===0无 null 空洞，scriptElements[j]恒非 null。作为未来防御可加，但不是现存 bug。PhysXPhysics.ts:88tolerances 校验（CodeRabbit 标 Minor）— 非"静默损坏"：构造期_updateScaledDefaults确实先用未校验值算了一遍 default，但对象要到 native init 才可用，_applyTolerancesScale（:340_assertPositiveFinite）会在初始化阶段抛错拦住非法 length/speed。至多 P3（校验挪到构造期能给更早/更准的报错点），非 P1。
结论
request-changes：确认存在真实 P1（mesh collider 事务性 throw 路径，CodeRabbit 已提）+ 越界 scene index 崩溃 + PR 范围/前缀问题。建议先处理事务性回滚与 bounds 校验，并对 PR 拆分/改前缀。物理迁移语义（kinematic sync、material combine、collision 事件按需）方向是对的。
2 participants
Summary
Validation
下落2048and保卫萝卜built against Engine 2.0.0-alpha.38 with runtime component validationSummary by CodeRabbit
scenes[n]queries.