Fix generic type deduce bug of ShaderLab compiler in verbose mode #2477
Conversation
WalkthroughThe pull request introduces modifications to the shader parsing and semantic analysis process, focusing on improvements in handling built-in functions. The changes primarily affect the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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.43% 68.94% +0.50%
===========================================
Files 922 922
Lines 95661 95670 +9
Branches 8121 8134 +13
===========================================
+ Hits 65466 65959 +493
+ Misses 29942 29458 -484
Partials 253 253
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
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
Tests
Chores