opti: skip redundant avatar layer weight writes and cache animator layer indices - #9701
opti: skip redundant avatar layer weight writes and cache animator layer indices#9701NickKhalow wants to merge 3 commits into
Conversation
… writes SetPointAtLayerWeight and SetRotationLayerWeight now shadow the last value written and skip the native Animator.SetLayerWeight call when it hasn't changed; ResetState clears the shadow so a rebind isn't skipped.
…ring-based lookups Completes the layer-index scaffolding introduced with the weight guard: - AnimatorEmoteLayers gains the BASE_LAYER_INDEX constant the scaffolding referenced (fixes compilation) and loses GetFromEmoteMask plus the unused ALL_LAYERS / NON_BASE_LAYERS arrays. - GetAnimatorCurrentStateTag(string) and SetLayerWeight(string) resolved the layer via a native Animator.GetLayerIndex call on every invocation, per frame in HeadIKSystem and HandsIKSystem. All callers now use the cached index API (UpperBodyLayerIndex, GetEmoteLayerIndex, int overloads) and the string overloads are removed. - Removes the unused DclAvatarMask using alias.
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings count reduced: 13292 => 13174 Warnings/errors in files changed by this PR (36)
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 1 — Context & Scope
Loaded CLAUDE.md, docs/README.md, docs/branch-and-pr-standards.md, and the full diff. The PR touches 7 production files (+116 −111) and adds 1 new test file (+132). Core change surface is AvatarBase.cs and the IAvatarView interface; all other files are mechanical consumer migrations.
STEP 2 — Root-Cause Check: PASS ✅
The PR solves two real per-frame costs:
- Redundant
Animator.SetLayerWeightnative calls — weights sit at 0 or 1 most of the time, so the common-case write is a no-op that still crosses the managed→native boundary. - Per-frame
Animator.GetLayerIndex(string)calls — string marshaling on every frame per avatar inHeadIKSystem,HandsIKSystem, and emote systems.
Both fixes address the root cause directly (cache-and-guard), not symptoms. No workarounds, no exception swallowing, no suppressed checks.
STEP 3 — Design & Integration: PASS ✅
No new long-lived units introduced. All changes live in the existing AvatarBase class, which already owns the Animator, rightPointAtLayerIndex, and rotationLayerIndex. The new upperBodyLayerIndex and shadow fields follow the identical pattern.
GetEmoteLayerIndex placement is correct. Moved from static utility (AnimatorEmoteLayers.GetFromEmoteMask) to AvatarBase/IAvatarView because it now needs the cached upperBodyLayerIndex instance data. The avatar owns the animator → the avatar owns the index resolution. The removed static method, ALL_LAYERS, and NON_BASE_LAYERS arrays have zero remaining consumers (verified via code search).
float.NaN sentinel is sound. IEEE-754 NaN ≠ NaN guarantees the first write always goes through after ResetState(). Animator.Rebind() resets native layer weights, and the shadow clear happens after the rebind — correct ordering for pooled avatar reuse.
Teardown / consumption trace: The shadow fields (lastPointAtLayerWeight, lastRotationLayerWeight) are value types with no subscriptions, handles, or resources to dispose. ResetState() clears them. No leak surface.
Consumer migration: Verified all callers are updated — CharacterEmoteSystem (3 sites), EmotePlayer (4 sites), SceneMaskedEmoteSystem (2 sites), HandsIKSystem (1 site), HeadIKSystem (1 site). Zero remaining references to the removed string-based overloads, GetFromEmoteMask, ALL_LAYERS, or NON_BASE_LAYERS (confirmed via gh search code). Existing test mocks (CharacterEmoteSystemShould, MovePlayerWithDurationSystemShould) use Substitute.For<IAvatarView>() but do not call the changed methods — no compilation risk.
STEP 4 — Member Audit
| New Member | Consumers | Assessment |
|---|---|---|
UpperBodyLayerIndex (property) |
HeadIKSystem (1), HandsIKSystem (1) |
Thin forwarding accessor centralizing access to one field — legitimate encapsulation even with 2 consumers. Alternative (GetEmoteLayerIndex(AemUpperBody)) would couple IK systems to AvatarEmoteMask unnecessarily. |
GetEmoteLayerIndex(AvatarEmoteMask) |
EmotePlayer (4), SceneMaskedEmoteSystem (2) |
Direct replacement of removed static GetFromEmoteMask. Logic preserved: AemUpperBody → upperBodyLayerIndex, default → BASE_LAYER_INDEX. |
GetAnimatorCurrentStateTag(int) |
9 call sites across 5 files | Replaces string overload. All callers migrated. |
SetLayerWeight(int, float) |
EmotePlayer (2) |
Replaces string overload. All callers migrated. |
No single-use intermediaries, no absent≠false conflation, no redundant guards.
STEP 5 — Line-Level Review
P2 — null! on non-serialized GhostRenderer (AvatarBase.cs)
GhostRenderer lacks [field: SerializeField], so Unity's serialization won't auto-assign it. The = null! suppresses the nullable warning, asserting it will be set before use. This is consistent with the null! pattern applied to all other properties in this cleanup sweep, and the property is set during avatar initialization — but it is worth noting this is a null! on regular code rather than a serialized/DTO field.
Severity: Non-blocking observation. The convention is borderline for non-serialized MonoBehaviour properties set in lifecycle methods — current usage is acceptable.
P2 — Performance test timing assertion stability (AvatarLayerWeightGuardPerformanceTest.cs:120)
Assert.Less(bestRedundant, bestAlternating * 0.5, ...) — hard timing assertion (redundant must be ≥2× faster). The best-of-3 design and the generous 2× margin mitigate CI flakiness, and the optimization should comfortably exceed 2× since guarded writes skip the native call entirely. Noting for awareness — if this ever flakes on heavily loaded CI runners, widening to 0.75 would still demonstrate the optimization.
Security Review: No issues found ✅
Client-side Unity animation code with no network input, no auth, no secrets, no DB/shell calls. Reflection usage in the test is guarded by #if UNITY_EDITOR and does not ship. ProfilerRecorder is properly disposed; test GameObjects are destroyed in TearDown.
STEP 6 — Complexity
COMPLEX — Modifies the avatar rendering pipeline's animation layer API (IAvatarView interface), changes AvatarBase (the central avatar MonoBehaviour), and requires coordinated migration across 5 consumer systems spanning multiple assemblies.
STEP 7 — QA Assessment
QA_REQUIRED: YES — Changes affect runtime avatar animation (layer weights, emote layer selection, IK behavior). All changes are in the hot path that drives visible avatar behavior. The test plan in the PR description covers the key risk surfaces well (pool reuse, masked emotes, IK disable/enable, emote cancellation).
STEP 8 — Non-Blocking Warnings
None. Main.unity is not modified.
STEP 9 — Verdict
No P0 or P1 issues found. The optimizations are correct, well-placed, and thoroughly tested. All consumers are properly migrated with no orphaned references. The float.NaN sentinel pattern is sound (verified callers never produce NaN). The null! annotation sweep and API cleanup are clean.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies IAvatarView interface and AvatarBase animation layer API with coordinated migration across 5 consumer systems in avatar rendering, emote, and IK assemblies.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| public void SetPointAtLayerWeight(float weight) => | ||
| public void SetPointAtLayerWeight(float weight) | ||
| { | ||
| if (weight == lastPointAtLayerWeight) return; |
There was a problem hiding this comment.
[P2] Non-blocking observation — NaN-as-weight edge case.
The float.NaN sentinel is clever and correct for pool reuse. One theoretical edge: if a caller ever passes float.NaN as weight, the guard would never short-circuit (NaN ≠ NaN), reverting to pre-PR behavior (always-write). This is not a regression and is currently unreachable — I traced the call chain through HandPointAtHelper.ApplyAnimationWeight and PlayerRotationAnimation, both of which feed Mathf.MoveTowards output that never produces NaN for valid settings.
No change needed — documenting for completeness.
| Measure.Custom(new SampleGroup("RedundantWeightWrites", SampleUnit.Nanosecond), bestRedundant); | ||
| Measure.Custom(new SampleGroup("AlternatingWeightWrites", SampleUnit.Nanosecond), bestAlternating); | ||
|
|
||
| Assert.Less(bestRedundant, bestAlternating * 0.5, $"redundant writes ({bestRedundant}) should be far cheaper than alternating ({bestAlternating})"); |
There was a problem hiding this comment.
[P2] Non-blocking — timing assertion stability.
The bestRedundant < bestAlternating * 0.5 threshold (≥2× speedup) should hold comfortably since guarded writes skip the native call entirely, and best-of-3 mitigates noise. If this ever flakes on loaded CI runners, widening to 0.75 would still demonstrate the optimization without weakening the test's intent.
| Assert.Less(bestRedundant, bestAlternating * 0.5, $"redundant writes ({bestRedundant}) should be far cheaper than alternating ({bestAlternating})"); | |
| Assert.Less(bestRedundant, bestAlternating * 0.5, $"redundant writes ({bestRedundant}) should be far cheaper than alternating ({bestAlternating})"); |
(No change suggested — keeping current threshold is fine.)
|
|
||
| int UpperBodyLayerIndex { get; } | ||
|
|
||
| int GetEmoteLayerIndex(AvatarEmoteMask mask); |
There was a problem hiding this comment.
[P2] Non-blocking — AvatarEmoteMask coupling on IAvatarView.
Before this PR, AvatarBase.cs / IAvatarView had no dependency on DCL.ECSComponents. Adding GetEmoteLayerIndex(AvatarEmoteMask) here introduces a coupling from the avatar view interface to the SDK protocol enum.
An alternative that keeps the interface dependency-free:
// In AnimatorEmoteLayers (already references DCL.ECSComponents):
public static int GetEmoteLayerIndex(AvatarEmoteMask mask, int upperBodyLayerIndex) =>
mask == AvatarEmoteMask.AemUpperBody ? upperBodyLayerIndex : BASE_LAYER_INDEX;
// Callers:
int layer = AnimatorEmoteLayers.GetEmoteLayerIndex(masked.Mask, avatarView.UpperBodyLayerIndex);This keeps IAvatarView exposing only primitive int indices while still avoiding per-frame string lookups.
That said, all consumers of IAvatarView.GetEmoteLayerIndex (EmotePlayer, SceneMaskedEmoteSystem) already depend on DCL.ECSComponents, so the practical impact is minimal. Up to you whether the cleaner interface boundary is worth the slightly more verbose call sites.
…9705 #9706) (#9707) * opti(avatar-rendering): bound bone-matrix job by each avatar's actual bone count BoneMatrixCalculationJob recomputed all MAX_BONE_COUNT matrices per avatar every frame regardless of how many ComputeSkinning actually uploads; refresh and pass through the authoritative per-avatar count so the job only computes the uploaded range. * opti(transforms): assign world cache directly in SetWorldTransform Transform.SetPositionAndRotation is world-authoritative, so the resulting world pose equals the arguments passed in; skip the native position/rotation readback and assign the cache from them directly. * opti(landscape): reuse persistent native containers in RenderGroundSystem RenderGroundInternal allocated and disposed a NativeArray<int> and a NativeList<Matrix4x4> with Allocator.TempJob every frame the ground was visible; hold them as Allocator.Persistent fields instead, clearing them each frame and disposing once in OnDispose. * opti(web-requests): pre-size PartialDownloadHandler and grow geometrically ReceiveData rented exactly PartialData.Length + dataLength and copied the full old buffer on every regrow, causing O(N^2) rent+memcpy churn over a chunked download. Pre-size the buffer from the Content-Length header when available, and otherwise double the buffer on regrow, copying only the bytes written so far. Exercising the growth path requires driving DownloadHandlerScript's native callbacks, which has no stable unit-test seam in headless batch mode. * opti(comms-profiles): skip the remove-intentions lock on empty frames RemoteEntitiesExtensions.Remove ran every frame and unconditionally built an OwnedBunch<RemoveIntention>, whose ctor acquires MutexSync even when there is nothing to remove. Add a racy, lock-free NewBunchAvailable() pre-check (mirrors RemoteProfiles.NewBunchAvailable()) so the lock is skipped entirely on empty frames. * fix: reset stale hand point-at on teleport and scene reload (#9578) A cached absolute point-at world hit-point survived a teleport or same-realm scene reload, leaving an avatar's arm pinned at a stale position. Reset the point-at target from both the PlayerTeleportIntent path and the scene-reload path (ECSReloadScene, both overloads). Verified on real Unity 6000.4.0f1 (compile + EditMode/PlayMode regression tests). * opti(avatar-rendering): hoist frustum-plane extraction out of the outline query GetAvatarsVisibleWithOutline recalculated the camera's frustum planes and re-fetched the camera component for every avatar; both are identical across a frame, so compute them once in Update and pass the camera in via [Data]. * opti(avatar-animation): skip redundant point-at/rotation layer weight writes SetPointAtLayerWeight and SetRotationLayerWeight now shadow the last value written and skip the native Animator.SetLayerWeight call when it hasn't changed; ResetState clears the shadow so a rebind isn't skipped. * refactor(landscape): replace init flag with nullable tuple in RenderGroundSystem * docs(landscape): explain why persistent ground containers never need reallocation * Fix formatting (no code changes) * changed how reload/teleport reset is managed * opti(avatar-animation): finish indexed animator-layer API and drop string-based lookups Completes the layer-index scaffolding introduced with the weight guard: - AnimatorEmoteLayers gains the BASE_LAYER_INDEX constant the scaffolding referenced (fixes compilation) and loses GetFromEmoteMask plus the unused ALL_LAYERS / NON_BASE_LAYERS arrays. - GetAnimatorCurrentStateTag(string) and SetLayerWeight(string) resolved the layer via a native Animator.GetLayerIndex call on every invocation, per frame in HeadIKSystem and HandsIKSystem. All callers now use the cached index API (UpperBodyLayerIndex, GetEmoteLayerIndex, int overloads) and the string overloads are removed. - Removes the unused DclAvatarMask using alias. * Update Explorer/Assets/DCL/Multiplayer/Profiles/RemoveIntentions/IRemoveIntentions.cs Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: Nick Khalow <71646502+NickKhalow@users.noreply.github.com> * docs(transforms): explain direct world cache write in SetWorldTransform * expected length "no double calculation" and comments * refactor(avatar-rendering): drop camera param from IsVisibleInCamera, pass CameraComponent as query data --------- Signed-off-by: Nick Khalow <71646502+NickKhalow@users.noreply.github.com> Co-authored-by: Esteban Ordano <esteban@decentraland.org> Co-authored-by: lorenzo-ranciaffi <lorenzo.ranciaffi@decentraland.org> Co-authored-by: lorenzo-ranciaffi <41125365+lorenzo-ranciaffi@users.noreply.github.com> Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com>
4 participants
Pull Request Description
What does this PR change?
Two related micro-optimizations in the avatar animation hot path, plus the API cleanup that falls out of them:
1. Layer-weight write guard (
AvatarBase)SetPointAtLayerWeightandSetRotationLayerWeightare called every frame per avatar and previously forwarded straight to the nativeAnimator.SetLayerWeightcall even when the weight hadn't changed — which is the common case (the weights sit at 0 or 1 most of the time). Both setters now shadow the last value written and skip the native call when it is unchanged.ResetState()clears the shadows: avatars are pooled andAnimator.Rebind()resets the real layer weights, so a reused avatar must not skip its first genuine write.2. Cached animator layer indices (all layer-API callers)
GetAnimatorCurrentStateTag(string)andSetLayerWeight(string)resolved the layer via a nativeAnimator.GetLayerIndex(name)call (with string marshaling) on every invocation — per frame inHeadIKSystemandHandsIKSystem.AvatarBasenow caches the upper-body layer index once inAwakeand exposes an index-based API (UpperBodyLayerIndex,GetEmoteLayerIndex(mask),intoverloads). All callers (EmotePlayer,SceneMaskedEmoteSystem,CharacterEmoteSystem,HeadIKSystem,HandsIKSystem) are migrated and the string overloads are deleted, along withAnimatorEmoteLayers.GetFromEmoteMaskand its unused layer arrays.Performance: no behavior change intended. The included PlayMode performance test (
AvatarLayerWeightGuardPerformanceTest) asserts that redundant weight writes are at least 2× cheaper than alternating writes, that the writes are allocation-free, and thatResetStatere-arms the guard so pool reuse is not skipped.Test Instructions
Steps (standard run):
Expected result:
Avatar animation behaves exactly as on
dev— this PR must cause no visible change. Verify the animator-layer-driven features below.Steps (fresh account):
Expected result:
Same as above — no visible difference in avatar animation on a clean profile.
Automation (if applicable):
metaforge explorer test 9701Prerequisites
Test Steps
dev.Additional Testing Notes
ResetStatere-arm path.SceneMaskedEmoteSystem/ replication paths) are worth a quick two-client check if available.Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.