fix: use deepClone for Skin._skinMatrices to prevent undefined after clone#2941
Conversation
When cloning a Skin object, _skinMatrices should be deep cloned instead of ignored. Using @ignoreClone causes the cloned Skin to share the same Float32Array reference with the original, leading to incorrect skinning behavior when the clone is modified. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2941 +/- ##
===========================================
+ Coverage 77.07% 77.12% +0.04%
===========================================
Files 899 899
Lines 98229 98229
Branches 9683 9693 +10
===========================================
+ Hits 75706 75755 +49
+ Misses 22355 22306 -49
Partials 168 168
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:
|
🤖 Augment PR SummarySummary: Fixes Skin cloning to avoid shared skinning matrix buffers between the original and its clone. 🤖 Was this summary useful? React with 👍 or 👎 |
|
|
||
| /** @internal */ | ||
| @ignoreClone | ||
| @deepClone |
There was a problem hiding this comment.
Consider adding a small regression test that Skin.clone() produces an independent _skinMatrices (i.e., not the same Float32Array reference), since this change is specifically fixing shared-state cloning bugs.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
将 Skin._skinMatrices 的 clone 装饰器从 @ignoreClone 改为 @deepClone,修复 clone 后 _skinMatrices 为 undefined 导致渲染异常的问题。改动一行,方向正确。
问题
-
[P2] packages/core/src/mesh/Skin.ts —
@deepClone对Float32Array会创建独立副本,这在语义上是正确的(每个 Skin 实例需要独立的 skinMatrices buffer)。但需要确认_skinMatrices的初始化时机:如果它是在update时按需分配的(lazy init),那么 clone 时源对象的_skinMatrices可能尚未初始化(仍为初始默认值),@deepClone会正确复制该初始值。反之,如果源对象已有一个正确 size 的Float32Array,clone 后 target 拿到的是一份快照,后续如果 target 的 bone 数量不同(通过 retarget),这个 buffer size 可能不匹配。建议确认 clone 后是否有 resize 逻辑覆盖此场景。 -
[P3] — PR 描述中的 test plan 全是 unchecked(
- [ ]),建议补齐或标注已通过。
简化建议
无,改动已经是最小的。
Summary
@ignoreCloneto@deepCloneforSkin._skinMatricespropertyFloat32Arrayfor skin matrices instead ofundefinedProblem
When cloning a Skin object,
_skinMatriceswas decorated with@ignoreClone. According to the clone logic inCloneManager.cloneProperty(), whenCloneMode.Ignoreis set, the property is skipped entirely during cloning. This results in the cloned Skin object having_skinMatricesasundefined, which causes rendering issues when the cloned SkinnedMeshRenderer tries to update skin matrices.Solution
Use
@deepCloneinstead to create a deep copy of theFloat32Array, ensuring each cloned Skin instance has its own properly initialized skin matrices array.Test plan
_skinMatrices🤖 Generated with Claude Code