feat: stagger avatar wearable loading and gate emotes on instantiation - #9890
feat: stagger avatar wearable loading and gate emotes on instantiation#9890dalkia wants to merge 2 commits into
Conversation
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 12543 => 12544 — remove at least 2 warnings to merge. Warnings/errors in files changed by this PR (12)
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #9890
STEP 1 — Context
Loaded CLAUDE.md, review-instructions.md, docs/README.md. Cloned repo, checked out PR head. Read full source of all changed files and their neighbours: AvatarInstantiatorSystem, AvatarCleanUpSystem, PartitionComponent, DistanceBasedComparer. Searched lifecycle owners for AvatarCustomSkinningComponent and AvatarWearableAssetsInFlight (rg over the tree).
STEP 2 — Root-cause check: PASS ✅
Problem: All avatars' wearable asset-bundle promises enter the shared deferred-loading queue in the same frame, interleaving downloads so every avatar's last wearable finishes at the tail of the batch — they all pop in simultaneously.
Fix: The PR gates admission to the asset-download phase through a slot cap in ResolveWearablePromisesSystem, so a limited number of avatars (nearest first) enter the download queue at once. This addresses the cause directly — not a symptom workaround.
STEP 3 — Design & integration: PASS ✅
New unit: AvatarWearableAssetsInFlight — a lightweight ECS marker struct (float Age) added to promise entities. Not a system, service, or persistent collection.
Owner search (mandatory):
- Who creates the promise entities?
AvatarLoaderSystem/AvatarInstantiatorSystem(viaAssetPromise<...>.Create), wired inWearablePlugin.InjectToWorld. - Who resolves them?
ResolveWearablePromisesSystem— the same system this PR modifies. - Who destroys them? Promise entities are destroyed when resolution completes (
StreamableResultadded), is cancelled (CancellationTokenSource), or the avatar entity is destroyed. - Could the gating logic live elsewhere? No — the gate sits at the exact seam between DTO resolution (bulk-batched, uncapped) and per-wearable asset-promise creation, both of which are already managed by this system. Splitting into a separate system would fragment a single sequential pipeline with no gain.
State held: assetPhaseCandidates (per-frame list, .Clear()-ed each Update()) and avatarsWithAssetsInFlight (per-frame int, reset to 0 each Update()). Both are sanctioned per-frame scratch — CLAUDE.md §1 ✅.
Slot lifecycle — teardown trace:
| Event | Slot freed how |
|---|---|
| Resolution completes | StreamableResult added → entity excluded by [None(typeof(StreamableResult))] on the counting query |
| Resolution cancelled | Cancellation check adds StreamableResult → same as above |
| Entity destroyed | Entity no longer exists → no longer matched by any query |
| Stuck > 30 s | inFlight.Age >= threshold → stops incrementing counter (entity continues processing without holding a slot) |
All paths covered. No slot leak. ✅
Emote gate: [All(typeof(AvatarCustomSkinningComponent))] on ConsumeEmoteIntent is the right signal — AvatarInstantiatorSystem adds this component at line 159 via World.Add(entity, avatarTransformMatrixComponent, skinningComponent) when the first wearable is instantiated. CharacterEmoteSystem runs [UpdateAfter(typeof(AvatarGroup))], so instantiation happens first in the same frame. Intent persists on the entity until the component appears. ✅
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
AvatarWearableAssetsInFlight.Age |
CountAvatarsWithAssetsInFlight (ref mutation + threshold check) |
Single producer/consumer in same system. Correct public field on ECS struct. ✅ |
Settings.MaxAvatarsWithAssetsInFlight |
WearablePlugin.InitializeAsync → constructor |
Follows existing BatchHeartbeatMs pattern. ✅ |
maxAvatarsWithAssetsInFlight (private readonly) |
AdmitNextAvatarsToAssetPhase |
Clamped via Math.Max(1, ...). ✅ |
assetPhaseCandidates (private list) |
Update (clear), ResolveWearablePromise (add), AdmitNextAvatarsToAssetPhase (sort + iterate) |
Per-frame scratch. ✅ |
avatarsWithAssetsInFlight (private int) |
Update (reset), CountAvatarsWithAssetsInFlight (increment), AdmitNextAvatarsToAssetPhase (read) |
Per-frame counter. ✅ |
No single-use-merge, absent≠false, or re-derivation issues.
STEP 5 — Line-level review
A. Blocking-issue scan: No issues found.
- Ref safety: Structural changes (
World.AddinAdmitNextAvatarsToAssetPhase) happen after all queries have completed and all refs are out of scope. The gating early-return inResolveWearablePromisedoes no structural work — only a list append and pool releases. ✅ - Pool release on early return: Both
WEARABLES_POOLandPOINTERS_POOLare correctly released on the new gating path. ✅ - Allocation-free Update: No heap allocations.
List.Clear()reuses backing array,List.Sortis in-place,staticlambda caches delegate,ValueTupleis stack-allocated,new AvatarWearableAssetsInFlight()is a struct. ✅ - No LINQ: ✅
- Resource leaks: No new subscriptions, callbacks, or event hookups introduced. ✅
- Nullability: No new nullable annotations or null-forgiving operators. ✅
B. Design smells: None found. No construction issues, no naming issues, no encapsulation leaks, no magic values (IN_FLIGHT_STUCK_TIMEOUT_SECS is a named constant).
See inline comments for P2 findings.
STEP 6 — Complexity: COMPLEX
Modifies the avatar wearable loading pipeline (ResolveWearablePromisesSystem), introduces a new ECS component with lifecycle implications, and changes emote system query filters affecting all avatar types.
STEP 7 — QA: YES
Runtime avatar loading order and emote playback behavior are user-visible changes.
STEP 8 — Non-blocking warnings
None. Main.unity is not in the changed file set.
Security review
No security issues found. All inputs are internal (serialized ScriptableObject settings, ECS component data). No network input processing, no auth changes, no user-data handling. Math.Max(1, ...) clamp prevents invalid cap values.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies avatar wearable loading pipeline (ResolveWearablePromisesSystem), adds new ECS component, and changes emote system query filters.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| }, | ||
| strandedAvatarView, | ||
| new AvatarShapeComponent { BodyShape = BodyShape.MALE }); | ||
| new AvatarShapeComponent { BodyShape = BodyShape.MALE }, |
There was a problem hiding this comment.
[P2] Missing test: emote gate negative case. The two modified tests verify that ConsumeEmoteIntent works when AvatarCustomSkinningComponent IS present (positive case). Consider adding a test that verifies the gate holds when the component is absent — a loading ghost should NOT consume emote intents.
| new AvatarShapeComponent { BodyShape = BodyShape.MALE }, | |
| new AvatarCustomSkinningComponent()); // instantiated avatar: the intent is consumable |
Suggested additional test (new method):
[Test]
public void NotConsumeEmoteIntentWhileAvatarIsStillLoading()
{
IAvatarView loadingView = Substitute.For<IAvatarView>();
loadingView.GetAnimatorBool(AnimationHashes.GROUNDED).Returns(true);
// No AvatarCustomSkinningComponent — avatar is still loading
Entity loadingEntity = world.Create(
new CharacterEmoteComponent(),
new CharacterEmoteIntent
{
EmoteId = new URN(SCENE_EMOTE_URN),
Mask = AvatarEmoteMask.AemFullBody,
},
loadingView,
new AvatarShapeComponent { BodyShape = BodyShape.MALE });
system!.Update(1f);
Assert.IsTrue(world.Has<CharacterEmoteIntent>(loadingEntity),
"A loading avatar (no AvatarCustomSkinningComponent) must not consume emote intents.");
}| assetPhaseCandidates.Sort(static (c1, c2) => c1.sqrDistance.CompareTo(c2.sqrDistance)); | ||
|
|
||
| for (var i = 0; i < assetPhaseCandidates.Count && i < freeSlots; i++) | ||
| World.Add(assetPhaseCandidates[i].entity, new AvatarWearableAssetsInFlight()); |
There was a problem hiding this comment.
[P2] Missing test: stagger mechanism. No unit tests cover the slot-gating behavior: max cap enforcement, nearest-first admission ordering, the 30 s stuck timeout, or slot recovery on resolution/cancellation/entity destruction. The existing test file (ResolveWearableByPointerSystemShould.cs) tests FinalizeAssetBundleWearableLoadingSystem, not this system's new gating logic.
| World.Add(assetPhaseCandidates[i].entity, new AvatarWearableAssetsInFlight()); | |
| World.Add(assetPhaseCandidates[i].entity, new AvatarWearableAssetsInFlight()); |
Suggested test scenarios for a new ResolveWearablePromisesSystemShould fixture:
- With cap = 2 and 4 candidates: only the 2 nearest get
AvatarWearableAssetsInFlighton the first update. - After a slot frees (entity gets
StreamableResult), the next nearest candidate is admitted. - An entity stuck for > 30 s stops counting against the cap, freeing a slot for the next candidate.
- A cancelled entity (via
CancellationTokenSource) does not hold a slot.
This comment has been minimized.
This comment has been minimized.
…tiation Cap how many avatars may download wearable assets concurrently (MaxAvatarsWithAssetsInFlight, default 3, nearest first): each avatar's promise waits for an in-flight slot before creating its per-wearable asset promises, so avatars complete one after another instead of interleaving downloads in the shared deferred queue and all popping in together. DTO resolution stays bulk-batched and uncapped. A resolution stuck >30s stops holding a slot so it cannot starve the queue. Gate CharacterEmoteSystem.ConsumeEmoteIntent on AvatarCustomSkinningComponent so emote intents wait for the avatar to be instantiated instead of animating the loading ghost, whose renderer is driven by the same skeleton the AvatarBase animator plays on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…xed cap The fixed MaxAvatarsWithAssetsInFlight cap (3) staggered reveals but throttled download concurrency to ~3xwearables, well under the shared 50-slot budget, so a full crowd loaded noticeably slower than the old all-at-once behavior. Replace it with budget-driven admission: each frame admit DTO-ready candidates nearest first only while the shared ConcurrentLoadingPerformanceBudget has free slots, decrementing a local estimate (EstimatedAssetsPerAvatar, default 6) per admission so we fill the pipe without flooding it. The admitted avatars' own downloads saturate the budget, and as each nearby avatar completes and releases budget the next wave is admitted, so admission self-tunes to the completion rate and keeps the pipe full while still revealing avatars one wave after another. Drop the avatar count query, the Age field, and the 30s stuck-timeout: a stalled avatar now holds no budget so it cannot block the queue. AvatarWearableAssetsInFlight becomes a pure admitted tag. Expose the concrete AssetsLoadingBudget from StaticContainer so the system can read its remaining slots. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bb7003d to
ec449df
Compare
|
PR #9890, run #33113992317 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Apple M1
|
2 participants
Pull Request Description
What does this PR change?
Two changes aimed at how avatars feel while loading (profile loading is untouched):
1. Avatars are admitted into wearable-asset downloading a wave at a time, nearest first, sized to keep the download pipe full.
Previously, all avatars' wearable definitions resolved in one bulk-batched request, which put every avatar's asset-bundle promises into the shared deferred-loading queue in the same frame. That queue (one global 50-slot
ConcurrentLoadingPerformanceBudget, sorted only by partition bucket) interleaves downloads across avatars — and since an avatar only instantiates when all of its wearables are resolved, every avatar's last wearable landed at the tail of the whole batch and they all popped in at once.ResolveWearablePromisesSystemnow gates the transition from DTO resolution to asset downloading through budget-driven admission:RawSqrDistance), but only while the sharedConcurrentLoadingPerformanceBudgethas free slots — decrementing a local estimate ofEstimatedAssetsPerAvatar(newWearablePlugin.Settingsfield, default 6, serialized inPlugin Settings.asset) per admission so it fills the pipe without flooding it. At least the nearest candidate is admitted whenever any slot is free, so avatars never stall behind a too-high estimate.AvatarWearableAssetsInFlightmarker (a pure "admitted" tag) so their per-wearable asset promises are created and they don't re-enter the candidate pool.This replaces an earlier revision of this branch that used a fixed
MaxAvatarsWithAssetsInFlightcap (default 3). That cap staggered reveals correctly but throttled download concurrency to ~3×wearables, well under the 50-slot pipe, so a full crowd loaded noticeably slower. Budget-driven admission keeps the stagger while recovering full throughput, and it also avoids the "3 avatars each on their last wearable → pipe idling at 3/50" waste that a fixed cap suffers as avatars wind down.2. Emotes no longer play on loading ghosts.
CharacterEmoteSystem.ConsumeEmoteIntentis now gated onAvatarCustomSkinningComponent, which appears with the first wearable instantiation. The pooledAvatarBase(animator + ghost renderer, both driven by the same skeleton) is attached before wearables load, so a remote emote intent arriving during loading used to animate the ghost. The intent now persists until the avatar is instantiated and then plays; a remote stop received meanwhile still removes the pending intent. The gate applies uniformly to remote players, the local player, SDK avatars, and the backpack preview — they all consume intents through this system.Known trade-offs (intentional for this experiment):
EstimatedAssetsPerAvataris a heuristic: near the real average downloads-per-avatar the pipe stays full with the fewest avatars in flight (tightest stagger at full throughput); lower values are burstier/coarser, higher values stagger more but may under-fill the pipe.Test Instructions
Steps (standard run):
Expected result: In a crowded area (e.g. Genesis Plaza), avatars materialize progressively — closest first, a wave at a time — instead of all appearing in the same instant, and the full crowd finishes loading roughly as fast as before (the download pipe stays busy). Ghosts never play emotes; an avatar that was emoting while loading starts its emote only once its wearables are on.
Test Steps
dev.EstimatedAssetsPerAvatarinPlugin Settings.asset(WearablePlugin section): higher values admit fewer avatars per wave (more staggered, may under-fill the pipe); lower values admit more at once (faster/coarser, approaching the old all-at-once behavior).Additional Testing Notes
dev— the point of budget-driven admission is that the shared 50-slot pipe stays saturated.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.
🤖 Generated with Claude Code