Fix: Reference Counting Cache refactoring #2182
Conversation
- Textures referencing, Sprites handling, all systems are changed accordingly - UIBackground - NFT - Avatar Attachment Thumbnails - Materials - Map Pins - Common Ref Counting Data - Asset Bundle Cache, Audio Cache, Profiles Cache are unified
- Unify Video Texture with Texture2DData - Fix tests
# Conflicts: # Explorer/Assets/DCL/PluginSystem/Global/MultiplayerMovementPlugin.cs
|
Windows and Mac build successfull in Unity Cloud! You can find a link to the downloadable artifact below.
|
There was a problem hiding this comment.
🟢 QA Approved. The assets no longer disappear after pressing the FULL button in memory settings.
This fix was verified on Windows and Mac. A smoke test was performed, covering:
- Backpack
- Emotes
- Nav bar (all shortcuts)
- Badges/Profile Customization
- Map navigation
- Scenes/worlds (Doll House/Seed/Casa Roustan/Metadynelabs).
| using Object = UnityEngine.Object; | ||
|
|
||
| namespace ECS.StreamableLoading.AssetBundles | ||
| { | ||
| /// <summary> | ||
| /// A wrapper over <see cref="AssetBundle" /> to provide additional data | ||
| /// </summary> | ||
| public class AssetBundleData : IAssetData | ||
| public class AssetBundleData : StreamableRefCountData<AssetBundle>, IStreamableRefCountData |
There was a problem hiding this comment.
IStreamableRefCountData is redudant here
| namespace ECS.StreamableLoading.Cache | ||
| { | ||
| public abstract class RefCountStreamableCacheBase<TAssetData, TAsset, TLoadingIntention> : IEqualityComparer<TLoadingIntention> | ||
| where TAssetData: StreamableRefCountData<TAsset> where TAsset: class |
There was a problem hiding this comment.
As discussed in DM, can we make this abstract class implement IStreamableCache so it appears in Rider? Right now, it doesnt.
Also, some redudancy will appear when doing so, the explicit inheritance wont be needed in the child of this class.
| public bool TryGet(in TLoadingIntention key, out TAssetData asset) => | ||
| cache.TryGetValue(key, out asset); | ||
|
|
||
| public void AddReference(in TLoadingIntention _, TAssetData asset) |
There was a problem hiding this comment.
Maybe we can make this one internal since its only used for testing?
| /// Needed for non-ECS code to properly handle reference counting | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public RefAcquisition AcquireRef() => |
There was a problem hiding this comment.
Isnt it a little bit YAGNI? Some examples come to mind? As far as I see, this is not beieng used.
There was a problem hiding this comment.
We have a mess with profiles where it will be needed, I wanted to address it straight-away but the scope was too big
There was a problem hiding this comment.
I would like to leave it here even for the existing assets so e.g. if the texture is requested from UI it can be owned and later unowned
| { | ||
| private int referencesCount; | ||
| public AudioClip AudioClip => Asset; |
There was a problem hiding this comment.
This is not used anywhere. Should we remove it for clarity? Seems like Asset is the way to access the Audio
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| // On Dispose video textures are dereferenced by material that acquired it |
There was a problem hiding this comment.
I think we have a leak here.
This texture was never added to the texture cache, since its not created through LoadSystemBase, but rather on the fly by VideoTextureUtils, gotten through the videoTexturesPool.I understand that by this point it should not hold any reference since it was dereferenced by the material that it was using it and now destroyed.
But shouldnt it be returned to the pool here, rather than nullifying it?
Whatsmore, cleaning of this pool cache for this pool is not clear.
I see that its injected in MediaPlayerPluginWrapper and then registered to the cache cleanear, as part of the avatarPools. The name is super misleading, but Im ignoring that. When this type of pool cache is cleared in ExtendedObjectPool.ClearThrottled, only objects in the pool are cleared. But I couldnt find the place where we returned the video texture to the pool.
It may be out of scopre of this PR, but as I see it, I think we have a leak here.
There was a problem hiding this comment.
It's the only place where we return the video texture to the pool and call Dispose:
private void CleanUpVideoTexture(ref VideoTextureConsumer videoTextureConsumer)
{
videoTexturesPool.Release(videoTextureConsumer.Texture);
videoTextureConsumer.Dispose();
}
in CleanUpMediaPlayerSystem
and VideoTextureConsumer itself is not bound to the pool itself (by design), that's why it does not handle returning to the pool.
I preserved the original design
# Conflicts: # Explorer/Assets/DCL/AvatarRendering/Emotes/Components/IEmote.cs # Explorer/Assets/DCL/SDKComponents/SceneUI/Systems/UIBackground/UIBackgroundInstantiationSystem.cs
Fixes #2126
RefCountStreamableCacheBasefor:ProfilesAsset BundlesAudio ClipsNft ShapeTexturesStreamableRefCountData<TAsset>, it is the base class for all cached assets with ref countingMaterialsalready were releasing properly: they required minimum adjustmentsHow to QA
Following this pattern:
Memory.FULLinDebug MenuMemory.NORMALAll the affected assets should be verified:
For Avatar Attachments check that thumbnails do not disappear (backpack, passport) after setting
Memory.FULL