Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…when size increase
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review Summary
Well-structured PR that introduces a concurrent loading budget for avatar wearables and splits the bone matrix pipeline into dedicated main-player and remote-avatar tracks. The architecture is clean, the budget lifecycle (acquire → load → release on instantiate/cleanup) is consistent across all code paths, and the test coverage is solid.
Highlights
- Budget-gated loading is well integrated — main player always bypasses, SDK component and Profile paths both defer correctly, and cleanup/delete paths properly release the budget.
- Pipeline split (
MainPlayerPipeline/RemoteAvatarPipeline) is a smart move. Completing the main player immediately avoids TransformAccessArray lock contention withInterpolateCharacterSystem. WearableLoadingStatewrapper is a clear improvement over relying onIsConsumed/ default checks on the raw promise. The explicitNone → Loading → Consumedstate machine makes the flow much easier to reason about.- Fallback try/catch in
AvatarInstantiatorSystemfor failed instantiation with retry using a fallback body shape is a nice resilience improvement. - Tests —
AvatarLoadingBudgetShouldcovers the key scenarios: budget exhaustion, deferred loading, release-on-delete, main player bypass, and re-acquisition on profile update.
Items to discuss
-
Default budget mismatch — The PR description says the default concurrent limit is 3, but
AvatarShapeSettings.maxConcurrentAvatarLoadsis set to5. Which value is intended for production? -
AvatarRootGatherJob— full matrix inverse —math.inverse((float4x4)transform.localToWorldMatrix)computes a general 4×4 inverse. Since avatar root transforms are rigid (uniform scale),math.fastinversewould be cheaper (transpose of rotation + negated translation).TransformAccessdoesn't exposeworldToLocalMatrixdirectly, so this is the only option — butfastinversewould be a low-risk perf win for the remote batch where it runs per-slot including dummy slots. -
High-water-mark scheduling —
RemoteAvatarPipeline.Schedule(batchCount)schedulesavatarIndextasks (the high-water mark), not the count of active avatars. Released slots are skipped viaUpdateAvatar[idx] = false, but after heavy churn (many avatars created then destroyed), the job iterates over all ghost slots. Consider compactingavatarIndexwhenreleasedIndexes.Countgrows large, or tracking an active count. -
WearableLoadingStateis a heap-allocated class inside a struct — EveryAvatarShapeComponentallocates anew WearableLoadingState(). This is clearly intentional for reference semantics (markedreadonly), but worth a brief doc comment noting why it's a class — future maintainers might be tempted to convert it to a struct. -
BoneMatrixCalculationJobparallelism change — Previously each bone was a parallel task; now each avatar is a task with an inner bone loop. With the batch count of 4, this means up to 4 avatars process per worker thread chunk. The Burst auto-vectorization of the inner loop should compensate, but if the remote avatar count stays low (≤5), this effectively serializes the bone calculation. Not a problem in practice given the budget limit, just noting the tradeoff. -
Minor:
[NativeDisableParallelForRestriction]onbonesMatricesResult— Added because each avatar task now writes to a range[offset..offset+boneCount). The ranges are non-overlapping so this is safe, but a brief comment explaining why the restriction is disabled would help reviewers.
Overall this looks good. The budget lifecycle is consistent, cleanup paths are covered, and the pipeline split is well-motivated. No blocking issues found.
Requested by Juan Ignacio Molteni via Slack
|
|
||
| public void Execute(int index, TransformAccess transform) | ||
| { | ||
| MatrixFromAllAvatars[index] = math.inverse((float4x4)transform.localToWorldMatrix); |
There was a problem hiding this comment.
Perf suggestion: Since avatar root transforms are rigid bodies (rotation + translation, uniform scale), math.fastinverse would be significantly cheaper than a full math.inverse here. fastinverse assumes an orthonormal rotation matrix and simply transposes the 3×3 + negates the translation — no cofactor expansion needed.
MatrixFromAllAvatars[index] = math.fastinverse((float4x4)transform.localToWorldMatrix);This runs for every slot (including dummies), so the savings accumulate.
| /// <summary> | ||
| /// Wraps a <see cref="WearablePromise"/> with an explicit loading status | ||
| /// so callers never rely on <c>default</c> or <c>IsConsumed</c> to decide | ||
| /// whether loading has been requested. |
There was a problem hiding this comment.
Consider adding a brief doc comment explaining why this is a class rather than a struct — e.g. "Reference type so that copies of the enclosing AvatarShapeComponent struct share the same loading state instance." This will prevent future contributors from accidentally converting it.
| [field: SerializeField] | ||
| public int defaultMaterialCapacity = 100; | ||
|
|
||
| [field: SerializeField] |
There was a problem hiding this comment.
The PR description says the default is 3, but this is set to 5. Which is the intended production default? Might be worth aligning the description or adding a comment explaining the choice.
| { | ||
| private readonly int boneCount; | ||
|
|
||
| [NativeDisableParallelForRestriction] |
There was a problem hiding this comment.
Minor: worth a one-liner explaining why the restriction is disabled — e.g. "Each avatar task writes to a non-overlapping [avatarIdx * boneCount .. (avatarIdx+1) * boneCount) range."
mikhail-dcl
left a comment
mikhail-dcl
left a comment
There was a problem hiding this comment.
I will review the part with matrices calculations separately
| HiddenByModifierArea = false; | ||
| IsPreview = false; | ||
| ShowOnlyWearables = showOnlyWearables; | ||
| this.LoadingBudget = loadingBudget; |
There was a problem hiding this comment.
You always pass NoBudget to the ctor, so maybe you just should assign it here straight away without passing an argument?
| /// so callers never rely on <c>default</c> or <c>IsConsumed</c> to decide | ||
| /// whether loading has been requested. | ||
| /// </summary> | ||
| public class WearableLoadingState |
There was a problem hiding this comment.
It's not the best idea to store the class ref in the structure due to the memory locality (and allocations of course), I checked a few usages and you have a ref to the original component when you call methods of this class, so I wonder what the real necessity is for having a class?
| if (avatarShapeComponent.WearableLoading.Status != AvatarShapeComponent.WearableLoadingStatus.None) | ||
| return; | ||
|
|
||
| if (!avatarLoadingBudget.TrySpendBudget(out IAcquiredBudget acquiredBudget)) |
There was a problem hiding this comment.
As you ignore PartitionComponent on spending budget, you are at a risk of resolving avatars further away first
| if (!ReadyToInstantiateNewAvatar(ref avatarShapeComponent)) return; | ||
|
|
||
| if (!avatarShapeComponent.WearablePromise.SafeTryConsume(World, GetReportCategory(), out WearablesLoadResult wearablesResult)) return; | ||
| if (!avatarShapeComponent.WearableLoading.SafeTryConsume(World, GetReportData(), out WearablesLoadResult wearablesResult)) return; |
There was a problem hiding this comment.
Before SafeTryConsume was here because of the unrelated concurrency issue, SafeTryConsume is a workaround that shouldn't exist. Considering you are already doing the changes and moved this method to the new class, please check if it can be simply replaced with TryConsume which is not a workaround
3 participants
Pull Request Description
What does this PR change?
When many avatars appear at once (e.g., teleporting to a crowded area),
AvatarLoaderSystemcreatesWearablePromisefor every avatar immediately. Each avatar triggers ~5-7 asset bundle downloads competing for bandwidth, causing slow loading for all avatars.This PR adds a
ConcurrentLoadingPerformanceBudgetthat limits how many avatars can load wearables concurrently (default: 3). Avatars beyond the limit are deferred until slots free up.Key changes:
AvatarShapeComponent: AddedIAcquiredBudget LoadingBudgetfield andIsWearableInstantiatedproperty to track budget ownership and wearable readinessAvatarLoaderSystem: Budget-gates non-player Profile-based avatar creation and updates. Entities without budget skipAvatarShapeComponentcreation and retry each frame. Main player always bypasses budget. Uses existingIAcquiredBudget/AcquiredBudgetpattern for safe, idempotent releaseAvatarInstantiatorSystem: Releases budget viaLoadingBudget.Release()after avatar instantiation completes (inInstantiateAvatar)AvatarCleanUpSystem: NewDestroyPendingAvatarquery releases budget for entities deleted before instantiation (noAvatarBase)AvatarPlugin: NewmaxConcurrentAvatarLoadssetting (default 3), createsConcurrentLoadingPerformanceBudgetand passes it toAvatarLoaderSystemCharacterEmoteSystem: Defers emote downloading/playback until avatar wearables are instantiated (IsWearableInstantiated)AvatarGhostSystem: UsesIsWearableInstantiatedinstead of rawInstantiatedWearables.CountcheckSDK component paths (PBAvatarShape) are not budgeted in this PR — will be handled separately.
Test Instructions
Prerequisites
Test Steps
NoAcquiredBudget)Additional Testing Notes
"Tried to release more budget than the max budget allows"exceptionsQuality Checklist