fix(anim): support cloning components with dynamically-added layers#8947
Conversation
Cloning a component crashed when a layer had been added at runtime via addLayer(), because the clone rebuilt its layers from the (stale) state graph and then indexed layers that didn't exist. Resolve each layer by name when assigning animations, creating it if missing. Fixes #6395 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Build size reportThis PR changes the size of the minified bundles.
|
mvaligursky
left a comment
There was a problem hiding this comment.
🤖 Automated PR review — posted on my behalf by Claude Code (Opus 4.8). Not a human review; flagging for awareness.
Verdict: looks correct and well-scoped. No blocking issues.
I verified the core assumption: AnimComponent.addLayer() (src/framework/components/anim/component.js:383) is idempotent — it calls findAnimationLayer(name) and returns the existing layer if present, only creating one otherwise. So resolving the clone's layer by name handles both paths cleanly:
- state-graph / Editor-driven layers — already recreated by
loadStateGraph(), soaddLayer()returns the existing instance (theweight/mask/blendTypeargs are ignored, which is fine since the existing layer already carries the graph's values). - dynamically-added layers — absent from
_stateGraph, soaddLayer()creates them with the source layer'sweight/mask/blendType, which is exactly what the old index-based code couldn't do.
Other angles checked:
- Breaking changes: none. Behavior is identical for the existing all-layers-present path.
- Layer ordering / index integrity: preserved — iteration is in
data.layersorder and_layerIndicesstays consistent. - Performance: negligible (one extra name lookup per layer at clone time).
- Test: the regression test mirrors the issue repro and also adds a single-layer guard. Good coverage of the additive blend-type round-trip and independent-instance check.
Minor (optional, non-blocking): the weight/mask/blendType passed to addLayer() are silently dropped for layers that already exist. That's correct today, but a short comment noting "args only apply when the layer is newly created here" could save a future reader a double-take.
Fixes #6395
Cloning an entity whose
animcomponent had a layer added at runtime viaaddLayer()(rather than via a loaded state graph) threwTypeError: Cannot read properties of undefined (reading 'assignAnimation'). This reproduces by opening theanimation/layer-masksexample and appendingmodelEntity.clone().Cause: layers (and states) added dynamically via
addLayer()/assignAnimation()are not recorded in the component's_stateGraph. On clone,initializeComponentDatarebuilds the clone's layers from that stale state graph, then iterates the source's live layers doingcomponent.layers[i].assignAnimation(...)— for any layer missing from the state graph,component.layers[i]isundefined.Fix: resolve the clone's layer by name (creating it via
addLayerif the state graph didn't recreate it) and assign onto that layer instead of the index. No behavior change for the state-graph/Editor-driven path, where all layers already exist.Verified with a new regression test in
test/framework/components/anim/component.test.mjsthat mirrors the issue's repro (base layer + a dynamically-added additive second layer). It throws the documented error before the fix and passes after.🤖 Generated with Claude Code