Skip to content
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

Gltf Iridescence #2425

Open
wants to merge 37 commits into
base: dev/1.4
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
5a26f3d
fix(shader-lab): compatible with empty macro
Sway007 Sep 20, 2023
5ecc318
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 20, 2023
cafc24f
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 20, 2023
dc69489
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 21, 2023
221b7b6
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 21, 2023
0d45d9c
fix(shader-lab): add break and continue syntax
Sway007 Sep 21, 2023
0f14c3f
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 27, 2023
8871d9b
fix: typo
Sway007 Oct 11, 2023
3b4ffd7
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 1, 2023
f649a58
fix(shader-lab): Make usepass compatible with buitin shader
Sway007 Nov 1, 2023
598fc56
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 2, 2023
e33a66f
fix(shader-lab): compatible with no varying variable
Sway007 Nov 2, 2023
41ef06f
feat(shader-lab): detect mismatch return type
Sway007 Nov 6, 2023
b5214fc
fix(shader-lab): renderState assignment
Sway007 Nov 7, 2023
f36ce02
feat: extend material loader data type
Sway007 Nov 13, 2023
ff8b7c2
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 13, 2023
634236f
fix(shader-lab): glsl type pattern
Sway007 Nov 13, 2023
91e6fa4
fix(shader-lab): glsl type pattern
Sway007 Nov 13, 2023
3932448
fix: switch case break
Sway007 Nov 15, 2023
671cace
fix: array index loss
Sway007 Jun 11, 2024
92b972e
feat: merge
Sway007 Jun 11, 2024
9226d38
fix: test-case
Sway007 Jun 11, 2024
ff6a69a
fix: test-case
Sway007 Jun 12, 2024
0838968
feat: support GLTF_Iridescence
hhhhkrx Nov 6, 2024
82d5a8f
Merge branch 'main' of github.com:galacean/engine into gltf_iridescence
hhhhkrx Nov 6, 2024
39043a2
fix: merge
hhhhkrx Nov 6, 2024
ad61eed
Merge branch 'dev/1.4' of github.com:galacean/engine into gltf_irides…
hhhhkrx Nov 6, 2024
2b308ec
fix: vector3
hhhhkrx Nov 6, 2024
2c1604f
fix: vector3
hhhhkrx Nov 6, 2024
0be9f2a
fix: texture name
hhhhkrx Nov 6, 2024
88f89f2
Merge branch 'dev/1.4' into gltf_iridescence
hhhhkrx Nov 6, 2024
b5b9a0a
fix: material uniform
hhhhkrx Nov 7, 2024
d7a82de
fix: enable iridescence
hhhhkrx Nov 7, 2024
d1547d4
Merge branch 'dev/1.4' of github.com:galacean/engine into gltf_irides…
hhhhkrx Nov 11, 2024
76c5423
fix: iridescence name
hhhhkrx Nov 11, 2024
aa41aaf
fix: add }
hhhhkrx Nov 11, 2024
f4b16fd
ci: prettier code
hhhhkrx Nov 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 99 additions & 1 deletion packages/core/src/material/PBRMaterial.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { MathUtil, Vector3 } from "@galacean/engine-math";
import { MathUtil, Vector3, Vector4 } from "@galacean/engine-math";
import { Engine } from "../Engine";
import { ShaderProperty } from "../shader";
import { Shader } from "../shader/Shader";
Expand All @@ -20,6 +20,10 @@ export class PBRMaterial extends PBRBaseMaterial {

private _anisotropyRotation: number = 0;

private static _iridescenceProp = ShaderProperty.getByName("material_IridescenceInfo");
private static _iridescenceThicknessTextureProp = ShaderProperty.getByName("material_IridescenceThicknessTexture");
private static _iridescenceTextureProp = ShaderProperty.getByName("material_IridescenceTexture");

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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_IridescenceInfo
  • material_IridescenceThicknessTexture
  • material_IridescenceTexture

This indicates that either:

  1. The shader implementation for iridescence is incomplete, or
  2. 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

/**
* Index Of Refraction.
* @defaultValue `1.5`
Expand Down Expand Up @@ -132,6 +136,98 @@ export class PBRMaterial extends PBRBaseMaterial {
}
}

/**
* The iridescence intensity factor.
* @defaultValue `1.0`
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default is 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;
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
/**
* 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same as below

}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
/**
* 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable start with lowercase, such as iridescenceInfo

}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for thickness values.

The thickness values should be:

  1. Non-negative
  2. 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.

Suggested change
/**
* 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)

/**
* The thickness texture of the thin-film layer.
*/
get irithicknessTexture(): Texture2D {
return <Texture2D>this.shaderData.getTexture(PBRMaterial._iridescenceThicknessTextureProp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iri is not a good abbreviation, use iridescenceThicknessTexture to instead

}

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");
}
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix property name typo and add texture handling tests.

  1. 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
  2. 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.

Suggested change
/**
* 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)

/**
* Create a pbr metallic-roughness workflow material instance.
* @param engine - Engine to which the material belongs
Expand All @@ -144,6 +240,8 @@ export class PBRMaterial extends PBRBaseMaterial {
shaderData.setFloat(PBRMaterial._roughnessProp, 1);
shaderData.setFloat(PBRMaterial._iorProp, 1.5);
shaderData.setVector3(PBRMaterial._anisotropyInfoProp, new Vector3(1, 0, 0));
shaderData.setVector4(PBRMaterial._iridescenceProp, new Vector4(0, 1.3, 100, 400));

}

/**
Expand Down
23 changes: 18 additions & 5 deletions packages/loader/src/gltf/extensions/GLTFExtensionSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ export interface IKHRLightsPunctual {
* Interfaces from the KHR_materials_clearcoat extension
*/
export interface IKHRMaterialsClearcoat {
clearcoatFactor: number;
clearcoatTexture: ITextureInfo;
clearcoatRoughnessFactor: number;
clearcoatRoughnessTexture: ITextureInfo;
clearcoatNormalTexture: IMaterialNormalTextureInfo;
clearcoatFactor?: number;
clearcoatTexture?: ITextureInfo;
clearcoatRoughnessFactor?: number;
clearcoatRoughnessTexture?: ITextureInfo;
clearcoatNormalTexture?: IMaterialNormalTextureInfo;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you

}

/**
Expand Down Expand Up @@ -176,6 +176,18 @@ export interface IGalaceanAnimation {
}[];
}

/**
* Interfaces from the KHR_materials_iridescence extension
*/
export interface IKHRMaterialsIridescence {
iridescenceFactor?: number;
iridescenceTexture?: ITextureInfo;
iridescenceIor?: number;
iridescenceThicknessMinimum?: number;
iridescenceThicknessMaximum?: number;
iridescenceThicknessTexture?: ITextureInfo;
}

export type GLTFExtensionSchema =
| IKHRLightsPunctual_Light
| IKHRMaterialsClearcoat
Expand All @@ -194,4 +206,5 @@ export type GLTFExtensionSchema =
| IKHRXmp
| IKHRXmp_Node
| IGalaceanAnimation
| IKHRMaterialsIridescence
| Object;
40 changes: 40 additions & 0 deletions packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
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,
iridescenceThicknessMinimum = 100,
iridescenceThicknessMaximum = 400,
iridescenceThicknessTexture
} = schema;

