PBR shader support Iridescence #2425
Conversation
![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
🧹 Outside diff range and nitpick comments (1)
packages/core/src/material/PBRMaterial.ts (1)
198-230: LGTM! Texture implementations follow established patterns.The texture property implementations correctly follow the class's established patterns for texture handling and macro management.
Would you like me to help generate tests for texture assignment and macro enabling/disabling?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 202-203: packages/core/src/material/PBRMaterial.ts#L202-L203
Added lines #L202 - L203 were not covered by tests
[warning] 206-213: packages/core/src/material/PBRMaterial.ts#L206-L213
Added lines #L206 - L213 were not covered by tests
[warning] 219-220: packages/core/src/material/PBRMaterial.ts#L219-L220
Added lines #L219 - L220 were not covered by tests
[warning] 223-230: packages/core/src/material/PBRMaterial.ts#L223-L230
Added lines #L223 - L230 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/material/PBRMaterial.ts(4 hunks)packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/material/PBRMaterial.ts
[warning] 144-145: packages/core/src/material/PBRMaterial.ts#L144-L145
Added lines #L144 - L145 were not covered by tests
[warning] 148-157: packages/core/src/material/PBRMaterial.ts#L148-L157
Added lines #L148 - L157 were not covered by tests
[warning] 164-165: packages/core/src/material/PBRMaterial.ts#L164-L165
Added lines #L164 - L165 were not covered by tests
[warning] 168-170: packages/core/src/material/PBRMaterial.ts#L168-L170
Added lines #L168 - L170 were not covered by tests
[warning] 177-178: packages/core/src/material/PBRMaterial.ts#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 181-183: packages/core/src/material/PBRMaterial.ts#L181-L183
Added lines #L181 - L183 were not covered by tests
[warning] 190-191: packages/core/src/material/PBRMaterial.ts#L190-L191
Added lines #L190 - L191 were not covered by tests
[warning] 194-196: packages/core/src/material/PBRMaterial.ts#L194-L196
Added lines #L194 - L196 were not covered by tests
[warning] 202-203: packages/core/src/material/PBRMaterial.ts#L202-L203
Added lines #L202 - L203 were not covered by tests
[warning] 206-213: packages/core/src/material/PBRMaterial.ts#L206-L213
Added lines #L206 - L213 were not covered by tests
[warning] 219-220: packages/core/src/material/PBRMaterial.ts#L219-L220
Added lines #L219 - L220 were not covered by tests
[warning] 223-230: packages/core/src/material/PBRMaterial.ts#L223-L230
Added lines #L223 - L230 were not covered by tests
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts
[warning] 11-39: packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts#L11-L39
Added lines #L11 - L39 were not covered by tests
🔇 Additional comments (8)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts (5)
1-9: LGTM! Class setup follows GLTF extension patterns
The imports are complete and the extension is properly registered with the correct mode.
20-23: LGTM! Property assignments are correct
The assignments are straightforward and the previously reported issue with swapped min/max values has been fixed.
25-38: 🛠️ Refactor suggestion
Add comprehensive error handling for texture loading
While the texture loading implementation is correct, it needs error handling as mentioned in the previous review. Additionally, consider these improvements:
- Add loading state tracking
- Handle texture disposal
if (iridescenceTexture) {
GLTFMaterialParser._checkOtherTextureTransform(iridescenceTexture, "Iridescence texture");
+ material._textureLoadPromises = material._textureLoadPromises || [];
+ const loadPromise =
context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => {
material.iridescenceTexture = texture;
- });
+ }).catch((error) => {
+ console.warn(`Failed to load iridescence texture: ${error.message}`);
+ });
+ material._textureLoadPromises.push(loadPromise);
}Similar changes should be applied to iridescenceThicknessTexture handling.
10-39:
Critical: Add test coverage for the extension
The static analysis indicates that this extension lacks test coverage, which is critical for ensuring correct behavior.
Would you like me to help generate a test suite that covers:
- Default values when properties are missing
- Texture loading success and failure cases
- Edge cases for thickness values
- Invalid input handling
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 11-39: packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts#L11-L39
Added lines #L11 - L39 were not covered by tests
10-18: Verify default values against GLTF spec
The default values look reasonable, but let's verify they align with the KHR_materials_iridescence specification.
✅ Verification successful
Default values match the implementation in PBRMaterial class
The default values in the GLTF loader match the documented defaults in the core PBRMaterial class:
- iridescenceIor: 1.3
- iridescenceThicknessMinimum: 100 nanometers
- iridescenceThicknessMaximum: 400 nanometers
- iridescenceFactor: 0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for KHR_materials_iridescence spec in the Khronos GLTF repository
gh api repos/KhronosGroup/glTF/contents/extensions/2.0/Khronos/KHR_materials_iridescence/README.md | \
jq -r '.content' | base64 -d | grep -A 5 "default values"
Length of output: 232
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to search for default values in the codebase
# and any spec-related documentation
# Search for any spec or documentation files related to KHR_materials_iridescence
fd -t f -e md -e txt . | xargs rg -i "khr.*iridescence"
# Search for comments or documentation near the iridescence implementation
rg -B 5 -A 10 "iridescenceFactor|iridescenceIor|iridescenceThickness"
Length of output: 11041
packages/core/src/material/PBRMaterial.ts (3)
23-25: LGTM! Static properties follow established patterns.
The new shader properties for iridescence follow the existing naming conventions and initialization patterns used in the class.
244-244: LGTM! Constructor initialization is correct.
The default values match the documented defaults and are appropriate for iridescence properties.
159-170:
Add validation for iridescence IOR.
The index of refraction must be greater than 1.0 as it represents a physical property.
Apply this diff to add validation:
set iridescenceIor(value: number) {
const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp);
+ value = Math.max(1.0, value);
iridescenceInfo.y = value;
}Would you like me to help generate unit tests for this property?
Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 164-165: packages/core/src/material/PBRMaterial.ts#L164-L165
Added lines #L164 - L165 were not covered by tests
[warning] 168-170: packages/core/src/material/PBRMaterial.ts#L168-L170
Added lines #L168 - L170 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/core/src/material/PBRMaterial.ts (2)
139-158: Add test coverage for iridescence property.The implementation looks good with proper value clamping and macro handling. However, test coverage is missing for:
- Value clamping behavior
- Macro enabling/disabling
- Edge cases
Would you like me to help generate unit tests for this property?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 144-145: packages/core/src/material/PBRMaterial.ts#L144-L145
Added lines #L144 - L145 were not covered by tests
[warning] 148-158: packages/core/src/material/PBRMaterial.ts#L148-L158
Added lines #L148 - L158 were not covered by tests
199-231: Add test coverage for texture properties.The implementation correctly handles texture assignment and macro toggling, following the established pattern. However, test coverage is missing for:
- Texture assignment
- Macro enabling/disabling
- Cleanup behavior
Would you like me to help generate unit tests for these properties?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 203-204: packages/core/src/material/PBRMaterial.ts#L203-L204
Added lines #L203 - L204 were not covered by tests
[warning] 207-214: packages/core/src/material/PBRMaterial.ts#L207-L214
Added lines #L207 - L214 were not covered by tests
[warning] 220-221: packages/core/src/material/PBRMaterial.ts#L220-L221
Added lines #L220 - L221 were not covered by tests
[warning] 224-231: packages/core/src/material/PBRMaterial.ts#L224-L231
Added lines #L224 - L231 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/material/PBRMaterial.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/material/PBRMaterial.ts
[warning] 144-145: packages/core/src/material/PBRMaterial.ts#L144-L145
Added lines #L144 - L145 were not covered by tests
[warning] 148-158: packages/core/src/material/PBRMaterial.ts#L148-L158
Added lines #L148 - L158 were not covered by tests
[warning] 165-166: packages/core/src/material/PBRMaterial.ts#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 169-171: packages/core/src/material/PBRMaterial.ts#L169-L171
Added lines #L169 - L171 were not covered by tests
[warning] 178-179: packages/core/src/material/PBRMaterial.ts#L178-L179
Added lines #L178 - L179 were not covered by tests
[warning] 182-184: packages/core/src/material/PBRMaterial.ts#L182-L184
Added lines #L182 - L184 were not covered by tests
[warning] 191-192: packages/core/src/material/PBRMaterial.ts#L191-L192
Added lines #L191 - L192 were not covered by tests
[warning] 195-197: packages/core/src/material/PBRMaterial.ts#L195-L197
Added lines #L195 - L197 were not covered by tests
[warning] 203-204: packages/core/src/material/PBRMaterial.ts#L203-L204
Added lines #L203 - L204 were not covered by tests
[warning] 207-214: packages/core/src/material/PBRMaterial.ts#L207-L214
Added lines #L207 - L214 were not covered by tests
[warning] 220-221: packages/core/src/material/PBRMaterial.ts#L220-L221
Added lines #L220 - L221 were not covered by tests
[warning] 224-231: packages/core/src/material/PBRMaterial.ts#L224-L231
Added lines #L224 - L231 were not covered by tests
🔇 Additional comments (3)
packages/core/src/material/PBRMaterial.ts (3)
23-25: LGTM! Static properties follow established patterns.
The shader property declarations are well-structured and follow the existing naming conventions.
160-171: LGTM! IOR implementation is physically correct.
The implementation correctly enforces IOR > 1.0, which is physically accurate for dielectric materials. The default value of 1.3 is reasonable for thin films.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 165-166: packages/core/src/material/PBRMaterial.ts#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 169-171: packages/core/src/material/PBRMaterial.ts#L169-L171
Added lines #L169 - L171 were not covered by tests
245-245: LGTM! Constructor initialization uses appropriate defaults.
The default values (0, 1.3, 100, 400) are reasonable for:
- iridescence: 0 (disabled by default)
- IOR: 1.3 (typical for thin films)
- thickness range: 100-400nm (reasonable range for thin films)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts (2)
11-23: Add documentation for parameter unitsThe thickness range values appear to be in nanometers, but this isn't explicitly documented. Consider adding a comment to clarify the units for better maintainability.
const { iridescenceFactor = 0, iridescenceTexture, iridescenceIor = 1.3, + // Thickness values in nanometers (nm) iridescenceThicknessMinimum = 100, iridescenceThicknessMaximum = 400, iridescenceThicknessTexture } = schema;🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 11-39: packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts#L11-L39
Added lines #L11 - L39 were not covered by tests
25-38: Consider cleanup for texture loadingThe texture promises should be properly handled to avoid potential memory leaks. Consider storing the promises or adding cleanup logic when the material is disposed.
if (iridescenceTexture) { GLTFMaterialParser._checkOtherTextureTransform(iridescenceTexture, "Iridescence texture"); - context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => { + const texturePromise = context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index); + material._disposableTextures.push(texturePromise); + texturePromise.then((texture) => { material.iridescenceTexture = texture; }); }packages/core/src/material/PBRMaterial.ts (2)
191-203: Merge thickness properties into a range vector.As suggested in past reviews, consider merging the thickness properties into a single
iridescenceThicknessRangeproperty for better encapsulation.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 196-197: packages/core/src/material/PBRMaterial.ts#L196-L197
Added lines #L196 - L197 were not covered by tests
[warning] 200-203: packages/core/src/material/PBRMaterial.ts#L200-L203
Added lines #L200 - L203 were not covered by tests
205-223: Enhance texture documentation.The documentation should specify:
- Which texture channel (green) is used
- How the values are interpolated
- Default behavior when texture is undefined
Apply this diff to improve the documentation:
/** - * The thickness texture of the thin-film layer, Sampling green channel. + * The thickness texture of the thin-film layer. * @remarks + * - Samples the green channel for thickness values * - If iridescenceThicknessTexture is defined, Values in-between will linearly interpolate between the elements of the iridescenceThicknessRange. * - If iridescenceThicknessTexture is not defined, iridescence thickness will use only iridescenceThicknessMax. */🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 212-213: packages/core/src/material/PBRMaterial.ts#L212-L213
Added lines #L212 - L213 were not covered by tests
[warning] 216-223: packages/core/src/material/PBRMaterial.ts#L216-L223
Added lines #L216 - L223 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/material/PBRMaterial.ts(4 hunks)packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/material/PBRMaterial.ts
[warning] 145-146: packages/core/src/material/PBRMaterial.ts#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 149-159: packages/core/src/material/PBRMaterial.ts#L149-L159
Added lines #L149 - L159 were not covered by tests
[warning] 165-166: packages/core/src/material/PBRMaterial.ts#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 169-176: packages/core/src/material/PBRMaterial.ts#L169-L176
Added lines #L169 - L176 were not covered by tests
[warning] 183-184: packages/core/src/material/PBRMaterial.ts#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 187-189: packages/core/src/material/PBRMaterial.ts#L187-L189
Added lines #L187 - L189 were not covered by tests
[warning] 196-197: packages/core/src/material/PBRMaterial.ts#L196-L197
Added lines #L196 - L197 were not covered by tests
[warning] 200-203: packages/core/src/material/PBRMaterial.ts#L200-L203
Added lines #L200 - L203 were not covered by tests
[warning] 212-213: packages/core/src/material/PBRMaterial.ts#L212-L213
Added lines #L212 - L213 were not covered by tests
[warning] 216-223: packages/core/src/material/PBRMaterial.ts#L216-L223
Added lines #L216 - L223 were not covered by tests
[warning] 243-245: packages/core/src/material/PBRMaterial.ts#L243-L245
Added lines #L243 - L245 were not covered by tests
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts
[warning] 11-39: packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts#L11-L39
Added lines #L11 - L39 were not covered by tests
🔇 Additional comments (2)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts (1)
1-9: LGTM: Clean implementation of extension registration
The imports are complete and the extension is properly registered with the correct mode.
packages/core/src/material/PBRMaterial.ts (1)
140-159: Improve iridescence property implementation.
Based on past reviews and best practices:
- Consider renaming to either:
iridescenceIntensityfor clarity- Keep as
iridescencefor consistency withclearCoat
- The value assignment and macro check order is correct after the fix
Let's verify the naming consistency:
✅ Verification successful
Based on the search results, I can now generate the final response:
Keep the property name as iridescence for consistency
The current naming is consistent with the existing pattern in the codebase where similar intensity properties use concise names without the "intensity" suffix:
clearCoatis used for the clear coat intensity factoriridescencefollows the same pattern for the iridescence intensity factor
Both properties represent intensity values from 0.0 to 1.0 and use similar implementation patterns with their respective shader macros.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar property patterns
rg "clearCoat|clearcoat|intensity" packages/core/src/material/
Length of output: 4220
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-146: packages/core/src/material/PBRMaterial.ts#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 149-159: packages/core/src/material/PBRMaterial.ts#L149-L159
Added lines #L149 - L159 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/core/src/material/PBRMaterial.ts (3)
140-159: Consider renamingiridescenceproperty for clarity.Based on past reviews, consider either:
- Renaming to
iridescenceIntensityto better reflect its purpose, or- Keeping as
iridescencefor consistency withclearCoatThe implementation looks good with proper value clamping and macro handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-146: packages/core/src/material/PBRMaterial.ts#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 149-159: packages/core/src/material/PBRMaterial.ts#L149-L159
Added lines #L149 - L159 were not covered by tests
205-223: Enhance texture documentation.The documentation should specify which texture channels are used for what purpose, similar to how other texture properties are documented in this class.
Apply this diff to improve the documentation:
/** * The thickness texture of the thin-film layer, Sampling green channel. * @remarks + * The green channel is used to control the thickness of the iridescent layer. * If iridescenceThicknessTexture is defined, Values in-between will linearly interpolate between the elements of the iridescenceThicknessRange. * If iridescenceThicknessTexture is not defined, iridescence thickness will use only iridescenceThicknessMax. */🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 212-213: packages/core/src/material/PBRMaterial.ts#L212-L213
Added lines #L212 - L213 were not covered by tests
[warning] 216-223: packages/core/src/material/PBRMaterial.ts#L216-L223
Added lines #L216 - L223 were not covered by tests
237-240: Add test coverage for initialization.The static analysis indicates missing test coverage for the initialization of iridescence properties.
Would you like me to help generate unit tests to verify:
- Default values initialization
- Thickness range change handling
- Value constraints and validation
- Texture handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/material/PBRMaterial.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/material/PBRMaterial.ts
[warning] 145-146: packages/core/src/material/PBRMaterial.ts#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 149-159: packages/core/src/material/PBRMaterial.ts#L149-L159
Added lines #L149 - L159 were not covered by tests
[warning] 165-166: packages/core/src/material/PBRMaterial.ts#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 169-176: packages/core/src/material/PBRMaterial.ts#L169-L176
Added lines #L169 - L176 were not covered by tests
[warning] 183-184: packages/core/src/material/PBRMaterial.ts#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 187-189: packages/core/src/material/PBRMaterial.ts#L187-L189
Added lines #L187 - L189 were not covered by tests
[warning] 196-197: packages/core/src/material/PBRMaterial.ts#L196-L197
Added lines #L196 - L197 were not covered by tests
[warning] 200-203: packages/core/src/material/PBRMaterial.ts#L200-L203
Added lines #L200 - L203 were not covered by tests
[warning] 212-213: packages/core/src/material/PBRMaterial.ts#L212-L213
Added lines #L212 - L213 were not covered by tests
[warning] 216-223: packages/core/src/material/PBRMaterial.ts#L216-L223
Added lines #L216 - L223 were not covered by tests
[warning] 243-245: packages/core/src/material/PBRMaterial.ts#L243-L245
Added lines #L243 - L245 were not covered by tests
🔇 Additional comments (1)
packages/core/src/material/PBRMaterial.ts (1)
23-26: LGTM! Static properties follow existing patterns.
The new shader properties and private field are well-structured and follow the established naming conventions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/core/src/material/PBRMaterial.ts (4)
140-159: Consider renaming to 'iridescenceIntensity' for clarity.The property name 'iridescence' could be more descriptive as 'iridescenceIntensity' since it represents the intensity factor.
Additionally, the implementation lacks test coverage for:
- Value clamping behavior
- Macro enabling/disabling
- Edge cases
Would you like me to help generate unit tests for this property?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-146: packages/core/src/material/PBRMaterial.ts#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 149-159: packages/core/src/material/PBRMaterial.ts#L149-L159
Added lines #L149 - L159 were not covered by tests
161-176: Enhance texture documentation.The current documentation "Sampling blue channel" lacks context. Consider expanding it to:
- * The iridescence intensity texture, Sampling blue channel, And multiply "iridescence". + * The iridescence intensity texture. The blue channel is sampled and multiplied with the iridescence factor + * to determine the final iridescence intensity.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 165-166: packages/core/src/material/PBRMaterial.ts#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 169-176: packages/core/src/material/PBRMaterial.ts#L169-L176
Added lines #L169 - L176 were not covered by tests
205-223: Enhance texture channel documentation.Similar to the iridescence texture, specify the green channel usage more explicitly:
- * The thickness texture of the thin-film layer, Sampling green channel. + * The thickness texture of the thin-film layer. The green channel is sampled to determine + * the interpolation factor between minimum and maximum thickness values.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 212-213: packages/core/src/material/PBRMaterial.ts#L212-L213
Added lines #L212 - L213 were not covered by tests
[warning] 216-223: packages/core/src/material/PBRMaterial.ts#L216-L223
Added lines #L216 - L223 were not covered by tests
237-240: Remove @ts-ignore by adding proper typing.Instead of using @ts-ignore, consider adding proper typing for the _onValueChanged callback:
- // @ts-ignore - this._iridescenceRange._onValueChanged = this._onIridescenceRangeChanged.bind(this); + interface Vector2WithCallback extends Vector2 { + _onValueChanged?: () => void; + } + (this._iridescenceRange as Vector2WithCallback)._onValueChanged = this._onIridescenceRangeChanged.bind(this);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/material/PBRMaterial.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/material/PBRMaterial.ts
[warning] 145-146: packages/core/src/material/PBRMaterial.ts#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 149-159: packages/core/src/material/PBRMaterial.ts#L149-L159
Added lines #L149 - L159 were not covered by tests
[warning] 165-166: packages/core/src/material/PBRMaterial.ts#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 169-176: packages/core/src/material/PBRMaterial.ts#L169-L176
Added lines #L169 - L176 were not covered by tests
[warning] 183-184: packages/core/src/material/PBRMaterial.ts#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 187-189: packages/core/src/material/PBRMaterial.ts#L187-L189
Added lines #L187 - L189 were not covered by tests
[warning] 196-197: packages/core/src/material/PBRMaterial.ts#L196-L197
Added lines #L196 - L197 were not covered by tests
[warning] 200-203: packages/core/src/material/PBRMaterial.ts#L200-L203
Added lines #L200 - L203 were not covered by tests
[warning] 212-213: packages/core/src/material/PBRMaterial.ts#L212-L213
Added lines #L212 - L213 were not covered by tests
[warning] 216-223: packages/core/src/material/PBRMaterial.ts#L216-L223
Added lines #L216 - L223 were not covered by tests
[warning] 243-245: packages/core/src/material/PBRMaterial.ts#L243-L245
Added lines #L243 - L245 were not covered by tests
🔇 Additional comments (2)
packages/core/src/material/PBRMaterial.ts (2)
23-26: LGTM! Static properties follow existing patterns.
The shader properties and thickness range field are properly initialized following the class's established patterns.
178-189: LGTM! Proper IOR validation.
The implementation correctly enforces the physical constraint that IOR must be ≥ 1.0.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 183-184: packages/core/src/material/PBRMaterial.ts#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 187-189: packages/core/src/material/PBRMaterial.ts#L187-L189
Added lines #L187 - L189 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
packages/core/src/material/PBRMaterial.ts (2)
140-159: Consider renaming for consistency.Based on past reviews, consider either:
- Renaming to
iridescenceIntensityto better reflect its purpose, or- Using just
iridescencefor consistency withclearCoatThe current implementation correctly:
- Clamps values to [0,1] range
- Handles macro enabling/disabling
- Updates shader data
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-146: packages/core/src/material/PBRMaterial.ts#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 149-159: packages/core/src/material/PBRMaterial.ts#L149-L159
Added lines #L149 - L159 were not covered by tests
161-176: Enhance texture documentation.The documentation should be more detailed about:
- Which texture channel is used (red channel)
- The expected format and range of the texture data
- How the texture value interacts with the iridescence factor
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 165-166: packages/core/src/material/PBRMaterial.ts#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 169-176: packages/core/src/material/PBRMaterial.ts#L169-L176
Added lines #L169 - L176 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/material/PBRMaterial.ts(4 hunks)packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/material/PBRMaterial.ts
[warning] 145-146: packages/core/src/material/PBRMaterial.ts#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 149-159: packages/core/src/material/PBRMaterial.ts#L149-L159
Added lines #L149 - L159 were not covered by tests
[warning] 165-166: packages/core/src/material/PBRMaterial.ts#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 169-176: packages/core/src/material/PBRMaterial.ts#L169-L176
Added lines #L169 - L176 were not covered by tests
[warning] 183-184: packages/core/src/material/PBRMaterial.ts#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 187-189: packages/core/src/material/PBRMaterial.ts#L187-L189
Added lines #L187 - L189 were not covered by tests
[warning] 196-197: packages/core/src/material/PBRMaterial.ts#L196-L197
Added lines #L196 - L197 were not covered by tests
[warning] 200-203: packages/core/src/material/PBRMaterial.ts#L200-L203
Added lines #L200 - L203 were not covered by tests
[warning] 212-213: packages/core/src/material/PBRMaterial.ts#L212-L213
Added lines #L212 - L213 were not covered by tests
[warning] 216-223: packages/core/src/material/PBRMaterial.ts#L216-L223
Added lines #L216 - L223 were not covered by tests
[warning] 243-245: packages/core/src/material/PBRMaterial.ts#L243-L245
Added lines #L243 - L245 were not covered by tests
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts
[warning] 11-38: packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts#L11-L38
Added lines #L11 - L38 were not covered by tests
🔇 Additional comments (3)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts (1)
1-9: LGTM! Clean implementation of extension registration
The imports are comprehensive and the extension registration is properly implemented using the decorator pattern.
packages/core/src/material/PBRMaterial.ts (2)
23-26: LGTM! Static properties are well-defined.
The shader properties follow the existing naming pattern and are properly initialized.
178-189: LGTM! IOR implementation is physically correct.
The implementation correctly:
- Enforces IOR ≥ 1.0 (physically valid constraint)
- Provides clear documentation
- Updates shader data properly
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 183-184: packages/core/src/material/PBRMaterial.ts#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 187-189: packages/core/src/material/PBRMaterial.ts#L187-L189
Added lines #L187 - L189 were not covered by tests
| context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => { | ||
| material.iridescenceTexture = texture; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Improve texture loading implementation
The current texture loading implementation has several issues:
- No error handling for failed texture loads
- Promises are not being tracked, which could lead to race conditions
- No cleanup handling if the material is disposed before texture loads complete
Consider implementing this pattern:
if (iridescenceTexture) {
GLTFMaterialParser._checkOtherTextureTransform(iridescenceTexture, "Iridescence texture");
- context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => {
- material.iridescenceTexture = texture;
- });
+ const texturePromise = context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index)
+ .then((texture) => {
+ if (!material.destroyed) {
+ material.iridescenceTexture = texture;
+ }
+ })
+ .catch((error) => {
+ console.warn(`Failed to load iridescence texture: ${error.message}`);
+ });
+ context.addPendingTask(texturePromise);
}Apply similar changes to the iridescenceThicknessTexture loading block.
Also applies to: 34-37
| const { | ||
| iridescenceFactor = 0, | ||
| iridescenceTexture, | ||
| iridescenceIor = 1.3, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for iridescenceIor
The IOR (Index of Refraction) value should be validated as it typically should be greater than 1.0 for physical accuracy.
- iridescenceIor = 1.3,
+ iridescenceIor = Math.max(1.0, schema.iridescenceIor ?? 1.3),📝 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.
| iridescenceIor = 1.3, | |
| iridescenceIor = Math.max(1.0, schema.iridescenceIor ?? 1.3), |
| iridescenceThicknessMinimum = 100, | ||
| iridescenceThicknessMaximum = 400, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate thickness range values
The thickness range values should be validated to ensure minimum is less than maximum and both are positive.
- iridescenceThicknessMinimum = 100,
- iridescenceThicknessMaximum = 400,
+ iridescenceThicknessMinimum = Math.max(0, schema.iridescenceThicknessMinimum ?? 100),
+ iridescenceThicknessMaximum = Math.max(
+ schema.iridescenceThicknessMinimum ?? 100,
+ schema.iridescenceThicknessMaximum ?? 400
+ ),📝 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.
| iridescenceThicknessMinimum = 100, | |
| iridescenceThicknessMaximum = 400, | |
| iridescenceThicknessMinimum = Math.max(0, schema.iridescenceThicknessMinimum ?? 100), | |
| iridescenceThicknessMaximum = Math.max( | |
| schema.iridescenceThicknessMinimum ?? 100, | |
| schema.iridescenceThicknessMaximum ?? 400 | |
| ), |
| const { | ||
| iridescenceFactor = 0, | ||
| iridescenceTexture, | ||
| iridescenceIor = 1.3, | ||
| iridescenceThicknessMinimum = 100, | ||
| iridescenceThicknessMaximum = 400, | ||
| iridescenceThicknessTexture | ||
| } = schema; | ||
|
|
||
| material.iridescence = iridescenceFactor; | ||
| material.iridescenceIOR = iridescenceIor; | ||
| material.iridescenceThicknessRange.set(iridescenceThicknessMinimum, iridescenceThicknessMaximum); | ||
|
|
||
| if (iridescenceTexture) { | ||
| GLTFMaterialParser._checkOtherTextureTransform(iridescenceTexture, "Iridescence texture"); | ||
|
|
||
| context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => { | ||
| material.iridescenceTexture = texture; | ||
| }); | ||
| } | ||
| if (iridescenceThicknessTexture) { | ||
| GLTFMaterialParser._checkOtherTextureTransform(iridescenceThicknessTexture, "IridescenceThickness texture"); | ||
|
|
||
| context.get<Texture2D>(GLTFParserType.Texture, iridescenceThicknessTexture.index).then((texture) => { | ||
| material.iridescenceThicknessTexture = texture; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Test coverage needed for KHR_materials_iridescence extension
The codebase shows that GLTF material extensions are primarily tested through integration tests in GLTFLoader.test.ts, but KHR_materials_iridescence is missing from these tests. A dedicated test suite should be added to cover:
- Default value initialization (iridescenceFactor=0, iridescenceIor=1.3, etc.)
- Texture loading scenarios for both iridescence and thickness textures
- Material property assignments
- Integration with the GLTFLoader
Reference the existing test patterns from other material extensions in tests/src/loader/GLTFLoader.test.ts which already cover KHR_materials_clearcoat, KHR_materials_unlit, and others.
🔗 Analysis chain
Add comprehensive test coverage
The static analysis indicates this extension lacks test coverage. This is critical for a parser implementation that handles material properties and async texture loading.
Let's verify the current test coverage:
Would you like me to help generate a test suite that covers:
- Default value handling
- Texture loading success/failure scenarios
- Input validation
- Edge cases for thickness values and IOR
Let me check the test coverage more thoroughly by examining the test files for material extensions and the implementation details.
Let me try a different approach to check for GLTF extension tests and material-related test patterns.
Let me check specifically for tests of other KHR material extensions to understand the testing patterns used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing tests
fd -e test.ts -e spec.ts . | grep -i "iridescence"
# Check if similar extensions have tests we can reference
fd -e test.ts -e spec.ts . | grep -i "material.*test"
Length of output: 480
Script:
#!/bin/bash
# Check for GLTF material extension tests
fd -e test.ts -e spec.ts . | grep -i "gltf.*material"
# Look at the implementation of other material extension tests
rg -l "describe.*material.*extension" -g "*.test.ts" -g "*.spec.ts"
# Check the structure of KHR_materials_iridescence implementation
ast-grep --pattern 'export class KHR_materials_iridescence {
$$$
}'
Length of output: 197
Script:
#!/bin/bash
# Check for any GLTF extension tests
rg -g "*.test.ts" -g "*.spec.ts" "KHR_materials" -A 5
# Look for test patterns in material-related tests
rg -g "*.test.ts" "describe\(.*Material" -A 2 tests/src/core/material/
# Check the actual implementation directory for other extension tests
fd . packages/loader/src/gltf/extensions/ -t f
Length of output: 5399
Script:
#!/bin/bash
# Check for test files of other KHR material extensions
rg -g "*.test.ts" -g "*.spec.ts" "KHR_materials_(clearcoat|unlit|transmission)" -B 2 -A 10
# Look at the implementation of a similar extension test
rg "class KHR_materials_clearcoat" -A 10 packages/loader/src/gltf/extensions/KHR_materials_clearcoat.ts
# Check if there's a test directory for GLTF extensions
fd . tests/src/loader/gltf/extensions/ -t d
Length of output: 3854
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 11-38: packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts#L11-L38
Added lines #L11 - L38 were not covered by tests
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