Skip to content

fix(core): disable cache for SceneLoader to avoid loadScene self-destroy #2993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cptbtptpbcptdtptp wants to merge 2 commits into
base: dev/2.0
Choose a base branch
from
+31 −1
Open

fix(core): disable cache for SceneLoader to avoid loadScene self-destroy #2993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1b18a2
fix(core): disable cache for SceneLoader to avoid loadScene self-destroy
cptbtptpbcptdtptp May 12, 2026
63b6b1f
test(loader): verify SceneLoader useCache is disabled
cptbtptpbcptdtptp May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/loader/src/SceneLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export function applySceneData(
return Promise.all(promises).then(() => {});
}

@resourceLoader(AssetType.Scene, ["scene"], true)
@resourceLoader(AssetType.Scene, ["scene"], false)
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 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' packages

Repository: 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.

class SceneLoader extends Loader<Scene> {
load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<Scene> {
const { engine } = resourceManager;
Expand Down
30 changes: 30 additions & 0 deletions tests/src/core/resource/SceneLoaderCache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { AssetType, ResourceManager } from "@galacean/engine-core";
import "@galacean/engine-loader";
import { WebGLEngine } from "@galacean/engine";
import { describe, beforeAll, expect, it } from "vitest";

describe("SceneLoader cache policy", () => {
let engine: WebGLEngine;

beforeAll(async () => {
engine = await WebGLEngine.create({ canvas: document.createElement("canvas") });
});
Comment on lines +9 to +11
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.


it("SceneLoader should have useCache disabled", () => {
const sceneLoader = ResourceManager._loaders[AssetType.Scene];
expect(sceneLoader).to.not.be.undefined;
expect(sceneLoader.useCache).to.eq(false);
});

it("PrimitiveMeshLoader should also have useCache disabled (existing convention)", () => {
const loader = ResourceManager._loaders[AssetType.PrimitiveMesh];
expect(loader).to.not.be.undefined;
expect(loader.useCache).to.eq(false);
});

it("Texture2D loader should still have useCache enabled (immutable asset)", () => {
const loader = ResourceManager._loaders[AssetType.Texture2D] || ResourceManager._loaders[AssetType.Texture];
expect(loader).to.not.be.undefined;
expect(loader.useCache).to.eq(true);
});
});
Loading