shaderlab pbr support refraction #2470
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
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 WalkthroughThis pull request introduces new transmission-related properties to the PBR (Physically Based Rendering) shader and updates the refraction mode handling in the PBRMaterial class. The shader now supports additional transmission characteristics such as attenuation color, attenuation distance, thickness, and thickness texture. Simultaneously, the material class refactors its refraction mode implementation to use numeric string representations instead of descriptive string literals. 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: 1
🧹 Nitpick comments (3)
packages/loader/src/gltf/extensions/KHR_materials_volume.ts (1)
17-24: Color Conversion is Properly Handled
Converting attenuationColor from linear to gamma space ensures more accurate color representation in common workflows.packages/core/src/material/PBRMaterial.ts (1)
34-41: Consistent Naming
"_refractionEnabled" is spelled correctly, but the macro is named "_refactionMacro". For clarity, consider renaming it to "_refractionMacro" to avoid typos or confusion.packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts (1)
Line range hint
191-220: Consider validation and fallback behavior for volume propertiesSince this extension adds complex material properties that affect rendering:
- Consider implementing validation for physical correctness (e.g., non-negative thickness and attenuation values)
- Plan for graceful fallbacks when the extension is not supported
- Document the performance implications of enabling volume-based materials
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/shader-shaderlab/src/shaders/shadingPBR/BRDF.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/BTDF.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightDirectPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/Refraction.glslis excluded by!**/*.glsl
📒 Files selected for processing (5)
packages/core/src/material/PBRMaterial.ts(5 hunks)packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts(2 hunks)packages/loader/src/gltf/extensions/KHR_materials_transmission.ts(1 hunks)packages/loader/src/gltf/extensions/KHR_materials_volume.ts(1 hunks)packages/shader-shaderlab/src/shaders/shadingPBR/index.ts(2 hunks)
🔇 Additional comments (15)
packages/loader/src/gltf/extensions/KHR_materials_transmission.ts (3)
8-9: Extension Registration Looks Good
Properly registering the extension using the AdditiveParse mode appears correct.
10-14: Explicit Defaults Are Helpful
Using default parameters (e.g., transmissionFactor = 0) clarifies expected behavior and prevents undefined states.
15-22: Consider Handling Texture Load Errors
The texture is loaded asynchronously. If there is a load failure or an exception, the material may remain in an unexpected state. Consider adding error-handling logic (catch block or fallback assignment) to ensure graceful degradation of rendering.
packages/loader/src/gltf/extensions/KHR_materials_volume.ts (3)
9-10: Extension Registration Approved
Similarly, the volume extension is registered correctly with AdditiveParse, providing a clear extension flow.
11-16: Well-Structured Default Values
Supplying defaults (e.g., thicknessFactor = 0) guards against schema omissions and ensures stable rendering.
26-32: Async Texture Handling
Similar to the transmission extension, consider possible failure states and confirm that an unloaded or missing texture doesn’t break the material.
packages/core/src/material/PBRMaterial.ts (5)
318-334: Transmission Macro Enablement
Enabling or disabling the "MATERIAL_HAS_TRANSMISSION" macro based on the float value aligns with typical PBR designs. The zero check is simple and efficient.
352-366: Attenuation Color Approach
Preventing direct reference replacement and copying the color is safe. This approach avoids unintentional shared references.
367-381: Imposed Bound Check
Throwing an error for non-positive attenuationDistance is valid. This enforces physically meaningful usage and helps catch erroneous user input.
382-399: Thickness Macro Usage
Checking for a zero thickness before enabling macros is consistent with prior checks, ensuring minimal overhead for disabled features.
447-455: Absorption Macro Trigger
Absorption logic triggers macros if the sum of color components exceeds 1.0. Validate that this threshold matches expected rendering results, as certain color spaces or saturations might require more nuanced checks.
packages/shader-shaderlab/src/shaders/shadingPBR/index.ts (2)
11-12: New Shaders
Imports for Refraction and BTDF expand possible shading models, aligning with the newly introduced refraction features in PBRMaterial.
24-26: Exported Array Updated
Listing Refraction and BTDF as separate shader modules ensures modular usage and clarity. No further changes required.
packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts (2)
220-220: LGTM! Correct addition to GLTFExtensionSchema type.
The IKHRMaterialsVolume interface is properly added to the union type while maintaining alphabetical ordering.
191-199: LGTM! The KHR_materials_volume interface is well-defined.
The interface follows the standard KHR extension pattern and includes all the necessary properties for volume-based materials:
thicknessFactorandthicknessTexturefor controlling material thicknessattenuationDistanceandattenuationColorfor light attenuation through the material
Let's verify this matches the official KHR spec:
✅ Verification successful
Interface implementation is correctly integrated and used in the codebase
The verification shows that:
- The interface is properly implemented and used in
KHR_materials_volume.ts - The extension is correctly registered using
@registerGLTFExtensiondecorator - The implementation properly maps the interface properties to material properties:
thicknessFactor→material.thicknessattenuationDistance→material.attenuationDistance- Default values are correctly set (thickness: 0, attenuationDistance: Infinity)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify interface against KHR_materials_volume spec
# Expected: Should find the spec file and verify property types
# Search for spec file
rg -l "KHR_materials_volume" README.md docs/
# Verify property types in spec
rg -A 10 "KHR_materials_volume" README.md docs/
Length of output: 92
Script:
#!/bin/bash
# Let's try a broader search to find any references to this extension
# and verify its implementation across the codebase
# Search for any mentions of KHR_materials_volume
rg "KHR_materials_volume" -A 5 -B 5
# Look for similar volume-related interfaces or implementations
ast-grep --pattern 'interface $_ {
thicknessFactor?: $_
$$$
}'
# Check for any test files that might validate this implementation
fd -e test.ts -e spec.ts -X rg "KHR_materials_volume"
Length of output: 3989
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2470 +/- ##
===========================================
- Coverage 68.42% 68.40% -0.03%
===========================================
Files 923 923
Lines 95940 95894 -46
Branches 8140 8131 -9
===========================================
- Hits 65647 65592 -55
- Misses 30040 30048 +8
- Partials 253 254 +1
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
🔭 Outside diff range comments (1)
packages/core/src/material/PBRMaterial.ts (1)
The absorption threshold logic needs correction
The current implementation in
PBRMaterial.tsenables absorption when the sum of RGB components is > 1, which is incorrect based on the shader implementation. The shader uses attenuation color values between 0 and 1 to calculate absorption coefficients (via-log(clamp(surfaceData.attenuationColor, 1e-5f, 1.0f))). The absorption should be enabled whenever an attenuation color is set, regardless of its sum, as even values between 0-1 contribute to the absorption calculation.
- The condition
attenuationColor.r + attenuationColor.g + attenuationColor.b > 1should be changed to enable absorption whenever attenuation color is set, as any valid color value affects the absorption coefficient calculation.🔗 Analysis chain
Line range hint
431-455: Verify: Logic for enabling absorption when color sum > 1.
Ensure that this threshold is indeed what you intend proportionally for a color-based absorption effect. If you expected any color values over 1.0 in a single channel to trigger absorption, or if you anticipated a different threshold, consider adjusting the condition.
Let me check the BTDF.glsl implementation to understand how the absorption is being used in the shader.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to "MATERIAL_HAS_ABSORPTION" to confirm how the code sets absorption more precisely. rg "MATERIAL_HAS_ABSORPTION"Length of output: 435
Script:
#!/bin/bash # Check the BTDF.glsl implementation cat packages/shader-shaderlab/src/shaders/shadingPBR/BTDF.glslLength of output: 2594
🧹 Nitpick comments (3)
packages/core/src/material/PBRMaterial.ts (3)
3-3: Nitpick: Check actual usage of these imports.
The new imports from "../shader" (ShaderMacro, ShaderProperty) appear to be fully utilized, but if any become unnecessary, consider removing them to avoid clutter.
34-41: Nitpick: Rename "_refactionMacro" to "_refractionMacro".
The private static field is likely intended to refer to refraction, but there is a small typographical discrepancy in the naming ("_refactionMacro" instead of "_refractionMacro"). Renaming it will make the code more consistent and clear.- private static _refactionMacro: ShaderMacro = ShaderMacro.getByName("MATERIAL_ENABLE_SS_REFRACTION"); + private static _refractionMacro: ShaderMacro = ShaderMacro.getByName("MATERIAL_ENABLE_SS_REFRACTION");
367-381: Suggestion: Consider clamping instead of throwing an error.
While throwing an error for invalid attenuationDistance is valid, you could clamp the input value to a minimum threshold to provide a more robust user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/material/PBRMaterial.ts(5 hunks)
🔇 Additional comments (5)
packages/core/src/material/PBRMaterial.ts (5)
300-316: Looks Good: Refraction enabling logic is correct.
The getter and setter for enableRefraction ensure that the shader macro is properly toggled. This also addresses the macro-disable issue from past feedback, so it now correctly disables the macro when refraction is off.
335-350: Approved: Transmission texture logic.
The texture is properly assigned and toggles the macro. No further issues found.
352-366: Approved: Attenuation color property.
The logic to copy over the user-provided color is consistent with the rest of the material properties.
382-399: Approved: Thickness property.
Clamping of the value to zero and toggling the macro if the thickness is non-zero is a sensible approach.
400-415: Approved: Thickness texture property.
This implementation is consistent with the rest of the texture-based toggles.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glsl
📒 Files selected for processing (2)
packages/core/src/material/PBRMaterial.ts(5 hunks)packages/shader-shaderlab/src/shaders/PBR.gs(1 hunks)
🔇 Additional comments (4)
packages/shader-shaderlab/src/shaders/PBR.gs (1)
58-65: LGTM! Property declarations align with PBR standards
The refraction properties are well-structured and follow PBR material conventions:
- AttenuationColor defaults to white (1,1,1,1)
- Transmission and AttenuationDistance use appropriate ranges (0-1)
- Thickness uses a reasonable range (0-5)
- Texture properties are correctly typed
packages/core/src/material/PBRMaterial.ts (3)
3-3: LGTM! Static properties match shader declarations
The static properties are correctly defined and match the corresponding shader properties.
Also applies to: 34-41
300-316: LGTM! Macro handling is correct
The enableRefraction property correctly toggles the screen-space refraction macro.
335-415: LGTM! Properties are well implemented
The remaining properties are correctly implemented with appropriate:
- Input validation
- Macro handling
- Documentation
|
add e2e test |
|
link issue |
|
Do you have reference diffuse transmission? |
|
@GuoLei1990 should support type between and in Unity: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/material/PBRMaterial.ts (1)
464-468: Consider adding validation for default values in constructor.The constructor sets default values but doesn't validate them. Consider adding validation to ensure the values are within acceptable ranges.
Apply this diff to add validation:
this.refractionMode = RefractionMode.Plane; - shaderData.setFloat(PBRMaterial._thicknessProp, 0); - shaderData.setFloat(PBRMaterial._attenuationDistanceProp, Infinity); + shaderData.setFloat(PBRMaterial._thicknessProp, Math.max(0, 0)); + shaderData.setFloat(PBRMaterial._attenuationDistanceProp, Math.max(0, Infinity)); const attenuationColor = new Color(1, 1, 1); shaderData.setColor(PBRMaterial._attenuationColorProp, attenuationColor);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/shader-shaderlab/src/shaders/shadingPBR/BTDF.glslis excluded by!**/*.glsl
📒 Files selected for processing (2)
packages/core/src/material/PBRMaterial.ts(5 hunks)packages/core/src/material/enums/Refraction.ts(1 hunks)
🔇 Additional comments (4)
packages/core/src/material/enums/Refraction.ts (1)
1-11: LGTM! Well-structured enum definition.
The RefractionMode enum is well-documented and follows TypeScript best practices with clear, descriptive names.
packages/core/src/material/PBRMaterial.ts (3)
36-51: LGTM! Well-organized static properties.
The static properties are well-organized and follow a consistent naming pattern. The separation between public and private properties is clear.
484-491:
Fix incorrect absorption logic.
The absorption macro is enabled when color sum > 1, which is incorrect. It should be enabled when the attenuation color is different from white (1,1,1).
Apply this diff to fix the logic:
attenuationColor._onValueChanged = () => {
- const enableAbsorption = attenuationColor.r + attenuationColor.g + attenuationColor.b > 1;
+ const enableAbsorption = attenuationColor.r !== 1 || attenuationColor.g !== 1 || attenuationColor.b !== 1;
if (enableAbsorption) {
this.shaderData.enableMacro(PBRMaterial._absorptionMacro);
} else {
this.shaderData.disableMacro(PBRMaterial._absorptionMacro);
}
};Likely invalid or redundant comment.
318-332:
Fix incorrect macro logic in refractionMode setter.
The condition if (value) is incorrect as value is an enum and not a boolean. This could lead to unexpected behavior.
Apply this diff to fix the logic:
set refractionMode(value: RefractionMode) {
if (value !== this._refractionMode) {
- if (value) {
+ if (value !== undefined) {
this.shaderData.enableMacro(PBRMaterial._refractionMacro);
this.renderState.renderQueueType = RenderQueueType.Transparent;
this.renderState.blendState.targetBlendState.enabled = false;
} else {
this.shaderData.disableMacro(PBRMaterial._refractionMacro);
this.renderState.renderQueueType = RenderQueueType.Opaque;
this.renderState.blendState.targetBlendState.enabled = true;
}
}
this._refractionMode = value;
this.setRefractionMode(value);
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/core/src/material/PBRMaterial.ts (3)
360-368:⚠️ Potential issueFix inverted macro logic in transmission setter.
The macro enabling/disabling logic is inverted. The current implementation disables the macro when the value changes and enables it when it doesn't change.
Apply this diff to fix the logic:
set transmission(value: number) { value = Math.max(0, Math.min(1, value)); if (!!this.shaderData.getFloat(PBRMaterial._transmissionProp) !== !!value) { - this.shaderData.disableMacro(PBRMaterial._transmissionMacro); - } else { - this.shaderData.enableMacro(PBRMaterial._transmissionMacro); + if (value) { + this.shaderData.enableMacro(PBRMaterial._transmissionMacro); + } else { + this.shaderData.disableMacro(PBRMaterial._transmissionMacro); + } } this.shaderData.setFloat(PBRMaterial._transmissionProp, value); }
488-496:⚠️ Potential issueFix absorption macro logic.
The absorption macro is enabled when color sum > 1, which is incorrect. It should be enabled when the attenuation color is different from white (1,1,1).
Apply this diff to fix the logic:
attenuationColor._onValueChanged = () => { - const enableAbsorption = attenuationColor.r + attenuationColor.g + attenuationColor.b > 1; + const enableAbsorption = attenuationColor.r !== 1 || attenuationColor.g !== 1 || attenuationColor.b !== 1; if (enableAbsorption) { this.shaderData.enableMacro(PBRMaterial._absorptionMacro); } else { this.shaderData.disableMacro(PBRMaterial._absorptionMacro); } };
423-431:⚠️ Potential issueFix inverted macro logic in thickness setter.
Similar to the transmission setter, the macro enabling/disabling logic is inverted.
Apply this diff to fix the logic:
set thickness(value: number) { value = Math.max(0, value); if (!!this.shaderData.getFloat(PBRMaterial._thicknessProp) !== !!value) { - this.shaderData.disableMacro(PBRMaterial._thicknessMacro); - } else { - this.shaderData.enableMacro(PBRMaterial._thicknessMacro); + if (value) { + this.shaderData.enableMacro(PBRMaterial._thicknessMacro); + } else { + this.shaderData.disableMacro(PBRMaterial._thicknessMacro); + } } this.shaderData.setFloat(PBRMaterial._thicknessProp, value); }
🧹 Nitpick comments (1)
packages/core/src/material/PBRMaterial.ts (1)
36-51: Reorder protected and private decorators.Follow the standard decorator order:
static→public→internal→private. Move the protected_refractionModeafter all static declarations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/material/PBRMaterial.ts(5 hunks)
🔇 Additional comments (2)
packages/core/src/material/PBRMaterial.ts (2)
310-350: LGTM! Well-structured refraction mode implementation.
The implementation correctly:
- Manages shader macros for different refraction modes
- Updates render queue type and blend states
- Separates concerns between the setter and helper method
464-469: LGTM! Proper initialization of new properties.
The constructor correctly:
- Sets default values for new properties
- Initializes color objects
- Sets up the refraction mode
| @@ -7,16 +7,30 @@ | |||
| sampler2D camera_OpaqueTexture; | |||
| vec3 evaluateRefraction(SurfaceData surfaceData, BRDFData brdfData) { | |||
| RefractionModelResult ray; | |||
| #ifdef REFRACTION_MODE | |||
| refractionModelBox(-surfaceData.viewDir, surfaceData.position, surfaceData.normal, surfaceData.IOR, 0.005, ray); | |||
| // '0' is refraction sphere, '1' is refraction plane, '2' is refraction thin | |||
There was a problem hiding this comment.
Function named evaluateTransmission is better
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
packages/core/src/material/PBRMaterial.ts (3)
Line range hint
332-337: Preserve render queue type when transmission is disabled.The current implementation doesn't restore the original render queue type when transmission is disabled. This could lead to incorrect rendering states.
override set isTransparent(value: boolean) { this._seIsTransparent(value); if (this.transmission > 0) { // If transmission enabled, always use transparent queue to ensure get correct opaque texture this.renderState.renderQueueType = RenderQueueType.Transparent; + this._lastRenderQueueType = this.renderState.renderQueueType; + } else if (this._lastRenderQueueType) { + this.renderState.renderQueueType = this._lastRenderQueueType; + this._lastRenderQueueType = null; } }
Line range hint
347-352: Duplicate render queue type logic.The render queue type setting logic is duplicated in
alphaCutoffsetter. Consider extracting this into a private method.+ private _updateRenderQueueForTransmission(): void { + if (this.transmission > 0) { + this._lastRenderQueueType = this.renderState.renderQueueType; + this.renderState.renderQueueType = RenderQueueType.Transparent; + } else if (this._lastRenderQueueType) { + this.renderState.renderQueueType = this._lastRenderQueueType; + this._lastRenderQueueType = null; + } + } override set alphaCutoff(value: number) { this._setAlphaCutoff(value); - if (this.transmission > 0) { - this.renderState.renderQueueType = RenderQueueType.Transparent; - } + this._updateRenderQueueForTransmission(); }
Line range hint
472-486: Fix absorption macro logic.The absorption macro is enabled when color components sum is greater than 0, which is incorrect. It should be enabled when the attenuation color is different from white (1,1,1).
private _attenuationColorChanged(): void { const attenuationColor = this.attenuationColor; - const enableAbsorption = attenuationColor.r + attenuationColor.g + attenuationColor.b > 0; + const enableAbsorption = + attenuationColor.r !== 1 || + attenuationColor.g !== 1 || + attenuationColor.b !== 1; if (enableAbsorption !== this._absorptionEnabled) { this._absorptionEnabled = enableAbsorption; if (enableAbsorption) { this.shaderData.enableMacro(PBRMaterial._absorptionMacro); } else { this.shaderData.disableMacro(PBRMaterial._absorptionMacro); } } }
🧹 Nitpick comments (3)
packages/core/src/material/PBRMaterial.ts (3)
320-320: Consider using variable macro values for refraction modes.Instead of using numeric string values directly, consider using descriptive macro values for better code readability and maintainability.
- this.shaderData.enableMacro("REFRACTION_MODE", value.toString()); + this.shaderData.enableMacro("REFRACTION_MODE", RefractionMode[value].toUpperCase());
Line range hint
417-434: Optimize transmission setter logic.The current implementation checks the previous value unnecessarily and has complex macro handling logic.
set transmission(value: number) { value = MathUtil.clamp(value, 0, 1); + this.shaderData.setFloat(PBRMaterial._transmissionProp, value); - if (!!this.shaderData.getFloat(PBRMaterial._transmissionProp) !== !!value) { - if (value > 0) { - this.shaderData.enableMacro(PBRMaterial._transmissionMacro); - this.renderState.renderQueueType = RenderQueueType.Transparent; - } else { - this.shaderData.disableMacro(PBRMaterial._transmissionMacro); - } + if (value > 0) { + this.shaderData.enableMacro(PBRMaterial._transmissionMacro); + this._updateRenderQueueForTransmission(); + } else { + this.shaderData.disableMacro(PBRMaterial._transmissionMacro); + this._updateRenderQueueForTransmission(); } - this.shaderData.setFloat(PBRMaterial._transmissionProp, value); }
Line range hint
436-449: Cache transmission texture macro name.For consistency with other properties and better performance, consider caching the macro name as a static property.
+ private static _transmissionTextureMacroName = "MATERIAL_HAS_TRANSMISSION_TEXTURE"; set transmissionTexture(value: Texture2D) { this.shaderData.setTexture(PBRMaterial._transmissionTextureProp, value); if (value) { - this.shaderData.enableMacro(PBRMaterial._transmissionTextureMacro); + this.shaderData.enableMacro(PBRMaterial._transmissionTextureMacroName); } else { - this.shaderData.disableMacro(PBRMaterial._transmissionTextureMacro); + this.shaderData.disableMacro(PBRMaterial._transmissionTextureMacroName); } }
packages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl
Outdated
Show resolved
Hide resolved
packages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl
Outdated
Show resolved
Hide resolved
packages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl
Outdated
Show resolved
Hide resolved
| #ifdef REFRACTION_MODE | ||
| refractionModelBox(-surfaceData.viewDir, surfaceData.position, surfaceData.normal, surfaceData.IOR, 0.005, ray); | ||
| // '0' is refraction sphere, '1' is refraction plane, '2' is refraction thin | ||
| #if REFRACTION_MODE == 0 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/material/PBRMaterial.ts (2)
469-474: Optimize property initialization order.The initialization order could be optimized to minimize shader recompilation:
- Initialize all transmission-related properties before setting refractionMode
- Missing initialization of transmissionTexture property
Apply this diff to optimize the initialization:
const sheenColor = new Color(0, 0, 0); shaderData.setColor(PBRMaterial._sheenColorProp, sheenColor); + // Initialize transmission properties first + shaderData.setFloat(PBRMaterial._transmissionProp, 0); + shaderData.setFloat(PBRMaterial._thicknessProp, 0); + shaderData.setFloat(PBRMaterial._attenuationDistanceProp, Infinity); + const attenuationColor = new Color(1, 1, 1); + shaderData.setColor(PBRMaterial._attenuationColorProp, attenuationColor); + // Set mode after all properties are initialized this.refractionMode = RefractionMode.Planar; - shaderData.setFloat(PBRMaterial._transmissionProp, 0); - shaderData.setFloat(PBRMaterial._thicknessProp, 0); - shaderData.setFloat(PBRMaterial._attenuationDistanceProp, Infinity); - const attenuationColor = new Color(1, 1, 1); - shaderData.setColor(PBRMaterial._attenuationColorProp, attenuationColor);
Line range hint
363-371: Document render queue type changes in transmission properties.The transmission and thickness setters affect the render queue type, but this behavior is not documented in the property JSDoc comments. This could lead to unexpected behavior for users.
Add documentation about render queue type changes:
/** * Transmission factor. * @defaultValue `0.0` + * @remarks When transmission is enabled (value > 0), the material's render queue type + * is automatically set to Transparent to ensure correct rendering order. */ get transmission(): number {Also applies to: 441-449
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/shader-shaderlab/src/shaders/shadingPBR/BTDF.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glsl
📒 Files selected for processing (1)
packages/core/src/material/PBRMaterial.ts(2 hunks)
🔇 Additional comments (1)
packages/core/src/material/PBRMaterial.ts (1)
318-318: Verify refraction implementation across shaders.
Let's verify that the shader implementation correctly handles all refraction modes.
✅ Verification successful
Refraction implementation is properly handled in shaders
Based on the verification results, the refraction implementation is correctly handled across the shader system:
-
The BTDF.glsl shader properly implements both refraction modes:
- Sphere mode (REFRACTION_MODE == 0)
- Planar mode (REFRACTION_MODE == 1)
-
The implementation includes:
- Complete refraction model implementations in Refraction.glsl
- Proper transmission calculations and blending in ForwardPassPBR.glsl
- Correct integration with the PBR material system
The enableMacro call in PBRMaterial.ts correctly sets up the shader compilation for the selected refraction mode.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for refraction mode handling in shaders
echo "Checking shader implementations for refraction modes..."
rg -t glsl "REFRACTION_MODE"
# Verify no hardcoded mode strings remain
echo "Checking for old hardcoded refraction modes..."
rg -i "sphere|planar|thin" --type glsl
Length of output: 351
Script:
#!/bin/bash
# Search for shader files with common extensions
echo "Looking for shader files..."
fd -e glsl -e frag -e vert
# Then search for refraction mode handling
echo "Checking shader implementations for refraction modes..."
rg "REFRACTION_MODE" $(fd -e glsl -e frag -e vert)
# Check for refraction-related terms
echo "Checking for refraction-related terms..."
rg -i "refraction|sphere|planar|thin" $(fd -e glsl -e frag -e vert)
# Also check TypeScript files for refraction mode definitions
echo "Checking TypeScript files for refraction mode definitions..."
rg "REFRACTION_MODE" -t ts
Length of output: 29899
| @@ -317,7 +315,7 @@ export class PBRMaterial extends PBRBaseMaterial { | |||
| set refractionMode(value: RefractionMode) { | |||
| if (value !== this._refractionMode) { | |||
| this._refractionMode = value; | |||
| this._setRefractionMode(value); | |||
| this.shaderData.enableMacro("REFRACTION_MODE", value.toString()); | |||
There was a problem hiding this comment.
Add macro cleanup and value validation.
The current implementation has potential issues:
- No cleanup of previous macro value before setting new one
- No handling of RefractionMode.None
Apply this diff to fix these issues:
- this.shaderData.enableMacro("REFRACTION_MODE", value.toString());
+ if (value === RefractionMode.None) {
+ this.shaderData.disableMacro("REFRACTION_MODE");
+ } else {
+ this.shaderData.enableMacro("REFRACTION_MODE", value.toString());
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.shaderData.enableMacro("REFRACTION_MODE", value.toString()); | |
| if (value === RefractionMode.None) { | |
| this.shaderData.disableMacro("REFRACTION_MODE"); | |
| } else { | |
| this.shaderData.enableMacro("REFRACTION_MODE", value.toString()); | |
| } |
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
New Features
Bug Fixes
Enhancements
RefractionModeenum to include explicit numeric values, improving clarity and consistency.