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.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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).
Single-root GLTF scenes no longer wrap the root node in a GLTF_ROOT container, which avoids redundant nesting and fixes animation path resolution for models like Mixamo characters. - GLTFSceneParser: single-root scenes use the node directly as scene root - GLTFAnimationParser: remove single-root path prefixing (no longer needed) - Entity.findByPath: prefer real same-name child over legacy self-name prefix - Add AGENTS.md to .gitignore
… wrapper" This reverts commit 8a86f279eca9b2bc4a2fa56f94f174236f9a5674.
PhysX 射线/sweep 起点在碰撞体内部时会返回 distance=0 的命中， 与 Unity 行为不一致。通过启用 POST_FILTER 并在 postFilter 中 过滤 distance<=0 的结果，使 PhysX 跳过 initial overlap 并继续 查找下一个最近命中。 - 启用 POST_FILTER query flag - raycast 和 sweep callback 添加 postFilter 过滤 distance<=0 - 更新 PhysX wasm CDN URL（对应 QueryBinding.h postFilter 改动） - 更新本地 wasm 产物
…e bound args Previously `Signal._addListener` applied bound arguments first and runtime signal args last, producing `method(...boundArgs, ...signalArgs)`. This made the event object's position shift with the number of bound arguments and diverged from the DOM / Cocos `(event, customEventData)` convention. Flip the order so the listener method receives runtime args first and bound args last: `method(...signalArgs, ...boundArgs)`. The event object now always sits at index 0, making migrated Cocos scripts with `(event, customData)` signatures work without rewrites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…to fix/shaderlab
…destroy When loadScene is called with a URL whose cached Scene is the current active scene, resourceManager.load returned the same instance, then destroyOldScene branch destroyed it and re-added it—leaving rootEntities empty and the native PhysicsScene released. Now evict the cached Scene asset before load so a fresh Scene instance is created, matching director.loadScene semantics in Cocos.
从 PR #2957 (feat/gpu-instancing) cherry-pick 69 个 commit 到 fix/shaderlab 分支，压缩为单次提交。 ## 核心改动 ### 新增文件 - `InstanceBuffer.ts` — UBO-based 实例数据管理，将多个 renderer 的 worldMatrix/shaderData 打包到一个 UBO 中 - `VertexMergeBatcher.ts` — 替代旧 BatchUtils，统一 2D/3D 合批入口 - `ShaderBlockProperty.ts` — UBO block 属性描述 - `ShaderProgramMap.ts` — 替代旧 ShaderProgramPool，支持 instancing layout 缓存 - `ConstantBufferBindingPoint.ts` — UBO binding point 枚举 ### 修改文件 - `MeshRenderer._canBatch/_batch` — 同 mesh+material+macros 自动合批判定 - `SkinnedMeshRenderer` — 标记不可 GPU instance - `RenderQueue` — 按 material/primitive 排序（替代按距离排序）， 渲染时检测 instanced batch 并通过 InstanceBuffer 一次 draw - `RenderElement` — 扁平化（移除 SubRenderElement），支持 instancedRenderers 列表 - `ShaderFactory` — UBO 布局计算、instancing GLSL 注入（RENDERER_GPU_INSTANCE macro） - `ShaderPass` — 编译时检测 GPU instance macro，注入 UBO 声明 - `ShaderProgram` — 存储 instanceLayout - `Renderer` — 移除 batched 相关字段 ### 删除文件 - `BatchUtils.ts` → 替换为 `VertexMergeBatcher.ts` - `SubRenderElement.ts` → 合并到 `RenderElement.ts` - `ShaderProgramPool.ts` → 替换为 `ShaderProgramMap.ts` ## cherry-pick 冲突解决记录 fix/shaderlab 分支和 PR 基线 (dev/2.0) 的差异主要在以下文件： 1. **ShaderPass.ts** — fix/shaderlab 使用 `Shader._shaderLab._parseMacros` 处理 ShaderLab 宏，而 PR 使用 `ShaderMacroProcessor.evaluate`（fix/shaderlab 上不存在）。解决方式：保留 fix/shaderlab 的宏处理，加入 PR 的 instancing UBO 注入逻辑。 2. **Transform.glsl** — fix/shaderlab 已有 `camera_VPMat` 声明，PR 也添加了。 解决方式：合并两边声明。 3. **UIRenderer.ts** — PR 将 `BatchUtils` 重命名为 `VertexMergeBatcher`， `batchFor2D` 重命名为 `batch`。fix/shaderlab 的 UI 包未同步。 解决方式：手动更新 UI 包的 import 和调用。 4. **GLSLIfdefResolver.ts** — PR 早期 commit 新增此文件，后续 commit 删除。 cherry-pick 后 ShaderPass.ts 残留了 import。解决方式：删除无用 import。 ## 验证结果 CarParking 游戏 DrawCall 从 905 降至 ~80（同 mesh+material 的座椅、轮子等 自动合批）。
cherry-pick from #2957 commit 2956730 compilePlatformSource 路径中 #ifdef 块未展开就执行 _scanInstanceUniforms， 导致非活跃分支内的 renderer_JointMatrix 被正则命中报错。 - compilePlatformSource 构建 activeMacros 传入 injectInstanceUBO - 新增 _scanInstanceUniformsWithMacros，逐行追踪 #ifdef/#ifndef/#else/#endif - array uniform 从 return "" 改为 return match（保留为普通 uniform）
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
5 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