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: Match behavior of attenuationDistance to KHR_materials_volume #24622

Merged
merged 2 commits into from
Sep 27, 2022

Conversation

zach-capalbo
Copy link
Contributor

Related issue: N/A

Description

This is an alternative approach to #24620 See also #24620 (comment)

The default attenuationDistance in MeshPhysicalMaterial is 0.0, which is functionally equivalent to Infinity (c.f.

)

The KHR_materials_volume spec requires that attenuationDistance be greater than zero if present. It defaults to Infinity if not present. (c.f. https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_materials_volume/README.md#properties )

The current GTLFExporter produces incorrect gltf files for MeshPhysicalMaterial if transmission is set greater than zero, but attenuationDistance is set to the default of zero. These incorrect files cannot be read in some popular GLTF software (like Blender 3.3 GLTF Importer)

This PR changes the default value of attenuationDistance to Infinity, and the behavior of 0 and Infinity to more closely align with the KHR_materials_volume specification.

Note: Only one of #24620 or this PR should be accepted. They are different approaches to the same problem.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 13, 2022

It seems webgl_nodes_loader_gltf_transmission is failing with this change.

@mrdoob
Copy link
Owner

mrdoob commented Sep 13, 2022

I do not think we can use Infinity...
JSON doesn't support Infinity so we wouldn't be able to de/serialize the value.

@WestLangley
Copy link
Collaborator

I do not think we can use Infinity...
JSON doesn't support Infinity so we wouldn't be able to de/serialize the value.

Why do we serialize default values sometimes -- and sometimes not?

@mrdoob
Copy link
Owner

mrdoob commented Sep 13, 2022

Why do we serialize default values sometimes -- and sometimes not?

Oh! That is a good point. The fact that Infinity is the default means we can ignore it when de/serializing it 👍

@WestLangley
Copy link
Collaborator

Right. But my larger point is the toJSON() method needs to be revisited. It is not at all consistent.

@zach-capalbo zach-capalbo force-pushed the infinite-attenuation-distance branch from 41e9552 to 1f8a77e Compare September 25, 2022 01:41
@zach-capalbo
Copy link
Contributor Author

It seems webgl_nodes_loader_gltf_transmission is failing with this change.

This should be fixed now. (Required an additional change to GLTFLoader to use the new default)

@mrdoob mrdoob added this to the r145 milestone Sep 27, 2022
@mrdoob mrdoob merged commit c08bcce into mrdoob:dev Sep 27, 2022
@mrdoob
Copy link
Owner

mrdoob commented Sep 27, 2022

Thanks!

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.

4 participants