material.iridescenceFactor = iridescenceFactor;

Check failure on line 20 in packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts

View workflow job for this annotation

GitHub Actions / lint

Delete `·`
material.iridescenceIor = iridescenceIor;

Check failure on line 21 in packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts

View workflow job for this annotation

GitHub Actions / lint

Delete `·`
material.iridescenceThicknessMin = iridescenceThicknessMaximum;

Check failure on line 22 in packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts

View workflow job for this annotation

GitHub Actions / lint

Delete `·`
material.iridescenceThicknessMax = iridescenceThicknessMinimum;

Check failure on line 23 in packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts

View workflow job for this annotation

GitHub Actions / lint

Delete `·`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert min/max


Check failure on line 24 in packages/loader/src/gltf/extensions/KHR_materials_iridescence.ts

View workflow job for this annotation

GitHub Actions / lint

Delete `·`
if (iridescenceTexture) {
GLTFMaterialParser._checkOtherTextureTransform(iridescenceTexture, "Iridescence texture");

context.get<Texture2D>(GLTFParserType.Texture, iridescenceTexture.index).then((texture) => {
material.iridescenceTexture = texture;
});
}
Comment on lines +28 to +31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 iridescenceThicknessTexture

Also applies to: 35-38

if (iridescenceThicknessTexture) {
GLTFMaterialParser._checkOtherTextureTransform(iridescenceThicknessTexture, "IridescenceThickness texture");

context.get<Texture2D>(GLTFParserType.Texture, iridescenceThicknessTexture.index).then((texture) => {
material.irithicknessTexture = texture;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
material.irithicknessTexture = texture;
material.iridescenceThicknessTexture = texture;

});
}
}
}
1 change: 1 addition & 0 deletions packages/loader/src/gltf/extensions/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import "./GALACEAN_materials_remap";
import "./GALACEAN_animation_event";
import "./EXT_meshopt_compression";
import "./KHR_materials_anisotropy";
import "./KHR_materials_iridescence";
import "./EXT_texture_webp";

export { GLTFExtensionParser, GLTFExtensionMode } from "./GLTFExtensionParser";
Expand Down
Loading