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.
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.
增量审查（最终）
已关闭问题清单
| 问题 | 状态 |
|---|---|
loadScene(activeSceneUrl, destroyOldScene=true) self-destroy 竞态 |
✅ 已修复（useCache=false，loader 不再返回同一已激活 Scene 实例） |
| 方案选择：按需 evict vs 永不缓存 | ✅ 已关闭（永不缓存与 PrimitiveMeshLoader/ProjectLoader 一致，Scene 是有状态运行时树非不可变资产） |
useCache=false 是否同时阻断缓存读取 |
✅ 已确认（_addAsset 不调用 → _assetUrlPool 永无 Scene 条目 → ResourceManager._loadSingleItem line 370 的 cache-read 必 miss，读写双向都断） |
additive load（destroyOldScene=false）也绕过缓存 |
✅ 已关闭（有意为之，非副作用） |
ProjectLoader 第二条 load<Scene> 消费点受影响 |
✅ 已确认拿到 fresh Scene 正是期望行为，无负面影响 |
| [P2] 测试只验证 policy flag，非行为 regression | ✅ 已接受（loader 配置即契约，端到端模拟 destroyOldScene 竞态成本高，作为非阻塞接受） |
[P3] 测试缺 afterAll(engine.destroy) / PrimitiveMesh 对照测试隐式锁定 |
✅ 已提，非阻塞 |
总结
本 PR diff 仅 2 文件：SceneLoader.ts（@resourceLoader 第三参 true→false）+ 新增 SceneLoaderCache.test.ts。根因链已逐行对照源码确认：load<Scene> 命中 _assetUrlPool 返回同一 Scene 实例 → 当它正是 active scene 时，SceneManager.loadScene 的 destroyOldScene 分支把它销毁 → addScene 一个已 destroyed 对象 → rootEntities 空、PhysicsScene 释放、黑屏。useCache=false 后每次 load<Scene> miss → new Scene(...)，新旧实例必然不同，问题消除。这是根因层修复，方向正确，替代了 599a7e611 的主动 evict workaround。
问题
[P1] SceneManager.ts:97（本 PR diff 不含此文件，属同链路上的既有 latent bug）— destroyOldScene 销毁循环用 getArray() 遍历，destroy() 会在迭代中 splice 底层数组
const scenes = this._scenes.getArray(); // 返回 _scenes 的 live backing 引用
for (let i = 0, n = scenes.length; i < n; i++) {
scenes[i].destroy(); // → Scene._onDestroy → sceneManager.removeScene → scenes.removeByIndex(index)
}Scene._onDestroy()（Scene.ts:502）调用 removeScene → scenes.removeByIndex(index)，在遍历中修改了正在迭代的数组。当存在 ≥2 个 active scene 且 destroyOldScene=true 时，销毁 scenes[0] 后数组左移，i=1 读到的是原 scenes[2]，scenes[1] 被跳过不被销毁 → 泄漏一个本应销毁的 Scene。_scenes 是 SafeLoopArray，正是为此提供了 getLoopArray()（返回快照副本），应改用它：
const scenes = this._scenes.getLoopArray();说明：此 bug 在 base dev/2.0 已存在（line 97 同样是 getArray()），本 PR 未引入也未触碰 SceneManager。我此前多轮 review 误称"本 PR 已包含 getLoopArray() 修复"——经核对 PR diff（仅 SceneLoader.ts + 测试）该说法不成立，特此更正并把这个真实存在的 latent bug 重新提出。建议在本 PR 内顺手补一行（与 self-destroy 同属 loadScene 链路），或拆独立 PR 修复——不阻塞本 PR 合入。
简化建议 / 长期方向
底层真因：Scene 身兼"按 URL 缓存的 asset"与"引擎运行并销毁的有状态对象"两个职责且无 ref counting，本 PR（issue #2979 的 R2 小改面方案）绕开冲突而非拆分职责。若未来需"同 URL 多实例并存/复用"，R1（拆 SceneAsset 可缓存 schema vs 运行时 Scene，对标 Cocos/Unity）会更干净。留作 follow-up。
结论
本 PR 自身改动干净、最小、根因定位准确，可合入。P1 为同链路既有 latent bug（非本 PR 引入），建议处理但不阻塞。
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