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

MeshPhysicalMaterial: Support iridescence / thin-film materials #23869

Merged
merged 18 commits into from
May 25, 2022

Conversation

PascalSchoen
Copy link
Contributor

Description

Implementation of the KHR_materials_iridescence glTF extension.

@donmccurdy
Copy link
Collaborator

Preview Link:

Screen Shot 2022-04-08 at 12 06 13 PM

@donmccurdy donmccurdy changed the title Feature/khr materials iridescence MeshPhysicalMaterial: Support iridescence (KHR_materials_iridescence) Apr 8, 2022
@donmccurdy donmccurdy changed the title MeshPhysicalMaterial: Support iridescence (KHR_materials_iridescence) MeshPhysicalMaterial: Support iridescence / thin-film materials Apr 8, 2022
@mrdoob
Copy link
Owner

mrdoob commented Apr 8, 2022

Ops... the builds are already conflicting. Could you remove them from the PR?

@mrdoob mrdoob added this to the r140 milestone Apr 8, 2022
@PascalSchoen
Copy link
Contributor Author

Ops... the builds are already conflicting. Could you remove them from the PR?

@mrdoob Yeah, didn't think of this conflict. Removed the build files from the PR. ;)

@elalish
Copy link
Contributor

elalish commented Apr 11, 2022

Thanks @PascalSchoen! Looks good to me, though I'd like to know what @WestLangley thinks. I'd still like to refactor multiscattering, since I don't think it's right that we make up the energy difference with the diffuse lobe. Would be nice to simplify the code there too, but that can happen separately from this PR.

@mrdoob
Copy link
Owner

mrdoob commented Apr 14, 2022

@PascalSchoen Lookin good! Is there any sample model we can use in the examples that uses this?

@emackey
Copy link

emackey commented Apr 18, 2022

Just a heads-up, Khronos PBR TSG may need to change "iridescenceIOR" to "iridescenceIor" to fix the camelCase.

The following models have been updated to the new casing:

@takahirox
Copy link
Collaborator

IridescenceMetallicSpheres looks cool but looks like testing purpose too much? Personally I'm ok with it but if we don't really prefer testing purpose models, probably Suzanne may be the only option in them.

@@ -40,7 +40,7 @@ class MeshPhysicalMaterial extends MeshStandardMaterial {
} );

this.iridescenceMap = null;
this.iridescenceIOR = 1.3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@PascalSchoen sorry for the churn here, but glTF's camelCase convention for acronyms like XYZ (fooBarXyz) is different from what we follow in three.js (fooBarXYZ). I think for the public API of MeshPhysicalMaterial, this should remain iridescenceIOR. Feel free to use whichever you prefer for internal code in the rest of the PR. 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@donmccurdy I reverted the change and only kept it for the glTF extension parts. 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

@donmccurdy You prefer using .ior and .iridescenceIOR?

Note, we also have the function applyIorToRoughness().

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so, yes — for example we use:

  • attribute.getXYZ not .getXyz
  • attribute.x not attribute.X
  • GLTFLoader not GltfLoader

I'm comfortable with either convention but this seems to be the norm in three.js today, and (I think?) more common/idiomatic in JavaScript generally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I don't have a strong feeling on this one. Maybe applyIORToRoughness()?

There is also the option of .refractiveIndex instead of .ior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Slight preference for applyIORToRoughness, yes - but not as worried about internal methods.

Somehow .ior does not bother me, feels similar to vec.xyz in GLSL. 🤷‍♂️

@mrdoob mrdoob modified the milestones: r140, r141 Apr 30, 2022
@mrdoob mrdoob merged commit 12f550e into mrdoob:dev May 25, 2022
@mrdoob
Copy link
Owner

mrdoob commented May 25, 2022

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented May 25, 2022

Depending on the model the shader could crash because of dotNV redefinition. e60a802

@emackey
Copy link

emackey commented May 25, 2022

IridescenceMetallicSpheres looks cool but looks like testing purpose too much?

@takahirox Khronos has a new photo-realistic model now from Wayfair: IridescenceLamp.

@takahirox
Copy link
Collaborator

takahirox commented May 25, 2022

@takahirox Khronos has a new photo-realistic model now from Wayfair: IridescenceLamp.

Thanks. That looks much nicer. If we add an example for iridescence / thin-film materials we can use it.

@mrdoob
Copy link
Owner

mrdoob commented May 26, 2022

Yes, I noticed. I was going to do an example today 😊

Screen Shot 2022-05-26 at 11 15 34 AM

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
…ob#23869)

* Add iridescence parameters to Physical Material

* Add iridescence fragment shader code

* Iridescence shader integration and glTF loading

* Update iridescence default values

* Add KHR_materials_iridescence to supported extensions in GLTFLoader docu

* Enable iridescence in Editor

* Update build results

* Remove build files from PR

* Honor iridescence parameters in program cache

* Use range for iridescence thin-film thickness

* Fixed linting errors

* Fix always-true conditional

* Add iridescence to glTF export

* Instantiate iridescenceThicknessRange in GLTFLoader if undefined

* Make iridescence Fresnel consistent for IBL

* Rename iridescenceIOR to iridescenceIor

* Rename iridescenceIor back to iridescenceIOR except for glTF extension
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants