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

Implement glTF extension KHR_materials_clearcoat #12006

Merged
merged 24 commits into from
May 31, 2024
Merged

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented May 30, 2024

Description

This PR implements the glTF extension KHR_materials_clearcoat in physically-based rendering of models.

How it works (and how it affects the code)

Parsing the extension at load time

When the glTF is loaded, GltfLoader reads factors and loads textures from the extension. See the new loadClearcoat method, which is very similar to the loadSpecular method from #11970. The factors and textures are then attached to the material. See the updates to the Material class in ModelComponents.

Processing the extension properties in the pipeline

In MaterialPipelineStage, if material.clearcoat is defined, its properties are processed by the new method processClearcoatUniforms, which is very similar to processSpecularUniforms.

The clearcoat extension, if used, will extend the metallic roughness model. As a result, the check for the extension usually takes place inside the same code branch that handles metallic-roughness processing.

Using the extension in the shader

In MaterialStageFS, the clearcoat "factor" and roughness are retrieved from uniforms and/or textures. See the new setClearcoat function. The clearcoat surface normal defaults to the geometry normal, ignoring the normal map from the underlying surface. This makes the clearcoat smooth by default. If a user provides a normal map for the clearcoat itself, this is loaded in the new method getClearcoatNormalFromTexture. See also the changes to the czm_modelMaterial struct.

The clearcoat is rendered as a layer on top of the base layer. The clearcoat layer modifies the reflection from the base layer below due to transmission losses. It also adds its own specular reflection. See the rewritten computePbrLighting function in LightingStageFS, as well as the new addClearcoatReflection function in the same file.

Since the reflection from the clearcoat layer itself is specular only, the functions for direct and image-based lighting (in pbrLighting.glsl and ImageBasedLightingStageFS.glsl had to be significantly refactored to separate the diffuse and specular components. This refactoring makes for a messy code diff, but the end result is unchanged for materials without a clearcoat.

Other changes

  • Added docstrings throughout
  • Renamed some variables for clarity (v becomes viewDirection, l becomes lightDirection, etc).
  • Added glsl helper function czm_maximumComponent to find the largest component of a vector (this is used several times in PBR calculations).
  • Removed the struct czm_pbrParameters, since all information was simply copied from the czm_modelMaterial. Relevant functions now input the material directly. Constructor methods czm_defaultPbrMaterial, czm_pbrMetallicRoughnessMaterial and czm_pbrSpecularGlossinessMaterial were also removed.
  • A constant coordinate transform yUpToZUp for IBL texture lookups is now applied on the CPU, rather than constructing and applying it in the fragment shader.
  • Updated promise handling in MaterialPipelineStageSpec to use async/await.
  • Fixed bug in specular calculation for environment map IBL. See Error in specular calculation for image-based lighting #12008.

Issue number and link

Resolves #11995. Resolves #12008.

Testing plan

Load the development Sandcastle glTF PBR Clearcoat and toggle through the testing models, comparing to the rendering in the reference implementation.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@ggetz
Copy link
Contributor

ggetz commented May 30, 2024

@jjhembd The check-cla failure is unrelated. I'll open a separate PR to fix.

@ggetz
Copy link
Contributor

ggetz commented May 30, 2024

Fix over in #12007

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @jjhembd! I tested this out and it is looking good when compared to the reference implementation. I have a few comments.

As a quick follow-up to this PR, I would suggestion moving the developer examples for these PBR extensions into a single streamlined showcase example.

packages/engine/Source/Scene/GltfLoader.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Model/Model.js Show resolved Hide resolved
*/
float computeDirectSpecularStrength(vec3 normal, vec3 lightDirection, vec3 viewDirection, vec3 halfwayDirection, float roughness)
{
float NdotL = clamp(dot(normal, lightDirection), 0.001, 1.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you're preventing a divide by 0 error within the above function with this clamp, right?

Can we move that logic to the respective function so its more obvious what's happening?

@jjhembd
Copy link
Contributor Author

jjhembd commented May 31, 2024

Thanks @ggetz for the quick feedback!
I tried to clarify what we are doing in pbrLighting to avoid divide-by-zero errors. I also opened #12010 to discuss remaining questions about our geometric shadowing function.
I will open a follow-up PR to consolidate the new PBR Sandcastles.

@ggetz
Copy link
Contributor

ggetz commented May 31, 2024

Thanks @jjhembd! Thanks for explaining and writing up the additional issue.

When running locally, I'm seeing quite a few failing tests due to the error RuntimeError: Fragment shader failed to compile. Compile log: ERROR: 0:786: 'iblColor' : undeclared identifier. Can you please take a look?

@jjhembd
Copy link
Contributor Author

jjhembd commented May 31, 2024

When running locally, I'm seeing quite a few failing tests due to the error RuntimeError: Fragment shader failed to compile. Compile log: ERROR: 0:786: 'iblColor' : undeclared identifier. Can you please take a look?

@ggetz sorry I missed that! I fixed both this and another shader compile error. All tests now run through locally for me.

@ggetz
Copy link
Contributor

ggetz commented May 31, 2024

All good! Thanks @jjhembd. This all looks good to me.

@ggetz ggetz merged commit 16674c1 into main May 31, 2024
9 checks passed
@ggetz ggetz deleted the gltf-pbr-clearcoat branch May 31, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants