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: Rename .transparency → .transmission, add .transmissionMap #19690

Merged
merged 4 commits into from
Jul 17, 2020

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Jun 19, 2020

Renames .transparency -> .transmission on MeshPhysicalMaterial, and adds .transmissionMap. Implements the KHR_materials_transmission thin transparency extension in GLTFLoader.

The mechanical aspects of this are easy enough, but our implementation (below) is a bit simpler than what KHR_materials_transmission suggests for implementation.

// this is a stub for the transparency model
#ifdef TRANSPARENCY
diffuseColor.a *= saturate( 1. - transparency + linearToRelativeLuminance( reflectedLight.directSpecular + reflectedLight.indirectSpecular ) );
#endif


current result intended result
Screen Shot 2020-06-18 at 8 49 07 PM Screen Shot 2020-06-18 at 8 48 39 PM

^There may be a texture scaling issue in this screenshot as well, but that can be ignored for now.

@WestLangley based on the proposed spec here, do these look like reasonable changes for us to make in MeshPhysicalMaterial? Or does anything there concern you?

@donmccurdy donmccurdy changed the title Implement KHR_materials_transmission. MeshPhysicalMaterial: Implement KHR_materials_transmission. Jun 19, 2020
@donmccurdy
Copy link
Collaborator Author

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 19, 2020

As a reminder: MeshPhysicalMaterial.transparency is documented and needs to be changed, too.

https://threejs.org/docs/index.html#api/en/materials/MeshPhysicalMaterial.transparency

@WestLangley
Copy link
Collaborator

This PR is excellent, IMO.

The glTF extension is intended to model thin-surface plastics and glass. I am not aware of transparent metals. I find the label "metalic transmission" odd.

As in https://threejs.org/examples/webgl_materials_physical_transparency.html, I'd render front and back faces separately to prevent artifacts. (Remove that example?)

@mrdoob mrdoob added this to the r119 milestone Jun 19, 2020
@donmccurdy
Copy link
Collaborator Author

I am not aware of transparent metals. I find the label "metalic transmission" odd.

Yes, sorry that's unclear. The metallic rows are included only to demonstrate materials with metalness=1, transmission=1 are not, and should not be, transparent.

The glTF extension is intended to model thin-surface plastics and glass.

Good clarification — this glTF extension, alone, models only thin-surface plastics and glass. However, in the longer-term roadmap the format will also have an extension for refraction which builds on transmission rather than introducing yet another transparency-like property. So it is not our intention that transmission=1 necessarily implies a thin surface: that is only the case in the absence of additional, not-yet-available properties like .ior or .thickness.

Or that is the glTF roadmap, at least. Whether MeshPhysicalMaterial should work the same way is up for discussion!

/cc @bhouston @elalish

@elalish
Copy link
Contributor

elalish commented Jun 19, 2020

This is exciting! Can you give any more detail on the differences between your approximation and the equations in the extension? And do you know why those four spheres end up nearly completely transparent, as opposed to the reference?

@WestLangley
Copy link
Collaborator

@elalish This PR does not change the approximation introduced in #17114. If you are interested in working on an improved physical model or approximation, that would be great.

@elalish
Copy link
Contributor

elalish commented Jun 19, 2020

@WestLangley Thanks, the description in that PR is just what I was looking for. I'd love to look deeper in to the physical model, but I'm afraid that's going to have to wait until someday when I have slightly less on my plate...

@donmccurdy donmccurdy mentioned this pull request Jul 2, 2020
11 tasks
prideout added a commit to prideout/cgltf that referenced this pull request Jul 13, 2020
Khronos has finalized the spec and is close to ratifying it. Filament
will be a reference implementation so we require cgltf support.

More information at:
KhronosGroup/glTF#1698

ThreeJS support is arriving here:
mrdoob/three.js#19690
prideout added a commit to prideout/cgltf that referenced this pull request Jul 13, 2020
Khronos has finalized the spec and is close to ratifying it. Filament
will be a reference implementation so we require cgltf support.

More information at:
KhronosGroup/glTF#1698

ThreeJS support is arriving here:
mrdoob/three.js#19690
@donmccurdy
Copy link
Collaborator Author

I don't think I'm going to be able to invest time improving the physical model any time soon either. 😞 Would it be OK to merge the transparency -> transmission renaming, and introduce transmissionMap, in the meantime?

prideout added a commit to prideout/cgltf that referenced this pull request Jul 16, 2020
Khronos has finalized the spec and is close to ratifying it. Filament
will be a reference implementation so we require cgltf support.

More information at:
KhronosGroup/glTF#1698

ThreeJS support is arriving here:
mrdoob/three.js#19690
@donmccurdy donmccurdy force-pushed the feat-gltfloader-transmission branch from 21a782e to b2e251e Compare July 17, 2020 04:35
@donmccurdy donmccurdy marked this pull request as ready for review July 17, 2020 04:37
@donmccurdy donmccurdy changed the title MeshPhysicalMaterial: Implement KHR_materials_transmission. MeshPhysicalMaterial: Rename .transparency → .transmission, add .transmissionMap Jul 17, 2020
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jul 17, 2020

Ok, I think this is ready for review. I've pulled the glTF example out for now, since we aren't implementing enough of the transmission physical model to display the glTF asset as intended.

@mrdoob mrdoob merged commit 0de94ab into mrdoob:dev Jul 17, 2020
@mrdoob
Copy link
Owner

mrdoob commented Jul 17, 2020

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Jul 17, 2020

I guess we should remove

if ( json.transparency !== undefined ) material.transparency = json.transparency;

from MaterialLoader?

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.

MeshPhysicalMaterial: Add .transparencyMap
5 participants