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_anisotropy #11988

Merged
merged 13 commits into from
May 29, 2024
Merged

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented May 15, 2024

Merge #11970 first!

Description

This PR implements the glTF extension KHR_materials_anisotropy 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 loadAnisotropy 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.anisotropy is defined, its properties are processed by the new method processAnisotropyUniforms, which is very similar to processSpecularUniforms. The sine and cosine of the anisotropyRotation factor are pre-computed, and combined with anisotropyStrength into one vec3 uniform.

The anisotropy 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

The orientation of the anisotropic effect is defined relative to the "tangent" vector on the surface. The tangent vector is aligned in the direction of increasing u coordinate in the normal map. These tangent vectors are either supplied as a vertex attribute, or computed via derivatives of the normal map texture coordinates.

We already had code using and/or computing tangent vectors, but discarded the information early once the normals were prepared. To preserve the tangent information for anisotropic calculations, a new NormalInfo struct is defined, and returned from the new getNormalInfo function. Along the way, some of the tangent calculations were broken down into smaller functions to enable re-use for both the isotropic and anisotropic cases. The setAnisotropy function then stores the relevant anisotropic information on the material. See also the corresponding changes to the czm_modelMaterial and czm_pbrParameters structs.

The actual anisotropic lighting calculations can be found in pbrLighting and ImageBasedLightingStageFS. They only modify the specular reflection component of the final color.

Note that the implementation in ImageBasedLightingStageFS is complete only for the case of supplied environment maps in textureIBL. The proceduralIBL function will require a more significant rewrite as part of a wider IBL update, so the anisotropy calculation can be incorporated at that stage.

Other changes

Issue number and link

Resolves #11976. Resolves #11994.

Testing plan

Load the development Sandcastle glTF PBR Anisotropy and toggle through the 4 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

Other TODO:

  • Load test dataset for the Sandcastle from a server
  • Report normalization bug in IBL
  • Describe code changes
  • Add image comparisons
  • Explain non-implementation in procedural IBL
  • Clean up: use TBN matrix?

Copy link

Thank you for the pull request, @jjhembd!

✅ We can confirm we have a CLA on file for you.

@jjhembd
Copy link
Contributor Author

jjhembd commented May 17, 2024

Image comparisons

All images captured from the glTF PBR Anisotropy Sandcastle

"Barn lamp" test dataset

Before this PR:
image

With extension enabled. Notice the stretching of the specular reflection perpendicular to the brush marks in the metal.
image

Anisotropy strength test

Extension disabled:
image

Extension enabled:
image

Anisotropy disc test

The discs in each square are not visible without the extension.
image

@jjhembd jjhembd marked this pull request as ready for review May 17, 2024 23:45
@jjhembd
Copy link
Contributor Author

jjhembd commented May 17, 2024

@ggetz this is ready for review. But it is written on top of #11970 so we will want to merge that one first.

@ggetz
Copy link
Contributor

ggetz commented May 20, 2024

I'm seeing this blown-out areas on the barn lamp sample model when using non-procedural IBL:

image

It appears to be related to the image we're using for IBL, but is it expected to be this extreme? Or is this something that will be fixed with the model IBL follow up?

@@ -721,6 +723,19 @@ describe(
verifyRender(model, true);
});

it("renders model with the KHR_materials_anisotropy extension", async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting the following error when running this test:

     RuntimeError: Fragment shader failed to compile.  Compile log: ERROR: 0:467: 'normalTexCoords' : undeclared identifier
ERROR: 0:467: 'computeTangent' : no matching overloaded function found
ERROR: 0:467: '=' : dimension mismatch
ERROR: 0:467: '=' : cannot convert from 'const mediump float' to 'highp 3-component vector of float'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an error in my test dataset. For this extension, either tangents or a normal map are required.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Can we catch this early and provide a better error message?

packages/engine/Source/Scene/GltfLoader.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/GltfLoader.js Outdated Show resolved Hide resolved
@ggetz
Copy link
Contributor

ggetz commented May 20, 2024

Thanks @jjhembd! I left a few comments, but overall this looks close.

@jjhembd
Copy link
Contributor Author

jjhembd commented May 21, 2024

Thanks for the feedback @ggetz! I think I addressed everything.
I also see the rendering artifacts in the "barn lamp" sample model. It almost looks like our environment map is not normalized correctly. The artifacts appear even before this PR, so I think it is something we should look at as part of the IBL work.

@ggetz
Copy link
Contributor

ggetz commented May 29, 2024

@jjhembd All TODO's have been completed and this is ready to merge, correct?

@jjhembd
Copy link
Contributor Author

jjhembd commented May 29, 2024

@jjhembd All TODO's have been completed and this is ready to merge, correct?

@ggetz Yes, the only comment I haven't addressed is your question about whether we can catch a problem with a glTF that is not according to spec. I looked at this, but it is non-trivial.

We often ask users to run problematic models through the glTF-Validator. Would that be appropriate for this case, or do we want CesiumJS to identify model problems?

@ggetz
Copy link
Contributor

ggetz commented May 29, 2024

Your question about whether we can catch a problem with a glTF that is not according to spec. I looked at this, but it is non-trivial.

OK to skip this if it blows up the scope of this PR. Better error messages for models that do not conform to spec is something we've mentioned before, but I understand there's a trade off in terms of effort and performance.

@ggetz ggetz merged commit 3b39344 into main May 29, 2024
9 checks passed
@ggetz ggetz deleted the gltf-pbr-anisotropy branch May 29, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Normalization error in image-based lighting Add support for glTF KHR_materials_anisotropy extension
2 participants