fix(shader-compiler, loader): silence verbose-mode noise + drop ShaderLoader dead code#3007
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
WalkthroughReplaces console.warn 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.
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% 77.96% -0.19%
===========================================
Files 900 901 +1
Lines 99524 99608 +84
Branches 10254 10272 +18
===========================================
- Hits 77779 77664 -115
- Misses 21571 21769 +198
- 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:
|
|
Two more
Without these, Out of scope but worth a follow-up:
The error path ( |
- `SymbolTable.insert` fired `console.warn` on symbol redeclaration, and `LALR1._addAction` fired `console.warn` on shift/reduce conflict detect. Both sit on the same verbose-bundle compile path as `SemanticAnalyzer. reportWarning` (94e2800), so `Logger.disable()` still leaked them. - Switch both to `Logger.warn` so the verbose shader-compiler bundle honors the engine-wide logger toggle end-to-end.
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
本轮增量审查覆盖新增 commit c64bf8a7,该 commit 直接响应了 cptbtptpbcptdtptp 指出的两处遗漏——将 SymbolTable.ts 和 LALR1.ts 的 console.warn 也改为 Logger.warn,与 PR 的核心目标对齐。修复完整,方向正确。
已关闭问题(不再重提)
- P2 循环依赖风险 — 已确认,
engine-core在运行时不导入shader-compiler,module 构建正确 external。 - P3 test plan 未勾选 — 已勾选。
ShaderLoader._getBuiltinShader删除 — 已确认,#2961 后为死代码,删除干净。- bootstrap 构建
runtimeExternal = []— 已知 CLI 场景 trade-off,非 bug。 SymbolTable.ts/LALR1.ts遗漏的console.warn— 本 commit 已修复。
对 cptbtptpbcptdtptp 反馈的核查
已修复(scope 内):
SymbolTable.ts:14— 已改为Logger.warn✓LALR1.ts:172— 已改为Logger.warn,且该调用在// #if _VERBOSE ... // #endif守卫内,仅存在于 verbose bundle ✓
实际不成问题(out-of-scope 条目的核查结论):
ShaderTargetParser.ts:145的console.info— 已确认整个_printStack方法体在// #if _VERBOSE ... // #endif守卫内,release bundle 中该代码不存在,无需处理。
问题
[P2] packages/shader-compiler/src/index.ts:13 — console.log banner 在 release bundle 中无条件打印
let mode = "Release";
// #if _VERBOSE
mode = "Verbose";
// #endif
console.log(`Galacean Engine Shader Compiler Version: ${version} | Mode: ${mode}`);
// ↑ 在 #if _VERBOSE 守卫**外面**,release 和 verbose bundle 均打印该 banner 每次包加载(编辑器启动)都打印一次,在量上虽然只有一行,但它绕过了 Logger 门控——本 PR 的核心目标正是让 shader-compiler 的噪音可由 Logger.enable/disable 统一控制,这一行是例外。
建议纳入 // #if _VERBOSE 守卫(verbose bundle 打印 banner 是合理的调试辅助),或改为 Logger.info(...) 同样受门控。这不阻塞合并,但与本 PR 的既定目标略有出入,建议跟进或在本 PR 一并处理。
Logger.warn 多参数兼容性确认
Logger.warn 在启用时是 console.warn.bind(console),接受 variadic 参数,LALR1.ts 中的 Logger.warn(msg, Utils.printAction(exist), "\n", Utils.printAction(action)) 多参数调用完全兼容,无问题。
无阻塞问题,可以合并。
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).Summary by CodeRabbit