lywsvip/engine
1
0
Fork 0
mirror of https://github.com/galacean/engine.git synced 2026-06-02 08:40:12 +08:00
Code Issues Packages Projects Releases Wiki Activity

refactor(shader-lab): collapse MacroDefineInfo classifier into single referenceName

Browse Source 
Before: `MacroDefineInfo` carried `valueType: MacroValueType`, `value: string`,
and `functionCallName: string` — three fields that together encoded one fact
("the external identifier this macro references, if any"). The `Other` branch
of `MacroValueType` plus the `Logger.warn` in `getReferenceSymbolNames`
encoded a second fact ("the regex doesn't understand this value"). After the
AST-first rewrite, the second fact is misleading: AST-form macros with
`Other`-shaped values (e.g. `v.v_uv`, `mat3(...)`) are handled by the visitor
pipeline, not a failure to recognize.

Collapse the classifier into one field:

  interface MacroDefineInfo {
    isFunction: boolean;
    name: string;
    params: string[];
    valueAst?: ASTNode.AssignmentExpression;
    referenceName: string;       // new
  }

- `valueType` / `value` / `functionCallName` removed
- `enum MacroValueType` removed (no external consumers — verified via grep)
- `_parseMacroDefines` simplified: regex classification collapsed into a
  single `_extractReferenceName` helper
- `getReferenceSymbolNames` simplified: a linear "push each entry's
  non-empty, non-parameter referenceName" loop; no branches, no warnings
- `_isExist` dedup uses `referenceName` instead of `value`/`functionCallName`

Also: `MacroDefine.semanticAnalyze` now upgrades the existing preprocessor
entry in place (attaching `valueAst`) rather than pushing a duplicate. The
duplicate caused the spurious warnings this commit originally targeted,
then `MacroValueType` going away removed the warning itself entirely.

Regression guard: `macro-member-access-builtin-arg` test spies on
`Logger.warn` and asserts zero "unrecognized value" warnings — ensures
the removed warning can't sneak back via similar double-bookkeeping.

Net: -34 source lines, -40 with tests. No behavior change beyond the
warning no longer firing; 1301/1301 suite passes.
This commit is contained in:
chenmo.gl
2026-04-24 18:39:17 +08:00
parent 828ac81e1f
commit b09fd5019b
3 changed files with 98 additions and 118 deletions
Download Patch File Download Diff File Expand all files Collapse all files

119
packages/shader-lab/src/Preprocessor.ts
View File

