perf: emote optimizations — scene-emote URN prefix match + animator layer helpers - #9740
Open
eordano wants to merge 2 commits into
Open
perf: emote optimizations — scene-emote URN prefix match + animator layer helpers#9740eordano wants to merge 2 commits into
eordano wants to merge 2 commits into
Conversation
decentraland-bot
added
the
ext-contribution
Contributor
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 13169 => 13173 — remove at least 5 warnings to merge. Warnings/errors in files changed by this PR (25)All Unity tests passed ✅
|
Contributor
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
PR Review: perf: emote optimizations — scene-emote URN prefix match + animator layer helpers
STEP 2 — Root-cause check: ✅ PASS
This PR solves three genuine performance/correctness problems:
UpdateEmoteTagsover-polling — The query polled native MecanimGetAnimatorCurrentStateTagon every entity carrying aCharacterEmoteComponent, including idle avatars. The new guard (CurrentEmoteReference == null && CurrentAnimationTag == 0) correctly skips entities that have no emote state to track, avoiding native interop overhead in crowded scenes.- Scene-emote prefix-match allocation —
TryResolveSceneByNamePrefixallocated a new string (candidateName + "-") on every candidate iteration. The extractedTryMatchSceneEmotePayloaduses span-based comparison that is allocation-free on the non-matching path. emotesInUsetracking asymmetry —emotesInUse.Addwas placed after early-return paths inPlay/PlayMasked, soStop()(which callsemotesInUse.Remove) could not find the reference to release it back to the pool. This was a real resource leak.
All three fixes address root causes, not symptoms.
STEP 3 — Design & integration: ✅ PASS
No new long-lived units are introduced. The changes are:
- A guard added to an existing ECS query method (
UpdateEmoteTags) - A static helper extracted from an existing static method (
TryResolveSceneByNamePrefix→TryMatchSceneEmotePayload) - A line reorder within an existing class (
EmotePlayer) - Constants/helpers added to an existing static utility class (
AnimatorEmoteLayers) - A method overload added to an existing concrete class (
AvatarBase)
The TryMatchSceneEmotePayload extraction is appropriate: it enables direct unit testing of the prefix-match logic without constructing ISceneFacade instances, and its internal static visibility correctly limits its scope.
Teardown/consumption trace: No new subscriptions, event hookups, connections, or buffers are introduced. The emotesInUse.Add reorder is purely a fix to an existing lifecycle.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
AnimatorEmoteLayers.BASE_LAYER |
2 (perf test only) | P2 — no production consumer |
AnimatorEmoteLayers.ALL_LAYERS |
0 | P2 — dead code |
AnimatorEmoteLayers.NON_BASE_LAYERS |
0 | P2 — dead code |
AnimatorEmoteLayers.GetFromEmoteMask |
0 | P2 — dead code |
AvatarBase.GetAnimatorCurrentStateTag(string) |
2 (perf test only) | P2 — not on IAvatarView, no production consumer |
CharacterEmoteSystem.TryMatchSceneEmotePayload |
1 production + 1 test | ✅ justified |
STEP 5 — Line-level findings
See inline comments below. All findings are P2.
STEP 6 — Complexity: COMPLEX
Touches emote ECS system queries, animation pipeline helpers, and emote pooling lifecycle — all performance-sensitive runtime paths.
STEP 7 — QA: YES
Modifies runtime emote playback code (animator polling, emote pool tracking) that directly affects avatar animations visible to users.
STEP 8 — Non-blocking warnings
None.
Security review
No security issues found. All changes are internal Unity C# animation/emote system optimizations with no network I/O, user input parsing, auth, or file operations.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches emote ECS system queries (CharacterEmoteSystem.UpdateEmoteTags), animation utility infrastructure (AnimatorEmoteLayers), and emote pool lifecycle (EmotePlayer.emotesInUse)
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown via Slack
Comment on lines
+1
to
+21
| using DCL.ECSComponents; | ||
|
|
||
| namespace Utility.Animations | ||
| { | ||
| public static class AnimatorEmoteLayers | ||
| { | ||
| // Unity's Animator always places the base layer at index 0. | ||
| public const string BASE_LAYER = "Base Layer"; | ||
| public const string UPPER_BODY_LAYER = "Upper Body Layer"; | ||
|
|
||
| public const int BASE_LAYER_INDEX = 0; | ||
|
|
||
| public const string UPPER_BODY_LAYER = "Upper Body Layer"; | ||
| public static readonly string[] ALL_LAYERS = | ||
| { | ||
| BASE_LAYER, | ||
| UPPER_BODY_LAYER, | ||
| }; | ||
|
|
||
| public static readonly string[] NON_BASE_LAYERS = | ||
| { | ||
| UPPER_BODY_LAYER, | ||
| }; |
Contributor
There was a problem hiding this comment.
[P2] Dead infrastructure — ALL_LAYERS, NON_BASE_LAYERS, and using DCL.ECSComponents have zero production consumers. ALL_LAYERS and NON_BASE_LAYERS are not referenced by any code outside this file, and the using DCL.ECSComponents import is only needed by the equally unused GetFromEmoteMask below. Adding unused infrastructure pulls the Protocol assembly dependency into Utility for no runtime benefit. Per CLAUDE.md §11 (anti-patterns), unused code should not be added speculatively — add these when the consumer that needs them arrives.
Suggested change
| using DCL.ECSComponents; | |
| namespace Utility.Animations | |
| { | |
| public static class AnimatorEmoteLayers | |
| { | |
| // Unity's Animator always places the base layer at index 0. | |
| public const string BASE_LAYER = "Base Layer"; | |
| public const string UPPER_BODY_LAYER = "Upper Body Layer"; | |
| public const int BASE_LAYER_INDEX = 0; | |
| public const string UPPER_BODY_LAYER = "Upper Body Layer"; | |
| public static readonly string[] ALL_LAYERS = | |
| { | |
| BASE_LAYER, | |
| UPPER_BODY_LAYER, | |
| }; | |
| public static readonly string[] NON_BASE_LAYERS = | |
| { | |
| UPPER_BODY_LAYER, | |
| }; | |
| namespace Utility.Animations | |
| { | |
| public static class AnimatorEmoteLayers | |
| { | |
| public const string BASE_LAYER = "Base Layer"; | |
| public const string UPPER_BODY_LAYER = "Upper Body Layer"; | |
| public const int BASE_LAYER_INDEX = 0; |
Comment on lines
23
to
30
| public static string GetFromEmoteMask(AvatarEmoteMask mask) => | ||
| mask switch | ||
| { | ||
| AvatarEmoteMask.AemFullBody => BASE_LAYER, | ||
| AvatarEmoteMask.AemUpperBody => UPPER_BODY_LAYER, | ||
| _ => BASE_LAYER, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
[P2] Dead infrastructure — GetFromEmoteMask has zero consumers in the codebase. Additionally, the switch body has inconsistent indentation (extra leading spaces on the switch expression). Remove until a consumer exists; if kept, fix the indentation to match project style.
Suggested change
| public static string GetFromEmoteMask(AvatarEmoteMask mask) => | |
| mask switch | |
| { | |
| AvatarEmoteMask.AemFullBody => BASE_LAYER, | |
| AvatarEmoteMask.AemUpperBody => UPPER_BODY_LAYER, | |
| _ => BASE_LAYER, | |
| }; | |
| } | |
| } | |
| } |
Comment on lines
+259
to
+263
| public int GetAnimatorCurrentStateTag(string layerName) | ||
| { | ||
| int layerIndex = AvatarAnimator.GetLayerIndex(layerName); | ||
| return AvatarAnimator.GetCurrentAnimatorStateInfo(layerIndex).tagHash; | ||
| } |
Contributor
There was a problem hiding this comment.
[P2] Dead production code — GetAnimatorCurrentStateTag(string) is not declared on the IAvatarView interface and has no production consumer — only the perf test (AvatarAnimatorLayerIndexCachePerformanceTest) calls it on the concrete AvatarBase. Since all production code goes through IAvatarView, this overload is unreachable outside tests. Consider removing it and having the perf test call AvatarAnimator.GetLayerIndex + the int overload directly, or defer adding it until a production consumer exists.
Suggested change
| public int GetAnimatorCurrentStateTag(string layerName) | |
| { | |
| int layerIndex = AvatarAnimator.GetLayerIndex(layerName); | |
| return AvatarAnimator.GetCurrentAnimatorStateInfo(layerIndex).tagHash; | |
| } |
Contributor
|
PR #9740, run #31739159760 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
… default gate
AvatarBase.GetAnimatorCurrentStateTag(string) and AnimatorEmoteLayers'
ALL_LAYERS / NON_BASE_LAYERS / GetFromEmoteMask existed only to give a
performance test something to benchmark against: zero production call sites, so
two shipped classes carried API nobody owns. Both files are byte-identical to
their pre-PR state again, and the benchmark resolves the string layer path
through the Animator it already holds.
All three behaviour changes shipped only under [Category("Performance")].
test.yml runs the standard EditMode/PlayMode matrix with -testCategory
"!Performance", and the only workflow that runs Performance tests is gated
behind workflow_dispatch / the perf_test label, so the default PR gate covered
none of them. CharacterEmoteSystemShould now covers the UpdateEmoteTags
early-out (no poll for idle avatars, one poll while a reference is held, and
polling continues until a stale tag clears) and TryMatchSceneEmotePayload's
match/reject contract.
The emotesInUse.Add move in EmotePlayer.Play/PlayMasked is a leak fix, not a
reshuffle: on the !legacyAnimationsEnabled path and the masked-legacy-failure
path, Stop(emoteReferences) ran before the entry existed, so emotesInUse.Remove
returned false and pool.Release was skipped, stranding the pooled
EmoteReferences GameObject on the avatar. EmotePlayerShould drives both paths
and asserts the instance ends up back in the pool hierarchy.
The guard's safety rests on untagged Mecanim states reporting tagHash 0 while
CharacterEmoteComponent.Reset() leaves currentAnimationTag untouched; that
invariant is now stated at the guard.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018c638dR1vPysCMbYt2qQg5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Recovers the emote optimizations that were deferred out of the consolidated #9685 during its resync onto dev. Dev's perf roll-up (#9707) re-implemented much of #9685's perf work, but not these — they're genuinely unique, so they get their own focused PR here.
What
CharacterEmoteSystem.TryMatchSceneEmote, avoids re-scanning on scene-emote lookups.AnimatorEmoteLayers:BASE_LAYER,ALL_LAYERS,GetFromEmoteMask, alongside dev's existingBASE_LAYER_INDEX.CharacterEmoteSystemPerformanceTest,AvatarAnimatorLayerIndexCachePerformanceTest,SceneEmoteUrnPrefixMatchPerformanceTest.11 files, +412/−8, based on current dev.
Note
The bot's P2 on
EmotePlayer(emotesInUse.Addbefore early-return → tracking asymmetry) from #9685 lives in this area — worth folding the fix in here.Part of the dataroom 1+2 decomposition (see #9685).