Fix shader pass can't use SubShader content cause by scope loss when ShaderLab parse Pass
#2393
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2393 +/- ##
==========================================
- Coverage 69.50% 69.49% -0.02%
==========================================
Files 524 522 -2
Lines 27405 27356 -49
Branches 4100 4096 -4
==========================================
- Hits 19049 19010 -39
+ Misses 6852 6845 -7
+ Partials 1504 1501 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Sway007
added
bug
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 Biome
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/src/shader-lab/shaders/multi-pass.shader (2)
63-63: LGTM: BlendState correctly applied to Pass "0"The
transparentBlendStateis properly applied to Pass "0", enabling transparent rendering for objects processed by this pass. This change aligns with the PR objective of fixing scope loss in Pass parsing.Consider adding a comment explaining why this specific BlendState is used for this pass, to improve code readability and maintainability.
92-92: LGTM: BlendState correctly applied to Pass "1"The
transparentBlendStateis properly applied to Pass "1", consistent with the changes in Pass "0" and the PR objective.Consider refactoring the shader to use a shared BlendState configuration for multiple passes. This could improve maintainability by centralizing the blend state definition. For example:
Properties { _SharedBlendState ("Shared Blend State", BlendState) = "transparentBlendState" {} } // ... in each pass that needs the blend state BlendState = _SharedBlendStateThis approach would allow easier updates to the blend state across multiple passes.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (1)
392-392: LGTM! Consider adding a comment for clarity.The addition of
this._newScope();at the beginning of the_parsePassmethod is a good improvement. It creates a new symbol table scope for each pass, which helps in encapsulating variables and preventing scope-related issues.Consider adding a brief comment explaining the purpose of creating a new scope here, for example:
+ // Create a new scope for the pass to encapsulate its variables this._newScope();This would enhance code readability and make the intent clearer for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/shader-lab/src/contentParser/ShaderContentParser.ts (1 hunks)
- tests/src/shader-lab/shaders/multi-pass.shader (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
tests/src/shader-lab/shaders/multi-pass.shader (2)
5-11: LGTM: Well-defined BlendState for transparencyThe new
transparentBlendStateis correctly defined with appropriate blend factors for transparent rendering. This configuration enables alpha blending, which is crucial for properly rendering transparent objects.
Line range hint
1-150: Overall: Successful implementation of transparent renderingThe changes successfully implement a new BlendState for transparent rendering and apply it consistently across multiple passes. This aligns well with the PR objective of fixing scope loss when parsing the
Passscope in ShaderLab.Key points:
- A well-defined
transparentBlendStateis introduced.- The blend state is correctly applied to Pass "0" and Pass "1".
- The implementation enables proper alpha blending for transparent objects.
These changes should resolve the scope loss issue and improve the shader's capability to handle transparent rendering. Consider the suggested optimizations to further enhance code maintainability.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (2)
392-392: Summary: Effective solution for scope management in Pass parsingThe changes made to the
_parsePassmethod effectively address the scope loss issue when parsing thePassscope in ShaderLab. By creating a new scope at the beginning of the method, variables within each pass are properly encapsulated, preventing potential conflicts or unintended variable sharing between passes.The updated method signature accurately reflects the addition of
globalContentsto the return type, maintaining consistency with the existing code structure. This change aligns well with the PR objectives and should resolve the reported issue without introducing new problems.Overall, this is a well-implemented solution that improves the robustness of the shader parsing process.
392-392: Verify the impact of the signature change.The updated method signature for
_parsePassnow explicitly includesglobalContentsin the return type. This change accurately reflects the method's behavior and is consistent with how the return value is used in the_parseSubShadermethod.To ensure this change doesn't introduce any unintended side effects, please run the following verification script:
This script will help identify any potential issues related to the signature change and ensure that
globalContentsis properly handled in the calling method.✅ Verification successful
Signature change verified successfully.
The updated
_parsePassmethod signature correctly includesglobalContentsin the return type. All usages withinShaderContentParser.tshandleglobalContentsappropriately, ensuring consistency across the file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of _parsePass method and its return value # Test: Check if there are any other usages of _parsePass that might be affected rg --type typescript '_parsePass\(' packages/shader-lab/src # Test: Verify that globalContents is correctly handled in _parseSubShader rg --type typescript -A 10 '_parsePass\(' packages/shader-lab/src | rg 'globalContents'Length of output: 219
Script:
#!/bin/bash # Description: Verify the usage of _parsePass method and its return value # Test: Check if there are any other usages of _parsePass that might be affected rg --type ts '_parsePass\(' packages/shader-lab/src # Test: Verify that globalContents is correctly handled in _parseSubShader rg --type ts -A 10 '_parsePass\(' packages/shader-lab/src | rg 'globalContents'Length of output: 556
github-actions
bot
added
the
documentation
GuoLei1990
changed the title
ShaderLab parse PassShaderLab parse Pass
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fix scope loss bug when parse
Passscope. A new symbol table should be created forPassscope.Summary by CodeRabbit
New Features
BlendStateconfiguration for enhanced color blending in the "Triangle" shader.Bug Fixes
Style