@@ -4,47 +4,32 @@ import { ShaderLib } from "@galacean/engine";
import type { ASTNode } from "./parser/AST";
/**
* Classification of a `#define` value. Used by the opaque-token (legacy) path;
* expression-style macros go straight to AST and don't need this tag.
* Record for a single `#define` directive.
*
* @internal
*/
export enum MacroValueType {
Number, // 1, 1.1
Symbol, // variable name
FunctionCall, // function call, e.g. clamp(a, 0.0, 1.0)
Other // shaderLab does not check this
}
/**
* Record for a single `#define` directive. Two shapes coexist:
* Two shapes coexist and can be combined on the same entry:
*
* - **Expression macros** — have `valueAst` pointing at an `AssignmentExpression`
* subtree. These participate in type inference, varying flattening, and reference
* tracking through the normal visitor pattern, just like inline expressions.
* - **Opaque macros** — the value is a qualifier/type/partial-syntax form that can't
* be parsed as a GLSL expression. `valueAst` is undefined and the lexer kept the
* whole directive in a legacy `MACRO_DEFINE_EXPRESSION` token.
* - **Expression macros** — `valueAst` holds an `AssignmentExpression` subtree
* produced by the `macro_define` CFG rule. These participate in type
* inference, varying flattening, and visitor-based codegen.
* - **Legacy opaque macros** — the directive is emitted verbatim as a
* `MACRO_DEFINE_EXPRESSION` token and expanded by the GLSL driver. `valueAst`
* is undefined.
*
* Both shapes share `isFunction`, `name`, `params`, and `value` for display and
* reference tracking. `functionCallName` is set only for opaque macros whose value
* is a function-call form, enabling `getReferenceSymbolNames` to record the callee.
* `referenceName` captures the top-level external identifier the value refers
* to, if any — the callee of a function-call value (`#define F foo(a,b)`
* → `foo`) or the single-identifier value (`#define F foo` → `foo`). Empty
* for numeric literals, qualifier fragments, or structurally complex values.
* Populated by the regex pass; drives symbol-table lookup for macro call
* sites, independent of whether the value also has an AST form.
*/
export interface MacroDefineInfo {
isFunction: boolean;
name: string;
/** Classification of the raw value text. Populated for opaque (legacy) macros
* only — AST-form macros leave this as `Other` since classification doesn't
* drive their semantics. */
valueType: MacroValueType;
/** Raw value text for opaque macros; empty string for AST-form macros. */
value: string;
params: string[];
/** AST for expression-style macros. Absent for opaque macros. */
valueAst?: ASTNode.AssignmentExpression;
params: string[];
/** Function callee name for opaque macros classified as `FunctionCall`; empty
* string otherwise. */
functionCallName: string;
/** Top-level external identifier the value references, or empty string. */
referenceName: string;
}
export interface MacroDefineList {
@@ -56,7 +41,7 @@ export class Preprocessor {
private static readonly _macroRegex =
/^\s*#define\s+(\w+)[ ]*(\(([^)]*)\))?[ ]+(\(?\w+\)?.*?)(?:\/\/.*|\/\*.*?\*\/)?\s*$/gm;
private static readonly _symbolReg = /^[a-zA-Z_][a-zA-Z0-9_]*$/;
private static readonly _funcCallReg = /^([a-zA-Z_][a-zA-Z0-9_]*)\s*\((.*)\)$/;
private static readonly _funcCallReg = /^([a-zA-Z_][a-zA-Z0-9_]*)\s*\(.*\)$/;
private static readonly _macroDefineIncludeMap = new Map<string, MacroDefineList>();
/**
@@ -79,47 +64,40 @@ export class Preprocessor {
}
/**
* For a given macro name, collect the external symbols its body references so
* callers of the macro can drive symbol-table lookup. Only the opaque-macro path
* needs this preprocessor-driven collection; expression macros carry their refs
* in the AST and `MacroDefine.semanticAnalyze` collects them directly.
* For each registered definition of `macroName`, push its `referenceName`
* (when non-empty and not a macro parameter) so call sites can drive
* symbol-table lookup.
*/
static getReferenceSymbolNames(macroDefineList: MacroDefineList, macroName: string, out: string[]): void {
out.length = 0;
const infos = macroDefineList[macroName];
if (!infos) return;
for (let i = 0; i < infos.length; i++) {
for (let i = 0, n = infos.length; i < n; i++) {
const info = infos[i];
// Expression macros handle their own reference tracking via the AST subtree
// (see `MacroDefine.semanticAnalyze`). Nothing to collect here for them.
if (info.valueAst) continue;
const valueType = info.valueType;
if (valueType === MacroValueType.FunctionCall || valueType === MacroValueType.Symbol) {
const referencedName = valueType === MacroValueType.FunctionCall ? info.functionCallName : info.value;
if (info.params.indexOf(referencedName) !== -1) continue;
if (out.indexOf(referencedName) === -1) out.push(referencedName);
} else if (valueType === MacroValueType.Other) {
// #if _VERBOSE
Logger.warn(
`Warning: Macro "${info.name}" has an unrecognized value "${info.value}". ShaderLab does not validate this type.`
);
// #endif
}
const ref = info.referenceName;
if (!ref) continue;
if (info.params.indexOf(ref) !== -1) continue;
if (out.indexOf(ref) === -1) out.push(ref);
}
}
private static _isNumber(str: string): boolean {
return !isNaN(Number(str));
/**
* Extract the top-level identifier a value refers to, or empty string if
* the value is a numeric literal, qualifier fragment, or structurally
* complex form we don't try to introspect.
*/
private static _extractReferenceName(value: string): string {
if (this._symbolReg.test(value)) return value;
const callMatch = this._funcCallReg.exec(value);
return callMatch ? callMatch[1] : "";
}
private static _isExist(list: MacroDefineInfo[], item: MacroDefineInfo): boolean {
return list.some(
(e) =>
e.valueType === item.valueType &&
e.value === item.value &&
e.isFunction === item.isFunction &&
e.functionCallName === item.functionCallName &&
e.referenceName === item.referenceName &&
e.params.length === item.params.length &&
e.params.every((p, i) => p === item.params[i])
);
@@ -140,30 +118,9 @@ export class Preprocessor {
.filter(Boolean)
: [];
const value = valueRaw ? valueRaw.trim() : "";
const referenceName = value ? this._extractReferenceName(value) : "";
let valueType = MacroValueType.Other;
let functionCallName = "";
if (this._isNumber(value)) {
valueType = MacroValueType.Number;
} else if (this._symbolReg.test(value)) {
valueType = MacroValueType.Symbol;
} else {
const callMatch = this._funcCallReg.exec(value);
if (callMatch) {
valueType = MacroValueType.FunctionCall;
functionCallName = callMatch[1];
}
}
const info: MacroDefineInfo = {
isFunction,
name,
value,
valueType,
params,
functionCallName
};
const info: MacroDefineInfo = { isFunction, name, params, referenceName };
const arr = outMacroList[name];
if (arr) {

43
packages/shader-lab/src/parser/AST.ts
View File

@@ -4,7 +4,7 @@ import { ETokenType, GalaceanDataType, ShaderRange, TokenType, TypeAny } from ".
import { BaseToken } from "../common/BaseToken";
import { Keyword } from "../common/enums/Keyword";
import { ParserUtils } from "../ParserUtils";
import { MacroDefineInfo, MacroValueType, Preprocessor } from "../Preprocessor";
import { MacroDefineInfo, Preprocessor } from "../Preprocessor";
import { ShaderLabUtils } from "../ShaderLabUtils";
import { BuiltinFunction, BuiltinVariable, NonGenericGalaceanType } from "./builtin";
import { NoneTerminal } from "./GrammarSymbol";
@@ -1735,24 +1735,33 @@ export namespace ASTNode {
this.valueExpression = children[valueIdx] as AssignmentExpression;
}
// Register this macro in the preprocessor's define list so downstream passes
// (e.g. `MacroCallSymbol.semanticAnalyze` resolving references) see it. The
// AST path stores `valueAst` and leaves the legacy opaque fields empty.
const info: MacroDefineInfo = {
isFunction: this.isFunction,
name: this.macroName,
value: "",
valueType: MacroValueType.Other,
valueAst: this.valueExpression,
params,
functionCallName: ""
};
// `Preprocessor._parseMacroDefines` already registered a regex-derived
// entry for this directive (so the lexer can classify the name as
// `MACRO_CALL`). Upgrade that entry in place by attaching the AST subtree,
// rather than pushing a duplicate record — duplicates leak into
// `getReferenceSymbolNames` and cause phantom issues like spurious
// warnings or double-counted references.
const list = sa.macroDefineList;
if (list[this.macroName]) {
list[this.macroName].push(info);
const entries = list[this.macroName];
const sameArity = (info: MacroDefineInfo) =>
info.isFunction === this.isFunction &&
info.params.length === params.length &&
info.params.every((p, i) => p === params[i]);
const upgradable = entries?.find((info) => !info.valueAst && sameArity(info));
if (upgradable) {
upgradable.valueAst = this.valueExpression;
} else {
list[this.macroName] = [info];
// No matching preprocessor entry (e.g. the lexer was fed the directive
// directly without a `Preprocessor.parse` pass). Push a fresh entry.
const info: MacroDefineInfo = {
isFunction: this.isFunction,
name: this.macroName,
params,
valueAst: this.valueExpression,
referenceName: ""
};
if (entries) entries.push(info);
else list[this.macroName] = [info];
}
}

54
tests/src/shader-lab/ShaderLab.test.ts
View File

@@ -14,7 +14,7 @@ import { glslValidate } from "./ShaderValidate";
import { Logger, WebGLEngine } from "@galacean/engine";
import { server } from "@vitest/browser/context";
import { describe, expect, it } from "vitest";
import { describe, expect, it, vi } from "vitest";
const { readFile } = server.commands;
Logger.enable();
registerIncludes();
@@ -300,28 +300,42 @@ describe("ShaderLab", async () => {
it("macro-member-access-builtin-arg (Cocos FSInput pattern: member access macro as builtin fn arg)", async () => {
const shaderSource = await readFile("./shaders/macro-member-access-builtin-arg.shader");
glslValidate(engine, shaderSource, shaderLabRelease);
// Also verify verbose mode (semantic analysis) succeeds — this was the original bug:
// member access macros like #define FSInput_worldNormal v.v_normal.xyz resolved to
// struct type "Varyings" instead of TypeAny, causing builtin overload matching to fail.
const shader = shaderLabVerbose._parseShaderSource(shaderSource);
const passSource = shader.subShaders[0].passes[0];
const { vertex, fragment } = shaderLabVerbose._parseShaderPass(
passSource.contents,
passSource.vertexEntry,
passSource.fragmentEntry,
0,
""
)!;
// Regression guard: before the preprocessor/AST deduplication fix, each
// AST-form member-access macro (e.g. `#define FSInput_worldNormal v.v_normal.xyz`)
// fired a spurious "has an unrecognized value" warning on every access.
const warnSpy = vi.spyOn(Logger, "warn");
try {
glslValidate(engine, shaderSource, shaderLabRelease);
expect(vertex).to.be.a("string").and.not.empty;
expect(fragment).to.be.a("string").and.not.empty;
// Also verify verbose mode (semantic analysis) succeeds — this was the original bug:
// member access macros resolved to struct type "Varyings" instead of TypeAny,
// causing builtin overload matching to fail.
const shader = shaderLabVerbose._parseShaderSource(shaderSource);
const passSource = shader.subShaders[0].passes[0];
const { vertex, fragment } = shaderLabVerbose._parseShaderPass(
passSource.contents,
passSource.vertexEntry,
passSource.fragmentEntry,
0,
""
)!;
// Verify key builtins are present in output (macros expanded correctly)
expect(fragment).to.contain("normalize");
expect(fragment).to.contain("dot");
expect(fragment).to.contain("texture2D");
expect(vertex).to.be.a("string").and.not.empty;
expect(fragment).to.be.a("string").and.not.empty;
// Verify key builtins are present in output (macros expanded correctly)
expect(fragment).to.contain("normalize");
expect(fragment).to.contain("dot");
expect(fragment).to.contain("texture2D");
const unrecognizedCalls = warnSpy.mock.calls.filter((args) =>
args.some((a) => typeof a === "string" && a.includes("unrecognized value"))
);
expect(unrecognizedCalls).to.have.lengthOf(0);
} finally {
warnSpy.mockRestore();
}
});
it("global-varying-var (Cocos VSOutput pattern: global Varyings var with #define macros)", async () => {