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

GLTFExporter: Add KHR_materials_clearcoat support. #22761

Merged
merged 1 commit into from
Oct 30, 2021
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Oct 30, 2021

Related issue: Fixed #22751.

Description

Adds support of KHR_materials_clearcoat to GLTFExporter.

@mrdoob The PR also fixes an issue in context of KHR_materials_volume ( #22214). Right now, the exporter always includes this extensions when MeshPhysicalMaterial is used because the default of MeshPhysicalMaterial.thickness is 0.01.

According to the KHR_materials_volume spec, the extension always needs to be combined with another one like KHR_materials_transmission. The early-out condition of GLTFMaterialsVolumeExtension now checks for transmission usage instead.

Besides, any reasons why the default of MeshPhysicalMaterial.thickness is 0.01? Changing it to 0 seems more glTF conform.

@mrdoob
Copy link
Owner

mrdoob commented Oct 30, 2021

Besides, any reasons why the default of MeshPhysicalMaterial.thickness is 0.01? Changing it to 0 seems more glTF conform.

Changing it to 0 sounds like a better solution to me. Would you like to do that in a different PR?

@mrdoob mrdoob added this to the r135 milestone Oct 30, 2021
@mrdoob mrdoob merged commit 0943d7f into mrdoob:dev Oct 30, 2021
@mrdoob
Copy link
Owner

mrdoob commented Oct 30, 2021

Thanks!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 30, 2021

Would you like to do that in a different PR?

Yes, I do!

@optimus007
Copy link

Side note, when a glb with transmission is exported,

@donmccurdy 's gltf validator throws
a "value not in range" error with message "value 0 is out of range"

And it's pointing to "KHR_materials_volume/attunationDistance"

Maybe that also could be looked into while updating the thickness @Mugen87

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 30, 2021

It seems the three.js default 0 is no valid value for attenuationDistance according to the glTF spec.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 30, 2021

@donmccurdy What do you recommend to fix this warning of glTF validator? Should we target a different default value for MeshPhysicalMaterial.attenuationDistance or enhance the exporter with a new condition instead?

@donmccurdy
Copy link
Collaborator

@Mugen87 The specification for KHR_materials_volume is not as clear as I'd like on this, so I've opened KhronosGroup/glTF#2090 and there may be some other comments there.

The default attenuation distance for glTF is +Inf, implied when the attenuationDistance property is omitted. Is that what we're using 0 to achieve? I think that's OK if so, assuming we're avoiding the divide-by-zero situation. We can just include a condition in the exporter:

if ( material.attenuationDistance > 0 ) {

  extensionDef.attenuationDistance = material.attenuationDistance;
  extensionDef.attenuationColor = material.attenuationColor.toArray();

}

@donmccurdy
Copy link
Collaborator

^KhronosGroup/glTF#2090 is resolved.

@mrdoob
Copy link
Owner

mrdoob commented Nov 11, 2021

Is there anything we have to do on our side?

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.

GLTFExporter add support for exporting clearcoat data
4 participants