pbr support transmission #2478
pbr support transmission #2478
Conversation
WalkthroughThis pull request introduces extensive enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (13)
packages/loader/src/gltf/extensions/KHR_materials_transmission.ts (2)
7-12: Ensure consistency with user-defined refraction behavior
The code setsmaterial.refractionModetoRefractionMode.Planeunconditionally. If there's a need to allow other refraction shapes (e.g., sphere, thin), consider making this configurable or derived from the GLTF schema to avoid hardcoding a single refraction mode.
14-20: Handle potential texture loading errors
The asynchronous call to retrievetransmissionTexturelacks error handling. If the texture fails to load, the material might remain in an undefined state. Consider adding a.catch()block or a fallback mechanism to handle texture load failures gracefully.packages/loader/src/gltf/extensions/KHR_materials_volume.ts (2)
9-15: Use schema-defined refraction if applicable
LikeKHR_materials_transmission, this code setsmaterial.refractionModetoRefractionMode.Planeby default. If the volume schema supports different refraction modes, consider making this assignment more flexible to accommodate varying use cases.
26-32: Consolidate texture error handling
As withKHR_materials_transmission, carefully handle potential texture load failures. Consider adding.catch()or a fallback strategy if the texture is invalid or the call fails.packages/core/src/material/PBRMaterial.ts (8)
310-317: Documentation for refraction behavior
The doc comment is accurate but concise. Consider expanding it to clarify how different refraction modes affect the material’s transparency and final rendered appearance.
330-336: Transparent override
IfisTransparentis set totrue, it overrides the internal queue logic, or vice versa. Ensure that users understand thatrefractionModealso forces transparency. Consider clarifying in documentation to avoid confusion.
366-381: Transmission texture consistency
Enabling the macro whenevertransmissionTextureis set is correct. Ensure the actual channel usage (red channel fortransmission) is documented, so artists and pipeline developers can supply textures appropriately.
383-398: Attenuation color usage
Ensure that developers understand existing gamma or linear workflows. Consider clarifying in docs whether attenuation colors are specified in linear or gamma space.
400-409: Attenuation distance edge cases
Although you clamp to a minimum of 0,Infinityis an allowed default. Ensure that logic in shading handles infinite distances gracefully and doesn’t accidentally produce NaN.
431-446: Separate thickness texture
Using the green channel for thickness is workable, but might cause confusion if teams have standard channel usage. Document it clearly or ensure ext docs specify it.
462-468: Initialize absorption macro
Line 467 always enables_absorptionMacro. If a material starts with[1,1,1]attenuation color, it’s effectively no absorption. You toggle it off later if the color changes to zero sum, but consider the overhead of enabling macros by default.
487-498: Absorption toggling
The logic to compare the sum of[r, g, b]to 0 is simple and effective, though borderline user cases (like[0.0001, 0, 0]) might be flagged differently than if it were truly zero. If sensitivity becomes an issue, consider a small epsilon check.packages/shader-shaderlab/src/shaders/PBR.gs (1)
58-61: Consider making these property names more explicit.Your new properties,
material_Transmissionandmaterial_TransmissionTexture, suitably extend the shader for handling refraction. However, consider refactoring names to make their usage even clearer, for example:
- TransmissionFactor to emphasize that the property is a blend factor.
- TransmissionTexture to clarify that it is a texture controlling how light transmits.
If you'd like a sample rename to clarify these properties’ roles, let me know!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/shader-shaderlab/src/shaders/Common.glslis excluded by!**/*.glslpackages/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 (8)
packages/core/src/material/PBRMaterial.ts(6 hunks)packages/core/src/material/enums/Refraction.ts(1 hunks)packages/core/src/material/index.ts(1 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/PBR.gs(1 hunks)packages/shader-shaderlab/src/shaders/shadingPBR/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/material/enums/Refraction.ts
🔇 Additional comments (14)
packages/loader/src/gltf/extensions/KHR_materials_volume.ts (1)
17-24: Convert attenuation color carefully
The code correctly converts linear-to-gamma space for the attenuation color. Verify that any user-facing values or GLTF schema usage clarifies that input colors are linear, to avoid confusion about color space.
packages/core/src/material/PBRMaterial.ts (9)
32-37: Macros naming consistency
The newly added macros (_refractionMacro, _transmissionMacro, _thicknessMacro, _absorptionMacro, etc.) follow a clear pattern. Keep future macros consistent in naming and usage to avoid confusion and improve maintainability.
38-44: Shader property references
These new shader properties (_transmissionProp, _transmissionTextureProp, etc.) are well-organized. Verify that these names are synced with the shader code to avoid mismatches or typos.
45-51: Preserve backward compatibility
The new _refractionMode field changes how rendering queues are managed. Confirm that existing code without setting refractionMode will continue to behave as expected (e.g., defaulting to RefractionMode.None).
318-328: Dynamic render queue adjustment
When refractionMode changes from or to None, the renderQueueType is toggled between transparent and the last known queue. This logic is intuitive, but watch for scenarios where the user might require a specific queue while still using partial refraction.
338-344: Alpha cutoff synergy
The alpha cutoff logic also reverts the render queue to transparent if refraction is enabled. This is correct for many workflows, but confirm with design if alpha-cut transparency plus refraction is a valid combination for all cases.
346-364: Transmission factor logic
The macro is enabled or disabled based on a zero vs. non-zero check. This is an efficient approach, but be sure any intermediate fractional values near zero (like 0.0001) behave as intended.
411-429: Thickness macro toggling
Like transmission, thickness toggles its macro by checking zero vs non-zero. Confirm partial thickness values effectively trigger the correct macros.
470-474: Range recalculations
Iridescence thickness range triggers a recalculation in _onValueChanged. This is a good pattern. Confirm the performance is acceptable for frequent updates, especially if driven by animations or user interactions.
[approve]
510-528: Refraction macro management
Disabling _refractionMacro in the None case is correct, but the “REFRACTION_MODE” macro might remain set. Confirm that enabling _refractionMacro with a different define overwrites the old define, or explicitly clear it to avoid stale states.
packages/core/src/material/index.ts (1)
11-11: Exporting the RefractionMode enum
Exporting RefractionMode here is a clean approach for increasing accessibility. Confirm that this new export does not introduce naming collisions or overshadow existing symbols.
packages/shader-shaderlab/src/shaders/shadingPBR/index.ts (2)
11-12: New shader imports
Adding Refraction and BTDF is logical given the new material capabilities. Ensure the included GLSL files compile correctly under all rendering backends.
24-26: Extended shader pipeline
Appending slices for Refraction and BTDF in the exported array improves readability and keeps them in a clear layering near ReflectionLobe. Looks good.
packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts (1)
Line range hint 191-220: Ensure consistent usage of optional fields in calling code.
The newly introduced IKHRMaterialsVolume interface includes multiple optional fields (e.g., thicknessFactor, attenuationDistance). This design is flexible, but ensure that all calling code checks for undefined when handling these fields to avoid runtime errors or unexpected behaviors.
Search for usage of IKHRMaterialsVolume to ensure that calling code properly handles null/undefined fields:
✅ Verification successful
Let me gather more information about how the texture is handled.
Let me try a different pattern to get the implementation.
Let me check one more thing about the texture handling.
Optional fields are properly handled in the implementation
After reviewing the implementation in KHR_materials_volume.ts, I can confirm that all optional fields from IKHRMaterialsVolume are handled safely:
- Default values are provided for optional numeric fields:
thicknessFactor = 0andattenuationDistance = Infinity attenuationColoris checked with an if condition before use:if (attenuationColor) { ... }thicknessTextureis checked with an if condition before use:if (thicknessTexture) { ... }
The implementation in PBRMaterial class also handles the texture property safely with proper null checks in the setter:
set thicknessTexture(value: Texture2D) {
this.shaderData.setTexture(PBRMaterial._thicknessTextureProp, value);
if (value) {
this.shaderData.enableMacro(PBRMaterial._thicknessTextureMacro);
} else {
this.shaderData.disableMacro(PBRMaterial._thicknessTextureMacro);
}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Use ast-grep or rg to look for interface usage and references to these properties
rg -A 5 'IKHRMaterialsVolume'
Length of output: 1885
Script:
#!/bin/bash
# Check the full implementation of KHR_materials_volume to see texture handling
ast-grep --pattern 'class KHR_materials_volume {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Check the full implementation of KHR_materials_volume
rg -A 15 "class KHR_materials_volume"
Length of output: 1696
Script:
#!/bin/bash
# Check for texture handling in the rest of the implementation
rg -A 10 "thicknessTexture"
Length of output: 6326
GuoLei1990
added
enhancement
| // @ts-ignore | ||
| this._iridescenceRange._onValueChanged = this._onIridescenceRangeChanged.bind(this); | ||
| this._iridescenceRange._onValueChanged = () => { |
There was a problem hiding this comment.
Use bind this is more readability
packages/loader/src/gltf/extensions/KHR_materials_transmission.ts
Outdated
Show resolved
Hide resolved
packages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/core/src/material/PBRMaterial.ts (1)
Line range hint
470-498: Consider using a type-safe approach instead of @ts-ignore.The current implementation uses @ts-ignore to bypass TypeScript checks. Consider creating proper interfaces or types for the value change handlers.
+interface ValueChangeNotifier { + _onValueChanged?: () => void; +} +class ObservableVector2 extends Vector2 implements ValueChangeNotifier { + _onValueChanged?: () => void; +} +class ObservableColor extends Color implements ValueChangeNotifier { + _onValueChanged?: () => void; +} constructor(engine: Engine) { // ... - // @ts-ignore - this._iridescenceRange._onValueChanged = () => { + (this._iridescenceRange as ValueChangeNotifier)._onValueChanged = () => {
🧹 Nitpick comments (5)
packages/core/src/material/PBRMaterial.ts (5)
318-328: Add input validation for refractionMode.Consider adding null/undefined check and type validation to ensure the refractionMode value is valid.
set refractionMode(value: RefractionMode) { + if (value === undefined || value === null) { + value = RefractionMode.None; + } + if (!Object.values(RefractionMode).includes(value)) { + throw new Error(`Invalid RefractionMode value: ${value}`); + } if (value !== this._refractionMode) { if (value) {
330-344: Consider extracting common queue type logic.Both override methods share similar logic for handling render queue type. Consider extracting this into a private method to improve maintainability.
+ private _updateRenderQueueType(): void { + this._lastRenderQueueType = this.renderState.renderQueueType; + if (this.refractionMode !== RefractionMode.None) { + this.renderState.renderQueueType = RenderQueueType.Transparent; + } + } override set isTransparent(value: boolean) { super.isTransparent = value; - this._lastRenderQueueType = this.renderState.renderQueueType; - if (this.refractionMode !== RefractionMode.None) { - this.renderState.renderQueueType = RenderQueueType.Transparent; - } + this._updateRenderQueueType(); } override set alphaCutoff(value: number) { super.alphaCutoff = value; - this._lastRenderQueueType = this.renderState.renderQueueType; - if (this.refractionMode !== RefractionMode.None) { - this.renderState.renderQueueType = RenderQueueType.Transparent; - } + this._updateRenderQueueType(); }
383-409: Consider using a constant for the default attenuation color.For better maintainability and to avoid magic values, consider defining a constant for the default white color (1,1,1).
+ private static readonly DEFAULT_ATTENUATION_COLOR = new Color(1, 1, 1); constructor(engine: Engine) { // ... - const attenuationColor = new Color(1, 1, 1); + const attenuationColor = PBRMaterial.DEFAULT_ATTENUATION_COLOR.clone(); // ... }
462-468: Consider using constants for default values.For better maintainability and to avoid magic values, consider defining constants for default transmission, thickness, and attenuation distance values.
+ private static readonly DEFAULT_TRANSMISSION = 0; + private static readonly DEFAULT_THICKNESS = 0; + private static readonly DEFAULT_ATTENUATION_DISTANCE = Infinity; constructor(engine: Engine) { // ... - shaderData.setFloat(PBRMaterial._transmissionProp, 0); - shaderData.setFloat(PBRMaterial._thicknessProp, 0); - shaderData.setFloat(PBRMaterial._attenuationDistanceProp, Infinity); + shaderData.setFloat(PBRMaterial._transmissionProp, PBRMaterial.DEFAULT_TRANSMISSION); + shaderData.setFloat(PBRMaterial._thicknessProp, PBRMaterial.DEFAULT_THICKNESS); + shaderData.setFloat(PBRMaterial._attenuationDistanceProp, PBRMaterial.DEFAULT_ATTENUATION_DISTANCE);
510-528: Consider using a map for refraction mode configurations.The switch statement could be replaced with a map to make it more maintainable and easier to extend.
+ private static readonly REFRACTION_MODE_CONFIG = { + [RefractionMode.None]: { + enableRefraction: false, + mode: null + }, + [RefractionMode.Sphere]: { + enableRefraction: true, + mode: "SPHERE" + }, + [RefractionMode.Plane]: { + enableRefraction: true, + mode: "PLANE" + }, + [RefractionMode.Thin]: { + enableRefraction: true, + mode: "THIN" + } + }; private _setRefractionMode(refractionMode: RefractionMode): void { - switch (refractionMode) { - case RefractionMode.None: - this.shaderData.disableMacro(PBRMaterial._refractionMacro); - break; - case RefractionMode.Sphere: - this.shaderData.enableMacro(PBRMaterial._refractionMacro); - this.shaderData.enableMacro("REFRACTION_MODE", "SPHERE"); - break; - case RefractionMode.Plane: - this.shaderData.enableMacro(PBRMaterial._refractionMacro); - this.shaderData.enableMacro("REFRACTION_MODE", "PLANE"); - break; - case RefractionMode.Thin: - this.shaderData.enableMacro(PBRMaterial._refractionMacro); - this.shaderData.enableMacro("REFRACTION_MODE", "THIN"); - break; + const config = PBRMaterial.REFRACTION_MODE_CONFIG[refractionMode]; + if (config.enableRefraction) { + this.shaderData.enableMacro(PBRMaterial._refractionMacro); + if (config.mode) { + this.shaderData.enableMacro("REFRACTION_MODE", config.mode); + } + } else { + this.shaderData.disableMacro(PBRMaterial._refractionMacro); } }
📜 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 (2)
packages/core/src/material/PBRMaterial.ts(6 hunks)packages/shader-shaderlab/src/shaders/PBR.gs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shader-shaderlab/src/shaders/PBR.gs
🔇 Additional comments (3)
packages/core/src/material/PBRMaterial.ts (3)
32-50: LGTM! Well-organized shader properties and macros.
The static properties and private fields are well-structured and follow consistent naming conventions.
346-381: LGTM! Well-implemented transmission properties.
The implementation includes proper value validation, efficient macro handling, and clear documentation.
411-446: LGTM! Well-implemented thickness properties.
The implementation includes proper value validation, efficient macro handling, and clear documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/core/src/material/PBRMaterial.ts (3)
309-327: Consider initializing _lastRenderQueueType in the constructor.While the refractionMode handling is correct,
_lastRenderQueueTypeshould be initialized in the constructor to ensure it has a valid value before the first mode change.constructor(engine: Engine) { super(engine, Shader.find("pbr")); + this._lastRenderQueueType = this.renderState.renderQueueType; // ... rest of the constructor }
329-343: Extract common render queue type logic to reduce duplication.The logic for updating
renderQueueTypeis duplicated in bothisTransparentandalphaCutoffsetters.+ private _updateRenderQueueType(): void { + this._lastRenderQueueType = this.renderState.renderQueueType; + if (this.refractionMode !== RefractionMode.None) { + this.renderState.renderQueueType = RenderQueueType.Transparent; + } + } override set isTransparent(value: boolean) { super.isTransparent = value; - this._lastRenderQueueType = this.renderState.renderQueueType; - if (this.refractionMode !== RefractionMode.None) { - this.renderState.renderQueueType = RenderQueueType.Transparent; - } + this._updateRenderQueueType(); } override set alphaCutoff(value: number) { super.alphaCutoff = value; - this._lastRenderQueueType = this.renderState.renderQueueType; - if (this.refractionMode !== RefractionMode.None) { - this.renderState.renderQueueType = RenderQueueType.Transparent; - } + this._updateRenderQueueType(); }
Line range hint
470-498: Consider improving type definitions to avoid @ts-ignore.The use of
@ts-ignorefor accessing_onValueChangedsuggests that the type definitions could be improved to properly type these event handlers.Consider defining an interface for the value change notification:
interface ValueChangeNotifier { _onValueChanged?: () => void; } class Vector2 implements ValueChangeNotifier { _onValueChanged?: () => void; } class Color implements ValueChangeNotifier { _onValueChanged?: () => void; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/material/PBRMaterial.ts(6 hunks)packages/core/src/material/enums/Refraction.ts(1 hunks)packages/loader/src/gltf/extensions/KHR_materials_transmission.ts(1 hunks)packages/loader/src/gltf/extensions/KHR_materials_volume.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/loader/src/gltf/extensions/KHR_materials_transmission.ts
- packages/loader/src/gltf/extensions/KHR_materials_volume.ts
- packages/core/src/material/enums/Refraction.ts
🔇 Additional comments (5)
packages/core/src/material/PBRMaterial.ts (5)
32-44: LGTM! Well-organized shader properties and macros.
The new static properties for transmission, thickness, and absorption features follow consistent naming patterns and are properly typed.
345-380: LGTM! Well-implemented transmission properties.
The implementation includes proper value clamping, correct shader macro handling, and clear documentation.
382-408: LGTM! Proper implementation of attenuation properties.
The implementation includes appropriate value validation and consistent color handling.
410-445: LGTM! Well-implemented thickness properties.
The implementation includes proper value validation, correct shader macro handling, and clear documentation.
461-468: LGTM! Proper initialization of new properties.
The constructor properly initializes all new properties with sensible default values.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/material/PBRMaterial.ts (1)
513-525:⚠️ Potential issueImprove refraction mode handling.
This implementation has the same issues as previously identified:
- It doesn't handle
RefractionMode.None- It doesn't disable the previously enabled macro before setting a new one
Please refer to the previous review comment for the suggested fix.
🧹 Nitpick comments (2)
packages/core/src/material/PBRMaterial.ts (2)
329-345: Reduce code duplication in transparency handling.The logic for handling transmission-based transparency is duplicated in both
isTransparentandalphaCutoffsetters.Consider extracting the common logic:
+private _updateTransparencyQueue(): void { + this._lastRenderQueueType = this.renderState.renderQueueType; + if (this.transmission > 0) { + // If transmission enabled, always use transparent queue to ensure get correct opaque texture + this.renderState.renderQueueType = RenderQueueType.Transparent; + } +} override set isTransparent(value: boolean) { super.isTransparent = value; - this._lastRenderQueueType = this.renderState.renderQueueType; - if (this.transmission > 0) { - // If transmission enabled, always use transparent queue to ensure get correct opaque texture - this.renderState.renderQueueType = RenderQueueType.Transparent; - } + this._updateTransparencyQueue(); } override set alphaCutoff(value: number) { super.alphaCutoff = value; - this._lastRenderQueueType = this.renderState.renderQueueType; - if (this.transmission > 0) { - // If transmission enabled, always use transparent queue to ensure get correct opaque texture - this.renderState.renderQueueType = RenderQueueType.Transparent; - } + this._updateTransparencyQueue(); }
Line range hint
473-501: Consider improving type safety in event handlers.The current implementation uses
@ts-ignoreto bypass TypeScript type checking, which might hide potential issues.Consider using proper TypeScript features:
- Define proper types for the event handlers:
interface ValueChangeHandler { _onValueChanged: () => void; } class Vector2 implements ValueChangeHandler { _onValueChanged?: () => void; // ... rest of the implementation } class Color implements ValueChangeHandler { _onValueChanged?: () => void; // ... rest of the implementation }
- Remove the
@ts-ignorecomments once proper types are in place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/material/PBRMaterial.ts(6 hunks)
🔇 Additional comments (4)
packages/core/src/material/PBRMaterial.ts (4)
32-50: LGTM! Well-structured static properties and fields.
The new static properties and fields are well-organized and follow consistent naming conventions.
347-383: LGTM! Well-implemented transmission properties.
The transmission properties are well-implemented with proper value validation, macro handling, and clear documentation.
385-448: LGTM! Well-implemented attenuation and thickness properties.
The properties are well-implemented with proper value validation, macro handling, and clear documentation.
464-471: LGTM! Good initialization of new properties.
The constructor properly initializes all new properties with sensible default values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/material/PBRMaterial.ts (3)
44-49: Consider initializing _refractionMode with RefractionMode.None.The
_refractionModefield is declared but not initialized with a default value. Consider initializing it withRefractionMode.Nonefor better type safety.- private _refractionMode: RefractionMode; + private _refractionMode: RefractionMode = RefractionMode.None;
348-359: Consider adding early return for unchanged values.The transmission setter could be optimized by adding an early return when the value hasn't changed.
set transmission(value: number) { value = MathUtil.clamp(value, 0, 1); + const currentValue = this.shaderData.getFloat(PBRMaterial._transmissionProp); + if (value === currentValue) return; + 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); } } this.shaderData.setFloat(PBRMaterial._transmissionProp, value); }
457-463: Consider documenting the default refraction mode.The constructor sets
RefractionMode.Planeas the default mode, but this isn't documented in the class or property documentation.Add a comment to document the default value:
/** * Refraction switch. * @remarks Use refractionMode to set the refraction shape. + * @defaultValue `RefractionMode.Plane` */ get refractionMode(): RefractionMode {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/material/PBRMaterial.ts(6 hunks)
🔇 Additional comments (5)
packages/core/src/material/PBRMaterial.ts (5)
32-42: LGTM! Well-organized static property declarations.
The static properties for transmission, thickness, and absorption features are properly defined using ShaderMacro and ShaderProperty.
324-338: LGTM! Proper handling of transparency with transmission.
The overrides for isTransparent and alphaCutoff correctly ensure transparent rendering when transmission is enabled.
506-518: Improve refraction mode handling.
The _setRefractionMode method has two potential issues:
- It doesn't handle
RefractionMode.None - It doesn't disable the previously enabled macro before setting a new one
466-470: Consider using bind for event handlers.
The current implementation uses arrow functions for event handlers. Using bind would be more readable as suggested in the past review comments.
Also applies to: 483-494
317-322:
Fix potential issues in refractionMode setter.
The current implementation doesn't handle the case when switching from a non-None mode to None mode, which could leave the render queue type in an incorrect state.
set refractionMode(value: RefractionMode) {
if (value !== this._refractionMode) {
+ if (value !== RefractionMode.None) {
+ this._lastRenderQueueType = this.renderState.renderQueueType;
+ this.renderState.renderQueueType = RenderQueueType.Transparent;
+ } else {
+ this.renderState.renderQueueType = this._lastRenderQueueType;
+ }
this._refractionMode = value;
this._setRefractionMode(value);
}
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/core/src/material/PBRMaterial.ts (5)
44-48: Consider initializing fields with default values in the constructor.Private fields are currently scattered between property declarations and constructor initialization. Consider moving all initializations to the constructor for better maintainability.
- private _refractionMode: RefractionMode; - private _anisotropyRotation: number = 0; - private _iridescenceRange = new Vector2(100, 400); - private _sheenEnabled = false; - private _absorptionEnabled = true; + private _refractionMode: RefractionMode; + private _anisotropyRotation: number; + private _iridescenceRange: Vector2; + private _sheenEnabled: boolean; + private _absorptionEnabled: boolean;
323-337: Consider consolidating duplicate queue type logic.The same queue type logic is duplicated in both
isTransparentandalphaCutoffsetters.+ private _updateQueueType(): void { + if (this.transmission > 0) { + // If transmission enabled, always use transparent queue to ensure get correct opaque texture + this.renderState.renderQueueType = RenderQueueType.Transparent; + } + } override set isTransparent(value: boolean) { super.isTransparent = value; - if (this.transmission > 0) { - // If transmission enabled, always use transparent queue to ensure get correct opaque texture - this.renderState.renderQueueType = RenderQueueType.Transparent; - } + this._updateQueueType(); } override set alphaCutoff(value: number) { super.alphaCutoff = value; - if (this.transmission > 0) { - // If transmission enabled, always use transparent queue to ensure get correct opaque texture - this.renderState.renderQueueType = RenderQueueType.Transparent; - } + this._updateQueueType(); }
347-358: Improve transmission value change detection.The current implementation uses a double negation (
!!) to compare boolean states. Consider using a more explicit comparison.set transmission(value: number) { value = MathUtil.clamp(value, 0, 1); - if (!!this.shaderData.getFloat(PBRMaterial._transmissionProp) !== !!value) { + const currentValue = this.shaderData.getFloat(PBRMaterial._transmissionProp); + const hasTransmission = value > 0; + const hadTransmission = currentValue > 0; + if (hasTransmission !== hadTransmission) { if (value > 0) { this.shaderData.enableMacro(PBRMaterial._transmissionMacro); this.renderState.renderQueueType = RenderQueueType.Transparent; } else { this.shaderData.disableMacro(PBRMaterial._transmissionMacro); } } this.shaderData.setFloat(PBRMaterial._transmissionProp, value); }
413-423: Similar improvement needed for thickness value change detection.The same double negation pattern is used here. Consider using the same explicit comparison pattern as suggested for transmission.
set thickness(value: number) { value = Math.max(0, value); - if (!!this.shaderData.getFloat(PBRMaterial._thicknessProp) !== !!value) { + const currentValue = this.shaderData.getFloat(PBRMaterial._thicknessProp); + const hasThickness = value > 0; + const hadThickness = currentValue > 0; + if (hasThickness !== hadThickness) { if (value > 0) { this.shaderData.enableMacro(PBRMaterial._thicknessMacro); } else { this.shaderData.disableMacro(PBRMaterial._thicknessMacro); } } this.shaderData.setFloat(PBRMaterial._thicknessProp, value); }
500-511: Consider using Color methods for color comparison.Instead of manually adding color components, consider using Color's built-in methods for a more maintainable solution.
private _attenuationColorChanged(): void { const attenuationColor = this.attenuationColor; - const enableAbsorption = attenuationColor.r + attenuationColor.g + attenuationColor.b > 0; + const enableAbsorption = !attenuationColor.equals(Color.BLACK); if (enableAbsorption !== this._absorptionEnabled) { this._absorptionEnabled = enableAbsorption; if (enableAbsorption) { this.shaderData.enableMacro(PBRMaterial._absorptionMacro); } else { this.shaderData.disableMacro(PBRMaterial._absorptionMacro); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
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/LightIndirectPBR.glslis excluded by!**/*.glsl
📒 Files selected for processing (1)
packages/core/src/material/PBRMaterial.ts(5 hunks)
🔇 Additional comments (4)
packages/core/src/material/PBRMaterial.ts (4)
32-42: LGTM! Well-organized shader properties and macros.
The static properties are well-organized and follow a consistent naming pattern, making the code maintainable.
316-321: Fix potential issues in refractionMode setter.
The current implementation has two issues:
- It doesn't handle the case when
valueisRefractionMode.None - It doesn't disable the previous mode's macro before setting a new one
456-463: LGTM! Constructor initialization is complete.
All required properties are properly initialized with appropriate default values.
513-525:
Fix refraction mode handling.
The current implementation of _setRefractionMode has two issues that were previously identified:
- It doesn't handle
RefractionMode.None - It doesn't disable the previously enabled macro before setting a new one
private _setRefractionMode(refractionMode: RefractionMode): void {
+ // Disable previous mode
+ this.shaderData.disableMacro("REFRACTION_MODE");
+
switch (refractionMode) {
case RefractionMode.Sphere:
this.shaderData.enableMacro("REFRACTION_MODE", "SPHERE");
break;
case RefractionMode.Plane:
this.shaderData.enableMacro("REFRACTION_MODE", "PLANE");
break;
case RefractionMode.Thin:
this.shaderData.enableMacro("REFRACTION_MODE", "THIN");
break;
+ case RefractionMode.None:
+ // No macro needed for None mode
+ break;
}
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/core/src/material/PBRMaterial.ts (3)
45-50: Introduction of new private fields for refraction and absorption.
The fields_refractionMode,_sheenEnabled, and_absorptionEnabledenhance the material's customizable behavior. Just ensure these booleans remain in sync with the corresponding shader macros.
324-353: OverridingisTransparentandalphaCutofffor advanced transmission logic.
- Correctly forcing the render queue to
Transparentiftransmission > 0.- Be cautious of performance implications if unintended transparency states are triggered often.
Would you like assistance with additional tests that confirm queue usage under different alpha cutoff and transmission conditions?
482-484: Attaching_onValueChangedcallbacks for color and range fields.
Be sure these listeners are removed if the material is destroyed or cloned excessively, to prevent memory leaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/shader-shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightDirectPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glslis excluded by!**/*.glsl
📒 Files selected for processing (2)
packages/core/src/material/BaseMaterial.ts(4 hunks)packages/core/src/material/PBRMaterial.ts(5 hunks)
🔇 Additional comments (18)
packages/core/src/material/BaseMaterial.ts (6)
27-27: Changing _alphaCutoffProp to protected is appropriate for subclass flexibility.
Exposing this property allows derived materials (e.g., PBRMaterial) to override or refine alpha cutoff behavior. Good job!
31-31: Shifting _isTransparent from private to protected is sensible.
This enables subclasses to access and refine transparent logic in an object-oriented manner.
76-76: Setter now delegates to _seIsTransparent.
Refactoring the logic into a dedicated helper method enhances maintainability and consistency across different sections of the code.
105-105: Setter now delegates to _setAlphaCutoff.
This improves code readability by reducing inline complexity, making the alpha cutoff logic easier to maintain.
237-257: Scrutinize transparency logic to ensure consistent queue updates for shadows.
- The new
_seIsTransparentmethod updates both the material's render queue and the shadow caster queue, which is good for consistent shading behavior. - Consider verifying that any specialized blending modes for child classes also hook into this method if required.
259-289: Ensure alpha cutoff logic aligns with partial transparency.
- Good job toggling the
_alphaCutoffMacroand adjusting the shadow caster queue. - The loop (lines 275–286) correctly sets
renderQueueTypefor each pass based on whether alpha cutoff or transparency is enabled. - Verify that no concurrent macros or blending states remain active when switching from alpha-cut to fully opaque.
packages/core/src/material/PBRMaterial.ts (12)
1-9: New imports and references for advanced PBR capabilities.
The added imports for RefractionMode, Texture2D, and others lay the groundwork for the new PBR features. Ensure these are all used as expected without introducing circular dependencies.
33-44: Definition of transmission/thickness/absorption-related macros.
Expanding macros for new PBR properties is well-structured, but consider verifying that each macro is consistently enabled/disabled to avoid conflicting states during runtime.
309-322: Refraction mode getter and setter.
- The
refractionModesetter calls_setRefractionMode, which is good for code clarity. - Ensure the old macro (if any) is disabled before enabling a new one to avoid leftover macros from the previous mode.
354-373: Transmission factor handling.
- Clever usage of macros for toggling advanced PBR transmission.
- Great that you clamp the input [0..1].
- Confirm that the disable state reverts to non-transparent queue if no other property (e.g.,
isTransparent) forces transparency.
375-390: Transmission texture.
Enabling or disabling _transmissionTextureMacro based on the presence of a texture is straightforward. Consider thorough tests to ensure the red channel usage is correct.
392-406: Attenuation color property.
- The
attenuationColorconcept is critical for volumetric and absorption effects. - Confirm possible edge cases if color channels are zero or negative.
407-419: Attenuation distance property.
Clamping to >= 0 prevents invalid negative distances. Ensure that an infinite value is handled gracefully in the shader.
420-438: Thickness property.
- Turning the
_thicknessMacroon/off is good. - Suggest verifying how zero thickness interacts with partial transmission or absorption in corner use-cases.
440-455: Thickness texture.
Uses the green channel for thickness. This is well-labeled in the docstring; it’s good that you multiply by the thickness factor.
471-478: Constructor initializes default PBR settings.
- Defaulting to
RefractionMode.Planeensures a known baseline. _absorptionMacrois always enabled initially. If attenuation color is black, the_absorptionEnabledeventually toggles off. Good pattern, just confirm it’s consistent.
495-513: Handling iridescence range changes.
Monitoring changes to _iridescenceRange is straightforward. Minimizing direct assignment in the property ensures correctness.
515-526: Absorption color callback.
- Solid approach for enabling or disabling
_absorptionMacro. - Recommend verifying the relationship between the attenuation color and
_absorptionEnabledwhen color is near zero.
| private _setRefractionMode(refractionMode: RefractionMode): void { | ||
| switch (refractionMode) { | ||
| case RefractionMode.Sphere: | ||
| this.shaderData.enableMacro("REFRACTION_MODE", "SPHERE"); | ||
| break; | ||
| case RefractionMode.Plane: | ||
| this.shaderData.enableMacro("REFRACTION_MODE", "PLANE"); | ||
| break; | ||
| case RefractionMode.Thin: | ||
| this.shaderData.enableMacro("REFRACTION_MODE", "THIN"); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refraction mode macro setting.
- Currently only enabling the new macro; consider disabling previously set macros when switching modes to avoid possible conflicts.
- A
RefractionMode.Nonecase (if it’s introduced later) would help handle disabling all macros.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2478 +/- ##
===========================================
- Coverage 68.43% 68.38% -0.06%
===========================================
Files 922 923 +1
Lines 95661 95928 +267
Branches 8121 8137 +16
===========================================
+ Hits 65466 65600 +134
- Misses 29942 30074 +132
- 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. |
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
Documentation
Chores