fix(shader-lab): resolve generic return type for texture/builtin functions#2966
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
… 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:
WalkthroughAdds a generic-family type system and a new builtin overload resolver that locks generic indices across arguments to compute concrete return types; replaces concrete texture builtin signatures with generic-family signatures; and adds three GLSL validation tests covering generic and macro cases. Changes
Sequence Diagram(s)sequenceDiagram
participant AST as AST.FunctionCallGeneric
participant Resolver as BuiltinFunction.resolveOverload
participant Families as GenericFamilies / familyIndexOf
participant TypeSys as Type projection
AST->>Resolver: request overload resolution (fnIdent, paramSig)
Resolver->>Families: inspect param positions, map generics to concrete indices
Families-->>Resolver: index locks per GenericDimension
Resolver->>TypeSys: project locked index through return family
TypeSys-->>Resolver: concrete realReturnType (or TypeAny)
Resolver-->>AST: builtinFn with realReturnType
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2966 +/- ##
===========================================
+ Coverage 77.09% 77.79% +0.69%
===========================================
Files 901 906 +5
Lines 98869 99074 +205
Branches 9847 10027 +180
===========================================
+ Hits 76225 77074 +849
+ Misses 22474 21831 -643
+ Partials 170 169 -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:
|
…ands in preprocessor Covers #if !0, #if !1, and compound expressions like #if FOO != 4 && (!0 || FOO).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/shader-lab/ShaderLab.test.ts (1)
261-264: Add verbose-mode validation for the new generic-return regression tests.Line 263 and Line 268 currently validate only release mode. Given these are parser return-type regressions, add
shaderLabVerbosechecks too for mode parity.♻️ Proposed test parity update
it("texture-generic (GVec4 → vec4 resolve)", async () => { const shaderSource = await readFile("./shaders/texture-generic.shader"); + glslValidate(engine, shaderSource, shaderLabVerbose); glslValidate(engine, shaderSource, shaderLabRelease); }); it("generic-return-type (builtin generic return as arg to user function)", async () => { const shaderSource = await readFile("./shaders/generic-return-type.shader"); + glslValidate(engine, shaderSource, shaderLabVerbose); glslValidate(engine, shaderSource, shaderLabRelease); });Also applies to: 266-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/shader-lab/ShaderLab.test.ts` around lines 261 - 264, The tests only call glslValidate(...) with shaderLabRelease; add a second validation call using shaderLabVerbose for parity. In the "texture-generic (GVec4 → vec4 resolve)" test (the it(...) block) call glslValidate(engine, shaderSource, shaderLabVerbose) after the existing release check, and do the same for the other regression test around the other it(...) block referenced (the one at 266-269) so both release and verbose modes are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/src/shader-lab/ShaderLab.test.ts`:
- Around line 261-264: The tests only call glslValidate(...) with
shaderLabRelease; add a second validation call using shaderLabVerbose for
parity. In the "texture-generic (GVec4 → vec4 resolve)" test (the it(...) block)
call glslValidate(engine, shaderSource, shaderLabVerbose) after the existing
release check, and do the same for the other regression test around the other
it(...) block referenced (the one at 266-269) so both release and verbose modes
are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1d6b47f2-440d-4633-925f-68fdf6985d7f
⛔ Files ignored due to path filters (1)
tests/src/shader-lab/shaders/macro-negate-number.shaderis excluded by!**/*.shader
📒 Files selected for processing (1)
tests/src/shader-lab/ShaderLab.test.ts
…ure-generic-return # Conflicts: # tests/src/shader-lab/ShaderLab.test.ts
Replace the ad-hoc `resolveGenericReturnType` switch with an explicit data-driven generic family system. Each family (GenType, GVec4, GSampler2D, ...) declares its members and the dimension it varies along (Size / Prefix); overload resolution locks an index per dimension while walking arguments and projects return families through that lock. This fixes the underlying defect that produced the texture/textureCube bugs: the previous algorithm assumed "return family == parameter family" and collapsed `GVec4 <- GSampler2D` to `SAMPLER2D`. Cross-family projection (sampler → gvec4) is now a native operation, so the textureCube/textureCubeLod/texture2D/texture2DLod overloads that the initial fix listed as concrete-only entries can be written as plain generic signatures again. Return type falls through as `TypeAny` when no argument locks the dimension (all args are TypeAny), preserving wildcard-compatible overload resolution downstream. Also exercises the Verbose semantic pass in the two new test shaders — the initial fix registered them under Release only, which skips semantic analysis and therefore could not catch the bug it was meant to prevent.
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/parser/builtin/functions.ts`:
- Around line 162-225: The texture() overloads that currently declare a concrete
sampler argument (Keyword.SAMPLER2D_ARRAY) fail to lock the generic return type
(EGenType.GVec4) so calls resolve to TypeAny; locate the texture overload
entries that use Keyword.SAMPLER2D_ARRAY (the ones mentioned near the GVec4
return) and change their sampler parameter type to the corresponding generic
sampler type (e.g., EGenType.GSampler2DArray) so GenericFamilies can produce a
prefix/dimension lock and _realReturnType resolves to vec4; ensure the modified
overloads match the pattern used by other texture() entries (like those using
EGenType.GSampler2D) so return type locking works.
🪄 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: e5e50ae5-c9ee-4f56-ac68-0af5c607acc9
📒 Files selected for processing (2)
packages/shader-lab/src/parser/builtin/functions.tstests/src/shader-lab/ShaderLab.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/shader-lab/ShaderLab.test.ts
Follow-up to the generic family refactor: tighten naming, remove dead code, and collapse redundant helpers. Naming - `EGenType` → `GenericType` (drop hungarian prefix for a private enum) - `GenericDim` → `GenericDimension`, `dim` → `dimension` - `GenericFamilies` / `FamilyIndexCache` → `FamilyMembers` / `FamilyMemberIndex` (matching names for the forward + reverse tables of the same concept) - `GenericDimension.Prefix` → `ScalarType` (scalar type rather than lexical prefix) - `BuiltinFunction.getFn` → `resolveOverload` (overload resolution is the standard compiler term for what it does) Dead code removal - Drop `isGenericType` helper: membership in `FamilyMembers` is the single source of truth, replacing the fragile enum-range check - Drop unused `BuiltinFunction.getReturnType` static method - Drop now-impossible `!family` / `!returnFamily` guards inside `resolveOverload` (covered by the membership check above) `resolveOverload` now accepts `undefined` parameter types to match the AST call site where an empty paren-only call yields no signature. Comments follow the project style (single-line `//` without trailing periods).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/shader-lab/src/parser/builtin/functions.ts (1)
400-401:⚠️ Potential issue | 🟠 MajorUse the generic sampler family for sampler2DArray texture overloads.
Lines 400-401 still pass concrete
Keyword.SAMPLER2D_ARRAY, soGenericType.GVec4has no scalar-type lock and resolves toTypeAnyinstead ofvec4/ivec4/uvec4.🐛 Proposed fix
-BuiltinFunction._create("texture", GenericType.GVec4, Keyword.SAMPLER2D_ARRAY, Keyword.VEC3, Keyword.FLOAT); -BuiltinFunction._create("texture", GenericType.GVec4, Keyword.SAMPLER2D_ARRAY, Keyword.VEC3); +BuiltinFunction._create("texture", GenericType.GVec4, GenericType.GSampler2DArray, Keyword.VEC3, Keyword.FLOAT); +BuiltinFunction._create("texture", GenericType.GVec4, GenericType.GSampler2DArray, Keyword.VEC3);🤖 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 400 - 401, The texture overloads registered via BuiltinFunction._create use the concrete Keyword.SAMPLER2D_ARRAY which prevents GenericType.GVec4 from getting a scalar-type lock; update both overload entries to use the generic sampler family type (the sampler family keyword used elsewhere in the parser) instead of Keyword.SAMPLER2D_ARRAY so GenericType.GVec4 correctly resolves to vec4/ivec4/uvec4 for the sampler2DArray overloads.
🤖 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/parser/builtin/functions.ts`:
- Around line 54-55: The formatting of the GenericType registrations (e.g.,
GenericType.GenType and GenericType.GenIntType in functions.ts) is inconsistent
and causing Prettier lint failures; run the repository formatter (e.g., npm/yarn
format or npx prettier --write .) to reformat the file and the other affected
places (lines around the other registrations at the referenced locations), then
commit the formatted changes so the lint check passes.
- Line 291: The BuiltinFunction registration for "floatBitsToInt" currently uses
GenericType.GenType as its return type; change the return type to
GenericType.GenIntType so floatBitsToInt(genType x) returns the integer family
(match the pattern used by floatBitsToUint), i.e. update the
BuiltinFunction._create call for "floatBitsToInt" to use GenericType.GenIntType
as the first argument.
---
Duplicate comments:
In `@packages/shader-lab/src/parser/builtin/functions.ts`:
- Around line 400-401: The texture overloads registered via
BuiltinFunction._create use the concrete Keyword.SAMPLER2D_ARRAY which prevents
GenericType.GVec4 from getting a scalar-type lock; update both overload entries
to use the generic sampler family type (the sampler family keyword used
elsewhere in the parser) instead of Keyword.SAMPLER2D_ARRAY so GenericType.GVec4
correctly resolves to vec4/ivec4/uvec4 for the sampler2DArray overloads.
🪄 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: 9ef4762f-dfce-46a7-8faf-d1d6dbb79584
📒 Files selected for processing (2)
packages/shader-lab/src/parser/AST.tspackages/shader-lab/src/parser/builtin/functions.ts
| BuiltinFunction._create("smoothstep", GenericType.GenType, Keyword.FLOAT, Keyword.FLOAT, GenericType.GenType); | ||
| BuiltinFunction._create("isnan", GenericType.GenBoolType, GenericType.GenType); | ||
| BuiltinFunction._create("isinf", GenericType.GenBoolType, GenericType.GenType); | ||
| BuiltinFunction._create("floatBitsToInt", GenericType.GenType, GenericType.GenType); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GLSL ES floatBitsToInt builtin return type genIType
💡 Result:
In GLSL ES (including versions 3.00, 3.10, 3.20), the builtin function floatBitsToInt has the signature genIType floatBitsToInt(genType x). The return type is genIType, which denotes the generic signed integer type matching the dimension of the input genType x. Specifically, genIType represents int, ivec2, ivec3, or ivec4, depending on whether x is scalar float, vec2, vec3, or vec4. This is consistent across GLSL ES 3.x specifications and reference pages.
Citations:
- 1: https://docs.gl/el3/floatBitsToInt
- 2: https://registry.khronos.org/OpenGL-Refpages/es3/html/floatBitsToInt.xhtml
- 3: https://stackoverflow.com/questions/23824374/what-does-the-term-gentype-mean-in-opengl-glsl
🏁 Script executed:
# Read the file around line 291 to confirm the current state
sed -n '285,295p' packages/shader-lab/src/parser/builtin/functions.tsRepository: galacean/engine
Length of output: 992
🏁 Script executed:
# Search for GenIntType usage in the file to confirm it exists
rg "GenIntType" packages/shader-lab/src/parser/builtin/functions.tsRepository: galacean/engine
Length of output: 1088
🏁 Script executed:
# Search for similar integer-returning functions to understand the pattern
rg "intBitsToFloat|uintBitsToFloat|GenIntType" packages/shader-lab/src/parser/builtin/functions.ts -A 1Repository: galacean/engine
Length of output: 1644
Return the integer family from floatBitsToInt.
Line 291 currently returns GenericType.GenType, but the GLSL ES builtin floatBitsToInt(genType x) returns genIType (integer family). The return type should be GenericType.GenIntType, consistent with the pattern established by floatBitsToUint on line 292 which correctly uses GenericType.GenUintType.
Proposed fix
-BuiltinFunction._create("floatBitsToInt", GenericType.GenType, GenericType.GenType);
+BuiltinFunction._create("floatBitsToInt", GenericType.GenIntType, GenericType.GenType);📝 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.
| BuiltinFunction._create("floatBitsToInt", GenericType.GenType, GenericType.GenType); | |
| BuiltinFunction._create("floatBitsToInt", GenericType.GenIntType, GenericType.GenType); |
🤖 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` at line 291, The
BuiltinFunction registration for "floatBitsToInt" currently uses
GenericType.GenType as its return type; change the return type to
GenericType.GenIntType so floatBitsToInt(genType x) returns the integer family
(match the pattern used by floatBitsToUint), i.e. update the
BuiltinFunction._create call for "floatBitsToInt" to use GenericType.GenIntType
as the first argument.
`FamilyMemberIndex` is built from `FamilyMembers` with the same key set, and callers only enter familyIndexOf after confirming the family exists in `FamilyMembers`. The `if (!map) return -1` branch is therefore unreachable — replace with a non-null assertion. One-line function now.
…Exist - Widen `FamilyMembers` / `FamilyMemberIndex` / `familyIndexOf` key type from `GenericType` to `BuiltinType`. Lookup returning undefined for non-generic keys is meaningful (signals "this is a concrete type"), so the wider key type matches the actual usage and lets the 3 callers drop their `as GenericType` assertions. - `BuiltinFunction.isExist` now uses `Map.has` instead of `!!Map.get`: semantic intent is "does the key exist", not "is the value truthy". No runtime behavior change.
The data table hides two rules the spec states in prose: - GenType-like families (Size dimension) can appear in parameter and return positions - Sampler families (GSampler*) are parameter-only; GVec4 is return-only and is projected from a GSampler* parameter Spell these out in the section comments so the role of each family is apparent at the definition site instead of only discoverable by grepping the _create call sites.
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
将 texture/builtin 函数的泛型返回类型解析从 ad-hoc 的"第一个泛型参数决定返回类型"改为基于 dimension-lock 的正式泛型家族系统。GenericDimension.Size 和 GenericDimension.ScalarType 两个独立维度分别锁定,通过投影解析返回类型。这个重构从根本上解决了 texture() 返回 GVec4 无法解析为 vec4 的问题,同时让 isnan(vec3)->bvec3 等跨类型泛型自然工作。设计方向正确,重构质量高。
已关闭问题
之前多轮提出的问题全部已修复或确认为非阻塞:
— 已删除 ✓getReturnType死代码verbose 模式测试缺失— 已补充 ✓— P3 非阻塞,当前消费者立即读取_realReturnType共享实例突变— 部分修复(见下方新发现)SAMPLER2D_ARRAY具体类型— P3 非阻塞isampler2D/usampler2D测试覆盖
问题
-
[P2] functions.ts:427-428 —
texture(sampler2DArray, ...)仍使用具体类型Keyword.SAMPLER2D_ARRAYBuiltinFunction._create("texture", GenericType.GVec4, Keyword.SAMPLER2D_ARRAY, Keyword.VEC3, Keyword.FLOAT); BuiltinFunction._create("texture", GenericType.GVec4, Keyword.SAMPLER2D_ARRAY, Keyword.VEC3);
其他所有
sampler2DArray相关函数(textureSize/Lod/Offset/texelFetch/Grad)都已正确使用GenericType.GSampler2DArray,唯独这两个textureoverload 遗漏。后果:texture(isampler2DArray, ...)无法匹配,且scalarTypeLock永远不会被设置,返回类型 fallback 到TypeAny而非vec4。 -
[P2] functions.ts:449 —
texture2DLodEXT有错误的GSampler3DoverloadBuiltinFunction._create("texture2DLodEXT", GenericType.GVec4, GenericType.GSampler3D, Keyword.VEC3, Keyword.FLOAT);
texture2DLodEXT是GL_EXT_shader_texture_lod扩展,仅适用于 sampler2D(WebGL1 ES 1.00)。不存在 sampler3D 变体。这行应删除。 -
[P3] functions.ts:312 —
floatBitsToInt返回类型应为GenIntTypeBuiltinFunction._create("floatBitsToInt", GenericType.GenType, GenericType.GenType);
Per GLSL ES 3.00 §8.3,
floatBitsToInt(genType) → genIType。对比同行的floatBitsToUint已正确使用GenUintType。当前floatBitsToInt(vec3)会被类型推断为vec3而非ivec3。
P2 不阻塞合并,建议后续补一个 commit 修掉。
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
这个 PR 将 ShaderLab 内置函数的泛型返回类型解析从 ad-hoc 的 getReturnType 替换为数据驱动的 generic family 系统。引入 GenericDimension(Size / ScalarType)两轴匹配,通过 FamilyMembers 表驱动重载解析。修复了 texture() 返回 GVec4 无法传给用户函数的 bug,以及 normalize() 等内置函数对 TypeAny 参数返回原始枚举值的问题。
设计方向优秀。两轴泛型匹配(Size × ScalarType)是对 GLSL spec 泛型语义的精确建模,FamilyMembers 表驱动的方式可扩展且易维护。代码量减少了(resolveOverload 替代了 getFn + getReturnType + isGenericType),符合"好设计产生更少代码"的原则。
已有 2 个 APPROVED,无阻塞问题。
问题
-
[P2] packages/shader-lab/src/parser/builtin/functions.ts —
texture2DLodEXT注册中包含GSampler3D重载(texture2DLodEXT(GSampler3D, vec3, float)),但texture2DLodEXT按 GLSL ES 1.0 扩展规范仅支持sampler2D。GSampler3D重载是textureLod的语义,不应出现在texture2DLodEXT上。不阻塞合并但建议清理。 -
[P3] packages/shader-lab/src/parser/builtin/functions.ts —
_realReturnType是共享实例上的可变字段(candidate._realReturnType = ...)。当前resolveOverload是同步调用不会重入,所以安全。但如果未来有并发场景(如 web worker 编译),这里会出问题。记录为技术债即可。
简化建议
无。当前实现已经很简洁。
cptbtptpbcptdtptp
left a comment
There was a problem hiding this comment.
Overall LGTM — the two-dimension generic family system is clean and models GLSL semantics correctly. Two minor suggestions:
1. FamilyMemberIndex Map-of-Maps can be replaced with indexOf
Each family has at most 4 members. A linear indexOf scan on a 3–4 element array is faster than Map hash lookup (no hashing overhead), and removes the FamilyMemberIndex init loop + familyIndexOf helper entirely:
// Before: pre-built Map<BuiltinType, Map<NonGenericGalaceanType, number>>
const memberIdx = familyIndexOf(declaredType, actualType);
// After: direct indexOf on the small array
const memberIdx = paramFamily.members.indexOf(actualType);2. _createWithScop → _createWithScope (typo)
Pre-existing typo, but easy to fix while you're in this file.
Each generic family has at most 4 members. A linear indexOf on the small members array is faster than a two-level Map lookup (measured ~1.26x on the resolveOverload hot path) and removes the FamilyMemberIndex init loop and familyIndexOf helper. Also fix the `_createWithScop` typo to `_createWithScope`.
|
@cptbtptpbcptdtptp Thanks for the review! Both addressed in 8261b67. 1. You were right — at 3-4 elements, hash overhead dominates. Removed 2. All shader-lab tests still pass (158/158). |
| if (!overloads) return undefined; | ||
| const argCount = callArgTypes?.length ?? 0; | ||
|
|
||
| for (let candidateIdx = 0, overloadCount = overloads.length; candidateIdx < overloadCount; candidateIdx++) { |
There was a problem hiding this comment.
candidateIdx 改成 i , overloadCount 改成 n 吧,更加简洁。
| let scalarTypeLock = -1; | ||
| let matched = true; | ||
|
|
||
| for (let argIdx = 0; argIdx < argCount; argIdx++) { |
There was a problem hiding this comment.
argIdx 改成 i , argCount 改成 n 吧,更加简洁。
Use i/m for candidate loop and j/n for arg loop. The shorter names are easier on the eyes in this hot path and align with the rest of the codebase's loop-var convention.
|
@cptbtptpbcptdtptp Done in 114f3b1 — outer loop uses |
Summary
GVec4generic return type to concretevec4/ivec4/uvec4fortexture(),textureCube(),texture2DLod()etc.TypeAny(instead of raw enum value) when generic builtin functions likenormalize()receiveTypeAnyargumentsTest plan
texture-generic.shadertest case for texture return type resolutiongeneric-return-type.shadertest case for builtin generic return handlingSummary by CodeRabbit
Bug Fixes
New Features
Tests