PBR support sheen #2448
PBR support sheen #2448
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 #2448 +/- ##
===========================================
+ Coverage 67.12% 67.89% +0.77%
===========================================
Files 897 908 +11
Lines 93218 94498 +1280
Branches 7581 7926 +345
===========================================
+ Hits 62572 64163 +1591
+ Misses 30401 30085 -316
- Partials 245 250 +5
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: 4
🧹 Outside diff range and nitpick comments (2)
packages/loader/src/gltf/extensions/KHR_materials_sheen.ts (1)
12-40: Unit tests are missing for the newKHR_materials_sheenextensionThe newly added
KHR_materials_sheenclass and its parsing logic are not covered by tests. Adding unit tests will help ensure the correctness and stability of the sheen extension parsing, especially for different schema variations.Would you like assistance in creating unit tests for this new extension?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 12-40: packages/loader/src/gltf/extensions/KHR_materials_sheen.ts#L12-L40
Added lines #L12 - L40 were not covered by testspackages/core/src/material/PBRMaterial.ts (1)
236-307: Unit tests are missing for the new sheen propertiesThe added properties and methods related to sheen are not covered by unit tests. Testing these will help prevent regressions and ensure they work as expected under various conditions.
Would you like help in creating unit tests for these new properties?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 236-237: packages/core/src/material/PBRMaterial.ts#L236-L237
Added lines #L236 - L237 were not covered by tests
[warning] 240-248: packages/core/src/material/PBRMaterial.ts#L240-L248
Added lines #L240 - L248 were not covered by tests
[warning] 255-256: packages/core/src/material/PBRMaterial.ts#L255-L256
Added lines #L255 - L256 were not covered by tests
[warning] 259-263: packages/core/src/material/PBRMaterial.ts#L259-L263
Added lines #L259 - L263 were not covered by tests
[warning] 270-271: packages/core/src/material/PBRMaterial.ts#L270-L271
Added lines #L270 - L271 were not covered by tests
[warning] 274-275: packages/core/src/material/PBRMaterial.ts#L274-L275
Added lines #L274 - L275 were not covered by tests
[warning] 281-282: packages/core/src/material/PBRMaterial.ts#L281-L282
Added lines #L281 - L282 were not covered by tests
[warning] 285-291: packages/core/src/material/PBRMaterial.ts#L285-L291
Added lines #L285 - L291 were not covered by tests
[warning] 297-298: packages/core/src/material/PBRMaterial.ts#L297-L298
Added lines #L297 - L298 were not covered by tests
[warning] 301-307: packages/core/src/material/PBRMaterial.ts#L301-L307
Added lines #L301 - L307 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_sheen.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/loader/src/gltf/extensions/KHR_materials_sheen.ts
[warning] 12-40: packages/loader/src/gltf/extensions/KHR_materials_sheen.ts#L12-L40
Added lines #L12 - L40 were not covered by tests
packages/core/src/material/PBRMaterial.ts
[warning] 236-237: packages/core/src/material/PBRMaterial.ts#L236-L237
Added lines #L236 - L237 were not covered by tests
[warning] 240-248: packages/core/src/material/PBRMaterial.ts#L240-L248
Added lines #L240 - L248 were not covered by tests
[warning] 255-256: packages/core/src/material/PBRMaterial.ts#L255-L256
Added lines #L255 - L256 were not covered by tests
[warning] 259-263: packages/core/src/material/PBRMaterial.ts#L259-L263
Added lines #L259 - L263 were not covered by tests
[warning] 270-271: packages/core/src/material/PBRMaterial.ts#L270-L271
Added lines #L270 - L271 were not covered by tests
[warning] 274-275: packages/core/src/material/PBRMaterial.ts#L274-L275
Added lines #L274 - L275 were not covered by tests
[warning] 281-282: packages/core/src/material/PBRMaterial.ts#L281-L282
Added lines #L281 - L282 were not covered by tests
[warning] 285-291: packages/core/src/material/PBRMaterial.ts#L285-L291
Added lines #L285 - L291 were not covered by tests
[warning] 297-298: packages/core/src/material/PBRMaterial.ts#L297-L298
Added lines #L297 - L298 were not covered by tests
[warning] 301-307: packages/core/src/material/PBRMaterial.ts#L301-L307
Added lines #L301 - L307 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/core/src/material/PBRMaterial.ts (3)
274-276: Improve documentation forsheenRoughnessTextureThe documentation is incomplete. Add details about the texture format and usage.
Update the documentation:
/** + * Sheen roughness texture * @remarks Use alpha channel, and multiply 'sheenRoughness'. + * @remarks The alpha channel of this texture is multiplied with the sheenRoughness property to get the final roughness value. */
265-272: Add null checks in texture settersThe texture setters should handle null values explicitly for better type safety.
Apply this pattern to both texture setters:
set sheenColorTexture(value: Texture2D) { + if (value === null || value === undefined) { + this.shaderData.setTexture(PBRMaterial._sheenTextureProp, null); + this.shaderData.disableMacro("MATERIAL_HAS_SHEEN_TEXTURE"); + return; + } this.shaderData.setTexture(PBRMaterial._sheenTextureProp, value); - if (value) { this.shaderData.enableMacro("MATERIAL_HAS_SHEEN_TEXTURE"); - } else { - this.shaderData.disableMacro("MATERIAL_HAS_SHEEN_TEXTURE"); - } }Also applies to: 281-288
303-324: Refine sheen color change handler implementationThe implementation could be improved for better maintainability and documentation.
Consider these improvements:
+ /** Threshold for enabling sheen effect based on color intensity */ + private static readonly SHEEN_INTENSITY_THRESHOLD = 0; constructor(engine: Engine) { // ... existing code ... shaderData.setColor(PBRMaterial._sheenColorProp, new Color(0, 0, 0, 0)); // @ts-ignore this.sheenColor._onValueChanged = () => { const { r, g, b, a } = this.sheenColor; - /** - * The sheen layer switch. - */ - const enableSheen = (r + g + b) * a > 0; + const colorIntensity = (r + g + b) * a; + const enableSheen = colorIntensity > PBRMaterial.SHEEN_INTENSITY_THRESHOLD; if (enableSheen !== this._sheen) { this._sheen = enableSheen; if (enableSheen) { this.shaderData.enableMacro("MATERIAL_ENABLE_SHEEN"); } else { this.shaderData.disableMacro("MATERIAL_ENABLE_SHEEN"); } } }; }
📜 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_sheen.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/gltf/extensions/KHR_materials_sheen.ts
🔇 Additional comments (2)
packages/core/src/material/PBRMaterial.ts (2)
28-32: LGTM! Property declarations follow established patterns
The static shader properties and private state tracking are well-structured and consistent with the existing codebase.
254-256: Clamp sheenRoughness value to the range [0.0, 1.0]
The sheenRoughness property should be within the range of 0.0 to 1.0. Without clamping, assigning values outside this range could cause rendering issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/core/src/material/PBRMaterial.ts (2)
306-308: Avoid using@ts-ignore; ensure proper typing for_onValueChangedUsing
@ts-ignorebypasses TypeScript's type checking and may conceal underlying issues. It's better to properly define the type or adjust the code to resolve the type error.Consider defining
_onValueChangedin theColorclass or extending the class to include this property.
317-323: Add unit tests for the sheen featureThe logic enabling and disabling the sheen effect based on
sheenColorchanges is not covered by unit tests. Adding tests will ensure that the sheen effect behaves as expected whensheenColoris modified.Would you like assistance in creating unit tests for the sheen functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 317-323: packages/core/src/material/PBRMaterial.ts#L317-L323
Added lines #L317 - L323 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(5 hunks)packages/loader/src/gltf/extensions/KHR_materials_sheen.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/material/PBRMaterial.ts
[warning] 240-244: packages/core/src/material/PBRMaterial.ts#L240-L244
Added lines #L240 - L244 were not covered by tests
[warning] 251-252: packages/core/src/material/PBRMaterial.ts#L251-L252
Added lines #L251 - L252 were not covered by tests
[warning] 255-256: packages/core/src/material/PBRMaterial.ts#L255-L256
Added lines #L255 - L256 were not covered by tests
[warning] 262-263: packages/core/src/material/PBRMaterial.ts#L262-L263
Added lines #L262 - L263 were not covered by tests
[warning] 266-272: packages/core/src/material/PBRMaterial.ts#L266-L272
Added lines #L266 - L272 were not covered by tests
[warning] 279-280: packages/core/src/material/PBRMaterial.ts#L279-L280
Added lines #L279 - L280 were not covered by tests
[warning] 283-289: packages/core/src/material/PBRMaterial.ts#L283-L289
Added lines #L283 - L289 were not covered by tests
[warning] 317-323: packages/core/src/material/PBRMaterial.ts#L317-L323
Added lines #L317 - L323 were not covered by tests
packages/loader/src/gltf/extensions/KHR_materials_sheen.ts
[warning] 12-40: packages/loader/src/gltf/extensions/KHR_materials_sheen.ts#L12-L40
Added lines #L12 - L40 were not covered by tests
🔇 Additional comments (5)
packages/core/src/material/PBRMaterial.ts (2)
239-244: Implement validation for sheenColor values
The sheenColor setter does not validate the input color values. To prevent unexpected behavior, consider clamping each color component to ensure they are within the valid range of [0, 1].
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 240-244: packages/core/src/material/PBRMaterial.ts#L240-L244
Added lines #L240 - L244 were not covered by tests
255-256: Clamp sheenRoughness value to the range [0.0, 1.0]
The sheenRoughness property should be within the range of 0.0 to 1.0. Adding validation will prevent invalid values from causing rendering issues.
Apply this diff to clamp the sheenRoughness value:
set sheenRoughness(value: number) {
+ value = Math.max(0, Math.min(1, value));
this.shaderData.setFloat(PBRMaterial._sheenRoughnessProp, value);
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 255-256: packages/core/src/material/PBRMaterial.ts#L255-L256
Added lines #L255 - L256 were not covered by tests
packages/loader/src/gltf/extensions/KHR_materials_sheen.ts (3)
19-19: Set alpha channel when assigning sheenColor
The alpha channel represents sheen intensity. It's currently set to undefined, which may cause issues. Consider setting it to 1.0 or an appropriate value.
23-23: Check if sheenRoughnessFactor is defined before assignment
Assigning undefined to material.sheenRoughness could lead to unintended behavior. Add a check to ensure it is defined.
Apply this diff:
+ if (sheenRoughnessFactor !== undefined) {
material.sheenRoughness = sheenRoughnessFactor;
+ }28-30: Ensure asynchronous texture loading does not cause race conditions
When setting sheenColorTexture, the asynchronous loading might result in textures being applied after material initialization. Consider handling potential race conditions.
Run the following script to check for other instances where asynchronous texture loading is handled:
✅ Verification successful
Let me analyze the context further to understand how texture loading is handled in the codebase.
Let me check one more aspect to see how the material loading is coordinated.
Race conditions in texture loading are properly handled
The codebase shows a well-structured approach to texture loading coordination:
- The
MaterialLoaderclass usesPromise.all(texturePromises)to ensure all textures are loaded before completing material initialization. - Texture assignments are handled through a uniform system that coordinates texture uploads during the render pipeline.
- The asynchronous texture loading pattern is consistently used across all material types (base, sheen, clearcoat, etc.), indicating a standardized approach.
The current implementation in KHR_materials_sheen.ts follows the established pattern and doesn't introduce race conditions, as texture assignments are part of the material's initialization promise chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for asynchronous texture assignments in material parsers.
# Find occurrences of '.then(' after 'context.get<Texture2D>'
rg 'context\.get<Texture2D>.*\.then\(' -A 5
Length of output: 9057
Script:
#!/bin/bash
# Check if there's a Promise.all or similar mechanism to handle multiple texture loads
rg -B 5 -A 5 'Promise\.all.*texture'
# Check the GLTFParser class implementation for texture loading coordination
ast-grep --pattern 'class GLTFParser {
$$$
}'
Length of output: 631
Script:
#!/bin/bash
# Check the material parser implementation for texture loading coordination
rg -B 5 -A 10 'class GLTFMaterialParser'
# Check if there's a texture loading queue or coordination mechanism
rg -B 3 -A 3 'texturePromises|loadTexture'
Length of output: 10501
| */ | ||
| get sheenRoughness(): number { | ||
| return this.shaderData.getFloat(PBRMaterial._sheenRoughnessProp); | ||
| } |
There was a problem hiding this comment.
Maybe should add value = Math.max(0, Math.min(1, value)); if you want clamp the range like comments
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