pbr sheen #312
pbr sheen #312
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (3)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe changes in this pull request introduce a new "Sheen" section to the Changes
Possibly related PRs
Suggested reviewers
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 (1)
packages/shaderlab/src/shaders/PBR.gs (1)
50-56: Consider performance implications of sheen calculationsWhile the implementation is correct, sheen calculations can be computationally expensive, especially when using both color and roughness textures. Consider:
- Adding a note in the documentation about performance impact
- Implementing shader LOD (Level of Detail) variants for performance-critical scenarios
- Providing best practices for texture resolution and format choices
Also applies to: 80-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
packages/shaderlab/src/shaders/shadingPBR/BRDF.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/ReflectionLobe.glslis excluded by!**/*.glsl
📒 Files selected for processing (1)
packages/shaderlab/src/shaders/PBR.gs(2 hunks)
🔇 Additional comments (2)
packages/shaderlab/src/shaders/PBR.gs (2)
50-56: Well-structured implementation of Sheen properties!
The Sheen properties section follows the established pattern and includes all necessary parameters for a PBR sheen implementation. The property ranges, types, and default values are appropriate and consistent with industry standards.
80-82: Verify macro ordering impact on existing shaders
While the new Sheen macros follow the correct naming convention, they're inserted between feature macros and transparency macros. Please verify that the ordering doesn't affect any existing shader variants or compilation.
packages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl
Outdated
Show resolved
Hide resolved
packages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/shaderlab/src/shaders/PBR.gs (1)
50-56: Add documentation for the Sheen propertiesConsider adding comments to describe:
- The visual effect of sheen on materials
- Typical use cases (e.g., cloth, fabric materials)
- How the roughness and color textures affect the final appearance
Example documentation:
Header("Sheen"){ + // Sheen creates a soft, fabric-like appearance often used for cloth materials + // Intensity: Controls the overall strength of the sheen effect + // Color: Tints the sheen reflection, usually kept subtle for realism + // Roughness: Affects the spread of the sheen reflection UI_SheenIntensity("Intensity", Range(0, 1, 0.01)) = 0; UI_SheenColor("Color", Color ) = (0, 0, 0, 0); material_SheenRoughness("Roughness", Range(0, 1, 0.01)) = 0; material_SheenTexture("ColorTexture", Texture2D); material_SheenRoughnessTexture("RoughnessTexture", Texture2D); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (7)
packages/custom-material/src/advanced-shader/thinfilm/IridescenceIndirectLight.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/BRDF.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/LightIndirectFunctions.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glslis excluded by!**/*.glslpackages/shaderlab/src/shaders/shadingPBR/ReflectionLobe.glslis excluded by!**/*.glsl
📒 Files selected for processing (2)
packages/shaderlab/src/shaders/PBR.gs(1 hunks)packages/shaderlab/src/shaders/shadingPBR/index.ts(2 hunks)
🔇 Additional comments (4)
packages/shaderlab/src/shaders/PBR.gs (2)
51-52: 🛠️ Refactor suggestion
Standardize property naming convention
The "UI_" prefix for sheen properties is inconsistent with other sections that use the "material_" prefix. Consider renaming:
UI_SheenIntensity→material_SheenIntensityUI_SheenColor→material_SheenColor
This aligns with the existing naming pattern seen in other properties like material_BaseColor, material_Metal, etc.
Note: As mentioned in a previous review, sheenIntensity should map to material_SheenColor.a in UIScript.
50-56: Verify shader implementation for sheen properties
The property declarations look correct, but we should verify their integration with the shader implementation.
✅ Verification successful
Sheen properties are properly implemented in the shader
The shader implementation in FragmentPBR.glsl correctly handles all the declared sheen properties:
- Proper declaration of sheen color and roughness uniforms
- Texture sampling for both sheen color and roughness textures
- Correct gamma correction for sheen color texture
- Integration with the surface data structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for sheen-related implementations in shader files
echo "Searching for sheen property usage in shader code..."
rg -l "material_(Sheen|UI_Sheen)" --glob "*.glsl"
# Check if ForwardPassPBR.glsl includes sheen calculations
echo "Checking ForwardPassPBR.glsl for sheen implementation..."
rg "sheen" "ForwardPassPBR.glsl"
Length of output: 439
Script:
#!/bin/bash
# Let's check the content of FragmentPBR.glsl for sheen implementation
echo "Checking sheen implementation in FragmentPBR.glsl..."
rg -A 10 "material_Sheen|UI_Sheen" "packages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl"
# Let's also check for any other sheen-related code
echo "Checking for any other sheen-related implementations..."
rg -i "sheen" "packages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl"
Length of output: 2545
packages/shaderlab/src/shaders/shadingPBR/index.ts (2)
3-3: Verify the migration from LightProbe to LightIndirectFunctions
The replacement of LightProbe with LightIndirectFunctions suggests a significant refactoring of the lighting calculation system.
✅ Verification successful
Let me check the actual content of these files to better understand the context of the LightProbe references.
No migration issues found - LightProbe references are function names
The occurrences of "LightProbe" in the codebase are actually function names (getLightProbeIrradiance and getLightProbeRadiance) that are still in use and are now properly defined in the new LightIndirectFunctions.glsl. This is not a breaking change as the functionality is preserved, just reorganized into a more appropriate location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify complete migration from LightProbe to LightIndirectFunctions
# Check for any remaining references to LightProbe
echo "Checking for remaining LightProbe references:"
rg "LightProbe" -l
# Check for new LightIndirectFunctions usage
echo "Checking LightIndirectFunctions implementation:"
ast-grep --pattern 'import $_ from "./LightIndirectFunctions.glsl"'
Length of output: 683
Script:
#!/bin/bash
# Check content of files with LightProbe references
echo "Content of LightIndirectPBR.glsl:"
cat packages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl
echo -e "\nContent of LightIndirectFunctions.glsl:"
cat packages/shaderlab/src/shaders/shadingPBR/LightIndirectFunctions.glsl
echo -e "\nContent of IridescenceIndirectLight.glsl:"
cat packages/custom-material/src/advanced-shader/thinfilm/IridescenceIndirectLight.glsl
echo -e "\nRelevant LightProbe references in fragment.ts:"
rg "LightProbe" -C 2 packages/custom-material/src/bake-pbr/fragment.ts
Length of output: 9555
21-21: Breaking Change: LightProbe replacement
The export entry follows the consistent pattern of other shader includes, but removing LightProbe is a breaking change that may affect existing implementations.
packages/shaderlab/src/shaders/shadingPBR/LightIndirectFunctions.glsl
Outdated
Show resolved
Hide resolved
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
LightProbefunctionality, replacing it withLightIndirectFunctionsfor improved shader performance.