fix(shader-lab): transform struct member access in #define values#2967
Open
zhuxudong wants to merge 7 commits intogalacean:dev/2.0from
Open
fix(shader-lab): transform struct member access in #define values#2967zhuxudong wants to merge 7 commits intogalacean:dev/2.0from
zhuxudong wants to merge 7 commits intogalacean:dev/2.0from
Conversation
…ing CodeGen When #define values contain struct member access like `o.v_uv` (where `o` is a Varyings/Attributes/MRT struct variable), the CodeGen now correctly transforms them to just the property name (e.g. `v_uv`), matching the behavior of direct struct member access in regular code. Closes galacean#2944.
…uct-access Verify the actual CodeGen output instead of just checking GLSL compilation: - #define values with struct member access are correctly transformed - varying/attribute declarations are emitted for referenced properties
…ions Add assertions for macro usage in expressions (not just #define transformation): - Macro as RHS in multiplication, as LHS in assignment - Macro as function argument in dot(), texture2D() - Multiple varying properties (v_uv, v_normal) referenced via #define
…ransform Build a combined _globalStructVarMap in visitShaderProgram by scanning both vertex and fragment entry functions, so global #define values like `attr.POSITION` or `o.v_uv` are correctly transformed in all stages. Rewrite define-struct-access tests to use snapshot file comparison against expected/ GLSL outputs for clearer verification.
…cro scanning - Suppress `uniform` output for global struct-typed variables (e.g. `Varyings o;`) - Register global struct vars in both per-function and cross-stage maps - Unify macro member access scanning into callback-based _forEachMacroMemberAccess - Add registerStructVar() encapsulation in VisitorContext - Add Cocos VSOutput pattern test (global-varying-var)
…cro as builtin arg
WalkthroughThe shader compiler now supports member-access patterns in macro definitions (e.g., Changes
Sequence Diagram(s)sequenceDiagram
actor Parser
participant Preprocessor
participant AST
participant VisitorContext
participant CodeGenVisitor
participant GLESVisitor
Parser->>Preprocessor: Parse macro with member access<br/>(e.g., `#define` MACRO input.v_uv)
activate Preprocessor
Preprocessor->>Preprocessor: Detect pattern with<br/>_memberAccessReg
Preprocessor->>Preprocessor: Set valueType =<br/>MacroValueType.MemberAccess
Preprocessor-->>Parser: Return macro definition
deactivate Preprocessor
Parser->>AST: Analyze variable identifier<br/>referencing macro
activate AST
AST->>AST: Check _isMemberAccessMacro
AST->>AST: Skip type assignment<br/>→ TypeAny
AST-->>Parser: Return with TypeAny
deactivate AST
Parser->>GLESVisitor: Begin code generation
activate GLESVisitor
GLESVisitor->>CodeGenVisitor: _collectStructVars from<br/>entry functions
activate CodeGenVisitor
CodeGenVisitor->>VisitorContext: registerStructVar(varName, role)
activate VisitorContext
VisitorContext->>VisitorContext: Store in _structVarMap<br/>and _globalStructVarMap
VisitorContext-->>CodeGenVisitor: Registered
deactivate VisitorContext
CodeGenVisitor-->>GLESVisitor: Struct map populated
deactivate CodeGenVisitor
GLESVisitor->>CodeGenVisitor: _transformMacroDefineValue<br/>(using _globalStructVarMap)
activate CodeGenVisitor
CodeGenVisitor->>CodeGenVisitor: Extract root var<br/>from member access
CodeGenVisitor->>VisitorContext: referenceStructPropByName<br/>(role, propName)
activate VisitorContext
VisitorContext->>VisitorContext: Register property<br/>in role's reference list
VisitorContext-->>CodeGenVisitor: Referenced
deactivate VisitorContext
CodeGenVisitor-->>GLESVisitor: Return transformed<br/>expression
deactivate CodeGenVisitor
GLESVisitor->>GLESVisitor: Emit GLSL with<br/>transformed expressions
GLESVisitor->>GLESVisitor: Skip uniform emit<br/>for struct vars
GLESVisitor-->>Parser: Return compiled GLSL
deactivate GLESVisitor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shader-lab/src/codeGen/CodeGenVisitor.ts`:
- Around line 408-426: _collectStructVars currently wipes context._structVarMap
losing globals registered by visitGlobalVariableDeclaration; fix this by first
cloning the existing map (save preserved = { ...context._structVarMap }), then
clear context._structVarMap as before, collect params and locals via
fnNode.protoType.parameterList and
this._collectStructVarsFromNode(fnNode.statements, context, map), and finally
restore preserved entries only for keys not already set (for each key in
preserved if (!(key in map)) map[key] = preserved[key]) so global struct vars
remain available to function-body macros.
In `@packages/shader-lab/src/codeGen/GLESVisitor.ts`:
- Around line 279-295: Inside the _forEachMacroMemberAccess loop, symOut is
reused across rootName lookups causing stale symbols to persist; before calling
symbolTable.getSymbols(lookupSymbol, true, symOut) reset symOut (e.g.
symOut.length = 0 or reassign a new array) so each rootName lookup only examines
its own symbols when populating map using structTypeRoles, while keeping the
existing checked, lookupSymbol and map logic unchanged.
In `@packages/shader-lab/src/codeGen/VisitorContext.ts`:
- Around line 80-92: The function referenceStructPropByName currently does
nothing when a struct member is missing, which lets callers strip the root
prefix and produce a later undeclared-identifier error; update
referenceStructPropByName to record a placeholder for missing properties so
macro rewriting knows the name was referenced but unresolved. Specifically, in
referenceStructPropByName (and when choosing list via
varyingList/attributeList/mrtList and the corresponding
_referencedVaryingList/_referencedAttributeList/_referencedMRTList), if
props.length === 0 set refList[propName] to an explicit placeholder (e.g. an
empty array or null) instead of leaving it undefined/returning, so downstream
code can detect the unresolved member and preserve the original root.prop (or
emit the correct compile-time error).
In `@packages/shader-lab/src/Preprocessor.ts`:
- Around line 66-69: The MemberAccess branch in Preprocessor.ts is incorrectly
treating macro parameter placeholders (e.g., `input` in `#define GET_UV(input)
input.v_uv`) as external references; update the logic in the
MacroValueType.MemberAccess handling so that after extracting rootName from
info.value you check whether rootName is a parameter of the current macro and
skip pushing it into out when it is (i.e., only push rootName if it is not
listed among the macro's parameter names). Reference symbols:
MacroValueType.MemberAccess, info.value, rootName, out, and
VariableIdentifier.semanticAnalyze to ensure macro-local placeholders are not
treated as external identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b35f62a1-31c4-4cab-8d3f-9a561bf80d82
⛔ Files ignored due to path filters (8)
tests/src/shader-lab/expected/define-struct-access-global.frag.glslis excluded by!**/*.glsltests/src/shader-lab/expected/define-struct-access-global.vert.glslis excluded by!**/*.glsltests/src/shader-lab/expected/define-struct-access.frag.glslis excluded by!**/*.glsltests/src/shader-lab/expected/define-struct-access.vert.glslis excluded by!**/*.glsltests/src/shader-lab/shaders/define-struct-access-global.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/define-struct-access.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/global-varying-var.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/macro-member-access-builtin-arg.shaderis excluded by!**/*.shader
📒 Files selected for processing (6)
packages/shader-lab/src/Preprocessor.tspackages/shader-lab/src/codeGen/CodeGenVisitor.tspackages/shader-lab/src/codeGen/GLESVisitor.tspackages/shader-lab/src/codeGen/VisitorContext.tspackages/shader-lab/src/parser/AST.tstests/src/shader-lab/ShaderLab.test.ts
Comment on lines
+408
to
+426
| private _collectStructVars(fnNode: ASTNode.FunctionDefinition, context: VisitorContext): void { | ||
| const map = context._structVarMap; | ||
| // Clear previous function's mappings | ||
| for (const key in map) delete map[key]; | ||
|
|
||
| // Collect from function parameters | ||
| const paramList = fnNode.protoType.parameterList; | ||
| if (paramList) { | ||
| for (const param of paramList) { | ||
| if (param.ident && param.typeInfo && typeof param.typeInfo.type === "string") { | ||
| const role = context.getStructRole(param.typeInfo.typeLexeme); | ||
| if (role) map[param.ident.lexeme] = role; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Collect from local variable declarations in function body | ||
| this._collectStructVarsFromNode(fnNode.statements, context, map); | ||
| } |
There was a problem hiding this comment.
Preserve global struct vars when rebuilding the function map.
_collectStructVars() clears _structVarMap and only re-adds params and locals. That drops global struct variables registered earlier by visitGlobalVariableDeclaration(), so a function-body macro like #define UV o.v_uv stops transforming when o is global.
Suggested fix
private _collectStructVars(fnNode: ASTNode.FunctionDefinition, context: VisitorContext): void {
const map = context._structVarMap;
// Clear previous function's mappings
for (const key in map) delete map[key];
+ for (const key in context._globalStructVarMap) {
+ map[key] = context._globalStructVarMap[key];
+ }
// Collect from function parameters
const paramList = fnNode.protoType.parameterList;
if (paramList) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shader-lab/src/codeGen/CodeGenVisitor.ts` around lines 408 - 426,
_collectStructVars currently wipes context._structVarMap losing globals
registered by visitGlobalVariableDeclaration; fix this by first cloning the
existing map (save preserved = { ...context._structVarMap }), then clear
context._structVarMap as before, collect params and locals via
fnNode.protoType.parameterList and
this._collectStructVarsFromNode(fnNode.statements, context, map), and finally
restore preserved entries only for keys not already set (for each key in
preserved if (!(key in map)) map[key] = preserved[key]) so global struct vars
remain available to function-body macros.
Comment on lines
+279
to
+295
| const checked = new Set<string>(); | ||
| const symOut: SymbolInfo[] = []; | ||
| this._forEachMacroMemberAccess(globalMacros, (rootName) => { | ||
| if (map[rootName] || checked.has(rootName)) return; | ||
| checked.add(rootName); | ||
| lookupSymbol.set(rootName, ESymbolType.VAR); | ||
| symbolTable.getSymbols(lookupSymbol, true, symOut); | ||
| for (const sym of symOut) { | ||
| if (sym.dataType) { | ||
| const role = structTypeRoles[sym.dataType.typeLexeme]; | ||
| if (role) { | ||
| map[rootName] = role; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Reset symOut before each root lookup.
symOut is reused across different rootNames, but symbolTable.getSymbols(..., symOut) appends into the provided array. If one root resolves and the next one doesn't, the stale symbols are still iterated and the later root can inherit the earlier root's role.
Suggested fix
this._forEachMacroMemberAccess(globalMacros, (rootName) => {
if (map[rootName] || checked.has(rootName)) return;
checked.add(rootName);
lookupSymbol.set(rootName, ESymbolType.VAR);
+ symOut.length = 0;
symbolTable.getSymbols(lookupSymbol, true, symOut);
for (const sym of symOut) {
if (sym.dataType) {
const role = structTypeRoles[sym.dataType.typeLexeme];
if (role) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const checked = new Set<string>(); | |
| const symOut: SymbolInfo[] = []; | |
| this._forEachMacroMemberAccess(globalMacros, (rootName) => { | |
| if (map[rootName] || checked.has(rootName)) return; | |
| checked.add(rootName); | |
| lookupSymbol.set(rootName, ESymbolType.VAR); | |
| symbolTable.getSymbols(lookupSymbol, true, symOut); | |
| for (const sym of symOut) { | |
| if (sym.dataType) { | |
| const role = structTypeRoles[sym.dataType.typeLexeme]; | |
| if (role) { | |
| map[rootName] = role; | |
| break; | |
| } | |
| } | |
| } | |
| }); | |
| const checked = new Set<string>(); | |
| const symOut: SymbolInfo[] = []; | |
| this._forEachMacroMemberAccess(globalMacros, (rootName) => { | |
| if (map[rootName] || checked.has(rootName)) return; | |
| checked.add(rootName); | |
| lookupSymbol.set(rootName, ESymbolType.VAR); | |
| symOut.length = 0; | |
| symbolTable.getSymbols(lookupSymbol, true, symOut); | |
| for (const sym of symOut) { | |
| if (sym.dataType) { | |
| const role = structTypeRoles[sym.dataType.typeLexeme]; | |
| if (role) { | |
| map[rootName] = role; | |
| break; | |
| } | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shader-lab/src/codeGen/GLESVisitor.ts` around lines 279 - 295,
Inside the _forEachMacroMemberAccess loop, symOut is reused across rootName
lookups causing stale symbols to persist; before calling
symbolTable.getSymbols(lookupSymbol, true, symOut) reset symOut (e.g.
symOut.length = 0 or reassign a new array) so each rootName lookup only examines
its own symbols when populating map using structTypeRoles, while keeping the
existing checked, lookupSymbol and map logic unchanged.
Comment on lines
+80
to
+92
| referenceStructPropByName(role: "varying" | "attribute" | "mrt", propName: string): void { | ||
| const list = role === "varying" ? this.varyingList : role === "attribute" ? this.attributeList : this.mrtList; | ||
| const refList = | ||
| role === "varying" | ||
| ? this._referencedVaryingList | ||
| : role === "attribute" | ||
| ? this._referencedAttributeList | ||
| : this._referencedMRTList; | ||
| if (refList[propName]) return; | ||
| const props = list.filter((item) => item.ident.lexeme === propName); | ||
| if (props.length) { | ||
| refList[propName] = props; | ||
| } |
There was a problem hiding this comment.
Don't silently swallow unknown struct members in macro rewrites.
referenceStructPropByName() no-ops when propName is missing, but the callers still strip the root. prefix. That means #define UV o.v_uu turns into v_uu and fails much later as an undeclared identifier instead of producing the same compilation error that normal member access gets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shader-lab/src/codeGen/VisitorContext.ts` around lines 80 - 92, The
function referenceStructPropByName currently does nothing when a struct member
is missing, which lets callers strip the root prefix and produce a later
undeclared-identifier error; update referenceStructPropByName to record a
placeholder for missing properties so macro rewriting knows the name was
referenced but unresolved. Specifically, in referenceStructPropByName (and when
choosing list via varyingList/attributeList/mrtList and the corresponding
_referencedVaryingList/_referencedAttributeList/_referencedMRTList), if
props.length === 0 set refList[propName] to an explicit placeholder (e.g. an
empty array or null) instead of leaving it undefined/returning, so downstream
code can detect the unresolved member and preserve the original root.prop (or
emit the correct compile-time error).
Comment on lines
+66
to
+69
| } else if (valueType === MacroValueType.MemberAccess) { | ||
| // Extract root symbol: "input.v_uv" → "input" | ||
| const rootName = info.value.substring(0, info.value.indexOf(".")); | ||
| if (out.indexOf(rootName) === -1) out.push(rootName); |
There was a problem hiding this comment.
Skip member-access roots that are macro parameters.
For function-like macros such as #define GET_UV(input) input.v_uv, this branch currently records input as an external reference. That makes VariableIdentifier.semanticAnalyze() look up and potentially mark an unrelated symbol named input, which is wrong for macro-local placeholders.
Suggested fix
} else if (valueType === MacroValueType.MemberAccess) {
// Extract root symbol: "input.v_uv" → "input"
const rootName = info.value.substring(0, info.value.indexOf("."));
+ if (info.params.indexOf(rootName) !== -1) continue;
if (out.indexOf(rootName) === -1) out.push(rootName);
} else if (valueType === MacroValueType.Other) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shader-lab/src/Preprocessor.ts` around lines 66 - 69, The
MemberAccess branch in Preprocessor.ts is incorrectly treating macro parameter
placeholders (e.g., `input` in `#define GET_UV(input) input.v_uv`) as external
references; update the logic in the MacroValueType.MemberAccess handling so that
after extracting rootName from info.value you check whether rootName is a
parameter of the current macro and skip pushing it into out when it is (i.e.,
only push rootName if it is not listed among the macro's parameter names).
Reference symbols: MacroValueType.MemberAccess, info.value, rootName, out, and
VariableIdentifier.semanticAnalyze to ensure macro-local placeholders are not
treated as external identifiers.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
requested changes
Apr 17, 2026
Member
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
支持 #define VAR o.v_uv 风格的宏——CodeGen 在变换时自动剥离 struct variable 前缀。Preprocessor 新增 MacroValueType.MemberAccess 类型，AST 跳过成员访问宏的类型推断，CodeGen 执行 varName.propName → propName 替换并抑制全局 struct 变量的 uniform 输出。三层设计清晰，测试覆盖 4 种模式。
自上次 CHANGES_REQUESTED (2026-04-17) 以来无新 commit。之前提出的 P1 问题仍未修复。
已关闭问题清单
| # | 问题 | 状态 |
|---|---|---|
| 1 | Preprocessor vs CodeGenVisitor 正则分类不一致 | P2，不阻塞 |
| 2 | 静态共享 /g flag regex 重入风险 |
P3，不阻塞 |
| 3 | referenceStructPropByName 对不存在的成员静默跳过 |
P2，不阻塞 |
| 4 | _collectStructVarsFromBody / _collectStructVarsFromNode 重复逻辑 |
简化建议，不阻塞 |
| 5 | symOut 数组复用问题 |
误报——SymbolTable.getSymbols() 内部已 out.length = 0 |
问题
- [P1]
Preprocessor.ts:63-69—MemberAccess分支缺少 macro param 守卫 — 对于函数式宏如#define GET_UV(input) input.v_uv，rootName("input") 是宏参数而非外部引用。Symbol和FunctionCall分支已有if (info.params.indexOf(referencedName) !== -1) continue守卫，MemberAccess分支遗漏。修复：
} else if (valueType === MacroValueType.MemberAccess) {
const rootName = info.value.substring(0, info.value.indexOf("."));
if (info.params.indexOf(rootName) !== -1) continue; // ← 加这行
if (out.indexOf(rootName) === -1) out.push(rootName);
}- [P1]
CodeGenVisitor.ts:405-426—_collectStructVars清空_structVarMap时丢失全局结构体变量 — 函数体宏（如#define UV o.v_uv）中o是全局变量时（通过visitGlobalVariableDeclaration→registerStructVar注册），清空后变换停止工作。当前全局宏路径通过overrideMap参数绕过了此问题，但函数体内引用全局 struct 变量的宏会失败。修复：清空后从_globalStructVarMap恢复全局注册：
private _collectStructVars(fnNode, context): void {
const map = context._structVarMap;
for (const key in map) delete map[key];
// Restore global struct variable registrations
const globalMap = context._globalStructVarMap;
for (const key in globalMap) map[key] = globalMap[key];
// ... rest of collection
}以上两个 P1 问题自首次 review (2026-04-15) 以来已连续 4 轮提出，请修复后再合入。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#define VAR o.v_uvstyle macros: CodeGen now strips struct variable prefix during transformation#definewith cross-stage struct variable transformationuniformoutput)MacroValueType.MemberAccessclassification in PreprocessorTest plan
define-struct-access.shaderwith expected GLSL output snapshot comparisondefine-struct-access-global.shaderfor global define cross-stage testmacro-member-access-builtin-arg.shaderfor Cocos FSInput patternglobal-varying-var.shaderfor Cocos VSOutput patternuniform Varyingsin outputSummary by CodeRabbit
Release Notes
New Features
input.v_uv,v.rgb).Bug Fixes
Tests