Enhancement ShaderLab PBR #2502
Conversation
WalkthroughThe pull request introduces significant structural and naming modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (5)
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2502 +/- ##
========================================
Coverage 68.50% 68.50%
========================================
Files 956 957 +1
Lines 100007 100057 +50
Branches 8545 8543 -2
========================================
+ Hits 68506 68549 +43
- Misses 31245 31252 +7
Partials 256 256
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/shader-shaderlab/src/index.ts(2 hunks)packages/shader-shaderlab/src/shaders/PBR.gs(3 hunks)
🔇 Additional comments (7)
packages/shader-shaderlab/src/index.ts (1)
1-3: Verify shader registration after removal of registerShader.The removal of the
registerShaderfunction suggests a change in shader registration responsibility. Please ensure that PBR shader registration is properly handled elsewhere in the codebase.Also applies to: 16-16
packages/shader-shaderlab/src/shaders/PBR.gs (6)
2-3: LGTM! Improved structure with Editor block.The reorganization of properties under the Editor block improves code organization.
67-67: Verify initialization of new refractionMode property.The new
refractionModeenum property needs proper initialization in existing materials. Default value is set to Planar (1).
106-106: Verify initialization of renderQueueType.Ensure
renderQueueTypeis properly set based on the material's transparency settings.
90-104: Verify initialization of render state variables.The shader uses several variables for render state configuration:
depthWriteEnabledblendEnabled,sourceColorBlendFactor,destinationColorBlendFactorrasterStateCullModePlease ensure these variables are properly initialized based on the new material properties.
✅ Verification successful
Render state variables are properly initialized
The render state variables are correctly initialized through the shader data system with appropriate default values and runtime configuration support. The implementation in
BlendState.tsensures proper state management and type safety.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for render state variable initialization rg "depthWriteEnabled|blendEnabled|sourceColorBlendFactor|destinationColorBlendFactor|rasterStateCullMode"Length of output: 7304
55-56: Verify impact of sheen property renames.The renaming of sheen properties (
ui_SheenIntensity→sheenIntensity,ui_SheenColor→sheenColor) improves consistency but may affect existing materials.
73-75: Verify handling of new rendering control properties.New properties added for enhanced rendering control:
isTransparent: Controls transparencyrenderFace: Controls face cullingblendMode: Controls blend modePlease ensure these properties are properly initialized and handled in the shader code.
| anisotropy("Intensity", Range(0, 1, 0.01)) = 0; | ||
| anisotropyRotation("Rotation", Range(0, 360, 1)) = 0; | ||
| material_AnisotropyTexture("Texture", Texture2D); |
There was a problem hiding this comment.
💡 Codebase verification
Property rename breaks shader functionality
The anisotropy property renames in PBR.gs are incomplete and will break the shader system. The old property names material_AnisotropyInfo and material_AnisotropyTexture are still used in:
- Shader uniform declarations and calculations in
FragmentPBR.glsl,pbr_frag_define.glsl, andpbr_helper.glsl - Material implementation in
PBRMaterial.ts
Either keep the original property names or update all references consistently across the codebase.
🔗 Analysis chain
Verify impact of anisotropy property renames.
The renaming of anisotropy properties (material_AnisotropyInfo → anisotropy, material_AnisotropyTexture name change) improves clarity but may affect existing materials.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for uses of old anisotropy property names
rg "material_AnisotropyInfo|material_AnisotropyTexture"
Length of output: 1632
| material_Iridescence("Iridescence", Range(0, 1, 0.01)) = 0; | ||
| material_IridescenceIOR("IOR", Range(1, 5, 0.01)) = 1.3; | ||
| material_IridescenceRange("ThicknessRange", Vector2) = (100,400); | ||
| material_IridescenceThicknessTexture("ThicknessTexture", Texture2D); |
There was a problem hiding this comment.
💡 Codebase verification
Incomplete iridescence property refactoring detected
The iridescence properties have been restructured in PBR.gs but the implementation hasn't been updated accordingly:
- FragmentPBR.glsl still uses the old
material_IridescenceInfovec4 property which no longer exists - PBRMaterial.ts still references the old property via
_iridescenceInfoProp
🔗 Analysis chain
Verify impact of iridescence property restructuring.
The iridescence properties have been significantly restructured:
material_IridescenceInfosplit into separate properties- Added new
material_IridescenceIORandmaterial_IridescenceRange
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for uses of old iridescence property
rg "material_IridescenceInfo"
Length of output: 872
Script:
#!/bin/bash
# Search for new iridescence properties
echo "=== Searching for new properties ==="
rg "material_Iridescence[^I]" -A 2
echo -e "\n=== Searching for PBRMaterial.ts changes ==="
rg "material_Iridescence" packages/core/src/material/PBRMaterial.ts -A 2
Length of output: 3605
GuoLei1990
added
enhancement
sync https://github.com/galacean/editor/pull/3011
Summary by CodeRabbit
New Features
Refactor