fix(core): disable cache for SceneLoader to avoid loadScene self-destroy#2993
fix(core): disable cache for SceneLoader to avoid loadScene self-destroy#2993cptbtptpbcptdtptp wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2993 +/- ##
===========================================
- Coverage 78.25% 78.09% -0.16%
===========================================
Files 900 900
Lines 99234 99234
Branches 10172 10198 +26
===========================================
- Hits 77657 77499 -158
- Misses 21406 21564 +158
Partials 171 171
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThe PR flips the ChangesScene Loader Registration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/core/src/SceneManager.ts (3)
93-116: Consider documenting concurrent loadScene behavior.If
loadScene(url, true)is called multiple times concurrently with overlapping execution, the second promise resolution will destroy scenes added by the first (lines 108-111 destroy ALL scenes in_scenes). While this may be acceptable given typical usage patterns, it could cause unexpected behavior in edge cases.Consider adding a guard or documentation noting that concurrent
loadScenecalls withdestroyOldScene=trueshould be avoided, or implementing a loading state flag to prevent concurrent loads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/SceneManager.ts` around lines 93 - 116, The loadScene method can concurrently destroy scenes added by overlapping calls when destroyOldScene=true; add a simple guard to prevent concurrent destructive loads by introducing a private flag (e.g., this._isLoadingDestructiveScene) or a pending load tracker (e.g., this._pendingDestructiveLoadUrl) and check it at the start of loadScene(url, destroyOldScene) to either reject/return the existing promise or queue the new request; set the flag before calling resourceManager.load and clear it in the scenePromise.then and its rejection path, and use the existing symbols loadScene, destroyOldScene, scenePromise, this._scenes and resourceManager to locate where to implement the guard.
99-99: ⚡ Quick winAdd defensive check for _virtualPathResourceMap existence.
If
resourceManager._virtualPathResourceMapisundefined, accessing[url]?.pathwill throw a TypeError. Consider adding a defensive check or null coalescing for the entire map.🛡️ Suggested defensive fix
- const realPath = resourceManager._virtualPathResourceMap[url]?.path ?? url; + const realPath = resourceManager._virtualPathResourceMap?.[url]?.path ?? url;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/SceneManager.ts` at line 99, The code assumes resourceManager._virtualPathResourceMap exists before indexing it which can throw if undefined; update the realPath resolution in SceneManager (the line referencing resourceManager._virtualPathResourceMap and url) to defensively access the map (e.g., check or use optional chaining on the entire map before indexing) so you use the mapped path when present and fall back to url otherwise.
99-102: ⚡ Quick winUse the existing
_getRemoteUrl()helper method instead of duplicating its logic.Line 99 duplicates the logic from
ResourceManager._getRemoteUrl()(which doesthis._virtualPathResourceMap[url]?.path ?? url). Replace the inline logic with:const realPath = resourceManager._getRemoteUrl(url);This avoids duplicating the internal path resolution logic and keeps the code maintainable if the resolution strategy changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/SceneManager.ts` around lines 99 - 102, The code duplicates ResourceManager path-resolution logic by inlining this._virtualPathResourceMap[url]?.path ?? url to compute realPath; replace that inline expression with a call to resourceManager._getRemoteUrl(url) so the SceneManager uses ResourceManager's canonical resolution strategy (affecting realPath, the subsequent getFromCache<Scene>(realPath) lookup, and the _deleteAsset(cached) branch).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/SceneManager.ts`:
- Around line 93-116: The loadScene method can concurrently destroy scenes added
by overlapping calls when destroyOldScene=true; add a simple guard to prevent
concurrent destructive loads by introducing a private flag (e.g.,
this._isLoadingDestructiveScene) or a pending load tracker (e.g.,
this._pendingDestructiveLoadUrl) and check it at the start of loadScene(url,
destroyOldScene) to either reject/return the existing promise or queue the new
request; set the flag before calling resourceManager.load and clear it in the
scenePromise.then and its rejection path, and use the existing symbols
loadScene, destroyOldScene, scenePromise, this._scenes and resourceManager to
locate where to implement the guard.
- Line 99: The code assumes resourceManager._virtualPathResourceMap exists
before indexing it which can throw if undefined; update the realPath resolution
in SceneManager (the line referencing resourceManager._virtualPathResourceMap
and url) to defensively access the map (e.g., check or use optional chaining on
the entire map before indexing) so you use the mapped path when present and fall
back to url otherwise.
- Around line 99-102: The code duplicates ResourceManager path-resolution logic
by inlining this._virtualPathResourceMap[url]?.path ?? url to compute realPath;
replace that inline expression with a call to resourceManager._getRemoteUrl(url)
so the SceneManager uses ResourceManager's canonical resolution strategy
(affecting realPath, the subsequent getFromCache<Scene>(realPath) lookup, and
the _deleteAsset(cached) branch).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4f5f2e47-1909-4c4b-a9e3-acc848386503
📒 Files selected for processing (1)
packages/core/src/SceneManager.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Scene is a live runtime tree, not an immutable asset. Caching the loaded
Scene caused a self-destroy race when loadScene(url) was called with a URL
whose cached Scene was the current active scene:
1. resourceManager.load<Scene>({url}) returned the cached (= currently
active) Scene instance
2. SceneManager.loadScene then entered the destroyOldScene branch and
destroyed the old scenes — including the one just returned
3. The "new" scene was the same object, now destroyed: rootEntities
empty, native PhysicsScene released, screen blank, no error logged
The root cause is the cache-vs-construct conflict. A Scene is both an
asset (a JSON blob on disk) and a constructed live runtime. Caching the
constructed instance and returning it for "load same URL again" violates
the user's intent ("reload this level").
Other engines (Unity SceneManager.LoadScene, Cocos director.loadScene,
Unreal OpenLevel) all create a fresh Scene per load — none cache.
Aligning Galacean to the same convention.
`PrimitiveMeshLoader` and `ProjectLoader` already use `useCache: false`
for similar reasons (constructed at load time, not immutable assets),
so the pattern is established in the codebase.
cptbtptpbcptdtptp
force-pushed
the
fix/scene-asset-cache-eviction
branch
from
2dc6913 to
e1b18a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/loader/src/SceneLoader.ts`:
- Line 170: The resourceLoader registration currently sets useCache globally for
AssetType.Scene (the decorator at resourceLoader(AssetType.Scene, ["scene"],
false)), which disables caching for all SceneLoader instances and breaks
additive loads; instead revert the registration to allow normal caching and
implement targeted eviction inside SceneManager.loadScene: keep the
decorator/SceneLoader registration default (allow cache), and when
loadScene(...) is called with destroyOldScene === true, explicitly evict or
reload the specific scene from the loader/cache (e.g., call the SceneLoader
instance's cache invalidation method or a cache.clearFor(sceneId) function)
before loading; reference resourceLoader(AssetType.Scene,...), the SceneLoader
class, and SceneManager.loadScene(destroyOldScene) to locate and modify the
code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 93d29346-a827-4e96-b28c-9ee3b0cfecba
📒 Files selected for processing (1)
packages/loader/src/SceneLoader.ts
| } | ||
|
|
||
| @resourceLoader(AssetType.Scene, ["scene"], true) | ||
| @resourceLoader(AssetType.Scene, ["scene"], false) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify resourceLoader third-arg semantics and scene load call-path impact (read-only).
set -euo pipefail
echo "== Locate resourceLoader declaration/signature =="
rg -n -C3 '\bfunction\s+resourceLoader\b|\bconst\s+resourceLoader\b|\bexport\s+function\s+resourceLoader\b'
echo
echo "== Find all Scene loader registrations and third argument values =="
rg -n -C2 '@resourceLoader\(\s*AssetType\.Scene'
echo
echo "== Find SceneManager.loadScene implementation and cache eviction logic =="
rg -n -C6 '\bclass\s+SceneManager\b|\bloadScene\s*\(' packages
echo
echo "== Find internal cache operations relevant to scene assets =="
rg -n -C4 '\b_deleteAsset\b|\bget\b.*Asset|virtualPath|cache' packagesRepository: galacean/engine
Length of output: 50371
The cache behavior change is global and affects additive scene loads unintentionally.
Setting useCache: false on the Scene loader registration at line 170 disables caching for all SceneManager.loadScene() calls, regardless of the destroyOldScene parameter. The resourceLoader decorator passes useCache to the loader constructor at registration time, not per-load call, so additive scene loads (destroyOldScene = false) will also bypass the cache. The PR objective specifies targeted cache eviction only when destroying old scenes, but this implementation affects both destructive and additive loads equally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/loader/src/SceneLoader.ts` at line 170, The resourceLoader
registration currently sets useCache globally for AssetType.Scene (the decorator
at resourceLoader(AssetType.Scene, ["scene"], false)), which disables caching
for all SceneLoader instances and breaks additive loads; instead revert the
registration to allow normal caching and implement targeted eviction inside
SceneManager.loadScene: keep the decorator/SceneLoader registration default
(allow cache), and when loadScene(...) is called with destroyOldScene === true,
explicitly evict or reload the specific scene from the loader/cache (e.g., call
the SceneLoader instance's cache invalidation method or a
cache.clearFor(sceneId) function) before loading; reference
resourceLoader(AssetType.Scene,...), the SceneLoader class, and
SceneManager.loadScene(destroyOldScene) to locate and modify the code.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Adds regression tests for SceneLoader cache policy: - SceneLoader.useCache === false (the fix in this PR) - PrimitiveMeshLoader.useCache === false (existing convention, contrast) - Texture2D loader.useCache === true (immutable asset, contrast) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/src/core/resource/SceneLoaderCache.test.ts`:
- Around line 9-11: Add an afterAll teardown that destroys the WebGLEngine
created in beforeAll to prevent WebGL resource leaks: implement afterAll(async
() => { if (engine) await engine.destroy(); }); referencing the existing engine
variable and the WebGLEngine API (engine.destroy) so the test file
SceneLoaderCache.test.ts cleans up after itself and avoids cross-test
contamination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bd3993f9-f2cd-4660-8771-990d197fcbcb
📒 Files selected for processing (1)
tests/src/core/resource/SceneLoaderCache.test.ts
| beforeAll(async () => { | ||
| engine = await WebGLEngine.create({ canvas: document.createElement("canvas") }); | ||
| }); |
There was a problem hiding this comment.
Add teardown for WebGLEngine to avoid test resource leaks.
WebGLEngine is created in beforeAll but never destroyed. Please add afterAll(async () => { await engine.destroy(); }) (or the engine’s correct dispose API) to prevent cross-test contamination and WebGL resource leaks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/core/resource/SceneLoaderCache.test.ts` around lines 9 - 11, Add an
afterAll teardown that destroys the WebGLEngine created in beforeAll to prevent
WebGL resource leaks: implement afterAll(async () => { if (engine) await
engine.destroy(); }); referencing the existing engine variable and the
WebGLEngine API (engine.destroy) so the test file SceneLoaderCache.test.ts
cleans up after itself and avoids cross-test contamination.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
缓存 Scene 后 loadScene(..., destroyOldScene=true) 会把刚加载的同一 Scene 实例当作旧 Scene 销毁——禁用缓存后每次 loadScene 得到新实例，旧/新 Scene 必然是不同对象，问题消除。一行改动，方向正确。
测试验证了核心断言（useCache === false），额外的 PrimitiveMesh/Texture2D 对照测试属于增量覆盖，不阻塞合并。
LGTM，可合入。
2 participants
Disable resource cache for SceneLoader so loadScene always produces a fresh Scene instance.
Bug: loadScene(url) with destroyOldScene=true, cached Scene is active scene -> resourceManager.load returns same instance -> then-handler destroys it -> rootEntities empty, PhysicsScene released, screen blank.
Fix: useCache true to false for SceneLoader. Consistent with PrimitiveMeshLoader and ProjectLoader.
Aligned with Unity/Unreal which all create fresh Scene instances.
Origin: Replaces evict workaround from commit 599a7e6 on fix/shaderlab (by @luzhuang) with root-cause fix.
Summary by CodeRabbit