Add iri/sheen/rafraction in glsl version #2518
Conversation
WalkthroughThis pull request introduces two new GLSL imports, 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 (
|
![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
🧹 Nitpick comments (1)
packages/core/src/shaderlib/pbr/index.ts (1)
22-25: Consider grouping related shader components.The export object could benefit from logical grouping of related shader components. Consider organizing the exports by functionality (e.g., grouping transmission-related components like
btdfandrefractiontogether).export default { pbr_frag_define, pbr_helper, brdf, direct_irradiance_frag_define, ibl_frag_define, pbr_frag, - - btdf, - refraction + // Transmission components + btdf, + refraction };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
packages/core/src/shaderlib/common.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/pbr.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/brdf.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/btdf.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/direct_irradiance_frag_define.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/ibl_frag_define.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/pbr_frag.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/pbr_frag_define.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/pbr_helper.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/refraction.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glslis excluded by!**/*.glsl
📒 Files selected for processing (1)
packages/core/src/shaderlib/pbr/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (2)
packages/core/src/shaderlib/pbr/index.ts (2)
Line range hint
1-25: Verify IBL BRDF fix implementation.The PR objectives mention a fix for IBL BRDF in
LightIndirectPBR.glsl, but this file isn't included in the changes. Please ensure this fix has been implemented.Let's verify the IBL BRDF changes:
11-12: Verify missing shader imports for complete feature set.The PR objectives mention adding iri and sheen features, but these imports are missing. Additionally, clarify if the
btdfimport is related to the transmission feature mentioned in the objectives.Let's check for the presence of these shader files:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2518 +/- ##
==========================================
- Coverage 68.58% 68.57% -0.01%
==========================================
Files 957 957
Lines 100052 100068 +16
Branches 8566 8560 -6
==========================================
+ Hits 68617 68626 +9
- Misses 31179 31186 +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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/shader-shaderlab/src/shaders/PBR.gs (1)
47-49: Consider standardizing property naming conventions across all material features.The codebase appears to be transitioning away from the
material_prefix, but this transition is incomplete. Some sections (like Sheen) mix both conventions:
sheenColorvsmaterial_SheenRoughnessiridescencevsmaterial_IridescenceTextureConsider creating a tracking issue to standardize the naming convention across all material properties. This would improve code maintainability and reduce confusion. Would you like me to help create this issue?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/shader-shaderlab/src/shaders/PBR.gs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/shader-shaderlab/src/shaders/PBR.gs (1)
47-49: Verify the complete iridescence implementation.The PR objectives mention adding "iri" from engine toolkit PR #310, but these changes only show property renames.
Let's verify the complete implementation:
✅ Verification successful
Implementation verified - iridescence is fully implemented
The changes in PBR.gs are part of a complete iridescence implementation that includes:
- Full thin-film interference BRDF implementation in shader files
- Material property system integration
- GLTF loader support via KHR_materials_iridescence
- Proper texture handling for both iridescence and thickness maps
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for iridescence-related shader implementations rg -t glsl "iridescence" --glob '!*.gs' # Search for any new shader includes that might contain the implementation ast-grep --pattern 'include "$_"'Length of output: 104
Script:
#!/bin/bash # Find all shader-related files echo "=== Shader Files ===" fd -e shader -e cginc -e hlsl -e compute # Search for iridescence in these files echo -e "\n=== Iridescence Implementation ===" rg -A 5 "iridescence" --ignore-case # Look for includes in shader files echo -e "\n=== Shader Includes ===" rg "^[[:space:]]*#include" --type-add 'shader:*.{shader,cginc,hlsl,compute}' -t shaderLength of output: 46771
| iridescence("Iridescence", Range(0, 1, 0.01)) = 0; | ||
| iridescenceIOR("IOR", Range(1, 5, 0.01)) = 1.3; | ||
| iridescenceRange("ThicknessRange", Vector2) = (100, 400); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent property naming convention within the iridescence group.
While the properties have been renamed to remove the material_ prefix, related texture properties still use the old prefix. This creates an inconsistent naming pattern within the same feature group.
Consider updating the texture properties as well for consistency:
iridescence("Iridescence", Range(0, 1, 0.01)) = 0;
iridescenceIOR("IOR", Range(1, 5, 0.01)) = 1.3;
iridescenceRange("ThicknessRange", Vector2) = (100, 400);
- material_IridescenceThicknessTexture("ThicknessTexture", Texture2D);
- material_IridescenceTexture("IridescenceTexture", Texture2D);
+ iridescenceThicknessTexture("ThicknessTexture", Texture2D);
+ iridescenceTexture("IridescenceTexture", Texture2D);Committable suggestion skipped: line range outside the PR's diff.
LightIndirectPBR.glslSummary by CodeRabbit