Skip to content

Fix inverted entity name guard in AnimComponentBinder - #9187

Merged
willeastcott merged 4 commits into
mainfrom
fix-anim-binder-entity-name-guard
Aug 19, 2026
Merged

Fix inverted entity name guard in AnimComponentBinder#9187
willeastcott merged 4 commits into
mainfrom
fix-anim-binder-entity-name-guard

Conversation

@willeastcott

@willeastcott willeastcott commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Description

AnimComponentBinder._getEntityFromHierarchy was written as if (!this.animComponent.entity.name === entityHierarchy[0]), which coerces the name to a boolean before comparing it to a string, so the guard has never fired since its introduction in #2007. Anim tracks targeting entity or component properties were bound without ever validating that the path is rooted at the anim component's entity.

Changes

  • Single element entity paths now bind the anim component's own entity without a root name check — the only entity such a path can refer to. This matches the previous runtime behavior, so content that renamed the entity or reuses clips across differently named entities keeps working.
  • Multi element entity paths validate that the root element matches the anim component's entity name (the intent of the original guard). On mismatch the curve is left unbound and a debug-build Debug.warnOnce names the authored path and the entity, instead of resolving through currEntity._parent.findByPath(...) into a sibling entity's subtree or throwing.
  • Restore null handling in resolve() after both _getEntityFromHierarchy call sites (present in the original implementation, dropped in the [Animation] Binding updates and bug fixes #2772 refactor). An unresolved path — including a matching root with a missing descendant — now returns null (curve left unbound, per the AnimEvaluator contract) instead of throwing.
  • Add regression tests covering the resolution conventions and the failure modes.

Behavioral consequences

  • A multi element path whose root did not name a sibling of the component's entity previously threw a TypeError during binding; it now unbinds gracefully (with a debug warning) and can bind on a later rebind — e.g. when the target entity is created after the animation starts.
  • The one deliberate break: a multi element path rooted at a sibling entity's name used to resolve into that sibling's subtree and animate it. That contradicted the binder's model (rebind, the graph and layer masks all assume the component's own hierarchy) and is now rejected. The debug warning makes affected content easy to identify.
  • Skeletal/morph (graph) tracks resolve through DefaultAnimBinder.findNode and its fallback chain, which this change does not touch.

Checklist

  • I have read the contributing guidelines
  • My code follows the project's coding standards
  • This PR focuses on a single change

🤖 Generated with Claude Code

The guard in _getEntityFromHierarchy negated the entity name before
comparing it to the path root, so it never fired. Fix the comparison and
restore null handling in resolve() so unresolved entity paths leave
curves unbound instead of misbinding or throwing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

Build size report

This PR changes the size of the minified bundles.

Bundle Minified Gzip Brotli
playcanvas.min.js 2371.0 KB (−0.0 KB, −0.00%) 609.2 KB (−0.0 KB, −0.00%) 473.1 KB (+0.0 KB, +0.01%)
playcanvas.min.mjs 2368.4 KB (−0.0 KB, −0.00%) 608.2 KB — 472.5 KB (+0.1 KB, +0.02%)

@mvaligursky
mvaligursky requested a balanced review from Copilot August 18, 2026 06:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@mvaligursky mvaligursky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated PR review — posted on my behalf by Claude Code (Opus 4.8). Not a human review. The points below are suggestions to weigh as possible improvements, not changes that necessarily need to be addressed.


The fix and the restored null handling are correct (I verified the evaluator contract claim: a null resolve() leaves the curve unbound, matching the unresolvable-component path), and the multi-element cases are a strict improvement — those either mis-bound into siblings or threw. Two points on the newly-strict behavior, both about the same risk:

  1. Single-element paths are a working-content regression, and arguably shouldn't be gated at all. With the old code, a one-element entityPath bound the anim component's own entity regardless of the name — so property animations kept working after the entity was renamed, and clips were reusable across differently-named entities. For a one-element path the target is unambiguous (there's nothing to resolve — it can only mean the component's own entity), so the name check there adds strictness without disambiguating anything, and silently breaks that previously-working pattern. Consider keeping the one-element case lenient (return currEntity without the name check, optionally with a debug warning on mismatch) and applying the guard only to multi-element paths, where the root name actually anchors _parent.findByPath and where the real mis-bind/crash bugs lived.

  2. The new rejections are silent. When the guard fires, curves just stop animating with no diagnostic — for anyone whose content depended on the old leniency, that's a hard thing to trace. A debug-build Debug.warn at the guard (naming the track's root vs the entity's name) would make both the new rejections and legacy-content migration self-explanatory, and matches how other unresolvable-binding cases in the anim system could be surfaced.

The regression tests are well-constructed and cover both conventions and all three failure modes.

Address review feedback: a one element entity path can only target the
anim component's own entity, so binding it regardless of the authored
root name preserves content that renamed the entity or reuses clips
across differently named entities. Multi element paths still require
the root name to match, and now emit a debug warning when rejected
instead of silently unbinding.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@willeastcott

Copy link
Copy Markdown
Contributor Author

Adopted both review suggestions in d0dbc73, with one refinement to the first:

  1. Single element paths are no longer gated — they bind the anim component's own entity regardless of the authored root name. History supports going exactly this far and no further: single element mismatches were the only mismatch class that ever worked under the old code (multi element mismatches either threw a TypeError or mis-bound into a sibling's subtree), so exempting them preserves all previously working content — renamed entities, clips reused across differently named entities — without re-permitting the broken cases. Multi element paths keep the root name anchor, since that's what roots _parent.findByPath(...) at the component's entity.
  2. Rejections now warnDebug.warnOnce naming the authored path and the entity (warnOnce because rebind() re-resolves every curve, so a plain warn would repeat).

One test flipped to lock in the lenient single element behavior; the PR description now also calls out the one deliberate break (intentional sibling-subtree animation) explicitly.

🤖 Generated with Claude Code

@willeastcott
willeastcott merged commit 75cee21 into main Aug 19, 2026
10 checks passed
@willeastcott
willeastcott deleted the fix-anim-binder-entity-name-guard branch August 19, 2026 12:48
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.

3 participants