fix(shader-compiler, loader): silence verbose-mode noise + drop ShaderLoader dead code#3007
fix(shader-compiler, loader): silence verbose-mode noise + drop ShaderLoader dead code#3007zhuxudong wants to merge 2 commits into
Conversation
`SemanticAnalyzer.reportWarning` called `console.warn` directly, so any embedder of the verbose shader-compiler bundle gets noisy output regardless of the engine's `Logger` toggle. In the editor (which uses `@galacean/engine-shader-compiler/verbose` to surface diagnostics) a single PBR shader compile fires several warnings — forward references to functions declared later in the same translation unit (`#define FUNCTION_DIFFUSE_IBL evaluateDiffuseIBL`), and runtime-injected macros (`RENDERER_JOINTS_NUM` / `RENDERER_BLENDSHAPE_COUNT` etc. that the engine prepends before compilation) — none of which AST-only static analysis can tell apart from real typos. Hundreds of warnings during an editing session drown out real errors. Switch to `Logger.warn` so warnings stay disabled by default and shader authors can opt in via `Logger.enable()` when actively debugging. Errors (`reportError`) keep going through `console.error` — they're always-on.
WalkthroughReplaces direct console warning emission with ChangesShader Compiler Warning Logging
Shader Loader Simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
`ShaderLoader._getBuiltinShader` parsed `// @Builtin <name>` from a `.shader` file's source and called `Shader.find(name)` instead of compiling. That indirection only mattered when the loader saw placeholder `.shader` files whose body was just a marker comment — a layout from the pre-ShaderLab era. Post `galacean#2961` (GLSL → ShaderLab migration) all builtin shaders ship via `_shaderMap` and are resolved directly by `Shader.find`; nothing in the engine emits a placeholder `.shader` for the loader to parse. Confirmed zero remaining callers (the only reference to `_builtinRegex` / `_getBuiltinShader` was the loader's own check). Remove the regex, the helper, and the check. Net -14 lines.
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
本轮为第三次审查。本 PR 自上次审查后无新变更(两个 commit 94e2800f + 145a5e8d 均已审过),代码状态未变。重新基于第一性原理审视后,方向与实现仍然正确。
已关闭问题(不再重提)
- P2 循环依赖风险 — 已确认
engine-core在运行时不导入shader-compiler,Logger 自包含零依赖,module 构建中engine-core已正确 external。 - P3 test plan 未勾选 — 上轮已确认。
ShaderLoader._getBuiltinShader删除 — 已确认packages/内无任何.shader文件或运行时路径产出// @builtin <name>占位文件,#2961 后该路径确为死代码。- bootstrap 构建
runtimeExternal = []— 已知 CLI 场景 trade-off,非 bug。
第一性原理重审
Change 1 (console.warn → Logger.warn) — 三个候选方向:
- 修根因(消除 forward ref / runtime macro 误报) — 需要 link-time 全局符号解析,超出 AST 静态分析能力,作者描述合理("real typos get caught at codegen / GLSL-compile time")
- 完全停止发出这些 warning — 损失 debug 价值
- 通过
Logger门控(本 PR 选择) — 复用现有引擎基础设施,opt-in,与引擎其他 warning 行为一致
方向 3 是最小复杂度的本质解。
Change 2 (删除 _getBuiltinShader) — 纯死代码移除,无新抽象。直接简化。
性能微观察(不构成问题)
Logger.warn(new GSError(...).toString()) 即使 Logger 关闭也会先构造 GSError + .toString()。但 shader 编译是 load-time 一次性操作,不在稳态热路径,每次 PBR 编译多产生几次额外字符串构造无实际影响。如未来想做严格 zero-cost when disabled,可改为 if (Logger.isEnabled) Logger.warn(...) 或让 Logger.warn 接受 lazy message thunk —— 但当前完全可接受,无需修改。
无新问题,可以合并。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3007 +/- ##
===========================================
- Coverage 78.15% 78.02% -0.13%
===========================================
Files 900 900
Lines 99524 99445 -79
Branches 10254 10270 +16
===========================================
- Hits 77779 77592 -187
- Misses 21571 21678 +107
- Partials 174 175 +1
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:
|
Summary
Two small fixes to the shader build path, both surfaced while integrating ShaderLab into the editor on
dev/2.0:shader-compiler: routeSemanticAnalyzer.reportWarningthroughLogger.warnso verbose-mode warnings stay disabled by default. Embedders that include the verbose bundle (e.g. the editor's viewport) now control whether to see warnings viaLogger.enable()/disable().loader: drop the@builtinplaceholder shader path fromShaderLoader— dead code after refactor(shader): migrate GLSL shaders to ShaderLab and clean up shader infrastructure #2961.Net: +2 / -13 lines, no behavior change for code that goes through
Logger.enable()or that doesn't ship placeholder.shaderfiles.1.
shader-compiler:CompilationWarning→Logger.warnWhy
SemanticAnalyzer.reportWarningwrote directly toconsole.warn. The verbose shader-compiler bundle is used by the editor viewport to surface diagnostics, but every PBR compile fires several warnings that are false positives in two shapes:Forward references inside
#definevalue expressionsMacro RHS is expanded lazily at the call site, so symbols declared later are legitimate references.
Runtime-injected macros —
RENDERER_JOINTS_NUM,RENDERER_BLENDSHAPE_COUNT,MATERIAL_HAS_*. The engine prepends these#defines before compilation; they don't appear in the static source the compiler analyzes.AST-only static analysis can't tell either shape apart from a real typo, and real typos get caught at codegen / GLSL-compile time. A single editing session in the editor generated hundreds of these warnings, drowning out genuine errors.
What changed
What didn't
reportErrorkeeps writing toconsole.errordirectly — errors are always-on, not gated byLogger.2.
loader: drop@builtinplaceholder shader path fromShaderLoaderWhy
ShaderLoader._getBuiltinShaderparsed// @builtin <name>from a.shaderfile body and calledShader.find(name)instead of compiling. That indirection only mattered when the loader saw placeholder.shaderfiles whose body was just a marker comment — a layout from the pre-ShaderLab era.After #2961 all builtin shaders ship via
_shaderMapand are resolved byShader.finddirectly; no path in the engine emits a placeholder.shaderfor the loader to parse. Verified zero remaining callers: the only references to_builtinRegex/_getBuiltinShaderwere the loader's own internal use.What changed
class ShaderLoader extends Loader<Shader> { - private static _builtinRegex = /^\s*\/\/\s*@builtin\s+(\w+)/; - load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<Shader> { ... return resourceManager._request<string>(url, { ...item, type: "text" }).then((code: string) => { - const builtinShader = this._getBuiltinShader(code); - if (builtinShader) { - return Shader.find(builtinShader); - } - return Shader.create(code, undefined, url); }); } - - private _getBuiltinShader(code: string) { - const match = code.match(ShaderLoader._builtinRegex); - if (match && match[1]) return match[1]; - } }Test plan
pnpm --filter @galacean/engine-shader-compiler build— release + verbose bundles produce.pnpm --filter @galacean/engine-loader exec tsc --noEmit— typecheck clean.@galacean/engine-shader-compiler/verbose): with defaultLoggerstate, noCompilationWarningflood; afterLogger.enable(), warnings reappear.builtinShaderNamecontinue to load (path now goes throughShader.findfrom the resolver, not through a placeholder.shaderfile).