From 1bb907edb72d701d114f1532f1d08d33ed1b73c2 Mon Sep 17 00:00:00 2001 From: cptbtptpbcptdtptp Date: Mon, 3 Aug 2026 18:17:27 +0800 Subject: [PATCH 1/2] fix(loader): reuse registered shaders --- packages/core/src/shader/Shader.ts | 6 +- packages/loader/src/ShaderLoader.ts | 10 +++- tests/src/core/Shader.test.ts | 3 +- tests/src/loader/ShaderLoader.test.ts | 86 +++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 5 deletions(-) create mode 100644 tests/src/loader/ShaderLoader.test.ts diff --git a/packages/core/src/shader/Shader.ts b/packages/core/src/shader/Shader.ts index d17d8630a8..090c6d106e 100644 --- a/packages/core/src/shader/Shader.ts +++ b/packages/core/src/shader/Shader.ts @@ -91,7 +91,7 @@ export class Shader implements IReferable { const shaderSource = shaderCompiler._parseShaderSource(nameOrShaderSource); if (shaderMap[shaderSource.name]) { console.error(`Shader named "${shaderSource.name}" already exists.`); - return; + return shaderMap[shaderSource.name]; } const basePathForIncludeKey = new URL(path ?? "", ShaderPass._shaderRootPath).href; @@ -141,7 +141,7 @@ export class Shader implements IReferable { } else { if (shaderMap[nameOrShaderSource]) { console.error(`Shader named "${nameOrShaderSource}" already exists.`); - return; + return shaderMap[nameOrShaderSource]; } if (shaderPassesOrSubShadersOrPlatformTarget.length > 0) { if (shaderPassesOrSubShadersOrPlatformTarget[0].constructor === ShaderPass) { @@ -175,7 +175,7 @@ export class Shader implements IReferable { const shaderMap = Shader._shaderMap; if (shaderMap[data.name]) { console.error(`Shader named "${data.name}" already exists.`); - return; + return shaderMap[data.name]; } const subShaderList = data.subShaders.map((subData) => { diff --git a/packages/loader/src/ShaderLoader.ts b/packages/loader/src/ShaderLoader.ts index 9f42c1c41b..9a5b3d12b8 100644 --- a/packages/loader/src/ShaderLoader.ts +++ b/packages/loader/src/ShaderLoader.ts @@ -10,16 +10,24 @@ import { @resourceLoader(AssetType.Shader, ["shader", "shaderc"]) class ShaderLoader extends Loader { + private static _shaderNameRegex = /^(?:(?:\s+)|(?:\/\/[^\r\n]*(?:\r?\n|$))|(?:\/\*[\s\S]*?\*\/))*Shader\s+"([^"]+)"/; + load(item: LoadItem, resourceManager: ResourceManager): AssetPromise { const url = item.url!; // @ts-expect-error _request is @internal return resourceManager._request(url, { ...item, type: "text" }).then((code) => { const source = code.trimStart(); if (source.startsWith("{")) { + const data = JSON.parse(source); // @ts-expect-error _createFromPrecompiled is @internal - return Shader._createFromPrecompiled(JSON.parse(source)); + return Shader.find(data.name) ?? Shader._createFromPrecompiled(data); } + const shaderName = ShaderLoader._shaderNameRegex.exec(source)?.[1]; + const existingShader = shaderName && Shader.find(shaderName); + if (existingShader) { + return existingShader; + } return Shader.create(code, undefined, url); }); } diff --git a/tests/src/core/Shader.test.ts b/tests/src/core/Shader.test.ts index 5f4902de40..11e540f354 100644 --- a/tests/src/core/Shader.test.ts +++ b/tests/src/core/Shader.test.ts @@ -35,8 +35,9 @@ describe("Shader", () => { // Create same name shader const errorSpy = vi.spyOn(console, "error"); - Shader.create("custom", [new SubShader("Default", [makePass()])]); + const duplicateShader = Shader.create("custom", [new SubShader("Default", [makePass()])]); expect(errorSpy).toHaveBeenCalledWith('Shader named "custom" already exists.'); + expect(duplicateShader).equal(customShader); vi.resetAllMocks(); // Create shader by empty SubShader array diff --git a/tests/src/loader/ShaderLoader.test.ts b/tests/src/loader/ShaderLoader.test.ts new file mode 100644 index 0000000000..576b840cd5 --- /dev/null +++ b/tests/src/loader/ShaderLoader.test.ts @@ -0,0 +1,86 @@ +import { AssetPromise, AssetType, Shader } from "@galacean/engine"; +import "@galacean/engine-loader"; +import { WebGLEngine } from "@galacean/engine"; +import { ShaderCompiler } from "@galacean/engine-shader-compiler"; +import { describe, expect, it, vi } from "vitest"; + +const shaderSource = ` +// Leading comments are valid ShaderLab source. +Shader "Loader/ReusableAcrossEngines" { + SubShader "Default" { + Pass "Default" { + struct Attributes { vec4 POSITION; }; + + VertexShader = vert; + FragmentShader = frag; + + void vert(Attributes attr) { + gl_Position = attr.POSITION; + } + + void frag() { + gl_FragColor = vec4(1.0); + } + } + } +}`; + +const precompiledShader = JSON.stringify({ + name: "Loader/ReusablePrecompiledAcrossEngines", + platformTarget: 0, + subShaders: [] +}); + +describe("ShaderLoader", () => { + it("reuses source and precompiled shaders across engine resource managers", async () => { + const sourceURL = "Shaders/reusable-across-engines.shader"; + const precompiledURL = "Shaders/reusable-precompiled-across-engines.shaderc"; + const canvas = document.createElement("canvas"); + const engine1 = await WebGLEngine.create({ + canvas, + shaderCompiler: new ShaderCompiler() + }); + const request1 = vi + // @ts-expect-error _requestByRemoteUrl is @internal + .spyOn(engine1.resourceManager, "_requestByRemoteUrl") + .mockImplementation((url: string) => AssetPromise.resolve(url === sourceURL ? shaderSource : precompiledShader)); + + let engine2: WebGLEngine; + let sourceShader: Shader; + let precompiled: Shader; + try { + sourceShader = await engine1.resourceManager.load({ url: sourceURL, type: AssetType.Shader }); + precompiled = await engine1.resourceManager.load({ url: precompiledURL, type: AssetType.Shader }); + expect(request1).toHaveBeenCalledTimes(2); + expect(sourceShader.compileVariant(engine1, [])).true; + + engine1.destroy(); + engine2 = await WebGLEngine.create({ + canvas, + shaderCompiler: new ShaderCompiler() + }); + const request2 = vi + // @ts-expect-error _requestByRemoteUrl is @internal + .spyOn(engine2.resourceManager, "_requestByRemoteUrl") + .mockImplementation((url: string) => + AssetPromise.resolve(url === sourceURL ? shaderSource : precompiledShader) + ); + + const reusedSource = await engine2.resourceManager.load({ url: sourceURL, type: AssetType.Shader }); + const reusedPrecompiled = await engine2.resourceManager.load({ + url: precompiledURL, + type: AssetType.Shader + }); + + expect(reusedSource).equal(sourceShader); + expect(reusedPrecompiled).equal(precompiled); + expect(request2).toHaveBeenCalledTimes(2); + expect(reusedSource.compileVariant(engine2, [])).true; + } finally { + engine1.destroy(); + engine2?.destroy(); + Shader.find("Loader/ReusableAcrossEngines")?.destroy(true); + Shader.find("Loader/ReusablePrecompiledAcrossEngines")?.destroy(true); + } + }); +}); From fa2510818c5f7d4ab8537fbccb00fe10b37278e2 Mon Sep 17 00:00:00 2001 From: cptbtptpbcptdtptp Date: Mon, 3 Aug 2026 19:39:44 +0800 Subject: [PATCH 2/2] fix(loader): validate reused shader source --- packages/loader/src/ShaderLoader.ts | 31 +++++++++++++++++++++----- tests/src/loader/ShaderLoader.test.ts | 32 +++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/packages/loader/src/ShaderLoader.ts b/packages/loader/src/ShaderLoader.ts index 9a5b3d12b8..46f0450d6b 100644 --- a/packages/loader/src/ShaderLoader.ts +++ b/packages/loader/src/ShaderLoader.ts @@ -11,6 +11,7 @@ import { @resourceLoader(AssetType.Shader, ["shader", "shaderc"]) class ShaderLoader extends Loader { private static _shaderNameRegex = /^(?:(?:\s+)|(?:\/\/[^\r\n]*(?:\r?\n|$))|(?:\/\*[\s\S]*?\*\/))*Shader\s+"([^"]+)"/; + private static _shaderSourceMap = new WeakMap(); load(item: LoadItem, resourceManager: ResourceManager): AssetPromise { const url = item.url!; @@ -19,16 +20,34 @@ class ShaderLoader extends Loader { const source = code.trimStart(); if (source.startsWith("{")) { const data = JSON.parse(source); - // @ts-expect-error _createFromPrecompiled is @internal - return Shader.find(data.name) ?? Shader._createFromPrecompiled(data); + return this._getOrCreateShader(data.name, url, code, () => { + // @ts-expect-error _createFromPrecompiled is @internal + return Shader._createFromPrecompiled(data); + }); } const shaderName = ShaderLoader._shaderNameRegex.exec(source)?.[1]; - const existingShader = shaderName && Shader.find(shaderName); - if (existingShader) { - return existingShader; + if (!shaderName) { + throw new Error(`Unable to parse shader name from "${url}".`); } - return Shader.create(code, undefined, url); + return this._getOrCreateShader(shaderName, url, code, () => Shader.create(code, undefined, url)); }); } + + private _getOrCreateShader(name: string, url: string, source: string, create: () => Shader): Shader { + const existingShader = Shader.find(name); + if (existingShader) { + const existingSource = ShaderLoader._shaderSourceMap.get(existingShader); + if (existingSource?.url === url && existingSource.source === source) { + return existingShader; + } + + const existingURL = existingSource ? `"${existingSource.url}"` : "an unknown source"; + throw new Error(`Shader named "${name}" from "${url}" conflicts with the shader registered from ${existingURL}.`); + } + + const shader = create(); + ShaderLoader._shaderSourceMap.set(shader, { url, source }); + return shader; + } } diff --git a/tests/src/loader/ShaderLoader.test.ts b/tests/src/loader/ShaderLoader.test.ts index 576b840cd5..c68ac1ec68 100644 --- a/tests/src/loader/ShaderLoader.test.ts +++ b/tests/src/loader/ShaderLoader.test.ts @@ -31,6 +31,10 @@ const precompiledShader = JSON.stringify({ subShaders: [] }); +const conflictingShaderSource = shaderSource + .replace("Loader/ReusableAcrossEngines", "Loader/ConflictingAcrossEngines") + .replace("vec4(1.0)", "vec4(0.0)"); + describe("ShaderLoader", () => { it("reuses source and precompiled shaders across engine resource managers", async () => { const sourceURL = "Shaders/reusable-across-engines.shader"; @@ -83,4 +87,32 @@ describe("ShaderLoader", () => { Shader.find("Loader/ReusablePrecompiledAcrossEngines")?.destroy(true); } }); + + it("rejects a same-name shader whose source changes across engines", async () => { + const url = "Shaders/conflicting-across-engines.shader"; + const canvas = document.createElement("canvas"); + const engine1 = await WebGLEngine.create({ canvas, shaderCompiler: new ShaderCompiler() }); + vi.spyOn(engine1.resourceManager, "_requestByRemoteUrl") + // @ts-expect-error _requestByRemoteUrl is @internal + .mockReturnValue(AssetPromise.resolve(conflictingShaderSource)); + + let engine2: WebGLEngine; + try { + await engine1.resourceManager.load({ url, type: AssetType.Shader }); + engine1.destroy(); + + engine2 = await WebGLEngine.create({ canvas, shaderCompiler: new ShaderCompiler() }); + vi.spyOn(engine2.resourceManager, "_requestByRemoteUrl") + // @ts-expect-error _requestByRemoteUrl is @internal + .mockReturnValue(AssetPromise.resolve(conflictingShaderSource.replace("vec4(0.0)", "vec4(0.5)"))); + + await expect(engine2.resourceManager.load({ url, type: AssetType.Shader })).rejects.toThrow( + `Shader named "Loader/ConflictingAcrossEngines" from "${url}" conflicts` + ); + } finally { + engine1.destroy(); + engine2?.destroy(); + Shader.find("Loader/ConflictingAcrossEngines")?.destroy(true); + } + }); });