Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,22 @@ public void Unload(IPerformanceBudget frameTimeBudget, int maxUnloadAmount)
ProfilingCounters.GltfInCacheAmount.Value -= unloadedAmount;
}

public void Remove(in string key)
{
if (cache.TryGetValue(key, out List<GltfContainerAsset> assets))
{
foreach (GltfContainerAsset asset in assets)
asset.Dispose();

ProfilingCounters.GltfInCacheAmount.Value -= assets.Count;
assets.Clear();
cache.Remove(key);
unloadQueue.TryRemove(key);
}

IrrecoverableFailures.Remove(key);
}

bool IEqualityComparer<string>.Equals(string x, string y) =>
string.Equals(x, y, StringComparison.OrdinalIgnoreCase);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ public interface IGltfContainerAssetsCache

void Unload(IPerformanceBudget frameTimeBudget, int maxUnloadAmount);

/// <summary>
/// Evict a single cache key, disposing every pooled asset under it. Lets an edited GLTF be
/// dropped in isolation while the rest of the cache stays warm across a scene reload.
/// </summary>
void Remove(in string key);

void Dereference(in string key, GltfContainerAsset asset, bool putInBridge = false, bool handleAssetLoad = true);

void SetAssetLoadCache(AssetPreLoadCache assetPreLoadCache);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ public interface IStreamableCache<TAsset, TLoadingIntention> : IDisposable where
/// </summary>
void Unload(IPerformanceBudget frameTimeBudget, int maxUnloadAmount);

/// <summary>
/// Evict a single entry by key, disposing its asset when it is no longer referenced.
/// Mirrors <see cref="Unload" /> scoped to one key, so an edited asset can be dropped
/// without draining the whole cache. Default is a no-op for caches that hold nothing.
/// </summary>
void Remove(in TLoadingIntention key) { }

#region Referencing
/// <summary>
/// Base implementation is empty as not every asset requires reference counting
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,5 +79,23 @@ public void Unload(IPerformanceBudget frameTimeBudget, int maxUnloadAmount)

inCacheCount.Value = cache.Count;
}

public void Remove(in TLoadingIntention key)
{
if (!cache.TryGetValue(key, out TAssetData asset) || !asset.CanBeDisposed())
return;

asset.Dispose();
cache.Remove(key);

for (int i = listedCache.Count - 1; i >= 0; i--)
if (IntentionsComparer<TLoadingIntention>.INSTANCE.Equals(listedCache[i].intention, key))
{
listedCache.RemoveAt(i);
break;
}

inCacheCount.Value = cache.Count;
Comment thread
dalkia marked this conversation as resolved.
}
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using DCL.Profiling;
using ECS.StreamableLoading.Cache;
using GLTFast;
using System;
using Unity.Profiling;

