There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,39 @@ | ||||||||||||||||
| import { PBRMaterial, Texture2D } from "@galacean/engine-core"; | ||||||||||||||||
| import { GLTFMaterialParser } from "../parser/GLTFMaterialParser"; | ||||||||||||||||
| import { registerGLTFExtension } from "../parser/GLTFParser"; | ||||||||||||||||
| import { GLTFParserContext, GLTFParserType } from "../parser/GLTFParserContext"; | ||||||||||||||||
| import { GLTFExtensionMode, GLTFExtensionParser } from "./GLTFExtensionParser"; | ||||||||||||||||
| import { IKHRMaterialsIridescence } from "./GLTFExtensionSchema"; | ||||||||||||||||
|
|
||||||||||||||||
| @registerGLTFExtension("KHR_materials_iridescence", GLTFExtensionMode.AdditiveParse) | ||||||||||||||||
| class KHR_materials_iridescence extends GLTFExtensionParser { | ||||||||||||||||
| override additiveParse(context: GLTFParserContext, material: PBRMaterial, schema: IKHRMaterialsIridescence): void { | ||||||||||||||||
| const { | ||||||||||||||||
| iridescenceFactor = 0, | ||||||||||||||||
| iridescenceTexture, | ||||||||||||||||
| iridescenceIor = 1.3, | ||||||||||||||||
|
There was a problem hiding this comment. 🛠️ Refactor suggestion Add validation for iridescenceIor The IOR (Index of Refraction) value should be validated as it typically should be greater than 1.0 for physical accuracy. - iridescenceIor = 1.3,
+ iridescenceIor = Math.max(1.0, schema.iridescenceIor ?? 1.3),📝 Committable suggestion
Suggested change
|
||||||||||||||||
| iridescenceThicknessMinimum = 100, | ||||||||||||||||
| iridescenceThicknessMaximum = 400, | ||||||||||||||||
|
Comment on lines
+15
to
+16
There was a problem hiding this comment. 🛠️ Refactor suggestion Validate thickness range values The thickness range values should be validated to ensure minimum is less than maximum and both are positive. - iridescenceThicknessMinimum = 100,
- iridescenceThicknessMaximum = 400,
+ iridescenceThicknessMinimum = Math.max(0, schema.iridescenceThicknessMinimum ?? 100),
+ iridescenceThicknessMaximum = Math.max(
+ schema.iridescenceThicknessMinimum ?? 100,
+ schema.iridescenceThicknessMaximum ?? 400
+ ),📝 Committable suggestion
Suggested change
|
||||||||||||||||
| iridescenceThicknessTexture | ||||||||||||||||
| } = schema; | ||||||||||||||||
|
|
||||||||||||||||
| material.iridescence = iridescenceFactor; | ||||||||||||||||
| material.iridescenceIOR = iridescenceIor; | ||||||||||||||||
| material.iridescenceThicknessRange.set(iridescenceThicknessMinimum, iridescenceThicknessMaximum); | ||||||||||||||||
|
|
||||||||||||||||
| if (iridescenceTexture) { | ||||||||||||||||
| GLTFMaterialParser._checkOtherTextureTransform(iridescenceTexture, "Iridescence texture"); | ||||||||||||||||
|
|
||||||||||||||||
| context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => { | ||||||||||||||||
| material.iridescenceTexture = texture; | ||||||||||||||||
| }); | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+27
to
+30
There was a problem hiding this comment. Improve texture loading implementation The current texture loading implementation has several issues:
Consider implementing this pattern: if (iridescenceTexture) {
GLTFMaterialParser._checkOtherTextureTransform(iridescenceTexture, "Iridescence texture");
- context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => {
- material.iridescenceTexture = texture;
- });
+ const texturePromise = context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index)
+ .then((texture) => {
+ if (!material.destroyed) {
+ material.iridescenceTexture = texture;
+ }
+ })
+ .catch((error) => {
+ console.warn(`Failed to load iridescence texture: ${error.message}`);
+ });
+ context.addPendingTask(texturePromise);
}Apply similar changes to the iridescenceThicknessTexture loading block. Also applies to: 34-37 |
||||||||||||||||
| if (iridescenceThicknessTexture) { | ||||||||||||||||
| GLTFMaterialParser._checkOtherTextureTransform(iridescenceThicknessTexture, "IridescenceThickness texture"); | ||||||||||||||||
|
|
||||||||||||||||
| context.get<Texture2D>(GLTFParserType.Texture, iridescenceThicknessTexture.index).then((texture) => { | ||||||||||||||||
| material.iridescenceThicknessTexture = texture; | ||||||||||||||||
| }); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+11
to
+38
There was a problem hiding this comment. 💡 Codebase verification Based on the gathered information, I can now provide the final response: Test coverage needed for KHR_materials_iridescence extension The codebase shows that GLTF material extensions are primarily tested through integration tests in
Reference the existing test patterns from other material extensions in 🔗 Analysis chainAdd comprehensive test coverage The static analysis indicates this extension lacks test coverage. This is critical for a parser implementation that handles material properties and async texture loading. Let's verify the current test coverage: Would you like me to help generate a test suite that covers:
Let me check the test coverage more thoroughly by examining the test files for material extensions and the implementation details. Let me try a different approach to check for GLTF extension tests and material-related test patterns. Let me check specifically for tests of other KHR material extensions to understand the testing patterns used. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check for existing tests
fd -e test.ts -e spec.ts . | grep -i "iridescence"
# Check if similar extensions have tests we can reference
fd -e test.ts -e spec.ts . | grep -i "material.*test"
Length of output: 480 Script: #!/bin/bash
# Check for GLTF material extension tests
fd -e test.ts -e spec.ts . | grep -i "gltf.*material"
# Look at the implementation of other material extension tests
rg -l "describe.*material.*extension" -g "*.test.ts" -g "*.spec.ts"
# Check the structure of KHR_materials_iridescence implementation
ast-grep --pattern 'export class KHR_materials_iridescence {
$$$
}'
Length of output: 197 Script: #!/bin/bash
# Check for any GLTF extension tests
rg -g "*.test.ts" -g "*.spec.ts" "KHR_materials" -A 5
# Look for test patterns in material-related tests
rg -g "*.test.ts" "describe\(.*Material" -A 2 tests/src/core/material/
# Check the actual implementation directory for other extension tests
fd . packages/loader/src/gltf/extensions/ -t f
Length of output: 5399 Script: #!/bin/bash
# Check for test files of other KHR material extensions
rg -g "*.test.ts" -g "*.spec.ts" "KHR_materials_(clearcoat|unlit|transmission)" -B 2 -A 10
# Look at the implementation of a similar extension test
rg "class KHR_materials_clearcoat" -A 10 packages/loader/src/gltf/extensions/KHR_materials_clearcoat.ts
# Check if there's a test directory for GLTF extensions
fd . tests/src/loader/gltf/extensions/ -t d
Length of output: 3854 🧰 Tools🪛 GitHub Check: codecov/patch[warning] 11-38: packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts#L11-L38 |
||||||||||||||||
| } | ||||||||||||||||
PBR shader support Iridescence #2425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account