fix(ui): guard manual Animator.Update against inactive objects#9434
fix(ui): guard manual Animator.Update against inactive objects#9434eordano wants to merge 1 commit into
Conversation
The shared UI views snap their animator to its resting frame on enable/toggle with Rebind() + Update(0). Unity only permits Animator.Update on an active-in-hierarchy object, but these views are routinely enabled or toggled while their panel is still hidden during UI bootstrap — producing a burst of "Can't call Animator.Update on inactive object" errors on every startup. Guard each manual Update(0) with a gameObject.activeInHierarchy check in the three shared views that account for the spam: - TabSelectorView.OnEnable - ButtonWithAnimationView.OnEnable - SectionSelectorController.SetAnimationState Behaviour-preserving: the resting-frame snap only mattered while the object was visible, and the animator re-evaluates normally once the panel is shown. Verified in editor Play entering Genesis Plaza: startup count 13 -> 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
decentraland-bot
added
the
ext-contribution
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: fix(ui): guard manual Animator.Update against inactive objects
STEP 2 — Root-cause check: PASS
This PR addresses a Unity Animator API constraint: Animator.Rebind() and Animator.Update() require the animator's GameObject to be activeInHierarchy. The activeInHierarchy guard is the standard Unity defensive pattern for this.
Nuance for the OnEnable() call sites (ButtonWithAnimationView, TabSelectorView): Unity only fires OnEnable() when the component is effectively active (activeInHierarchy == true). If ButtonAnimator / tabAnimator is on the same GameObject as the MonoBehaviour, the guard is always true (dead code). The guard is only meaningful if the Animator is serialized to a different (e.g. child) object that may be independently deactivated. The SectionSelectorController.SetAnimationState() guard is unambiguously correct since it's called programmatically.
Deferred initialization is safe: when the parent panel later becomes active, Unity fires OnEnable() again — the guard passes, and Rebind() + Update(0) runs at the right time.
STEP 3 — Design & integration: PASS
No new types, systems, or persistent state introduced. No owner search required.
Teardown trace: No new subscriptions, event hookups, or connections. The existing AddListener/RemoveListener pairs in ButtonWithAnimationView.OnEnable()/OnDisable() and TabSelectorView.OnEnable()/OnDisable() remain balanced. ✅
STEP 4 — Member audit
No new public properties or accessors added or changed.
STEP 5 — Line-level findings
See inline comments. Two P2 findings — no P0 or P1 issues.
Security review
No security issues found. Changes are purely defensive UI animation guards — no user input handling, secrets, auth, networking, or file system operations.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Small defensive guards added to 3 UI MonoBehaviour/controller files; no ECS, async, plugin, or architectural changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown (<@unknown>) via Slack
| // Animator.Update is only legal on an active-in-hierarchy object; these buttons | ||
| // are often enabled while their panel is still hidden during UI bootstrap. | ||
| if (ButtonAnimator.gameObject.activeInHierarchy) | ||
| { | ||
| ButtonAnimator.Rebind(); | ||
| ButtonAnimator.Update(0); | ||
| } |
There was a problem hiding this comment.
[P2] Centralization opportunity: The Rebind() + Update(0) pattern with the activeInHierarchy guard is repeated inline across all 3 files in this PR, and there are 10+ additional unguarded Rebind() + Update(0) call sites in the codebase (e.g. BackpackController, PlacesView, EventsView, SettingsController, CommunitiesBrowserView, NavmapController, CameraReelController, GiftingTabManager, AvatarTabNavigator).
The codebase already has AnimatorExtensions.ResetAnimator() doing exactly Rebind() + Update(0f) — but without the guard. Consider adding a safe variant there so all call sites benefit from the fix and the guard logic isn't duplicated:
// In AnimatorExtensions.cs
public static bool TryResetAnimator(this Animator animator)
{
if (!animator.gameObject.activeInHierarchy) return false;
animator.Rebind();
animator.Update(0f);
return true;
}Then this block simplifies to:
| // Animator.Update is only legal on an active-in-hierarchy object; these buttons | |
| // are often enabled while their panel is still hidden during UI bootstrap. | |
| if (ButtonAnimator.gameObject.activeInHierarchy) | |
| { | |
| ButtonAnimator.Rebind(); | |
| ButtonAnimator.Update(0); | |
| } | |
| ButtonAnimator.TryResetAnimator(); |
| if (tabAnimator != null) | ||
| // Animator.Update is only legal on an active-in-hierarchy object; tabs are | ||
| // frequently enabled while their panel is still hidden during UI bootstrap. | ||
| if (tabAnimator != null && tabAnimator.gameObject.activeInHierarchy) |
There was a problem hiding this comment.
[P2] Pre-existing null-check inconsistency (CLAUDE.md §11 "Defensive null-checks against non-null declarations"): tabAnimator is assumed non-null on line 40 (tabAnimator.enabled = true) and line 56 (tabAnimator.enabled = false), but this guard checks tabAnimator != null. Either the field can legitimately be null (in which case lines 40 and 56 also need guards) or it cannot (in which case the null check here is noise).
Since this PR didn't introduce the inconsistency, this is a pre-existing observation — but worth aligning when touching this code. If the field is always assigned via the inspector:
| if (tabAnimator != null && tabAnimator.gameObject.activeInHierarchy) | |
| if (tabAnimator.gameObject.activeInHierarchy) |
|
Warnings not reduced: 14668 => 14681 — remove at least one warning to merge. Warnings/errors in files changed by this PR (31)
|
2 participants
Testing Instructions