namespace ECS.StreamableLoading.GLTF
Expand All @@ -13,5 +14,23 @@ namespace ECS.StreamableLoading.GLTF
public class GltfLoadCache : RefCountStreamableCacheBase<GLTFData, GltfImport, GetGLTFIntention>
{
protected override ref ProfilerCounterValue<int> inCacheCount => ref ProfilingCounters.GltfDataInCache;

/// <summary>
/// Evict every entry whose content hash matches, regardless of the Name it was loaded
/// under. <see cref="GetGLTFIntention" /> identity is (Name, Hash) and Name is the verbatim
/// src string from scene code, which a caller evicting a changed file cannot reconstruct
/// reliably — the hash alone identifies the content.
/// </summary>
public void RemoveByHash(string hash)
{
if (string.IsNullOrEmpty(hash))
return;

for (int i = listedCache.Count - 1; i >= 0; i--)
{
if (StringComparer.OrdinalIgnoreCase.Equals(listedCache[i].intention.Hash, hash))
Remove(listedCache[i].intention);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using Arch.Core;
using Cysharp.Threading.Tasks;
using DCL.Character.Components;
using DCL.Ipfs;
using DCL.ResourcesUnloading;
using ECS.LifeCycle.Components;
using ECS.SceneLifeCycle.Components;
Expand Down Expand Up @@ -44,19 +45,19 @@ public ECSReloadScene(IScenesCache scenesCache,
var foundEntity = FindSceneEntity(sceneInCache);
if (foundEntity == Entity.Null) return null;

await DisposeAndRestartAsync(foundEntity, sceneInCache, ct);
await DisposeAndRestartAsync(foundEntity, sceneInCache, null, ct);

return sceneInCache;
}

public async UniTask<ISceneFacade?> TryReloadSceneAsync(CancellationToken ct, string sceneId)
public async UniTask<ISceneFacade?> TryReloadSceneAsync(CancellationToken ct, string sceneId, string? changedModelSrc = null)
{
if (!scenesCache.TryGetBySceneId(sceneId, out var sceneInCache)) return null;

var foundEntity = FindSceneEntity(sceneInCache!);
if (foundEntity == Entity.Null) return null;

await DisposeAndRestartAsync(foundEntity, sceneInCache!, ct);
await DisposeAndRestartAsync(foundEntity, sceneInCache!, changedModelSrc, ct);

return sceneInCache;
}
Expand All @@ -74,10 +75,15 @@ private Entity FindSceneEntity(ISceneFacade targetScene)
return sceneEntity;
}

private async UniTask DisposeAndRestartAsync(Entity entity, ISceneFacade currentScene, CancellationToken ct)
private async UniTask DisposeAndRestartAsync(Entity entity, ISceneFacade currentScene, string? changedModelSrc, CancellationToken ct)
{
ct.ThrowIfCancellationRequested();

// Captured before the teardown below strips the scene entity's components.
SceneEntityDefinition? definition = localSceneDevelopment
? world.Get<SceneDefinitionComponent>(entity).Definition
: null;

//There is a lingering promise we need to remove, and add the DeleteEntityIntention to make the standard unload flow.
world.Add<DeleteEntityIntention>(entity);

Expand All @@ -96,11 +102,23 @@ private async UniTask DisposeAndRestartAsync(Entity entity, ISceneFacade current
world.Query(in new QueryDescription().WithAll<RealmComponent>(),
(ref StaticScenePointers staticScenePointers) => { staticScenePointers.Promise = null; });

// Force-drain dereferenced caches on LSD reload. The local dev server derives hashes
// from the file path, not content, so an updated model keeps the same hash and cache
// hits would return stale assets. Draining guarantees fresh loads.
cacheCleaner.UnloadCache(budgeted: false);
Resources.UnloadUnusedAssets();
if (changedModelSrc != null
&& TryResolveContentHash(definition, changedModelSrc, out string contentHash)
&& IsRawGltfModel(definition, contentHash))
{
// The dev server named the exact model that changed. In raw-GLTF development its
// cache key is the bare content hash, so evict just that asset and let every other
// cache stay warm across the reload.
cacheCleaner.EvictGltfModel(contentHash);
}
else
{
// Force-drain dereferenced caches on LSD reload. The local dev server derives hashes
// from the file path, not content, so an updated model keeps the same hash and cache
// hits would return stale assets. Draining guarantees fresh loads.
cacheCleaner.UnloadCache(budgeted: false);
Resources.UnloadUnusedAssets();
}

await WaitUntilNewSceneIsFullyLoadedAsync();
}
Expand Down Expand Up @@ -134,5 +152,72 @@ await UniTask.WaitUntil(() =>
}, cancellationToken: ct);
}
}

/// <summary>
/// Resolve the changed file to the hash the caches are actually keyed on. The reload
/// message's own hash is minted from the watcher-relative path, while every cache key
/// derives from the content-mapping hash (minted from the absolute path) — the two never
/// match, so the file must be joined to the definition's content list by its src instead.
/// </summary>
internal static bool TryResolveContentHash(SceneEntityDefinition? definition, string src, out string hash)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Nullability annotation — same pattern as IsRawGltfModel: src is declared non-nullable string but guarded with string.IsNullOrEmpty(src). The caller in DisposeAndRestartAsync passes a string? (changedModelSrc) that flows through a null-check before reaching here, so in practice src is always non-null at the call site — but the method's own guard implies it accepts null. Either remove the IsNullOrEmpty guard on src (trusting the non-nullable contract) or declare string? to match the actual behavior.

