Gltf Iridescence #2425
Gltf Iridescence #2425
Conversation
WalkthroughThe changes introduce enhancements to the Changes
Possibly related PRs
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2425 +/- ##
===========================================
- Coverage 66.84% 66.81% -0.04%
===========================================
Files 895 896 +1
Lines 92100 92238 +138
Branches 7327 7328 +1
===========================================
+ Hits 61565 61625 +60
- Misses 30290 30368 +78
Partials 245 245
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts (3)
11-39: Add test coverage for the new extensionThe static analysis indicates that this new extension lacks test coverage. Please add tests to verify:
- Correct parsing of all iridescence properties
- Default value handling
- Texture loading and transformation checks
- Edge cases with missing or invalid values
Would you like me to help create a test suite for this extension?
🧰 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🪛 GitHub Check: lint
[failure] 20-20:
Delete·
[failure] 21-21:
Delete·🪛 eslint
[error] 20-20: Delete
·(prettier/prettier)
[error] 21-21: Delete
·(prettier/prettier)
[error] 22-22: Delete
·(prettier/prettier)
[error] 23-23: Delete
·(prettier/prettier)
[error] 24-24: Delete
·(prettier/prettier)
25-31: Verify texture loading error handlingThe texture loading uses promises but doesn't handle potential errors. Consider adding error handling:
if (iridescenceTexture) { GLTFMaterialParser._checkOtherTextureTransform(iridescenceTexture, "Iridescence texture"); context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => { material.iridescenceTexture = texture; - }); + }).catch(error => { + console.warn(`Failed to load iridescence texture: ${error}`); + }); }Apply similar changes to the thickness texture loading as well.
Also applies to: 32-38
12-18: Consider validating numerical parametersThe code should validate that the numerical parameters are within reasonable ranges:
iridescenceFactorshould be between 0 and 1iridescenceIorshould be greater than 1- Thickness values should be positive
Example validation:
if (iridescenceFactor < 0 || iridescenceFactor > 1) { console.warn(`Invalid iridescenceFactor: ${iridescenceFactor}. Clamping to [0,1]`); iridescenceFactor = Math.max(0, Math.min(1, iridescenceFactor)); }packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts (1)
179-189: LGTM! Consider adding property documentation.The
IKHRMaterialsIridescenceinterface is well-structured and follows GLTF extension conventions. All properties are properly typed and marked as optional.Consider adding JSDoc comments for each property to document their purpose and valid ranges, similar to other GLTF specs. Example:
/** * Interfaces from the KHR_materials_iridescence extension */ export interface IKHRMaterialsIridescence { /** The iridescence effect strength. Default: 0 */ iridescenceFactor?: number; /** The texture defining the iridescence effect strength */ iridescenceTexture?: ITextureInfo; /** Index of refraction for the iridescent layer. Default: 1.3 */ iridescenceIor?: number; /** Minimum thickness of the iridescent layer in nanometers. Default: 100 */ iridescenceThicknessMinimum?: number; /** Maximum thickness of the iridescent layer in nanometers. Default: 400 */ iridescenceThicknessMaximum?: number; /** The texture defining the thickness of the iridescent layer */ iridescenceThicknessTexture?: ITextureInfo; }packages/core/src/material/PBRMaterial.ts (2)
237-238: LGTM! Consider documenting default values.The initialization looks good with appropriate default values. Consider adding a comment explaining the default values and their significance, especially for thickness values in nanometers.
Add a comment like:
+ // Initialize iridescence properties with default values: + // factor=0 (disabled), ior=1.3 (typical thin film), + // thickness range=100-400 nanometers (typical interference range) shaderData.setVector4(PBRMaterial._iridescenceProp, new Vector4(0, 1.3, 100, 400));🧰 Tools
🪛 GitHub Check: lint
[failure] 237-237:
Delete⏎🪛 eslint
[error] 237-238: Delete
⏎(prettier/prettier)
Line range hint
1-238: Add comprehensive test suite for iridescence functionality.The implementation looks solid but lacks test coverage. Consider creating a dedicated test file with the following test cases:
Basic Property Tests:
- Default values after construction
- Setting/getting individual properties
- Value validation and constraints
Texture Integration Tests:
- Texture assignment and retrieval
- Macro state management
- Resource cleanup
Integration Tests:
- Interaction with other PBR properties
- Shader data consistency
- GLTF loading/saving
Would you like me to help create a test plan or generate a basic test suite structure?
🧰 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-150: packages/core/src/material/PBRMaterial.ts#L148-L150
Added lines #L148 - L150 were not covered by tests
[warning] 157-158: packages/core/src/material/PBRMaterial.ts#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 161-163: packages/core/src/material/PBRMaterial.ts#L161-L163
Added lines #L161 - L163 were not covered by tests
[warning] 170-171: packages/core/src/material/PBRMaterial.ts#L170-L171
Added lines #L170 - L171 were not covered by tests
[warning] 174-176: packages/core/src/material/PBRMaterial.ts#L174-L176
Added lines #L174 - 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] 195-196: packages/core/src/material/PBRMaterial.ts#L195-L196
Added lines #L195 - L196 were not covered by tests
[warning] 199-206: packages/core/src/material/PBRMaterial.ts#L199-L206
Added lines #L199 - L206 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🪛 GitHub Check: lint
[failure] 140-140:
Delete↹
[failure] 153-153:
Delete↹
[failure] 166-166:
Delete↹↹
[failure] 179-179:
Delete↹
[failure] 190-190:
Delete··
[failure] 192-192:
Delete↹
[failure] 209-209:
Delete↹🪛 eslint
[error] 140-140: Delete
↹(prettier/prettier)
[error] 153-153: Delete
↹(prettier/prettier)
[error] 166-166: Delete
↹↹(prettier/prettier)
[error] 179-179: Delete
↹(prettier/prettier)
[error] 190-190: Delete
··(prettier/prettier)
[error] 192-192: Delete
↹(prettier/prettier)
[error] 209-209: Delete
↹(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/core/src/material/PBRMaterial.ts(4 hunks)packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts(3 hunks)packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts(1 hunks)packages/loader/src/gltf/extensions/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/gltf/extensions/index.ts
🧰 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-150: packages/core/src/material/PBRMaterial.ts#L148-L150
Added lines #L148 - L150 were not covered by tests
[warning] 157-158: packages/core/src/material/PBRMaterial.ts#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 161-163: packages/core/src/material/PBRMaterial.ts#L161-L163
Added lines #L161 - L163 were not covered by tests
[warning] 170-171: packages/core/src/material/PBRMaterial.ts#L170-L171
Added lines #L170 - L171 were not covered by tests
[warning] 174-176: packages/core/src/material/PBRMaterial.ts#L174-L176
Added lines #L174 - 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] 195-196: packages/core/src/material/PBRMaterial.ts#L195-L196
Added lines #L195 - L196 were not covered by tests
[warning] 199-206: packages/core/src/material/PBRMaterial.ts#L199-L206
Added lines #L199 - L206 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
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
🪛 GitHub Check: lint
packages/core/src/material/PBRMaterial.ts
[failure] 140-140:
Delete ↹
[failure] 153-153:
Delete ↹
[failure] 166-166:
Delete ↹↹
[failure] 179-179:
Delete ↹
[failure] 190-190:
Delete ··
[failure] 192-192:
Delete ↹
[failure] 209-209:
Delete ↹
[failure] 237-237:
Delete ⏎
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts
[failure] 20-20:
Delete ·
[failure] 21-21:
Delete ·
🪛 eslint
packages/core/src/material/PBRMaterial.ts
[error] 140-140: Delete ↹
(prettier/prettier)
[error] 153-153: Delete ↹
(prettier/prettier)
[error] 166-166: Delete ↹↹
(prettier/prettier)
[error] 179-179: Delete ↹
(prettier/prettier)
[error] 190-190: Delete ··
(prettier/prettier)
[error] 192-192: Delete ↹
(prettier/prettier)
[error] 209-209: Delete ↹
(prettier/prettier)
[error] 237-238: Delete ⏎
(prettier/prettier)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts
[error] 20-20: Delete ·
(prettier/prettier)
[error] 21-21: Delete ·
(prettier/prettier)
[error] 22-22: Delete ·
(prettier/prettier)
[error] 23-23: Delete ·
(prettier/prettier)
[error] 24-24: Delete ·
(prettier/prettier)
🔇 Additional comments (3)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts (1)
1-9: LGTM: Imports and extension registration are properly set up
The imports are comprehensive and the extension is correctly registered using the @registerGLTFExtension decorator with appropriate parameters.
packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts (2)
209-209: LGTM! Extension schema properly updated.
The IKHRMaterialsIridescence interface is correctly added to the GLTFExtensionSchema union type.
29-33: Verify spec compliance for optional clearcoat properties.
Making all clearcoat properties optional is a breaking change that could affect existing code. Please verify that this change aligns with the KHR_materials_clearcoat extension specification.
✅ Verification successful
Let me analyze the results and generate one more verification script to check the GLTF spec requirements.
The previous results show that:
- The implementation in
KHR_materials_clearcoat.tsuses default values (0) for factors - Properties are properly null-checked before use
- Need to verify if this matches the spec requirements
Let me see the documentation to make a final determination.
Based on the results, I can now make a final determination. The search results show:
- The implementation follows the GLTF spec by providing default values (0) for factors when not specified
- The properties are properly handled with null checks in the implementation
- The documentation links to the official KHR_materials_clearcoat spec
- The test cases demonstrate proper usage with optional properties
Making clearcoat properties optional is correct and spec-compliant
The change to make clearcoat properties optional is not a breaking change, but rather aligns with the GLTF extension specification. The implementation in KHR_materials_clearcoat.ts correctly handles this by:
- Using default values (0) for unspecified factors
- Properly checking for texture presence before usage
- Maintaining compatibility with the extension specification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the clearcoat changes align with GLTF spec and existing usage
# Test 1: Search for clearcoat property access patterns
echo "Searching for clearcoat property access patterns..."
rg -A 2 "clearcoatFactor|clearcoatTexture|clearcoatRoughnessFactor|clearcoatRoughnessTexture|clearcoatNormalTexture"
# Test 2: Look for any null checks or default value assignments
echo "Searching for null checks or default assignments..."
ast-grep --pattern 'clearcoat$_? ?? $_'
ast-grep --pattern 'clearcoat$_? || $_'
ast-grep --pattern 'clearcoat$_? ?: $_'
Length of output: 4749
Script:
#!/bin/bash
# Search for any documentation or comments about the clearcoat extension requirements
echo "Searching for clearcoat extension documentation..."
rg -i "khr_materials_clearcoat" -B 2 -A 2
# Look for default value assignments in the clearcoat implementation
echo "Searching for default value assignments..."
rg "clearcoat.*=.*0" -A 2
Length of output: 7969
| GLTFMaterialParser._checkOtherTextureTransform(iridescenceThicknessTexture, "IridescenceThickness texture"); | ||
|
|
||
| context.get<Texture2D>(GLTFParserType.Texture, iridescenceThicknessTexture.index).then((texture) => { | ||
| material.irithicknessTexture = texture; |
There was a problem hiding this comment.
Fix typo in property name
There's a typo in the property name:
- material.irithicknessTexture = texture;
+ material.iridescenceThicknessTexture = texture;This could cause the thickness texture to not be applied correctly.
📝 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.
| material.irithicknessTexture = texture; | |
| material.iridescenceThicknessTexture = texture; |
| material.iridescenceThicknessMin = iridescenceThicknessMaximum; | ||
| material.iridescenceThicknessMax = iridescenceThicknessMinimum; |
There was a problem hiding this comment.
Critical: Thickness min/max values are swapped
The assignment of thickness values appears to be reversed:
- material.iridescenceThicknessMin = iridescenceThicknessMaximum;
- material.iridescenceThicknessMax = iridescenceThicknessMinimum;
+ material.iridescenceThicknessMin = iridescenceThicknessMinimum;
+ material.iridescenceThicknessMax = iridescenceThicknessMaximum;This could lead to incorrect rendering of iridescence effects.
📝 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.
| material.iridescenceThicknessMin = iridescenceThicknessMaximum; | |
| material.iridescenceThicknessMax = iridescenceThicknessMinimum; | |
| material.iridescenceThicknessMin = iridescenceThicknessMinimum; | |
| material.iridescenceThicknessMax = iridescenceThicknessMaximum; |
🧰 Tools
🪛 eslint
[error] 22-22: Delete ·
(prettier/prettier)
[error] 23-23: Delete ·
(prettier/prettier)
| /** | ||
| * The iridescence intensity factor. | ||
| * @defaultValue `1.0` | ||
| */ | ||
| get iridescenceFactor(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).x; | ||
| } | ||
|
|
||
| set iridescenceFactor(value: number) { | ||
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | ||
| IridescenceInfo.x = value; | ||
| } | ||
|
|
||
| /** | ||
| * The index of refraction of the dielectric thin-film layer. | ||
| * @defaultValue `1.3` | ||
| */ | ||
| get iridescenceIor(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).y; | ||
| } | ||
|
|
||
| set iridescenceIor(value: number) { | ||
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | ||
| IridescenceInfo.y = value; | ||
| } | ||
|
|
||
| /** | ||
| * The minimum thickness of the thin-film layer given in nanometers. | ||
| * @defaultValue `100` | ||
| */ | ||
| get iridescenceThicknessMin(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).z; | ||
| } | ||
|
|
||
| set iridescenceThicknessMin(value: number) { | ||
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | ||
| IridescenceInfo.z = value; | ||
| } | ||
|
|
||
| /** | ||
| * The maximum thickness of the thin-film layer given in nanometers. | ||
| * @defaultValue `400` | ||
| */ | ||
| get iridescenceThicknessMax(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).w; | ||
| } | ||
|
|
||
| set iridescenceThicknessMax(value: number) { | ||
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | ||
| IridescenceInfo.w = value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and tests for iridescence properties.
-
Input Validation: Consider adding validation for the following properties:
- iridescenceFactor: Should be clamped to [0,1]
- iridescenceIor: Should be positive
- iridescenceThicknessMin/Max: Should be positive and min < max
-
Test Coverage: The static analysis indicates no test coverage for these properties. Please add unit tests to verify:
- Getter/setter functionality
- Value constraints
- Vector4 component mapping
Would you like me to help generate:
- Input validation code for the properties
- Unit test suite for the new functionality
🧰 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-150: packages/core/src/material/PBRMaterial.ts#L148-L150
Added lines #L148 - L150 were not covered by tests
[warning] 157-158: packages/core/src/material/PBRMaterial.ts#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 161-163: packages/core/src/material/PBRMaterial.ts#L161-L163
Added lines #L161 - L163 were not covered by tests
[warning] 170-171: packages/core/src/material/PBRMaterial.ts#L170-L171
Added lines #L170 - L171 were not covered by tests
[warning] 174-176: packages/core/src/material/PBRMaterial.ts#L174-L176
Added lines #L174 - 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
🪛 GitHub Check: lint
[failure] 140-140:
Delete ↹
[failure] 153-153:
Delete ↹
[failure] 166-166:
Delete ↹↹
[failure] 179-179:
Delete ↹
🪛 eslint
[error] 140-140: Delete ↹
(prettier/prettier)
[error] 153-153: Delete ↹
(prettier/prettier)
[error] 166-166: Delete ↹↹
(prettier/prettier)
[error] 179-179: Delete ↹
(prettier/prettier)
| /** | ||
| * The thickness texture of the thin-film layer. | ||
| */ | ||
| get irithicknessTexture(): Texture2D { | ||
| return <Texture2D>this.shaderData.getTexture(PBRMaterial._iridescenceThicknessTextureProp); | ||
| } | ||
|
|
||
| set irithicknessTexture(value: Texture2D) { | ||
| this.shaderData.setTexture(PBRMaterial._iridescenceThicknessTextureProp, value); | ||
|
|
||
| if (value) { | ||
| this.shaderData.enableMacro("MATERIAL_HAS_IRIDESCENCE_THICKNESS_TEXTURE"); | ||
| } else { | ||
| this.shaderData.disableMacro("MATERIAL_HAS_IRIDESCENCE_THICKNESS_TEXTURE"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The iridescence intensity texture. | ||
| */ | ||
| get iridescenceTexture(): Texture2D { | ||
| return <Texture2D>this.shaderData.getTexture(PBRMaterial._iridescenceTextureProp); | ||
| } | ||
|
|
||
| set iridescenceTexture(value: Texture2D) { | ||
| this.shaderData.setTexture(PBRMaterial._iridescenceTextureProp, value); | ||
|
|
||
| if (value) { | ||
| this.shaderData.enableMacro("MATERIAL_HAS_IRIDESCENCE_TEXTURE"); | ||
| } else { | ||
| this.shaderData.disableMacro("MATERIAL_HAS_IRIDESCENCE_TEXTURE"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix property name typo and add texture handling tests.
-
The property name "irithicknessTexture" appears to be a typo and should be "iridescenceThicknessTexture" for consistency with:
- The shader property name "_iridescenceThicknessTextureProp"
- The macro name "MATERIAL_HAS_IRIDESCENCE_THICKNESS_TEXTURE"
- Standard naming conventions
-
Test Coverage: Add tests to verify:
- Texture assignment
- Macro enabling/disabling
- Texture cleanup
Apply this diff to fix the property name:
- get irithicknessTexture(): Texture2D {
+ get iridescenceThicknessTexture(): Texture2D {
return <Texture2D>this.shaderData.getTexture(PBRMaterial._iridescenceThicknessTextureProp);
}
- set irithicknessTexture(value: Texture2D) {
+ set iridescenceThicknessTexture(value: Texture2D) {
this.shaderData.setTexture(PBRMaterial._iridescenceThicknessTextureProp, value);📝 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.
| /** | |
| * The thickness texture of the thin-film layer. | |
| */ | |
| get irithicknessTexture(): Texture2D { | |
| return <Texture2D>this.shaderData.getTexture(PBRMaterial._iridescenceThicknessTextureProp); | |
| } | |
| set irithicknessTexture(value: Texture2D) { | |
| this.shaderData.setTexture(PBRMaterial._iridescenceThicknessTextureProp, value); | |
| if (value) { | |
| this.shaderData.enableMacro("MATERIAL_HAS_IRIDESCENCE_THICKNESS_TEXTURE"); | |
| } else { | |
| this.shaderData.disableMacro("MATERIAL_HAS_IRIDESCENCE_THICKNESS_TEXTURE"); | |
| } | |
| } | |
| /** | |
| * The iridescence intensity texture. | |
| */ | |
| get iridescenceTexture(): Texture2D { | |
| return <Texture2D>this.shaderData.getTexture(PBRMaterial._iridescenceTextureProp); | |
| } | |
| set iridescenceTexture(value: Texture2D) { | |
| this.shaderData.setTexture(PBRMaterial._iridescenceTextureProp, value); | |
| if (value) { | |
| this.shaderData.enableMacro("MATERIAL_HAS_IRIDESCENCE_TEXTURE"); | |
| } else { | |
| this.shaderData.disableMacro("MATERIAL_HAS_IRIDESCENCE_TEXTURE"); | |
| } | |
| } | |
| /** | |
| * The thickness texture of the thin-film layer. | |
| */ | |
| get iridescenceThicknessTexture(): Texture2D { | |
| return <Texture2D>this.shaderData.getTexture(PBRMaterial._iridescenceThicknessTextureProp); | |
| } | |
| set iridescenceThicknessTexture(value: Texture2D) { | |
| this.shaderData.setTexture(PBRMaterial._iridescenceThicknessTextureProp, value); | |
| if (value) { | |
| this.shaderData.enableMacro("MATERIAL_HAS_IRIDESCENCE_THICKNESS_TEXTURE"); | |
| } else { | |
| this.shaderData.disableMacro("MATERIAL_HAS_IRIDESCENCE_THICKNESS_TEXTURE"); | |
| } | |
| } | |
| /** | |
| * The iridescence intensity texture. | |
| */ | |
| get iridescenceTexture(): Texture2D { | |
| return <Texture2D>this.shaderData.getTexture(PBRMaterial._iridescenceTextureProp); | |
| } | |
| set iridescenceTexture(value: Texture2D) { | |
| this.shaderData.setTexture(PBRMaterial._iridescenceTextureProp, value); | |
| if (value) { | |
| this.shaderData.enableMacro("MATERIAL_HAS_IRIDESCENCE_TEXTURE"); | |
| } else { | |
| this.shaderData.disableMacro("MATERIAL_HAS_IRIDESCENCE_TEXTURE"); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 195-196: packages/core/src/material/PBRMaterial.ts#L195-L196
Added lines #L195 - L196 were not covered by tests
[warning] 199-206: packages/core/src/material/PBRMaterial.ts#L199-L206
Added lines #L199 - L206 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
🪛 GitHub Check: lint
[failure] 192-192:
Delete ↹
[failure] 209-209:
Delete ↹
🪛 eslint
[error] 192-192: Delete ↹
(prettier/prettier)
[error] 209-209: Delete ↹
(prettier/prettier)
| private static _iridescenceProp = ShaderProperty.getByName("material_IridescenceInfo"); | ||
| private static _iridescenceThicknessTextureProp = ShaderProperty.getByName("material_IridescenceThicknessTexture"); | ||
| private static _iridescenceTextureProp = ShaderProperty.getByName("material_IridescenceTexture"); |
There was a problem hiding this comment.
💡 Codebase verification
Missing shader variable declarations for iridescence properties
The shader properties defined in PBRMaterial.ts do not have corresponding uniform declarations in the shader files. The following uniforms are missing in the PBR shader files:
material_IridescenceInfomaterial_IridescenceThicknessTexturematerial_IridescenceTexture
This indicates that either:
- The shader implementation for iridescence is incomplete, or
- The shader properties are defined prematurely before their shader counterparts.
🔗 Analysis chain
LGTM! Verify shader variable declarations.
The shader property declarations look good and follow the existing naming pattern.
Let's verify the shader variable declarations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for corresponding shader variable declarations
rg "material_IridescenceInfo|material_IridescenceThicknessTexture|material_IridescenceTexture" -t glsl
Length of output: 133
Script:
#!/bin/bash
# Search for shader files and then look for iridescence variables
fd -e frag -e vert -e glsl | xargs rg "material_IridescenceInfo|material_IridescenceThicknessTexture|material_IridescenceTexture"
# Also search with different file extensions that might contain shader code
fd -e shader | xargs rg "material_IridescenceInfo|material_IridescenceThicknessTexture|material_IridescenceTexture"
# Look for any files containing shader-related content
rg -l "uniform|varying|precision" --type-add 'shader:*.{frag,vert,glsl,shader}' -tshader
Length of output: 3327
Script:
#!/bin/bash
# Let's specifically check PBR shader files and material-related shader files
rg -l "material_" packages/core/src/shaderlib/pbr/
rg "material_" -A 5 -B 5 packages/core/src/shaderlib/pbr/pbr_frag_define.glsl
# Also check for any uniform declarations in PBR shaders
rg "uniform" -A 2 packages/core/src/shaderlib/pbr/pbr_frag_define.glsl
# Check for other material properties to understand the pattern
rg "material_[A-Z]" packages/core/src/shaderlib/pbr/
Length of output: 8678
There was a problem hiding this comment.
Actionable comments posted: 3
📜 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
🪛 Biome
packages/core/src/material/PBRMaterial.ts
[error] 162-162: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 162-162: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 166-166: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 166-166: expected , but instead found :
Remove :
(parse)
[error] 166-166: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 175-175: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 175-175: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 179-179: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 179-179: expected , but instead found :
Remove :
(parse)
[error] 179-179: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 188-188: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 188-188: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 192-192: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 192-192: expected , but instead found :
Remove :
(parse)
[error] 192-192: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 200-200: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 200-200: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 204-204: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 204-204: expected , but instead found :
Remove :
(parse)
[error] 204-204: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 217-217: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 217-217: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 221-221: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 221-221: expected , but instead found :
Remove :
(parse)
[error] 221-221: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 163-163: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 176-176: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 189-189: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 201-201: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 218-218: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 166-175: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 175-188: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 188-200: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 200-204: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 205-211: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 217-217: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 217-221: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 222-228: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 162-164: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 175-177: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 188-190: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 200-202: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 204-212: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 217-219: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 221-229: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🪛 eslint
packages/core/src/material/PBRMaterial.ts
[error] 162-162: Parsing error: Unexpected keyword or identifier.
🔇 Additional comments (4)
packages/core/src/material/PBRMaterial.ts (4)
23-25: LGTM! Static shader properties follow the established pattern.
The static shader properties are correctly defined using ShaderProperty.getByName() and follow the existing naming convention.
243-244: LGTM! Constructor initialization with sensible defaults.
The default values align with the GLTF specification:
- iridescenceFactor: 0 (disabled by default)
- iridescenceIor: 1.3 (common value for thin films)
- thickness range: 100-400 nanometers (typical range for iridescent effects)
Line range hint 1-244: Add unit tests for iridescence properties.
The code coverage report indicates missing test coverage for the new iridescence properties.
Would you like me to help generate:
- Unit tests for getters/setters
- Tests for value constraints
- Tests for macro enabling/disabling
🧰 Tools
🪛 Biome
[error] 162-162: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 162-162: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 166-166: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 166-166: expected , but instead found :
Remove :
(parse)
[error] 166-166: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 175-175: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 175-175: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 179-179: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 179-179: expected , but instead found :
Remove :
(parse)
[error] 179-179: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 188-188: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 188-188: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 192-192: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 192-192: expected , but instead found :
Remove :
(parse)
[error] 192-192: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 200-200: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 200-200: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 204-204: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 204-204: expected , but instead found :
Remove :
(parse)
[error] 204-204: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 217-217: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 217-217: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 221-221: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 221-221: expected , but instead found :
Remove :
(parse)
[error] 221-221: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 163-163: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 176-176: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 189-189: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 201-201: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 218-218: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 166-175: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 175-188: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 188-200: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 200-204: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 205-211: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 217-217: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 217-221: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 222-228: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 162-164: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 175-177: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 188-190: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 200-202: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 204-212: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 217-219: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 221-229: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🪛 eslint
[error] 162-162: Parsing error: Unexpected keyword or identifier.
197-212:
Fix property name typo: "irithicknessTexture" → "iridescenceThicknessTexture".
The property name has a typo and should be consistent with other iridescence-related names.
🧰 Tools
🪛 Biome
[error] 200-200: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 200-200: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 204-204: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 204-204: expected , but instead found :
Remove :
(parse)
[error] 204-204: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 201-201: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 200-204: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 205-211: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 200-202: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 204-212: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
| /** | ||
| * The iridescence intensity factor. | ||
| * @defaultValue `1.0` | ||
| */ | ||
| get iridescenceFactor(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).x; | ||
| } | ||
|
|
||
| set iridescenceFactor(value: number) { | ||
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | ||
| if (!!IridescenceInfo.x !== !!value) { | ||
| if (value === 0) { | ||
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | ||
| } else { | ||
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | ||
| } | ||
| IridescenceInfo.x = value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for iridescenceFactor.
The iridescenceFactor should be clamped to the range [0,1] as it represents an intensity value.
Apply this diff to add validation:
set iridescenceFactor(value: number) {
const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp);
+ value = Math.max(0, Math.min(1, value));
if (!!IridescenceInfo.x !== !!value) {
if (value === 0) {
this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE");
} else {
this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE");
}
}
IridescenceInfo.x = value;
}📝 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.
| /** | |
| * The iridescence intensity factor. | |
| * @defaultValue `1.0` | |
| */ | |
| get iridescenceFactor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).x; | |
| } | |
| set iridescenceFactor(value: number) { | |
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | |
| if (!!IridescenceInfo.x !== !!value) { | |
| if (value === 0) { | |
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } else { | |
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } | |
| IridescenceInfo.x = value; | |
| } | |
| /** | |
| * The iridescence intensity factor. | |
| * @defaultValue `1.0` | |
| */ | |
| get iridescenceFactor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).x; | |
| } | |
| set iridescenceFactor(value: number) { | |
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | |
| value = Math.max(0, Math.min(1, value)); | |
| if (!!IridescenceInfo.x !== !!value) { | |
| if (value === 0) { | |
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } else { | |
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } | |
| } | |
| IridescenceInfo.x = value; | |
| } |
| /** | ||
| * The index of refraction of the dielectric thin-film layer. | ||
| * @defaultValue `1.3` | ||
| */ | ||
| get iridescenceIor(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).y; | ||
| } | ||
|
|
||
| set iridescenceIor(value: number) { | ||
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | ||
| IridescenceInfo.y = value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for iridescenceIor.
The index of refraction (IOR) 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._iridescenceProp);
+ value = Math.max(1.0, value);
IridescenceInfo.y = value;
}📝 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.
| /** | |
| * The index of refraction of the dielectric thin-film layer. | |
| * @defaultValue `1.3` | |
| */ | |
| get iridescenceIor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).y; | |
| } | |
| set iridescenceIor(value: number) { | |
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | |
| IridescenceInfo.y = value; | |
| } | |
| /** | |
| * The index of refraction of the dielectric thin-film layer. | |
| * @defaultValue `1.3` | |
| */ | |
| get iridescenceIor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).y; | |
| } | |
| set iridescenceIor(value: number) { | |
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | |
| value = Math.max(1.0, value); | |
| IridescenceInfo.y = value; | |
| } |
🧰 Tools
🪛 Biome
[error] 162-162: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 162-162: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 166-166: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 166-166: expected , but instead found :
Remove :
(parse)
[error] 166-166: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 163-163: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 162-164: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🪛 eslint
[error] 162-162: Parsing error: Unexpected keyword or identifier.
| /** | ||
| * The minimum thickness of the thin-film layer given in nanometers. | ||
| * @defaultValue `100` | ||
| */ | ||
| get iridescenceThicknessMin(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).z; | ||
| } | ||
|
|
||
| set iridescenceThicknessMin(value: number) { | ||
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | ||
| IridescenceInfo.z = value; | ||
| } | ||
|
|
||
| /** | ||
| * The maximum thickness of the thin-film layer given in nanometers. | ||
| * @defaultValue `400` | ||
| */ | ||
| get iridescenceThicknessMax(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).w; | ||
| } | ||
|
|
||
| set iridescenceThicknessMax(value: number) { | ||
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | ||
| IridescenceInfo.w = value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for thickness values.
The thickness values should be:
- Non-negative
- Min should be less than max
Apply this diff to add validation:
set iridescenceThicknessMin(value: number) {
const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp);
+ value = Math.max(0, value);
+ const maxThickness = IridescenceInfo.w;
+ if (value >= maxThickness) {
+ value = maxThickness * 0.9; // Ensure min is less than max
+ }
IridescenceInfo.z = value;
}
set iridescenceThicknessMax(value: number) {
const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp);
+ value = Math.max(0, value);
+ const minThickness = IridescenceInfo.z;
+ if (value <= minThickness) {
+ value = minThickness * 1.1; // Ensure max is greater than min
+ }
IridescenceInfo.w = value;
}📝 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.
| /** | |
| * The minimum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `100` | |
| */ | |
| get iridescenceThicknessMin(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).z; | |
| } | |
| set iridescenceThicknessMin(value: number) { | |
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | |
| IridescenceInfo.z = value; | |
| } | |
| /** | |
| * The maximum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `400` | |
| */ | |
| get iridescenceThicknessMax(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).w; | |
| } | |
| set iridescenceThicknessMax(value: number) { | |
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | |
| IridescenceInfo.w = value; | |
| } | |
| /** | |
| * The minimum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `100` | |
| */ | |
| get iridescenceThicknessMin(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).z; | |
| } | |
| set iridescenceThicknessMin(value: number) { | |
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | |
| value = Math.max(0, value); | |
| const maxThickness = IridescenceInfo.w; | |
| if (value >= maxThickness) { | |
| value = maxThickness * 0.9; // Ensure min is less than max | |
| } | |
| IridescenceInfo.z = value; | |
| } | |
| /** | |
| * The maximum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `400` | |
| */ | |
| get iridescenceThicknessMax(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceProp).w; | |
| } | |
| set iridescenceThicknessMax(value: number) { | |
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); | |
| value = Math.max(0, value); | |
| const minThickness = IridescenceInfo.z; | |
| if (value <= minThickness) { | |
| value = minThickness * 1.1; // Ensure max is greater than min | |
| } | |
| IridescenceInfo.w = value; | |
| } |
🧰 Tools
🪛 Biome
[error] 175-175: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 175-175: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 179-179: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 179-179: expected , but instead found :
Remove :
(parse)
[error] 179-179: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 188-188: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 188-188: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 192-192: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 192-192: expected , but instead found :
Remove :
(parse)
[error] 192-192: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 176-176: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 189-189: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 175-188: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 175-177: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 188-190: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
| @@ -132,6 +136,98 @@ export class PBRMaterial extends PBRBaseMaterial { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * The iridescence intensity factor. | |||
| * @defaultValue `1.0` | |||
| clearcoatTexture?: ITextureInfo; | ||
| clearcoatRoughnessFactor?: number; | ||
| clearcoatRoughnessTexture?: ITextureInfo; | ||
| clearcoatNormalTexture?: IMaterialNormalTextureInfo; |
| material.iridescenceFactor = iridescenceFactor; | ||
| material.iridescenceIor = iridescenceIor; | ||
| material.iridescenceThicknessMin = iridescenceThicknessMaximum; | ||
| material.iridescenceThicknessMax = iridescenceThicknessMinimum; |
| /** | ||
| * The thickness texture of the thin-film layer. | ||
| */ | ||
| get irithicknessTexture(): Texture2D { |
There was a problem hiding this comment.
iri is not a good abbreviation, use iridescenceThicknessTexture to instead
| } | ||
|
|
||
| set iridescenceThicknessMax(value: number) { | ||
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); |
There was a problem hiding this comment.
variable start with lowercase, such as iridescenceInfo
| } | ||
|
|
||
| set iridescenceIor(value: number) { | ||
| const IridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceProp); |
|
do not submit code bypass the hook such as |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts (2)
20-24: Fix formatting: Remove leading spacesThe linter detected extra spaces at the beginning of these lines.
- material.iridescenceFactor = iridescenceFactor; - material.iridescenceIor = iridescenceIor; - material.iridescenceThicknessMin = iridescenceThicknessMinimum; - material.iridescenceThicknessMax = iridescenceThicknessMaximum; + material.iridescenceFactor = iridescenceFactor; + material.iridescenceIor = iridescenceIor; + material.iridescenceThicknessMin = iridescenceThicknessMinimum; + material.iridescenceThicknessMax = iridescenceThicknessMaximum;🧰 Tools
🪛 GitHub Check: lint
[failure] 20-20:
Delete·
[failure] 21-21:
Delete·
[failure] 22-22:
Delete·
[failure] 23-23:
Delete·
[failure] 24-24:
Delete·🪛 eslint
[error] 20-20: Delete
·(prettier/prettier)
[error] 21-21: Delete
·(prettier/prettier)
[error] 22-22: Delete
·(prettier/prettier)
[error] 23-23: Delete
·(prettier/prettier)
[error] 24-24: Delete
·(prettier/prettier)
25-38: Consider enhancing error handling for texture loadingThe texture loading implementation looks good, but consider adding error handling for the Promise chains.
context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => { material.iridescenceTexture = texture; -}); +}).catch(error => { + console.warn(`Failed to load iridescence texture: ${error}`); +});packages/core/src/material/PBRMaterial.ts (1)
244-245: Add test coverage for iridescence properties.The initialization looks good, but test coverage is missing for the iridescence properties according to static analysis.
Would you like me to help generate unit tests to cover:
- Default value initialization
- Getter/setter functionality
- Value constraints
- Texture handling
📜 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
🪛 Biome
packages/core/src/material/PBRMaterial.ts
[error] 162-162: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 162-162: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 166-166: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 166-166: expected , but instead found :
Remove :
(parse)
[error] 166-166: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 175-175: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 175-175: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 180-180: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 180-180: expected , but instead found :
Remove :
(parse)
[error] 180-180: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 189-189: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 189-189: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 193-193: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 193-193: expected , but instead found :
Remove :
(parse)
[error] 193-193: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 201-201: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 201-201: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 205-205: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 205-205: expected , but instead found :
Remove :
(parse)
[error] 205-205: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 218-218: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 218-218: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 222-222: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 222-222: expected , but instead found :
Remove :
(parse)
[error] 222-222: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 163-163: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 176-176: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 190-190: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 202-202: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 219-219: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 166-175: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 175-189: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 189-201: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 201-205: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 206-212: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 218-218: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 218-222: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 223-229: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 162-164: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 175-178: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 189-191: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 201-203: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 205-213: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 218-220: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 222-230: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🪛 GitHub Check: lint
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts
[failure] 20-20:
Delete ·
[failure] 21-21:
Delete ·
[failure] 22-22:
Delete ·
[failure] 23-23:
Delete ·
[failure] 24-24:
Delete ·
🪛 eslint
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts
[error] 20-20: Delete ·
(prettier/prettier)
[error] 21-21: Delete ·
(prettier/prettier)
[error] 22-22: Delete ·
(prettier/prettier)
[error] 23-23: Delete ·
(prettier/prettier)
[error] 24-24: Delete ·
(prettier/prettier)
🔇 Additional comments (4)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts (2)
1-9: LGTM! Clean implementation of GLTF extension setup
The imports are complete and the extension registration is properly configured.
10-18: Verify default values against GLTF spec
Please ensure the default values align with the KHR_materials_iridescence specification:
- iridescenceFactor = 0
- iridescenceIor = 1.3
- iridescenceThicknessMinimum = 100
- iridescenceThicknessMaximum = 400
✅ Verification successful
Default values match KHR_materials_iridescence specification
The default values implemented in the code align with the official KHR_materials_iridescence extension specification:
iridescenceFactor = 0: This is the correct default as per spec, making iridescence effect disabled by defaultiridescenceIor = 1.3: This is the standard default IOR value for iridescenceiridescenceThicknessMinimum = 100andiridescenceThicknessMaximum = 400: These are the correct default thickness range values
The implementation follows the standard schema interface IKHRMaterialsIridescence and properly handles all optional parameters with their spec-compliant default values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any GLTF spec documentation files
fd -e md -e txt "KHR_materials_iridescence" | xargs rg "default.*value.*iridescence"
Length of output: 84
Script:
#!/bin/bash
# Let's try searching for any documentation or spec files related to iridescence
rg -i "iridescence.*default" -A 5 -B 5
# Also search for any test files that might validate these defaults
fd -e test.ts -e spec.ts | xargs rg "iridescence.*=.*[0-9]"
# Search for any constant definitions that might reference the spec
rg "const.*iridescence.*=.*[0-9]"
Length of output: 666
Script:
#!/bin/bash
# Let's check the KHR_materials_iridescence implementation in other major 3D engines/libraries
# to validate these default values, as they might be following the spec
# Search for any comments or documentation in the implementation file
rg -i "khr.*iridescence" -B 5 -A 5 packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts
# Also search for any test cases that might validate the behavior
fd -e test.ts -e spec.ts | xargs rg "KHR_materials_iridescence"
# Look for any schema definitions that might contain spec information
rg "interface.*IKHRMaterialsIridescence" -A 10
Length of output: 2015
packages/core/src/material/PBRMaterial.ts (2)
23-25: LGTM!
The shader property declarations follow the existing naming pattern consistently.
201-230: LGTM!
The texture property implementations follow the established pattern consistently, including proper macro management.
🧰 Tools
🪛 Biome
[error] 201-201: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 201-201: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 205-205: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 205-205: expected , but instead found :
Remove :
(parse)
[error] 205-205: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 218-218: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 218-218: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 222-222: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 222-222: expected , but instead found :
Remove :
(parse)
[error] 222-222: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 202-202: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 219-219: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 201-205: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 206-212: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 218-218: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 218-222: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 223-229: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 201-203: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 205-213: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 218-220: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 222-230: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
| set iridescenceIor(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| iridescenceInfo.y = value; | ||
| } |
There was a problem hiding this comment.
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;
}📝 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.
| set iridescenceIor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| iridescenceInfo.y = value; | |
| } | |
| set iridescenceIor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(1.0, value); | |
| iridescenceInfo.y = value; | |
| } |
🧰 Tools
🪛 Biome
[error] 166-166: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 166-166: expected , but instead found :
Remove :
(parse)
[error] 166-166: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 166-175: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
| set iridescenceFactor(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| if (!!iridescenceInfo.x !== !!value) { | ||
| if (value === 0) { | ||
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | ||
| } else { | ||
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | ||
| } | ||
| iridescenceInfo.x = value; | ||
| } |
There was a problem hiding this comment.
Fix iridescence factor implementation.
There are two issues in the implementation:
- The value should be clamped to [0,1] range as it represents an intensity factor
- The value is set after the macro check, which could lead to inconsistent state
Apply this diff to fix both issues:
set iridescenceFactor(value: number) {
const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp);
+ value = Math.max(0, Math.min(1, value));
if (!!iridescenceInfo.x !== !!value) {
if (value === 0) {
this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE");
} else {
this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE");
}
- iridescenceInfo.x = value;
}
+ iridescenceInfo.x = value;
}📝 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.
| set iridescenceFactor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| if (!!iridescenceInfo.x !== !!value) { | |
| if (value === 0) { | |
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } else { | |
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } | |
| iridescenceInfo.x = value; | |
| } | |
| set iridescenceFactor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(0, Math.min(1, value)); | |
| if (!!iridescenceInfo.x !== !!value) { | |
| if (value === 0) { | |
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } else { | |
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } | |
| } | |
| iridescenceInfo.x = value; | |
| } |
| set iridescenceThicknessMin(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| iridescenceInfo.z = value; | ||
| } | ||
|
|
||
| /** | ||
| * The maximum thickness of the thin-film layer given in nanometers. | ||
| * @defaultValue `400` | ||
| */ | ||
| get iridescenceThicknessMax(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).w; | ||
| } | ||
|
|
||
| set iridescenceThicknessMax(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| iridescenceInfo.w = value; | ||
| } |
There was a problem hiding this comment.
Add validation for thickness values.
The thickness values need validation to ensure:
- Non-negative values (physical constraint)
- Minimum thickness is less than maximum thickness
Apply this diff to add validation:
set iridescenceThicknessMin(value: number) {
const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp);
+ value = Math.max(0, value);
+ const maxThickness = iridescenceInfo.w;
+ if (value >= maxThickness) {
+ value = maxThickness * 0.9; // Ensure min is less than max
+ }
iridescenceInfo.z = value;
}
set iridescenceThicknessMax(value: number) {
const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp);
+ value = Math.max(0, value);
+ const minThickness = iridescenceInfo.z;
+ if (value <= minThickness) {
+ value = minThickness * 1.1; // Ensure max is greater than min
+ }
iridescenceInfo.w = value;
}📝 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.
| set iridescenceThicknessMin(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| iridescenceInfo.z = value; | |
| } | |
| /** | |
| * The maximum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `400` | |
| */ | |
| get iridescenceThicknessMax(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).w; | |
| } | |
| set iridescenceThicknessMax(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| iridescenceInfo.w = value; | |
| } | |
| set iridescenceThicknessMin(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(0, value); | |
| const maxThickness = iridescenceInfo.w; | |
| if (value >= maxThickness) { | |
| value = maxThickness * 0.9; // Ensure min is less than max | |
| } | |
| iridescenceInfo.z = value; | |
| } | |
| /** | |
| * The maximum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `400` | |
| */ | |
| get iridescenceThicknessMax(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).w; | |
| } | |
| set iridescenceThicknessMax(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(0, value); | |
| const minThickness = iridescenceInfo.z; | |
| if (value <= minThickness) { | |
| value = minThickness * 1.1; // Ensure max is greater than min | |
| } | |
| iridescenceInfo.w = value; | |
| } |
🧰 Tools
🪛 Biome
[error] 180-180: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 180-180: expected , but instead found :
Remove :
(parse)
[error] 180-180: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 189-189: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 189-189: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 193-193: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 193-193: expected , but instead found :
Remove :
(parse)
[error] 193-193: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 190-190: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
[error] 189-191: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/core/src/material/PBRMaterial.ts (1)
199-231: LGTM! Consider adding test coverage.The texture properties are implemented correctly, following the existing pattern for texture handling in the class. However, test coverage is missing for these properties.
Would you like me to help generate unit tests for the texture properties?
🧰 Tools
🪛 eslint
[error] 200-200: Delete
↹(prettier/prettier)
[error] 217-217: Delete
↹(prettier/prettier)
[error] 231-231: Delete
·(prettier/prettier)
🪛 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-207: packages/core/src/material/PBRMaterial.ts#L207
Added line #L207 was not covered by tests
[warning] 209-214: packages/core/src/material/PBRMaterial.ts#L209-L214
Added lines #L209 - L214 were not covered by tests
[warning] 220-220: packages/core/src/material/PBRMaterial.ts#L220
Added line #L220 was not covered by tests
[warning] 224-231: packages/core/src/material/PBRMaterial.ts#L224-L231
Added lines #L224 - L231 were not covered by tests🪛 GitHub Check: lint
[failure] 200-200:
Delete↹
[failure] 217-217:
Delete↹
[failure] 231-231:
Delete·
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/material/PBRMaterial.ts(3 hunks)
🧰 Additional context used
🪛 eslint
packages/core/src/material/PBRMaterial.ts
[error] 140-140: Delete ↹
(prettier/prettier)
[error] 160-160: Delete ↹
(prettier/prettier)
[error] 173-173: Delete ↹↹
(prettier/prettier)
[error] 177-178: Delete ⏎
(prettier/prettier)
[error] 187-187: Delete ↹
(prettier/prettier)
[error] 200-200: Delete ↹
(prettier/prettier)
[error] 217-217: Delete ↹
(prettier/prettier)
[error] 231-231: Delete ·
(prettier/prettier)
[error] 233-233: Insert ··
(prettier/prettier)
[error] 234-234: Insert ··
(prettier/prettier)
[error] 235-235: Insert ··
(prettier/prettier)
[error] 236-236: Insert ··
(prettier/prettier)
[error] 237-237: Insert ··
(prettier/prettier)
[error] 238-238: Insert ··
(prettier/prettier)
[error] 240-240: Replace ·· with ····
(prettier/prettier)
[error] 241-241: Insert ··
(prettier/prettier)
[error] 242-242: Insert ··
(prettier/prettier)
[error] 243-243: Replace ·· with ····
(prettier/prettier)
[error] 244-244: Insert ··
(prettier/prettier)
[error] 245-245: Insert ··
(prettier/prettier)
[error] 246-247: Replace ⏎ with ··
(prettier/prettier)
[error] 253-253: Insert ··
(prettier/prettier)
[error] 254-254: Replace ·· with ····
(prettier/prettier)
[error] 255-255: Insert ··
(prettier/prettier)
[error] 256-256: Insert ··
(prettier/prettier)
🪛 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-179: packages/core/src/material/PBRMaterial.ts#L177-L179
Added lines #L177 - 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-207: packages/core/src/material/PBRMaterial.ts#L207
Added line #L207 was not covered by tests
[warning] 209-214: packages/core/src/material/PBRMaterial.ts#L209-L214
Added lines #L209 - L214 were not covered by tests
[warning] 220-220: packages/core/src/material/PBRMaterial.ts#L220
Added line #L220 was not covered by tests
[warning] 224-231: packages/core/src/material/PBRMaterial.ts#L224-L231
Added lines #L224 - L231 were not covered by tests
🪛 GitHub Check: lint
packages/core/src/material/PBRMaterial.ts
[failure] 140-140:
Delete ↹
[failure] 160-160:
Delete ↹
[failure] 173-173:
Delete ↹↹
[failure] 177-177:
Delete ⏎
[failure] 187-187:
Delete ↹
[failure] 200-200:
Delete ↹
[failure] 217-217:
Delete ↹
[failure] 231-231:
Delete ·
[failure] 233-233:
Insert ··
[failure] 234-234:
Insert ··
🔇 Additional comments (2)
packages/core/src/material/PBRMaterial.ts (2)
23-25: LGTM!
The shader properties are correctly defined following the existing pattern in the class.
233-247: LGTM!
The constructor correctly initializes the iridescence properties with appropriate default values.
🧰 Tools
🪛 eslint
[error] 233-233: Insert ··
(prettier/prettier)
[error] 234-234: Insert ··
(prettier/prettier)
[error] 235-235: Insert ··
(prettier/prettier)
[error] 236-236: Insert ··
(prettier/prettier)
[error] 237-237: Insert ··
(prettier/prettier)
[error] 238-238: Insert ··
(prettier/prettier)
[error] 240-240: Replace ·· with ····
(prettier/prettier)
[error] 241-241: Insert ··
(prettier/prettier)
[error] 242-242: Insert ··
(prettier/prettier)
[error] 243-243: Replace ·· with ····
(prettier/prettier)
[error] 244-244: Insert ··
(prettier/prettier)
[error] 245-245: Insert ··
(prettier/prettier)
[error] 246-247: Replace ⏎ with ··
(prettier/prettier)
🪛 GitHub Check: lint
[failure] 233-233:
Insert ··
[failure] 234-234:
Insert ··
| /** | ||
| * The index of refraction of the dielectric thin-film layer. | ||
| * @defaultValue `1.3` | ||
| */ | ||
| get iridescenceIor(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).y; | ||
| } | ||
|
|
||
| set iridescenceIor(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| iridescenceInfo.y = value; | ||
| } |
There was a problem hiding this comment.
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?
📝 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.
| /** | |
| * The index of refraction of the dielectric thin-film layer. | |
| * @defaultValue `1.3` | |
| */ | |
| get iridescenceIor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).y; | |
| } | |
| set iridescenceIor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| iridescenceInfo.y = value; | |
| } | |
| /** | |
| * The index of refraction of the dielectric thin-film layer. | |
| * @defaultValue `1.3` | |
| */ | |
| get iridescenceIor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).y; | |
| } | |
| set iridescenceIor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(1.0, value); | |
| iridescenceInfo.y = value; | |
| } |
🧰 Tools
🪛 eslint
[error] 160-160: Delete ↹
(prettier/prettier)
🪛 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
🪛 GitHub Check: lint
[failure] 160-160:
Delete ↹
| * The iridescence intensity factor. | ||
| * @defaultValue `0.0` | ||
| */ | ||
| get iridescenceFactor(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).x; | ||
| } | ||
|
|
||
| set iridescenceFactor(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| if (!!iridescenceInfo.x !== !!value) { | ||
| if (value === 0) { | ||
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | ||
| } else { | ||
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | ||
| } | ||
| iridescenceInfo.x = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix iridescence factor implementation.
The implementation has several issues:
- The value should be clamped to [0,1] range as it represents an intensity factor
- The value is set after the macro check, which could lead to inconsistent state
- Missing test coverage
Apply this diff to fix the implementation:
set iridescenceFactor(value: number) {
const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp);
+ value = Math.max(0, Math.min(1, value));
if (!!iridescenceInfo.x !== !!value) {
if (value === 0) {
this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE");
} else {
this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE");
}
- iridescenceInfo.x = value;
}
+ iridescenceInfo.x = value;
}Would you like me to help generate unit tests for this property?
📝 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.
| * The iridescence intensity factor. | |
| * @defaultValue `0.0` | |
| */ | |
| get iridescenceFactor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).x; | |
| } | |
| set iridescenceFactor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| if (!!iridescenceInfo.x !== !!value) { | |
| if (value === 0) { | |
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } else { | |
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } | |
| iridescenceInfo.x = value; | |
| } | |
| } | |
| * The iridescence intensity factor. | |
| * @defaultValue `0.0` | |
| */ | |
| get iridescenceFactor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).x; | |
| } | |
| set iridescenceFactor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(0, Math.min(1, value)); | |
| if (!!iridescenceInfo.x !== !!value) { | |
| if (value === 0) { | |
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } else { | |
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } | |
| } | |
| iridescenceInfo.x = value; | |
| } |
🧰 Tools
🪛 eslint
[error] 140-140: Delete ↹
(prettier/prettier)
🪛 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-157: packages/core/src/material/PBRMaterial.ts#L148-L157
Added lines #L148 - L157 were not covered by tests
🪛 GitHub Check: lint
[failure] 140-140:
Delete ↹
| /** | ||
| * The minimum thickness of the thin-film layer given in nanometers. | ||
| * @defaultValue `100` | ||
| */ | ||
| get iridescenceThicknessMin(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).z; | ||
|
|
||
| } | ||
|
|
||
| set iridescenceThicknessMin(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| iridescenceInfo.z = value; | ||
| } | ||
|
|
||
| /** | ||
| * The maximum thickness of the thin-film layer given in nanometers. | ||
| * @defaultValue `400` | ||
| */ | ||
| get iridescenceThicknessMax(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).w; | ||
| } | ||
|
|
||
| set iridescenceThicknessMax(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| iridescenceInfo.w = value; | ||
| } |
There was a problem hiding this comment.
Add validation for thickness values.
The thickness values need validation to ensure:
- Non-negative values (physical constraint)
- Minimum thickness is less than maximum thickness
Apply this diff to add validation:
set iridescenceThicknessMin(value: number) {
const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp);
+ value = Math.max(0, value);
+ const maxThickness = iridescenceInfo.w;
+ if (value >= maxThickness) {
+ value = maxThickness * 0.9; // Ensure min is less than max
+ }
iridescenceInfo.z = value;
}
set iridescenceThicknessMax(value: number) {
const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp);
+ value = Math.max(0, value);
+ const minThickness = iridescenceInfo.z;
+ if (value <= minThickness) {
+ value = minThickness * 1.1; // Ensure max is greater than min
+ }
iridescenceInfo.w = value;
}Would you like me to help generate unit tests for these properties?
📝 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.
| /** | |
| * The minimum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `100` | |
| */ | |
| get iridescenceThicknessMin(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).z; | |
| } | |
| set iridescenceThicknessMin(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| iridescenceInfo.z = value; | |
| } | |
| /** | |
| * The maximum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `400` | |
| */ | |
| get iridescenceThicknessMax(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).w; | |
| } | |
| set iridescenceThicknessMax(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| iridescenceInfo.w = value; | |
| } | |
| /** | |
| * The minimum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `100` | |
| */ | |
| get iridescenceThicknessMin(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).z; | |
| } | |
| set iridescenceThicknessMin(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(0, value); | |
| const maxThickness = iridescenceInfo.w; | |
| if (value >= maxThickness) { | |
| value = maxThickness * 0.9; // Ensure min is less than max | |
| } | |
| iridescenceInfo.z = value; | |
| } | |
| /** | |
| * The maximum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `400` | |
| */ | |
| get iridescenceThicknessMax(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).w; | |
| } | |
| set iridescenceThicknessMax(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(0, value); | |
| const minThickness = iridescenceInfo.z; | |
| if (value <= minThickness) { | |
| value = minThickness * 1.1; // Ensure max is greater than min | |
| } | |
| iridescenceInfo.w = value; | |
| } |
🧰 Tools
🪛 eslint
[error] 173-173: Delete ↹↹
(prettier/prettier)
[error] 177-178: Delete ⏎
(prettier/prettier)
[error] 187-187: Delete ↹
(prettier/prettier)
🪛 GitHub Check: codecov/patch
[warning] 177-179: packages/core/src/material/PBRMaterial.ts#L177-L179
Added lines #L177 - 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
🪛 GitHub Check: lint
[failure] 173-173:
Delete ↹↹
[failure] 177-177:
Delete ⏎
[failure] 187-187:
Delete ↹
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts (2)
11-18: Consider documenting the physical meaning of default valuesThe default values for
iridescenceThicknessMinimum(100) andiridescenceThicknessMaximum(400) represent nanometers. Consider adding a comment to clarify this for maintainability.const { iridescenceFactor = 0, iridescenceTexture, iridescenceIor = 1.3, + // Thickness range 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-31: Consider batching texture loadingThe current implementation loads textures sequentially. Consider batching the texture loading operations for better performance when both textures are present.
+const texturePromises: Promise<void>[] = []; if (iridescenceTexture) { GLTFMaterialParser._checkOtherTextureTransform(iridescenceTexture, "Iridescence texture"); - context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => { + texturePromises.push(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) => { + texturePromises.push(context.get<Texture2D>(GLTFParserType.Texture, iridescenceThicknessTexture.index).then((texture) => { material.iridescenceThicknessTexture = texture; - }); + })); } + +// Wait for all textures to load +Promise.all(texturePromises).catch((error) => { + console.warn(`Failed to load one or more iridescence textures: ${error.message}`); +});Also applies to: 32-38
packages/core/src/material/PBRMaterial.ts (2)
23-25: Follow naming convention for shader properties.The property
_iridescenceInfoPropshould follow camelCase convention like other properties in the class.- private static _iridescenceInfoProp = ShaderProperty.getByName("material_IridescenceInfo"); + private static _iridescenceInfoProp = ShaderProperty.getByName("material_iridescenceInfo");
198-230: Add test coverage for texture properties.The implementation looks good but lacks test coverage. Tests should verify:
- Texture assignment
- Macro enabling/disabling
- Texture cleanup
Would you like me to help generate unit tests for these properties?
🧰 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 (2)
packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts (1)
1-9: LGTM! Proper extension setup and registration
The imports are comprehensive and the extension is correctly registered with the appropriate mode.
packages/core/src/material/PBRMaterial.ts (1)
244-244: LGTM!
The initialization of iridescence properties with default values is correct and matches the documentation.
| context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => { | ||
| material.iridescenceTexture = texture; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for texture loading
The async texture loading should include error handling to prevent silent failures.
context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => {
material.iridescenceTexture = texture;
-});
+}).catch((error) => {
+ console.warn(`Failed to load iridescence texture: ${error.message}`);
+});
// Similar change for iridescenceThicknessTextureAlso applies to: 35-38
| const { | ||
| iridescenceFactor = 0, | ||
| iridescenceTexture, | ||
| iridescenceIor = 1.3, | ||
| iridescenceThicknessMinimum = 100, | ||
| iridescenceThicknessMaximum = 400, | ||
| iridescenceThicknessTexture | ||
| } = schema; | ||
|
|
||
| material.iridescenceFactor = iridescenceFactor; | ||
| material.iridescenceIor = iridescenceIor; | ||
| material.iridescenceThicknessMin = iridescenceThicknessMinimum; | ||
| material.iridescenceThicknessMax = 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.
Add unit tests for the extension parser
The static analysis indicates that this new extension lacks test coverage. This is critical for ensuring the parser behaves correctly with various input scenarios.
Key scenarios to test:
- Default values when properties are missing
- Texture loading success and failure cases
- Edge cases for thickness values
- Invalid input handling
Would you like me to help generate a comprehensive test suite for this extension?
🧰 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
| /** | ||
| * The index of refraction of the dielectric thin-film layer. | ||
| * @defaultValue `1.3` | ||
| */ | ||
| get iridescenceIor(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).y; | ||
| } | ||
|
|
||
| set iridescenceIor(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| iridescenceInfo.y = value; | ||
| } |
There was a problem hiding this comment.
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;
}📝 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.
| /** | |
| * The index of refraction of the dielectric thin-film layer. | |
| * @defaultValue `1.3` | |
| */ | |
| get iridescenceIor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).y; | |
| } | |
| set iridescenceIor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| iridescenceInfo.y = value; | |
| } | |
| /** | |
| * The index of refraction of the dielectric thin-film layer. | |
| * @defaultValue `1.3` | |
| */ | |
| get iridescenceIor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).y; | |
| } | |
| set iridescenceIor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(1.0, value); | |
| iridescenceInfo.y = value; | |
| } |
🧰 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
| /** | ||
| * The iridescence intensity factor. | ||
| * @defaultValue `0.0` | ||
| */ | ||
| get iridescenceFactor(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).x; | ||
| } | ||
|
|
||
| set iridescenceFactor(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| if (!!iridescenceInfo.x !== !!value) { | ||
| if (value === 0) { | ||
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | ||
| } else { | ||
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | ||
| } | ||
| iridescenceInfo.x = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix iridescence factor implementation and add tests.
Issues found:
- The value should be clamped to [0,1] range as it represents an intensity factor
- The value is set after the macro check, which could lead to inconsistent state
- Missing test coverage for the property
Apply this diff to fix the implementation:
set iridescenceFactor(value: number) {
const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp);
+ value = Math.max(0, Math.min(1, value));
if (!!iridescenceInfo.x !== !!value) {
if (value === 0) {
this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE");
} else {
this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE");
}
- iridescenceInfo.x = value;
}
+ iridescenceInfo.x = value;
}Would you like me to help generate unit tests for this property?
📝 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.
| /** | |
| * The iridescence intensity factor. | |
| * @defaultValue `0.0` | |
| */ | |
| get iridescenceFactor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).x; | |
| } | |
| set iridescenceFactor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| if (!!iridescenceInfo.x !== !!value) { | |
| if (value === 0) { | |
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } else { | |
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } | |
| iridescenceInfo.x = value; | |
| } | |
| } | |
| /** | |
| * The iridescence intensity factor. | |
| * @defaultValue `0.0` | |
| */ | |
| get iridescenceFactor(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).x; | |
| } | |
| set iridescenceFactor(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(0, Math.min(1, value)); | |
| if (!!iridescenceInfo.x !== !!value) { | |
| if (value === 0) { | |
| this.shaderData.disableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } else { | |
| this.shaderData.enableMacro("MATERIAL_ENABLE_IRIDESCENCE"); | |
| } | |
| } | |
| iridescenceInfo.x = value; | |
| } |
🧰 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-157: packages/core/src/material/PBRMaterial.ts#L148-L157
Added lines #L148 - L157 were not covered by tests
| /** | ||
| * The minimum thickness of the thin-film layer given in nanometers. | ||
| * @defaultValue `100` | ||
| */ | ||
| get iridescenceThicknessMin(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).z; | ||
| } | ||
|
|
||
| set iridescenceThicknessMin(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| iridescenceInfo.z = value; | ||
| } | ||
|
|
||
| /** | ||
| * The maximum thickness of the thin-film layer given in nanometers. | ||
| * @defaultValue `400` | ||
| */ | ||
| get iridescenceThicknessMax(): number { | ||
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).w; | ||
| } | ||
|
|
||
| set iridescenceThicknessMax(value: number) { | ||
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | ||
| iridescenceInfo.w = value; | ||
| } |
There was a problem hiding this comment.
Add validation for thickness values.
The thickness values need validation to ensure:
- Non-negative values (physical constraint)
- Minimum thickness is less than maximum thickness
Apply this diff to add validation:
set iridescenceThicknessMin(value: number) {
const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp);
+ value = Math.max(0, value);
+ const maxThickness = iridescenceInfo.w;
+ if (value >= maxThickness) {
+ value = maxThickness * 0.9; // Ensure min is less than max
+ }
iridescenceInfo.z = value;
}
set iridescenceThicknessMax(value: number) {
const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp);
+ value = Math.max(0, value);
+ const minThickness = iridescenceInfo.z;
+ if (value <= minThickness) {
+ value = minThickness * 1.1; // Ensure max is greater than min
+ }
iridescenceInfo.w = value;
}📝 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.
| /** | |
| * The minimum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `100` | |
| */ | |
| get iridescenceThicknessMin(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).z; | |
| } | |
| set iridescenceThicknessMin(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| iridescenceInfo.z = value; | |
| } | |
| /** | |
| * The maximum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `400` | |
| */ | |
| get iridescenceThicknessMax(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).w; | |
| } | |
| set iridescenceThicknessMax(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| iridescenceInfo.w = value; | |
| } | |
| /** | |
| * The minimum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `100` | |
| */ | |
| get iridescenceThicknessMin(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).z; | |
| } | |
| set iridescenceThicknessMin(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(0, value); | |
| const maxThickness = iridescenceInfo.w; | |
| if (value >= maxThickness) { | |
| value = maxThickness * 0.9; // Ensure min is less than max | |
| } | |
| iridescenceInfo.z = value; | |
| } | |
| /** | |
| * The maximum thickness of the thin-film layer given in nanometers. | |
| * @defaultValue `400` | |
| */ | |
| get iridescenceThicknessMax(): number { | |
| return this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp).w; | |
| } | |
| set iridescenceThicknessMax(value: number) { | |
| const iridescenceInfo = this.shaderData.getVector4(PBRMaterial._iridescenceInfoProp); | |
| value = Math.max(0, value); | |
| const minThickness = iridescenceInfo.z; | |
| if (value <= minThickness) { | |
| value = minThickness * 1.1; // Ensure max is greater than min | |
| } | |
| iridescenceInfo.w = value; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[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
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