Fix generic type deduce bug of ShaderLab compiler in verbose mode #2477
Conversation
WalkthroughThis pull request introduces modifications to the shader parsing and built-in function handling in the Galacean shader system. The changes primarily focus on improving the semantic analysis of function calls, specifically in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Sway007
added
bug
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2477 +/- ##
===========================================
- Coverage 68.44% 68.28% -0.16%
===========================================
Files 925 925
Lines 96170 96105 -65
Branches 8155 8168 +13
===========================================
- Hits 65821 65628 -193
- Misses 30094 30221 +127
- Partials 255 256 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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
🧹 Nitpick comments (3)
packages/shader-lab/src/parser/builtin/functions.ts (1)
35-36: Consider clarifying the purpose ofsignatures.It’s not immediately clear that this array is being used to store both the return type and parameter types. A more descriptive name (e.g.,
resolvedTypes) or explanatory comment could improve readability.tests/src/shader-lab/ShaderLab.test.ts (1)
21-42: Consider isolating or documenting common macros.Defining the macros inline with the test is convenient, but future expansions might complicate the file. Consider extracting them into a separate constants file or adding doc comments.
tests/src/shader-lab/ShaderValidate.ts (1)
93-97: Consider adding specific test cases for generic type deduction.Given that this PR fixes a bug with generic type deduction for the
clampfunction, it would be valuable to add specific test cases that verify:
- Correct type deduction for
vec3 clamp(vec3, float, float)- Error cases for incorrect type usage
Would you like me to help create test cases that specifically validate the generic type deduction fix for the
clampfunction?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/src/shader-lab/shaders/builtin-function.shaderis excluded by!**/*.shader
📒 Files selected for processing (6)
packages/shader-lab/src/parser/AST.ts(1 hunks)packages/shader-lab/src/parser/builtin/functions.ts(3 hunks)tests/package.json(1 hunks)tests/src/shader-lab/ShaderLab.test.ts(3 hunks)tests/src/shader-lab/ShaderValidate.ts(1 hunks)tests/vitest.config.ts(1 hunks)
🔇 Additional comments (9)
packages/shader-lab/src/parser/builtin/functions.ts (3)
67-67: Refine function signature for clarity.
The updated getFn method uses clearer parameter naming (parameterTypes). This improves maintainability by making the signature more explicit.
89-97: Validate type matching logic fully.
Proceeding with found = true if each compared type matches or is generic is correct. However, for more complex scenarios (e.g., multiple generics in a single built-in), you may need additional checks. If you anticipate more generics, consider systematically storing type mappings.
261-262: Confirm return type for texture2D.
Changing the return from SAMPLER2D to VEC4 aligns with GLSL usage (sampler2D typically returns vec4). Ensure that no other parts of the code still expect the deprecated return type.
tests/vitest.config.ts (1)
22-22: Ensure that running in non-headless mode meets CI requirements.
Removing the headless: true option can be beneficial for interactive debugging, but might break or slow automated CI pipelines. Verify that CI supports the new setting.
tests/src/shader-lab/ShaderLab.test.ts (2)
12-12: New import for registerIncludes.
This import is necessary for the new macros and include registration. Ensure that the underlying library is installed and well-documented in the test setup instructions.
150-156: Test coverage for built-in functions.
Introducing the "builtin-function" test is a good step. Confirm coverage includes scenarios where clamp with vector parameters is used, matching the PR’s objective about correct type deduction.
Would you like an additional test focusing specifically on clamp(vec3, float, float) usage?
packages/shader-lab/src/parser/AST.ts (2)
704-704: Retrieve built-in function carefully.
getFn(fnIdent, paramSig) may return undefined; ensure you handle that scenario before proceeding. Otherwise, an error might occur on the following lines.
706-706: Safely use builtinFn.realReturnType.
After verifying builtinFn is not null, using builtinFn.realReturnType is appropriate. Good job aligning this AST logic with the newly introduced property in BuiltinFunction.
tests/package.json (1)
26-27: Added @galacean/engine-shader-shaderlab dependency.
This is essential for the new shader-lab tests and macros. Verify that the version is pinned correctly to avoid mismatches during local or CI builds.
Sway007
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/src/shader-lab/ShaderLab.test.ts (2)
21-41: Add documentation for shader macros.While the macro definitions are well-structured, adding documentation would improve maintainability by explaining:
- The purpose of each macro
- When each macro should be enabled/disabled
- The impact of each macro on shader behavior
150-150: Add error handling for include registration.Consider wrapping the
registerIncludes()call in a try-catch block to handle potential registration failures gracefully.- registerIncludes(); + try { + registerIncludes(); + } catch (error) { + console.error('Failed to register shader includes:', error); + throw error; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/shader-lab/src/parser/AST.ts(1 hunks)packages/shader-lab/src/parser/builtin/functions.ts(3 hunks)tests/package.json(1 hunks)tests/src/shader-lab/ShaderLab.test.ts(3 hunks)tests/src/shader-lab/ShaderValidate.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/package.json
- tests/src/shader-lab/ShaderValidate.ts
- packages/shader-lab/src/parser/AST.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
- GitHub Check: build (22.x, macos-latest)
- GitHub Check: codecov
🔇 Additional comments (5)
packages/shader-lab/src/parser/builtin/functions.ts (4)
67-85: LGTM: Generic type deduction logic looks correct.The implementation properly handles generic parameter type matching, which should fix the type deduction bug mentioned in the PR objectives.
261-262: LGTM: Correct texture2D return type.The change to return
VEC4instead ofSAMPLER2Daligns with the GLSL specification wheretexture2Dreturns avec4.
35-39:⚠️ Potential issueAdd safety check for empty signatures array.
The
realReturnTypegetter could returnundefinedifsignaturesis empty, potentially causing runtime errors.Apply this diff to add a safety check:
get realReturnType(): NonGenericGalaceanType { - return this.signatures[0]; + return this.signatures.length > 0 ? this.signatures[0] : TypeAny; }Likely invalid or redundant comment.
89-96: 🛠️ Refactor suggestionConsider immutable approach to prevent shared state issues.
Modifying the shared
signaturesarray could cause issues with concurrent calls or re-entrancy.Apply this diff to create a new instance instead:
if (found) { - fn.signatures.length = 0; - fn.signatures.push(returnType); - for (let i = 0; i < argLength; i++) { - fn.signatures.push(parameterTypes[i]); - } - return fn; + const newFn = new BuiltinFunction(fn.ident, fn._returnType, fn.scope, ...fn.args); + newFn.signatures.push(returnType); + for (let i = 0; i < argLength; i++) { + newFn.signatures.push(parameterTypes[i]); + } + return newFn; }Likely invalid or redundant comment.
tests/src/shader-lab/ShaderLab.test.ts (1)
12-12: LGTM!The import statement follows the project's import style and is required for the new test functionality.
| it("builtin-function", async () => { | ||
| let shaderSource = await readFile("./shaders/builtin-function.shader"); | ||
| shaderSource = shaderSource.replace("__$$insert_maros$$__", commonMacros); | ||
| glslValidate(shaderSource, shaderLabVerbose, {}); |
There was a problem hiding this comment.
💡 Codebase verification
Review comment is accurate - test needs improvement
The test case doesn't properly validate the generic type deduction bug fix. The shader file doesn't contain any specific tests for the clamp function with vector types, and the test only verifies basic shader compilation. The original review suggestions remain valid:
- Add explicit test cases using
clampwithvec3parameters - Include assertions to verify correct type inference
- Add error cases with type mismatches
🔗 Analysis chain
Enhance test coverage for the generic type deduction bug.
The current test case doesn't fully validate the fix for the generic type deduction bug. Consider:
- Adding specific assertions to verify the correct type inference for the
clampfunction - Including test cases for both correct and incorrect generic type deductions
- Adding error cases to ensure the compiler properly handles type mismatches
Run this script to check if the test shader contains the relevant clamp function usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the test shader contains the clamp function usage with vec3 type
# Expected: Find usage of clamp with vec3 parameter
rg -A 3 'clamp\s*\(\s*vec3' ./shaders/builtin-function.shader
Length of output: 137
Script:
#!/bin/bash
# Find all shader files in the repository
fd -e shader
# If no .shader files found, try looking for common shader extensions
fd -e glsl -e vert -e frag
Length of output: 7753
Script:
#!/bin/bash
# Check the content of builtin-function shader
cat tests/src/shader-lab/shaders/builtin-function.shader
# Also search for any clamp usage in other shader files for context
rg "clamp\s*\(" --type glsl -A 2
Length of output: 1046
Script:
#!/bin/bash
# Search for clamp usage in all files without type restriction
rg "clamp\s*\(" -A 2
# Check the content of ForwardPassPBR.glsl
cat packages/shader-shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl
Length of output: 27020
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/shader-lab/src/parser/builtin/functions.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (3)
packages/shader-lab/src/parser/builtin/functions.ts (3)
35-40: LGTM! Clean implementation of return type storage.The addition of
_realReturnTypeand its getter provides a clean way to store and access the concrete return type after generic type deduction.
258-259: LGTM! Correct texture2D return type.The updated signatures now correctly specify
VEC4as the return type fortexture2Dfunctions, aligning with the GLSL specification.
68-94: Verify the generic type deduction with test cases.The refactored implementation correctly handles generic type deduction, particularly fixing the
clampfunction issue. However, let's verify the changes with different function signatures.Run the following script to find all generic function calls that could be affected:
✅ Verification successful
Generic type deduction implementation is correct and comprehensive.
The implementation correctly handles all generic function patterns found in the codebase, including:
- Single generic parameter functions (e.g.,
sin,cos)- Multiple generic parameters (e.g.,
clamp,mix)- Mixed generic and non-generic parameters (e.g.,
step,texture)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all generic function calls in shader files # Look for function calls with generic types (GenType, GenIntType, etc.) ast-grep --pattern 'EGenType.$_'Length of output: 24154
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fix generic type deduce bug in ShaderLab compiler of verbose mode.
Summary by CodeRabbit
New Features
Bug Fixes
Chores