Suggested change
internal static bool TryResolveContentHash(SceneEntityDefinition? definition, string src, out string hash)
internal static bool TryResolveContentHash(SceneEntityDefinition? definition, string? src, out string hash)

{
hash = string.Empty;

ContentDefinition[]? content = definition?.content;

if (content == null || string.IsNullOrEmpty(src))
return false;

foreach (ContentDefinition entry in content)
{
if (ContentPathEquals(entry.file, src))
{
hash = entry.hash;
return true;
}
}

return false;
}

/// <summary>
/// Case- and separator-insensitive content path comparison. Content mappings always spell
/// paths with '/', while the local dev server's file watcher reports the platform separator —
/// on Windows that is '\', so an ordinal comparison never matches there even though the two
/// name the same file. Compared in place to keep the reload path allocation-free.
/// </summary>
private static bool ContentPathEquals(string? contentFile, string src)
{
if (contentFile == null || contentFile.Length != src.Length)
return false;

for (var i = 0; i < contentFile.Length; i++)
{
char a = contentFile[i];
char b = src[i];

if (a == '\\') a = '/';
if (b == '\\') b = '/';

if (a != b && char.ToLowerInvariant(a) != char.ToLowerInvariant(b))
return false;
}

return true;
}

/// <summary>
/// True when the hash addresses a raw GLTF, i.e. no asset-bundle manifest maps it. The GLTF
/// container cache is keyed by <see cref="AssetBundleManifestVersion.ComposeCacheKey" />,
/// which returns the bare hash only in that case; under <c>--local-ab</c> the key differs and
/// the model lives in the asset-bundle caches instead, so scoped eviction must not be used.
/// </summary>
internal static bool IsRawGltfModel(SceneEntityDefinition? definition, string hash)
Comment thread
dalkia marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Nullability annotation (repeat from previous review) — hash is declared string (non-nullable) but is null-checked via string.IsNullOrEmpty. Per CLAUDE.md anti-patterns: "Defensive null-checks against non-null declarations — if the declared type is T (not T?), don't null-check it." Since null is a legitimate defensive case (the test in ECSReloadSceneShould exercises it), the parameter should honestly declare string?. This also lets the test pass null directly instead of null!.

Suggested change
internal static bool IsRawGltfModel(SceneEntityDefinition? definition, string hash)
internal static bool IsRawGltfModel(SceneEntityDefinition? definition, string? hash)

