Skip to content

Key compiled animation clip cache by identity, not asset name - #1220

Merged
untoldengine merged 3 commits into
developfrom
bugfix/fix_animation_clips
Sep 22, 2026
Merged

untoldengine merged 3 commits into
developfrom
bugfix/fix_animation_clips

Conversation

@untoldengine

Copy link
Copy Markdown
Owner

Summary

  • AnimationComponent.compiledClip(for:) cached compiled pose data keyed by AnimationClip.name, which comes from the asset file (e.g. the source Blender action name), not the lookup name passed to setEntityAnimations/changeAnimation.
  • Two independently exported clips (e.g. idle and running) commonly share the same embedded name, so the second clip's compiled data collided with the first's cache entry — changeAnimation correctly swapped currentAnimation, but playback stayed on the previously cached clip.
  • Fixes it by keying compiledClips on ObjectIdentifier(clip) instead of the name, so the cache is correct regardless of what name the source asset embeds. removeAnimationClip updated to match.

Test plan

  • swift build
  • swift test --filter AnimationCompiledSamplerTests (12/12 passing, including compiled-vs-legacy parity and the sampling performance baseline)

Independently exported clips can share the same asset-embedded name
(e.g. a default Blender action name), which caused the compiled-pose
cache to collide and silently serve one clip's compiled data for
another after changeAnimation switched to it.
@miogds

miogds commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Identity keys are the right fix. Two things worth closing while you're in there, plus one nice-to-have.

1. Stale entries can outlive their clip. registerRuntimeAnimationClips (RegistrationSystem.swift, the assignments at ~827 and ~836) writes into animationClips directly and never evicts compiledClips when it replaces an existing entry (the same embedded name registered twice, or a reload). CompiledAnimationClip does not retain its source clip, so the replaced AnimationClip is freed while its cache entry stays keyed on the old ObjectIdentifier. Swift reuses freed addresses, so a clip allocated later can inherit that identity and be served the dead clip's compiled data: the same failure, now nondeterministic instead of by name. The cheapest complete fix is pruning on mutation:

var animationClips: [String: AnimationClip] = [:] {
    didSet {
        let live = Set(animationClips.values.map(ObjectIdentifier.init))
        compiledClips = compiledClips.filter { live.contains($0.key) }
    }
}

removeAnimationClip then needs no special handling. Alternatively, have CompiledAnimationClip hold a strong reference to its source clip, which turns the hazard into a bounded leak instead of wrong data.

2. No regression test. Tests/UntoldEngineTests/AnimationCompiledSamplerTests.swift already builds clips from RuntimeAnimationClip (makeWalkClip / makeIdleClip), so the test is small: give both clips the same name, compile both through one AnimationComponent.compiledClip(for:skeleton:), and assert the channel data differs (one translation key from each is enough). A second case covers (1): register and compile a clip, replace it under the same key, allocate a new clip, compile it, and expect fresh data.

Nice-to-have: the preferredName alias creates a second AnimationClip from the same runtime clip, so with identity keys it now compiles twice. Registering the same instance under both names keeps a single compiled copy.

For the record, the CoolZombie clips are unaffected (each exported action is named after its clip), so this only bites assets whose actions kept a default name.

registerRuntimeAnimationClips overwrites animationClips[name] directly
rather than going through removeAnimationClip, so replacing a clip
(re-registration, reload, or a duplicate embedded name) freed the old
AnimationClip while its ObjectIdentifier-keyed compiledClips entry
stayed behind. CompiledAnimationClip holds no reference back to its
source clip, so if Swift reused the freed address for a later clip,
the stale entry could be served as that clip's compiled data.

Add a didSet on animationClips that prunes any compiledClips entry
whose key is no longer live, covering every mutation path instead of
only the removeAnimationClip case. Also have the preferredName alias
reuse the AnimationClip instance registered under the clip's own name
instead of allocating a second one, since identity-keyed compilation
would otherwise compile the same data twice.
@untoldengine
untoldengine merged commit 8ddbb93 into develop Sep 22, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants