Draft
Conversation
…tin functions texture(sampler2D, vec2) returns GVec4 which was incorrectly resolved to the sampler type instead of vec4, causing "No overload function type found" when passing the result to user-defined functions like decode32(vec4). Add resolveGenericReturnType() to correctly map GSampler* → GVec4: sampler2D/sampler3D/samplerCube → vec4 isampler2D/isampler3D/... → ivec4 usampler2D/usampler3D/... → uvec4
…exture2DLod signatures - Simplify resolveGenericReturnType: remove genericParamType param, only check if return type is GVec4 - Fix textureCube/textureCubeLod return type: SAMPLER_CUBE → VEC4 - Add missing texture2DLod builtin function registration - Add texture2DLod test cases to texture-generic.shader
* feat: implement HorizontalBillboard render mode
… type When a builtin generic function (e.g. normalize) receives TypeAny args, resolvedReturnType stays TypeAny. Previously the else branch returned the raw EGenType enum value (200), which is neither a concrete type nor a wildcard, causing downstream user-function overload matching to fail.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughClassify dotted member-access macro values, track struct-variable→role mappings at function and global scope, transform macro Changes
Sequence DiagramsequenceDiagram
participant Preprocessor as Preprocessor.ts
participant CodeGen as CodeGenVisitor.ts
participant Context as VisitorContext.ts
participant GLESGen as GLESVisitor.ts
rect rgba(100,150,200,0.5)
note over Preprocessor: Classify macro values, detect member-access forms
Preprocessor->>Preprocessor: Identify "identifier.member" macros
Preprocessor->>Context: expose reference root identifiers
end
rect rgba(100,150,200,0.5)
note over CodeGen,Context: Collect struct-var → role mappings
CodeGen->>Context: getStructRole(typeLexeme)
Context-->>CodeGen: role or undefined
CodeGen->>Context: populate _structVarMap from params & locals
end
rect rgba(150,100,200,0.5)
note over GLESGen,Context: Build global mapping & pre-register macro refs
GLESGen->>GLESGen: build _globalStructVarMap from entry funcs
GLESGen->>Preprocessor: scan global macros for member-access
GLESGen->>Context: referenceStructPropByName(role, prop)
Context->>Context: record referenced property
end
rect rgba(150,200,100,0.5)
note over CodeGen,Context: Transform and emit macro defines
CodeGen->>CodeGen: _transformMacroDefineValue(lexeme, overrideMap)
CodeGen->>Context: consult _structVarMap/_globalStructVarMap
CodeGen->>Context: referenceStructPropByName for matches
CodeGen->>CodeGen: emit transformed macro text
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2946 +/- ##
===========================================
+ Coverage 77.26% 77.51% +0.25%
===========================================
Files 899 899
Lines 98275 98616 +341
Branches 9670 9763 +93
===========================================
+ Hits 75928 76442 +514
+ Misses 22179 22007 -172
+ Partials 168 167 -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:
|
…tin functions texture(sampler2D, vec2) returns GVec4 which was incorrectly resolved to the sampler type instead of vec4, causing "No overload function type found" when passing the result to user-defined functions like decode32(vec4). Add resolveGenericReturnType() to correctly map GSampler* → GVec4: sampler2D/sampler3D/samplerCube → vec4 isampler2D/isampler3D/... → ivec4 usampler2D/usampler3D/... → uvec4
…exture2DLod signatures - Simplify resolveGenericReturnType: remove genericParamType param, only check if return type is GVec4 - Fix textureCube/textureCubeLod return type: SAMPLER_CUBE → VEC4 - Add missing texture2DLod builtin function registration - Add texture2DLod test cases to texture-generic.shader
… type When a builtin generic function (e.g. normalize) receives TypeAny args, resolvedReturnType stays TypeAny. Previously the else branch returned the raw EGenType enum value (200), which is neither a concrete type nor a wildcard, causing downstream user-function overload matching to fail.
…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 #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.
zhuxudong
force-pushed
the
fix/shaderlab
branch
from
af98e0b to
828752d
Compare
…cro as builtin arg
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/shader-lab/src/parser/builtin/functions.ts (2)
39-60: Well-designed helper for GVec4 resolution.The mapping logic for sampler variants to vec4/ivec4/uvec4 is correct. One minor inconsistency: when
actualParamTypeisTypeAny, GVec4 defaults toVEC4rather than returningTypeAny. This differs from non-GVec4 cases (e.g.,GenType) which would preserveTypeAny.If unresolved sampler types should propagate uncertainty downstream, consider:
💡 Optional: Preserve TypeAny for unresolved sampler types
function resolveGenericReturnType( genericReturnType: EGenType, actualParamType: NonGenericGalaceanType ): NonGenericGalaceanType { + if (actualParamType === TypeAny) { + return TypeAny; + } if (genericReturnType === EGenType.GVec4) { switch (actualParamType) {This is likely acceptable as-is since VEC4 is a reasonable default assumption for most shader code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shader-lab/src/parser/builtin/functions.ts` around lines 39 - 60, The resolveGenericReturnType function treats EGenType.GVec4 specially but force-defaults unknown/TypeAny sampler inputs to Keyword.VEC4; update resolveGenericReturnType to preserve uncertainty by checking if actualParamType === TypeAny (or the project’s Any type symbol) and return TypeAny immediately when genericReturnType === EGenType.GVec4, otherwise keep the existing sampler->IVEC4/UVEC4/VEC4 mapping; refer to resolveGenericReturnType, EGenType.GVec4, actualParamType, and Keyword.VEC4/IVEC4/UVEC4 to locate where to add the TypeAny short-circuit.
129-133: Shared state mutation onBuiltinFunctioninstance is limited to debug-only code path.The concern about mutating
fn._realReturnTypeon a sharedBuiltinFunctionTableinstance is technically valid, but has limited scope:getFn()is called in only one location atAST.ts:759, within a#if _VERBOSEdebug/verbose-only preprocessor block. The actual type analysis path usessymbolTableStack.lookup()instead.If the same function is queried with different parameter types within a single analysis pass, earlier resolutions would be overwritten. However, since this code executes only in debug mode and doesn't cache or persist the returned
BuiltinFunctionreference, the practical risk is minimal.Consider refactoring to avoid shared state mutation if this debug code ever becomes part of the main analysis path:
♻️ Suggested refactor: return a result tuple
- static getFn(ident: string, parameterTypes: NonGenericGalaceanType[]): BuiltinFunction | undefined { + static getFn(ident: string, parameterTypes: NonGenericGalaceanType[]): { fn: BuiltinFunction; realReturnType: NonGenericGalaceanType } | undefined { const list = BuiltinFunctionTable.get(ident); if (list) { for (let length = list.length, i = 0; i < length; i++) { const fn = list[i]; // ... matching logic ... if (found) { - fn._realReturnType = isGenericType(fn._returnType) - ? resolvedReturnType - : (fn._returnType as NonGenericGalaceanType); - return fn; + const realReturnType = isGenericType(fn._returnType) + ? resolvedReturnType + : (fn._returnType as NonGenericGalaceanType); + return { fn, realReturnType }; } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shader-lab/src/parser/builtin/functions.ts` around lines 129 - 133, The debug path mutates shared BuiltinFunction state by assigning fn._realReturnType in getFn; instead avoid modifying the shared instance—change getFn (and callers) to return an immutable result (e.g., a tuple/object like { fn: BuiltinFunction, resolvedReturnType: GalaceanType }) or return a shallow-cloned BuiltinFunction with the resolved return type attached, so callers that need the debug-resolved type use the returned resolvedReturnType (or the clone) instead of mutating fn._realReturnType; update the call site that currently expects a BuiltinFunction to accept the new shape and consume resolvedReturnType for verbose output only.packages/shader-lab/src/codeGen/VisitorContext.ts (1)
75-88: Silent no-op when property doesn't exist in struct list.Unlike
referenceVarying/referenceAttribute/referenceMRTPropwhich return errors when the property is not found, this method silently does nothing ifpropNamedoesn't exist in the target list.This is likely intentional for macro-based references (macros can reference properties conditionally via
#ifdef), but consider adding a debug log for diagnostics if this causes issues during development.🤖 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 75 - 88, The method referenceStructPropByName currently silently returns when propName is not found; update it to emit a debug/diagnostic log before returning so missing conditional references are visible during development — keep the no-op behavior but call the VisitorContext's existing logging/diagnostic facility (e.g., this.logger or this.diagnostics) with a message that includes role and propName; modify referenceStructPropByName and ensure the log is at debug/verbose level and only triggers when props.length === 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/shader-lab/src/codeGen/VisitorContext.ts`:
- Around line 75-88: The method referenceStructPropByName currently silently
returns when propName is not found; update it to emit a debug/diagnostic log
before returning so missing conditional references are visible during
development — keep the no-op behavior but call the VisitorContext's existing
logging/diagnostic facility (e.g., this.logger or this.diagnostics) with a
message that includes role and propName; modify referenceStructPropByName and
ensure the log is at debug/verbose level and only triggers when props.length ===
0.
In `@packages/shader-lab/src/parser/builtin/functions.ts`:
- Around line 39-60: The resolveGenericReturnType function treats EGenType.GVec4
specially but force-defaults unknown/TypeAny sampler inputs to Keyword.VEC4;
update resolveGenericReturnType to preserve uncertainty by checking if
actualParamType === TypeAny (or the project’s Any type symbol) and return
TypeAny immediately when genericReturnType === EGenType.GVec4, otherwise keep
the existing sampler->IVEC4/UVEC4/VEC4 mapping; refer to
resolveGenericReturnType, EGenType.GVec4, actualParamType, and
Keyword.VEC4/IVEC4/UVEC4 to locate where to add the TypeAny short-circuit.
- Around line 129-133: The debug path mutates shared BuiltinFunction state by
assigning fn._realReturnType in getFn; instead avoid modifying the shared
instance—change getFn (and callers) to return an immutable result (e.g., a
tuple/object like { fn: BuiltinFunction, resolvedReturnType: GalaceanType }) or
return a shallow-cloned BuiltinFunction with the resolved return type attached,
so callers that need the debug-resolved type use the returned resolvedReturnType
(or the clone) instead of mutating fn._realReturnType; update the call site that
currently expects a BuiltinFunction to accept the new shape and consume
resolvedReturnType for verbose output only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: abd38781-e816-40bf-a49c-fc2c7f48470d
⛔ 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/generic-return-type.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/texture-generic.shaderis excluded by!**/*.shader
📒 Files selected for processing (7)
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.tspackages/shader-lab/src/parser/builtin/functions.tstests/src/shader-lab/ShaderLab.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/src/shader-lab/ShaderLab.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shader-lab/src/codeGen/GLESVisitor.ts
…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)
zhuxudong
force-pushed
the
fix/shaderlab
branch
from
f37867e to
80e6e3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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: The function-local macro limitation is caused by
_collectStructVars wiping context._structVarMap (so function scope loses
globals) while visitGlobalVariableDeclaration stores globals in
context._globalStructVarMap; fix by making struct resolution fall back to
globals or by merging maps: either update _transformMacroDefineValue (used by
defaultCodeGen) to resolve structVarMap as overrideMap ?? context._structVarMap
?? context._globalStructVarMap, or change _collectStructVars to preserve/merge
entries from context._globalStructVarMap into context._structVarMap (so global
struct variables remain available for function-local `#define`), keeping
visitGlobalVariableDeclaration behavior unchanged.
🪄 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: 07e3a56a-3c7c-41dd-b970-efc5baa53b31
⛔ Files ignored due to path filters (1)
tests/src/shader-lab/shaders/global-varying-var.shaderis excluded by!**/*.shader
📒 Files selected for processing (4)
packages/shader-lab/src/codeGen/CodeGenVisitor.tspackages/shader-lab/src/codeGen/GLESVisitor.tspackages/shader-lab/src/codeGen/VisitorContext.tstests/src/shader-lab/ShaderLab.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/src/shader-lab/ShaderLab.test.ts
- packages/shader-lab/src/codeGen/VisitorContext.ts
- packages/shader-lab/src/codeGen/GLESVisitor.ts
…version GLES100 visitJumpStatement converted `return expr;` to `gl_FragColor = expr` without a trailing semicolon, causing WebGL compilation errors. Only triggered when fragment entry returns vec4 (Cocos pattern), not void (standard Galacean).
…eprocessor #if !0 and similar expressions now work correctly, matching C/GLSL preprocessor behavior.
…atrix The viewMatrix getter intentionally ignores the camera entity's world scale (using Matrix.rotationTranslation), but _getInvViewProjMat() was using the full entity.transform.worldMatrix which includes inherited scale. This inconsistency causes screenPointToRay to produce incorrect world-space rays when the camera inherits scale from a parent entity (e.g. UICanvas in ScreenSpaceCamera mode with camera as a child of canvas). Closes #2748 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verify that screenPointToRay and viewport-world round-trip produce correct results when the camera inherits non-identity scale from a parent entity. Without the fix, the round-trip deviates by the inherited scale factor (e.g. 105 -> 107.5 at scale 1.5). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ReflectionParser 解析 IComponentRef（entityPath + componentType）时， 目标组件可能在 GLB clone 的子 entity 上而非 clone 根 entity 自身。 getComponents 找不到时 fallback 到 getComponentsIncludeChildren。
CloneManager: 当 source 和 target 属性是同类型 Object 实例（如 Vector4）时， 自动升级为 DeepClone，避免 prefab 模板的引用覆盖克隆体的独立实例。 新增 Map/Set 类型的 deep clone 支持。 ModelMesh: throw string 改为 throw Error 以获得正确堆栈。
AnimatorState.speed is part of the shared AnimatorController asset. Modifying it at runtime pollutes all Animator instances sharing the same controller, causing animation speed corruption after cloning. - Add speed field to AnimatorStatePlayData, initialized from AnimatorState.speed on reset - Add proxy properties (name/clip/wrapMode/transitions/addStateMachineScript) - Change speed calculation to playData.speed * animator.speed - findAnimatorState now returns per-instance AnimatorStatePlayData - Export AnimatorStatePlayData for consumer code
When sizeMode is set to Automatic, the UITransform size is automatically synchronized to the sprite's natural dimensions when the sprite changes. This matches Cocos Creator's Sprite.SizeMode.TRIMMED behavior. - Add SpriteSizeMode enum (Custom / Automatic) - Add sizeMode property to Image with getter/setter - Sync UITransform.size in set sprite and _onSpriteChange - Export SpriteSizeMode from component index
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
4 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
(GVec4 resolved to sampler type instead of vec4), causing "No overload function type found" errors
are unresolved, breaking downstream overload matching
during CodeGen, causing invalid GLSL output
Test plan
varying properties
Summary by CodeRabbit
New Features
Bug Fixes
Tests