Retarget scene AnimationGroups to mesh skeletons and handle group/mesh inputs in _switchToAnimationModel#562
Conversation
Deploying flockxr with
|
| Latest commit: |
b29fbcd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3b032c88.flockxr.pages.dev |
| Branch Preview URL: | https://codex-fix-animation-error-fo.flockxr.pages.dev |
📝 WalkthroughWalkthroughAnimation switching now awaits Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant whenReady as whenModelReady
participant Flock as flock.switchToAnimation
participant Resolver as MeshResolver
participant Scene as Scene.animationGroups
participant Cache as rootMesh.metadata.embeddedAnimationGroups
participant Retarget as RetargetEngine
participant Animator as rootMesh.animationGroups[0]
Caller->>whenReady: request switchAnimation(meshOrGroup, name)
whenReady->>Flock: await flock.switchToAnimation(...)
whenReady->>Resolver: resolve meshOrGroup -> rootMesh (skeleton)
Resolver->>Scene: search for matching AnimationGroup (exact/suffix)
alt Scene group found
Scene->>Animator: use scene group
else Scene group not found
Resolver->>Cache: check embeddedAnimationGroups[name]
alt cached group found
Cache->>Animator: use cached group
else not cached
Resolver->>Retarget: find source group, map bones by name, clone animations
Retarget->>Cache: if targeted animations -> cache new group
Retarget->>Animator: assign new retargeted group
end
end
Animator->>Animator: stop/reset/start as appropriate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
api/animate.js (4)
1625-1628:⚠️ Potential issue | 🟠 MajorUpdate the capsule on the physics owner, not always the skinned child.
After resolving
meshto a child skeleton mesh, parent-level physics capsules are skipped here. This diverges from_switchToAnimationLoad, which updatesmeshOrGroup.🐛 Proposed fix
// Update physics shape based on animation - const physicsMesh = mesh; + const physicsMesh = meshOrGroup?.physics ? meshOrGroup : mesh; updateCapsuleShapeForAnimation(physicsMesh, animationName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/animate.js` around lines 1625 - 1628, The code always passes the resolved child skeleton mesh (mesh) into updateCapsuleShapeForAnimation, which skips parent-level physics capsules; instead locate the physics owner (the parent/group used elsewhere as meshOrGroup in _switchToAnimationLoad) and call updateCapsuleShapeForAnimation with that owner rather than the skinned child—e.g., if mesh is a Skeleton/skinned mesh, walk up to the parent/group that holds the physics capsule or reuse meshOrGroup, then call updateCapsuleShapeForAnimation(physicsOwner, animationName) so parent-level capsules are updated.
1527-1531:⚠️ Potential issue | 🟠 MajorInclude animation groups that target the resolved mesh directly.
This lookup only matches descendant targets, so a scene
AnimationGroupnamednewAnimationNamethat targetsmeshitself is skipped and may fail the retarget fallback.🐛 Proposed fix
+ const groupTargetsMeshOrDescendant = (group, targetMesh) => + group.targetedAnimations?.some(({ target }) => target === targetMesh) || + (typeof targetMesh?.getDescendants === "function" && + flock._animationGroupTargetsDescendant(group, targetMesh)); + let targetAnimationGroup = flock.scene?.animationGroups?.find( (group) => group.name === newAnimationName && - flock._animationGroupTargetsDescendant(group, mesh), + groupTargetsMeshOrDescendant(group, mesh), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/animate.js` around lines 1527 - 1531, The current find predicate for targetAnimationGroup only checks descendant targets via flock._animationGroupTargetsDescendant and thus skips AnimationGroups that target mesh itself; update the predicate so it also matches groups that target the resolved mesh directly (i.e., change the condition to true when flock._animationGroupTargetsDescendant(group, mesh) OR the group directly targets mesh). Use the existing group metadata (e.g., group.targetedAnimations/targets) or add/call a helper like flock._animationGroupTargetsDirect(group, mesh) to detect direct targets, and apply that to the find used to assign targetAnimationGroup.
1599-1613:⚠️ Potential issue | 🟠 MajorCompare the active group object, not the original animation name.
Retargeted groups are named like
${mesh.name}.${newAnimationName}, so Line 1601 treats the same cached retargeted animation as different and ignoresrestart = false.🐛 Proposed fix
if ( mesh.animationGroups[0] && - mesh.animationGroups[0].name !== newAnimationName + mesh.animationGroups[0] !== targetAnimationGroup ) { flock.stopAnimationsTargetingMesh(scene, mesh); mesh.animationGroups[0].stop(); mesh.animationGroups = []; } if ( !mesh.animationGroups[0] || - (mesh.animationGroups[0].name == newAnimationName && restart) + mesh.animationGroups[0] !== targetAnimationGroup || + restart ) { flock.stopAnimationsTargetingMesh(scene, mesh); mesh.animationGroups[0] = targetAnimationGroup;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/animate.js` around lines 1599 - 1613, The code is comparing animation group names (mesh.animationGroups[0].name) to newAnimationName which fails for retargeted groups; instead compare the active group object to the target group object. Change the checks around mesh.animationGroups[0] to use object identity with targetAnimationGroup (e.g., mesh.animationGroups[0] !== targetAnimationGroup and mesh.animationGroups[0] === targetAnimationGroup) and guard for existence before accessing [0], keeping calls to flock.stopAnimationsTargetingMesh and assignments to mesh.animationGroups[0] as-is so restart/skip logic works correctly.
1509-1522:⚠️ Potential issue | 🟠 MajorFix the missing-mesh guard before it throws.
Line 1521 dereferences
mesh.nameinsideif (!mesh), so a missing input throws instead of returningnull.🐛 Proposed fix
const mesh = findMeshWithSkeleton(meshOrGroup); if (!mesh) { - console.error(`Mesh ${mesh.name} not found.`); + console.error(`Mesh ${meshOrGroup?.name ?? "<unknown>"} not found.`); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/animate.js` around lines 1509 - 1522, The guard after calling findMeshWithSkeleton incorrectly dereferences mesh.name when mesh can be null; update the not-found branch to avoid using mesh when falsy and report a safe identifier instead. Specifically, in the block that checks if (!mesh) replace console.error(`Mesh ${mesh.name} not found.`) with a safe message that uses meshOrGroup?.name or a fallback (e.g. 'unknown') or simply "Mesh not found", then return null; keep findMeshWithSkeleton and the mesh assignment unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/animate.js`:
- Around line 1625-1628: The code always passes the resolved child skeleton mesh
(mesh) into updateCapsuleShapeForAnimation, which skips parent-level physics
capsules; instead locate the physics owner (the parent/group used elsewhere as
meshOrGroup in _switchToAnimationLoad) and call updateCapsuleShapeForAnimation
with that owner rather than the skinned child—e.g., if mesh is a
Skeleton/skinned mesh, walk up to the parent/group that holds the physics
capsule or reuse meshOrGroup, then call
updateCapsuleShapeForAnimation(physicsOwner, animationName) so parent-level
capsules are updated.
- Around line 1527-1531: The current find predicate for targetAnimationGroup
only checks descendant targets via flock._animationGroupTargetsDescendant and
thus skips AnimationGroups that target mesh itself; update the predicate so it
also matches groups that target the resolved mesh directly (i.e., change the
condition to true when flock._animationGroupTargetsDescendant(group, mesh) OR
the group directly targets mesh). Use the existing group metadata (e.g.,
group.targetedAnimations/targets) or add/call a helper like
flock._animationGroupTargetsDirect(group, mesh) to detect direct targets, and
apply that to the find used to assign targetAnimationGroup.
- Around line 1599-1613: The code is comparing animation group names
(mesh.animationGroups[0].name) to newAnimationName which fails for retargeted
groups; instead compare the active group object to the target group object.
Change the checks around mesh.animationGroups[0] to use object identity with
targetAnimationGroup (e.g., mesh.animationGroups[0] !== targetAnimationGroup and
mesh.animationGroups[0] === targetAnimationGroup) and guard for existence before
accessing [0], keeping calls to flock.stopAnimationsTargetingMesh and
assignments to mesh.animationGroups[0] as-is so restart/skip logic works
correctly.
- Around line 1509-1522: The guard after calling findMeshWithSkeleton
incorrectly dereferences mesh.name when mesh can be null; update the not-found
branch to avoid using mesh when falsy and report a safe identifier instead.
Specifically, in the block that checks if (!mesh) replace console.error(`Mesh
${mesh.name} not found.`) with a safe message that uses meshOrGroup?.name or a
fallback (e.g. 'unknown') or simply "Mesh not found", then return null; keep
findMeshWithSkeleton and the mesh assignment unchanged.
Deploying flockdev with
|
| Latest commit: |
b29fbcd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e7c29ba9.flockdev.pages.dev |
| Branch Preview URL: | https://codex-fix-animation-error-fo.flockdev.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/animate.js (2)
1-1:⚠️ Potential issue | 🟡 MinorPrettier formatting check failing.
Pipeline reports a Prettier warning on this file. Please run
npx prettier --write api/animate.js(or the project's equivalent) and commit the result so CI passes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/animate.js` at line 1, The file api/animate.js is failing Prettier formatting; run the project's formatter (e.g., npx prettier --write api/animate.js) to reformat the file and commit the changes so CI passes; ensure the import statement (import { blockNames, modelAnimationNames } from "../config.js";) and the rest of api/animate.js adhere to the project's Prettier rules before pushing.
114-126:⚠️ Potential issue | 🟡 MinorMissing
meshnull guard before callingswitchToAnimation.If
whenModelReadyyields a falsy mesh,flock.switchToAnimation(...)will throw aTypeErroronmeshOrGroup.metadata?.modelName(line 1472) — the surroundingnew Promisethen never resolves because the error is swallowed by theasynccallback.playAnimationjust above already guards this case; please mirror that here:Proposed fix
flock.whenModelReady(meshName, async (mesh) => { + if (!mesh) { + console.error(`Mesh "${meshName}" not found.`); + resolve(); + return; + } await Promise.resolve( flock.switchToAnimation( flock.scene, mesh, animationName, loop, restart, ), ); resolve(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/animate.js` around lines 114 - 126, The callback passed to flock.whenModelReady must check for a falsy mesh before calling flock.switchToAnimation to avoid the TypeError; mirror the guard used in playAnimation by early-returning (and resolving the outer promise) when mesh is null/undefined, e.g., inside the whenModelReady callback check if mesh is falsy and call resolve() then return, otherwise await flock.switchToAnimation(flock.scene, mesh, animationName, loop, restart) and then resolve(); ensure you reference the whenModelReady callback, flock.switchToAnimation call, and the outer Promise resolution flow so the promise always settles even when mesh is missing.
🧹 Nitpick comments (1)
api/animate.js (1)
1503-1654: Return type is now conditionally async.
_switchToAnimationModelreturns either anAnimationGroupsynchronously or aPromise<AnimationGroup>(via the_switchToAnimationLoadfallback on line 1607). Callers already compensate withPromise.resolve(...), so this isn't broken, but declaring the functionasync(andawaiting the fallback) would make the contract uniform and prevent future callers from forgetting the wrapper.- _switchToAnimationModel( + async _switchToAnimationModel( scene, meshOrGroup, ... ) { ... if (!targetAnimationGroup) { - return flock._switchToAnimationLoad( + return await flock._switchToAnimationLoad( scene, rootMesh, newAnimationName, loop, restart, true, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/animate.js` around lines 1503 - 1654, _switchToAnimationModel currently returns either an AnimationGroup synchronously or a Promise when it falls back to flock._switchToAnimationLoad; make its return contract uniform by declaring _switchToAnimationModel as async and awaiting the flock._switchToAnimationLoad call (use await flock._switchToAnimationLoad(...)) so it always returns a Promise<AnimationGroup|null>, leaving other synchronous returns (e.g., return null, or returning targetAnimationGroup) intact (they will auto-wrap in a Promise); update any local references to animationName/newAnimationName and keep the call to updateCapsuleShapeForAnimation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/animate.js`:
- Around line 1511-1526: The code currently aliases rootMesh = meshOrGroup and
computes skeletonMesh via findMeshWithSkeleton but never guards against a
missing skeletonMesh, causing silent fall-through and unintended fetches in
_switchToAnimationLoad; change the flow in the function containing
findMeshWithSkeleton/rootMesh/skeletonMesh so that after computing skeletonMesh
you check if (!skeletonMesh) then log a clear error that includes
meshOrGroup?.name and whether the node was a group with no skinned descendant,
and return null (or otherwise abort) before any use of
skeletonMesh?.skeleton?.bones or calling _switchToAnimationLoad; keep
findMeshWithSkeleton as the locator and ensure the early-return prevents
disposal/empty animation groups and downstream 404 fetches.
---
Outside diff comments:
In `@api/animate.js`:
- Line 1: The file api/animate.js is failing Prettier formatting; run the
project's formatter (e.g., npx prettier --write api/animate.js) to reformat the
file and commit the changes so CI passes; ensure the import statement (import {
blockNames, modelAnimationNames } from "../config.js";) and the rest of
api/animate.js adhere to the project's Prettier rules before pushing.
- Around line 114-126: The callback passed to flock.whenModelReady must check
for a falsy mesh before calling flock.switchToAnimation to avoid the TypeError;
mirror the guard used in playAnimation by early-returning (and resolving the
outer promise) when mesh is null/undefined, e.g., inside the whenModelReady
callback check if mesh is falsy and call resolve() then return, otherwise await
flock.switchToAnimation(flock.scene, mesh, animationName, loop, restart) and
then resolve(); ensure you reference the whenModelReady callback,
flock.switchToAnimation call, and the outer Promise resolution flow so the
promise always settles even when mesh is missing.
---
Nitpick comments:
In `@api/animate.js`:
- Around line 1503-1654: _switchToAnimationModel currently returns either an
AnimationGroup synchronously or a Promise when it falls back to
flock._switchToAnimationLoad; make its return contract uniform by declaring
_switchToAnimationModel as async and awaiting the flock._switchToAnimationLoad
call (use await flock._switchToAnimationLoad(...)) so it always returns a
Promise<AnimationGroup|null>, leaving other synchronous returns (e.g., return
null, or returning targetAnimationGroup) intact (they will auto-wrap in a
Promise); update any local references to animationName/newAnimationName and keep
the call to updateCapsuleShapeForAnimation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const findMeshWithSkeleton = (candidateRoot) => { | ||
| if (candidateRoot?.skeleton) return candidateRoot; | ||
| if (candidateRoot?.getChildMeshes) { | ||
| for (const child of candidateRoot.getChildMeshes()) { | ||
| if (child.skeleton) return child; | ||
| } | ||
| } | ||
| return null; | ||
| }; | ||
| const rootMesh = meshOrGroup; | ||
| const skeletonMesh = findMeshWithSkeleton(rootMesh); | ||
|
|
||
| if (!mesh) { | ||
| console.error(`Mesh ${mesh.name} not found.`); | ||
| if (!rootMesh) { | ||
| console.error(`Mesh ${rootMesh?.name} not found.`); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Null check is effectively dead; the meaningful skeleton guard was dropped.
rootMesh is just an alias for meshOrGroup, and by the time we reach this block switchToAnimation (line 1472) already dereferenced meshOrGroup.metadata?.modelName — so !rootMesh can never be true here, and the error message logs Mesh undefined not found. when it does fire.
More importantly, skeletonMesh can legitimately be null (group passed in without a skinned descendant), and the function now silently proceeds: skeletonMesh?.skeleton?.bones yields an empty boneMap, and unless descendants happen to match source target names by coincidence the resulting group has zero targeted animations, gets disposed (line 1600), and the function falls through to _switchToAnimationLoad, which will attempt to fetch ./animations/<name>.glb — potentially a 404 for model-bundled animations.
Consider guarding on skeletonMesh (or at least on the absence of both skeleton and descendants) and logging a meaningful message:
Proposed fix
const rootMesh = meshOrGroup;
const skeletonMesh = findMeshWithSkeleton(rootMesh);
- if (!rootMesh) {
- console.error(`Mesh ${rootMesh?.name} not found.`);
+ if (!rootMesh) {
+ console.error(`_switchToAnimationModel: no mesh/group provided.`);
+ return null;
+ }
+ if (!skeletonMesh) {
+ console.error(
+ `_switchToAnimationModel: no skeleton found under '${rootMesh.name}'.`,
+ );
return null;
}📝 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.
| const findMeshWithSkeleton = (candidateRoot) => { | |
| if (candidateRoot?.skeleton) return candidateRoot; | |
| if (candidateRoot?.getChildMeshes) { | |
| for (const child of candidateRoot.getChildMeshes()) { | |
| if (child.skeleton) return child; | |
| } | |
| } | |
| return null; | |
| }; | |
| const rootMesh = meshOrGroup; | |
| const skeletonMesh = findMeshWithSkeleton(rootMesh); | |
| if (!mesh) { | |
| console.error(`Mesh ${mesh.name} not found.`); | |
| if (!rootMesh) { | |
| console.error(`Mesh ${rootMesh?.name} not found.`); | |
| return null; | |
| } | |
| const findMeshWithSkeleton = (candidateRoot) => { | |
| if (candidateRoot?.skeleton) return candidateRoot; | |
| if (candidateRoot?.getChildMeshes) { | |
| for (const child of candidateRoot.getChildMeshes()) { | |
| if (child.skeleton) return child; | |
| } | |
| } | |
| return null; | |
| }; | |
| const rootMesh = meshOrGroup; | |
| const skeletonMesh = findMeshWithSkeleton(rootMesh); | |
| if (!rootMesh) { | |
| console.error(`_switchToAnimationModel: no mesh/group provided.`); | |
| return null; | |
| } | |
| if (!skeletonMesh) { | |
| console.error( | |
| `_switchToAnimationModel: no skeleton found under '${rootMesh.name}'.`, | |
| ); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/animate.js` around lines 1511 - 1526, The code currently aliases rootMesh
= meshOrGroup and computes skeletonMesh via findMeshWithSkeleton but never
guards against a missing skeletonMesh, causing silent fall-through and
unintended fetches in _switchToAnimationLoad; change the flow in the function
containing findMeshWithSkeleton/rootMesh/skeletonMesh so that after computing
skeletonMesh you check if (!skeletonMesh) then log a clear error that includes
meshOrGroup?.name and whether the node was a group with no skinned descendant,
and return null (or otherwise abort) before any use of
skeletonMesh?.skeleton?.bones or calling _switchToAnimationLoad; keep
findMeshWithSkeleton as the locator and ensure the early-return prevents
disposal/empty animation groups and downstream 404 fetches.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/models.js (1)
360-396:⚠️ Potential issue | 🟠 MajorMissing rejection path leaks
modelsBeingLoadedand hangsreadyPromise.
loadPromise.then(...)has no.catch, andresolveReady/delete flock.modelsBeingLoaded[modelName]only run on success. If the loader rejects (bad URL, aborted fetch, parse failure), every caller awaitingmodelReadyPromises.get(meshName)— including the newly-addedawait flock.switchToAnimation(...)in the model-ready path — will wait forever, and a subsequentcreateObject(modelName, ...)will branch intomodelsBeingLoaded[modelName].then(...)on a stale settled promise and never repopulatemodelCache. The outertry/catchat line 399 only covers the synchronous setup.🛡️ Proposed fix
- loadPromise.then((container) => { + loadPromise + .then((container) => { container.addAllToScene(); // ... finalizeMesh(mesh, meshName, groupName, bKey); resolveReady(mesh); releaseContainer(container); delete flock.modelsBeingLoaded[modelName]; - }); + }) + .catch((error) => { + console.error("createObject load failed:", error); + delete flock.modelsBeingLoaded[modelName]; + flock.modelReadyPromises.delete(meshName); + flock._releaseName?.(meshName); + // Surface the failure to awaiters instead of hanging forever + resolveReady(null); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/models.js` around lines 360 - 396, The loadPromise.then block has no rejection path, which leaks modelsBeingLoaded and leaves callers waiting; add a .catch handler on loadPromise that cleans up on error: releaseContainer(container) if defined, delete flock.modelsBeingLoaded[modelName], and reject the associated ready promise (the counterpart to resolveReady) with the loader error so callers awaiting modelReadyPromises.get(meshName) fail fast; ensure you only call flock.modelCache[modelName] assignment, flock._registerInstance, finalizeMesh, resolveReady and any material/mesh cloning inside the success path so failures never leave partial state.
🧹 Nitpick comments (2)
api/models.js (2)
329-343: Duplicate root-mesh name during the brief pre-setupMeshwindow.Both cache-hit paths clone with
flock.modelCache[modelName].name, which is identical to the still-in-scene template root's name. Between theclone()call andfinalizeMesh→setupMeshrename, two scene nodes share that name, so anyscene.getMeshByName(...)(or name-keyed lookup) fired during that window — e.g. from an event handler or another concurrentcreateObject— can resolve to the template (which isisEnabled(false)/isVisible=false) instead of the new instance. PassingmeshName(already reserved on line 314) directly intocloneavoids the ambiguity entirely:- const mesh = flock.modelCache[modelName].clone( - flock.modelCache[modelName].name, - ); + const mesh = flock.modelCache[modelName].clone(meshName);Same change applies at lines 341–343 and 389.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/models.js` around lines 329 - 343, The cloned mesh temporarily shares the template root's name because clone(...) is called with flock.modelCache[modelName].name, causing scene name collisions before finalizeMesh/setupMesh renames it; change both cache-hit clone calls (those inside the flock.modelsBeingLoaded[...] .then(...) path and the earlier cache-return path) to call clone(meshName) instead of clone(flock.modelCache[modelName].name), keeping the reserved meshName (registered via _registerInstance) as the new instance's name and avoiding getMeshByName races; apply the same replacement at the other occurrence noted (around the later occurrence at line ~389) so all clones use meshName.
250-275: Unconditionally clone instance metadata to prevent shared mutations.The asymmetry in
setInstanceFlagscreates a vulnerability: since Babylon'sclone()performs shallow copy of metadata, instance nodes whose metadata object was never explicitly flagged as template would still reference the template's metadata object. Any subsequent writes—likeembeddedAnimationGroupsat line 1596 in api/animate.js—would mutate the template's cached metadata.While
setTemplateFlagscomprehensively marks all descendants viagetDescendants(false), the defensive approach is to clone metadata unconditionally whenever it exists:list.forEach((m) => { - if (m.metadata?.isTemplate) { - m.metadata = { ...m.metadata, isTemplate: false }; - } + if (m.metadata) { + m.metadata = { ...m.metadata, isTemplate: false }; + } if ("isPickable" in m) m.isPickable = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/models.js` around lines 250 - 275, The issue is that setInstanceFlags leaves existing metadata objects shared with template nodes; update setInstanceFlags so that for every node in the list with an existing metadata object you replace it with a shallow clone (e.g., {...m.metadata}) before flipping isTemplate to false, ensuring no descendant retains a reference to the template metadata; reference the setInstanceFlags function and the list iteration where m.metadata is read/modified to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/models.js`:
- Around line 386-393: The cloned skinned-mesh instances currently share the
original skeleton causing all instances to animate together; after creating mesh
via root.clone(root.name) (see use near _registerInstance, finalizeMesh,
resolveReady) ensure you clone the skeleton per instance when root.skeleton
exists and assign it to mesh.skeleton (e.g., clone root.skeleton with a unique
name based on mesh.name). Also add a .catch() on the loadPromise.then() path to
handle failures: on error reject/resolve/cleanup modelReadyPromises[meshName]
and remove modelsBeingLoaded[modelName] so promises don't hang and loading state
is cleared.
---
Outside diff comments:
In `@api/models.js`:
- Around line 360-396: The loadPromise.then block has no rejection path, which
leaks modelsBeingLoaded and leaves callers waiting; add a .catch handler on
loadPromise that cleans up on error: releaseContainer(container) if defined,
delete flock.modelsBeingLoaded[modelName], and reject the associated ready
promise (the counterpart to resolveReady) with the loader error so callers
awaiting modelReadyPromises.get(meshName) fail fast; ensure you only call
flock.modelCache[modelName] assignment, flock._registerInstance, finalizeMesh,
resolveReady and any material/mesh cloning inside the success path so failures
never leave partial state.
---
Nitpick comments:
In `@api/models.js`:
- Around line 329-343: The cloned mesh temporarily shares the template root's
name because clone(...) is called with flock.modelCache[modelName].name, causing
scene name collisions before finalizeMesh/setupMesh renames it; change both
cache-hit clone calls (those inside the flock.modelsBeingLoaded[...] .then(...)
path and the earlier cache-return path) to call clone(meshName) instead of
clone(flock.modelCache[modelName].name), keeping the reserved meshName
(registered via _registerInstance) as the new instance's name and avoiding
getMeshByName races; apply the same replacement at the other occurrence noted
(around the later occurrence at line ~389) so all clones use meshName.
- Around line 250-275: The issue is that setInstanceFlags leaves existing
metadata objects shared with template nodes; update setInstanceFlags so that for
every node in the list with an existing metadata object you replace it with a
shallow clone (e.g., {...m.metadata}) before flipping isTemplate to false,
ensuring no descendant retains a reference to the template metadata; reference
the setInstanceFlags function and the list iteration where m.metadata is
read/modified to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| setTemplateFlags(root, modelName); | ||
| flock.modelCache[modelName] = root; | ||
|
|
||
| const mesh = root.clone(root.name); | ||
| flock._registerInstance(modelName, meshName); | ||
|
|
||
| finalizeMesh(root, meshName, groupName, bKey); | ||
| resolveReady(root); | ||
| finalizeMesh(mesh, meshName, groupName, bKey); | ||
| resolveReady(mesh); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how setupMesh and _switchToAnimationModel treat skeletons on cloned meshes.
fd -e js -e ts -x rg -nP -C4 '\b(setupMesh|_switchToAnimationModel|findMeshWithSkeleton|embeddedAnimationGroups)\b' {}
echo '--- skeleton clone sites ---'
rg -nP -C2 '\bskeleton\s*\.\s*clone\b|\.skeleton\s*=\s*'Repository: flipcomputing/flock
Length of output: 50377
🏁 Script executed:
# Get setupMesh function definition (lines 692+)
sed -n '692,750p' api/models.js
# Get _switchToAnimationModel function (lines 1503+)
sed -n '1503,1650p' api/models.js
# Check if skeleton is cloned anywhere in api/models.js
rg -n 'skeleton\s*\.\s*clone|\.skeleton\s*=' api/models.jsRepository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# First check file exists and get line count
wc -l api/models.js
# Read setupMesh and related functions
head -n 800 api/models.js | tail -n 150Repository: flipcomputing/flock
Length of output: 4394
🏁 Script executed:
# Search for setupMesh, _switchToAnimationModel, finalizeMesh in api/models.js
rg -n 'setupMesh|_switchToAnimationModel|finalizeMesh|setTemplateFlags|setInstanceFlags' api/models.js
# Also check for any skeleton-related code
rg -n 'skeleton' api/models.jsRepository: flipcomputing/flock
Length of output: 741
🏁 Script executed:
# Find where setupMesh is defined
rg -n 'setupMesh\s*\(' --type js --type ts | head -20
# Find where _switchToAnimationModel is defined
rg -n '_switchToAnimationModel\s*\(' --type js --type ts | head -20
# Search for skeleton clone patterns
rg -n 'skeleton\s*\.\s*clone|skeleton.*clone|linkedTransformNode' --type js --type tsRepository: flipcomputing/flock
Length of output: 50377
🏁 Script executed:
# Read the entire api/models.js file
cat api/models.jsRepository: flipcomputing/flock
Length of output: 13959
🏁 Script executed:
# Get setupMesh function from api/mesh.js
sed -n '692,750p' api/mesh.js
# Also check for any skeleton cloning in api/animate.js around _switchToAnimationModel
sed -n '1500,1650p' api/animate.jsRepository: flipcomputing/flock
Length of output: 6186
🏁 Script executed:
# Search for skeleton cloning or skeleton assignment in entire codebase
rg -n 'skeleton\s*=|skeleton.*clone|linkedTransformNode.*clone' --type js --type ts
# Check if there are any other files that handle skeleton management
fd -e js -e ts | xargs rg -l 'skeleton' | head -20Repository: flipcomputing/flock
Length of output: 50377
Clone each skinned-mesh instance with its own skeleton — shared bones currently animate all instances together.
root.clone(root.name) (lines 329, 341, 389) uses Babylon's default clone behavior, which shares the skeleton reference between the original and all clones. Since _switchToAnimationModel retargets animations onto bones from this shared skeleton, playing an animation on any instance will visibly animate all other instances using the same template.
Fix: After cloning, clone the skeleton per instance. For example:
const mesh = root.clone(root.name);
if (root.skeleton) {
mesh.skeleton = root.skeleton.clone(`${mesh.name}_skel`);
}Also missing: loadPromise.then() (lines 360+) has no .catch() handler. If loading fails, modelReadyPromises[meshName] will hang forever and modelsBeingLoaded[modelName] will not be cleaned up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/models.js` around lines 386 - 393, The cloned skinned-mesh instances
currently share the original skeleton causing all instances to animate together;
after creating mesh via root.clone(root.name) (see use near _registerInstance,
finalizeMesh, resolveReady) ensure you clone the skeleton per instance when
root.skeleton exists and assign it to mesh.skeleton (e.g., clone root.skeleton
with a unique name based on mesh.name). Also add a .catch() on the
loadPromise.then() path to handle failures: on error reject/resolve/cleanup
modelReadyPromises[meshName] and remove modelsBeingLoaded[modelName] so promises
don't hang and loading state is cleared.
1 participant
Motivation
AnimationGroups can be applied to imported/skinned meshes whose skeletons are not directly targeted by those groups._switchToAnimationModelto accept either a mesh or a parent group and find the actual skinned mesh to operate on.Description
_switchToAnimationModelparameter to acceptmeshOrGroupand addedfindMeshWithSkeletonto locate the actual mesh with askeletonamong children.AnimationGrouptargets the mesh, added logic to retarget a sourceAnimationGrouponto the mesh's bones/linked transform nodes by cloning animations and creating a newAnimationGroupnamed${mesh.name}.${animationName}.mesh.metadata.embeddedAnimationGroupsand reused them when available, and disposed of empty retargeted groups.mesh.animationGroups[0]to the resolved target group, and added conciseconsole.errormessages for missing mesh or animation cases.Testing
npm test, which completed successfully.AnimationGroupis created and played successfully.Codex Task
Summary by CodeRabbit