metallic roughness workflow support specular #2756
Conversation
|
""" WalkthroughThis update removes support for the "KHR_materials_pbrSpecularGlossiness" GLTF extension and its related material class, schema, and loader logic. It introduces new support for the "KHR_materials_specular" extension, updating the PBR material and shader system to handle specular reflection properties, textures, and color. Test cases and texture parsing logic are adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant GLTFLoader
participant KHR_materials_specular
participant PBRMaterial
participant TextureParser
GLTFLoader->>KHR_materials_specular: Parse KHR_materials_specular extension
KHR_materials_specular->>PBRMaterial: Set specularFactor, specularColorFactor
KHR_materials_specular->>TextureParser: Get specularTexture
TextureParser-->>KHR_materials_specular: Return Texture2D
KHR_materials_specular->>PBRMaterial: Set specularTexture
KHR_materials_specular->>TextureParser: Get specularColorTexture
TextureParser-->>KHR_materials_specular: Return Texture2D
KHR_materials_specular->>PBRMaterial: Set specularColorTexture
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
tests/src/loader/GLTFLoader.test.ts (2)
7-7: Critical: Remove unused PBRSpecularMaterial importThe test file still imports
PBRSpecularMaterialwhich was removed according to the PR changes. This will cause compilation errors.- PBRSpecularMaterial,
376-398: Critical: Update test assertions to reflect material class changesThe test code still references
PBRSpecularMaterialand its properties (specularColor,glossiness,specularGlossinessTexture) which were removed. These assertions will fail since the class no longer exists.The test needs to be updated to reflect the new specular workflow integrated into
PBRMaterial. Consider updating the test data and assertions to use the new specular extension properties.
🧹 Nitpick comments (1)
packages/loader/src/gltf/extensions/KHR_materials_specular.ts (1)
16-16: Clarify specular color alpha component handlingPassing
undefinedas the alpha component seems intentional but unclear. Consider using a default value or adding a comment to explain this behavior.- material.specularColor.set(specularColorFactor[0], specularColorFactor[1], specularColorFactor[2], undefined); + material.specularColor.set(specularColorFactor[0], specularColorFactor[1], specularColorFactor[2], 1.0);Or add a comment explaining why
undefinedis used for the alpha component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/shader-shaderlab/src/shaders/shadingPBR/BSDF.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl is excluded by !**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/ReflectionLobe.glslis excluded by!**/*.glsl
📒 Files selected for processing (9)
packages/core/src/material/PBRMaterial.ts(5 hunks)packages/core/src/material/PBRSpecularMaterial.ts(0 hunks)packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts(1 hunks)packages/loader/src/gltf/extensions/KHR_materials_pbrSpecularGlossiness.ts(0 hunks)packages/loader/src/gltf/extensions/KHR_materials_specular.ts(1 hunks)packages/loader/src/gltf/extensions/index.ts(1 hunks)packages/loader/src/gltf/parser/GLTFTextureParser.ts(0 hunks)packages/shader-shaderlab/src/shaders/PBR.gs(1 hunks)tests/src/loader/GLTFLoader.test.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/loader/src/gltf/parser/GLTFTextureParser.ts
- packages/core/src/material/PBRSpecularMaterial.ts
- packages/loader/src/gltf/extensions/KHR_materials_pbrSpecularGlossiness.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts (2)
packages/loader/src/gltf/GLTFSchema.ts (1)
ITextureInfo(772-781)packages/loader/src/gltf/index.ts (1)
ITextureInfo(9-9)
🔇 Additional comments (12)
packages/loader/src/gltf/extensions/index.ts (1)
18-18: LGTM - Clean extension replacementThe import statement correctly replaces the deprecated
KHR_materials_pbrSpecularGlossinessextension with the newKHR_materials_specularextension.tests/src/loader/GLTFLoader.test.ts (1)
119-119: LGTM - Correctly removed deprecated extensionThe removal of
KHR_materials_pbrSpecularGlossinessfrom theextensionsRequiredarray aligns with the deprecation of this extension.packages/shader-shaderlab/src/shaders/PBR.gs (1)
15-21: LGTM - Well-structured specular shader propertiesThe new "Specular" header section correctly defines the necessary shader properties for the specular workflow:
material_Specular: Specular factor with appropriate rangematerial_SpecularColor: Specular color propertymaterial_SpecularTextureandmaterial_SpecularColorTexture: Texture propertiesThe naming conventions and property definitions are consistent with existing shader properties.
packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts (1)
62-66: LGTM - Correct schema interface definitionThe
IKHRMaterialsSpecularinterface properly defines all specular-related properties as optional, which aligns with GLTF extension conventions. The property types (number,number[],ITextureInfo) are appropriate for their respective purposes.packages/loader/src/gltf/extensions/KHR_materials_specular.ts (1)
19-43: LGTM - Robust texture loading implementationThe texture loading logic correctly:
- Validates texture transforms before loading
- Uses async context.get() for texture retrieval
- Implements proper error handling with logging
- Follows the established pattern used in other extension parsers
The implementation is consistent and handles both specular and specular color textures appropriately.
packages/core/src/material/PBRMaterial.ts (7)
44-52: LGTM!The static property declarations for specular workflow follow the established pattern and are properly typed.
57-58: LGTM!The private fields for tracking specular state are properly declared.
68-71: LGTM!Correctly updates reflectance when IOR changes, as reflectance calculation depends on the index of refraction.
490-536: LGTM!The specular color and texture properties are correctly implemented, following the established patterns for color and texture properties with proper shader macro management.
559-563: LGTM!The specular properties are initialized with appropriate default values that match typical PBR material behavior.
568-569: LGTM!The specular color change handler is correctly implemented, following the same pattern as the sheen color handler.
Also applies to: 600-611
613-623: Fix the reflectance calculation logic.The current implementation has several issues:
- Line 615 treats
specularas a boolean when it's a number- The logic appears inverted - reflectance should be calculated from IOR when specular is NOT being used explicitly
- Line 616 gets reflectance but doesn't use it
Apply this diff to fix the logic:
private _updateReflectance(): void { - let reflectance: number; - if (this.specular) { - reflectance = this.shaderData.getFloat(PBRMaterial._reflectanceProp); + if (this._specularEnabled) { + // When specular is enabled, reflectance is controlled by specular value + // The shader will handle the calculation + return; } else { + // When specular is disabled, calculate reflectance from IOR const ior = this.ior; const f0 = (ior - 1.0) / (ior + 1.0); - reflectance = Math.sqrt((f0 * f0) / 0.16); + const reflectance = Math.sqrt((f0 * f0) / 0.16); + this.shaderData.setFloat(PBRMaterial._reflectanceProp, reflectance); } - this.shaderData.setFloat(PBRMaterial._reflectanceProp, reflectance); }Likely an incorrect or invalid review comment.
| override additiveParse(context: GLTFParserContext, material: PBRMaterial, schema: IKHRMaterialsSpecular): void { | ||
| const { specularFactor, specularTexture, specularColorFactor, specularColorTexture } = schema; | ||
|
|
||
| material.specular = specularFactor; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for specularFactor
The specularFactor could be undefined since it's optional in the schema. Add a null check before assignment.
- material.specular = specularFactor;
+ if (specularFactor !== undefined) {
+ material.specular = specularFactor;
+ }📝 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.specular = specularFactor; | |
| if (specularFactor !== undefined) { | |
| material.specular = specularFactor; | |
| } |
🤖 Prompt for AI Agents
In packages/loader/src/gltf/extensions/KHR_materials_specular.ts at line 13, the
assignment of specularFactor to material.specular lacks a null check. Add a
condition to verify that specularFactor is not n
8000
ull or undefined before
assigning it to material.specular to prevent potential runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/loader/src/gltf/parser/GLTFMaterialParser.ts (1)
89-89: Consider usinginstanceoffor type checking instead of constructor comparison.While the logic is correct for checking PBR material types, using
instanceofwould be more idiomatic and type-safe in TypeScript.- if (material.constructor === PBRMaterial) { + if (material instanceof PBRMaterial) {Note: The same pattern is used on line 72, which should also be updated for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/loader/src/gltf/parser/GLTFMaterialParser.ts(2 hunks)tests/src/core/material/PBRSpecularMaterial.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- tests/src/core/material/PBRSpecularMaterial.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (2)
packages/loader/src/gltf/parser/GLTFMaterialParser.ts (2)
34-34: Parameter type narrowing looks correct.The removal of
PBRSpecularMaterialfrom the union type aligns with the deprecation of the specular-glossiness workflow. The type is now properly constrained to the supported material types.
72-86: Verify PBRMaterial property existence and typesI couldn’t locate a local definition of
PBRMaterialin this package (it’s likely imported from the engine core), so please confirm that the external class exposes the following with the correct types:
metallic: numberroughness: numberroughnessMetallicTexture: Texture2D | nullFile to check:
• packages/loader/src/gltf/parser/GLTFMaterialParser.ts (lines 72–86)
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
Chores