{
if (definition == null || string.IsNullOrEmpty(hash))
return false;

return definition.AssetBundleManifestVersionOrFailed.ComposeCacheKey(hash) == hash;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,23 @@ private async UniTask ConnectToServerAsync(string localSceneWebsocketServer,
wsSceneMessage.MergeFrom(receiveBuffer.AsSpan(0, receiveResult.Count));
ReportHub.Log(ReportCategory.SDK_LOCAL_SCENE_DEVELOPMENT, $"Websocket scene message received: {wsSceneMessage.MessageCase}");

// TODO: Discriminate 'wsSceneMessage.MessageCase == WsSceneMessage.MessageOneofCase.UpdateModel' to only update GLTF models...
// An UpdateModel message names the single GLTF that changed; carry its src through so
// the reload can evict just that asset instead of draining every cache. The message's
// own hash is unusable — it is minted from the watcher-relative path while cache keys
// derive from the content-mapping hash, so the reload resolves the hash by src itself.
string sceneId;
string? changedModelSrc;

if (wsSceneMessage.MessageCase == WsSceneMessage.MessageOneofCase.UpdateModel)
{
sceneId = wsSceneMessage.UpdateModel.SceneId;
changedModelSrc = wsSceneMessage.UpdateModel.Src;
}
else
{
sceneId = wsSceneMessage.UpdateScene.SceneId;
changedModelSrc = null;
}

// Switch to the main thread because `TryReloadSceneAsync` requires that
await UniTask.SwitchToMainThread(cancellationToken: ct);
Expand All @@ -86,8 +102,7 @@ private async UniTask ConnectToServerAsync(string localSceneWebsocketServer,
// And pause the skybox update while loading to avoid transitions
globalWorld.AddOrGet(skyboxEntity, new PauseSkyboxTimeUpdate());

await reloadScene.TryReloadSceneAsync(ct,
wsSceneMessage.MessageCase == WsSceneMessage.MessageOneofCase.UpdateScene ? wsSceneMessage.UpdateScene.SceneId : wsSceneMessage.UpdateModel.SceneId)
await reloadScene.TryReloadSceneAsync(ct, sceneId, changedModelSrc)
.Timeout(TimeSpan.FromSeconds(RELOAD_SCENE_TIMEOUT_SECS));
}
catch (TimeoutException) { }
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
using DCL.Ipfs;
using ECS.SceneLifeCycle;
using NUnit.Framework;

namespace DCL.SceneLifeCycle.Tests
{
public class ECSReloadSceneShould
{
[Test]
public void TreatDefinitionWithoutManifestAsRawGltf()
{
//Arrange: no asset-bundle manifest -> the container cache is keyed by the bare hash
SceneEntityDefinition definition = CreateDefinition();

//Act & Assert
Assert.That(ECSReloadScene.IsRawGltfModel(definition, "b64-somehash"), Is.True);
}

[Test]
public void NotTreatMissingDefinitionOrEmptyHashAsRawGltf()
{
SceneEntityDefinition definition = CreateDefinition();

Assert.That(ECSReloadScene.IsRawGltfModel(null, "b64-somehash"), Is.False);
Assert.That(ECSReloadScene.IsRawGltfModel(definition, string.Empty), Is.False);
Assert.That(ECSReloadScene.IsRawGltfModel(definition, null!), Is.False);
}

[Test]
public void ResolveContentHashBySrcIgnoringCase()
{
//Arrange
SceneEntityDefinition definition = CreateDefinition(
new ContentDefinition { file = "models/shark.glb", hash = "b64-content-hash" });

//Act
bool resolved = ECSReloadScene.TryResolveContentHash(definition, "Models/Shark.GLB", out string hash);

//Assert
Assert.That(resolved, Is.True);
Assert.That(hash, Is.EqualTo("b64-content-hash"));
}

[Test]
public void ResolveContentHashWhenSrcUsesWindowsSeparators()
{
//Arrange: content mappings always spell paths with '/', but the local dev server's file
//watcher reports the platform separator — on Windows that is '\'.
SceneEntityDefinition definition = CreateDefinition(
new ContentDefinition { file = "assets/models/out/models/BenchStreet.glb", hash = "b64-content-hash" });

//Act
bool resolved = ECSReloadScene.TryResolveContentHash(definition, @"assets\models\out\models\BenchStreet.glb", out string hash);

//Assert
Assert.That(resolved, Is.True);
Assert.That(hash, Is.EqualTo("b64-content-hash"));
}

[Test]
public void NotResolveContentHashWhenSrcIsUnknownOrMissing()
{
//Arrange
SceneEntityDefinition definition = CreateDefinition(
new ContentDefinition { file = "models/shark.glb", hash = "b64-content-hash" });

//Act & Assert
Assert.That(ECSReloadScene.TryResolveContentHash(definition, "models/monster.glb", out _), Is.False);
Assert.That(ECSReloadScene.TryResolveContentHash(definition, string.Empty, out _), Is.False);
Assert.That(ECSReloadScene.TryResolveContentHash(null, "models/shark.glb", out _), Is.False);
}

private static SceneEntityDefinition CreateDefinition(params ContentDefinition[] content) =>
new ("test-scene", new SceneMetadata())
{
pointers = new[] { "0,0" },
content = content,
};
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading