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_specular #11970

Merged
merged 30 commits into from
May 20, 2024
Merged

Implement glTF extension KHR_materials_specular #11970

merged 30 commits into from
May 20, 2024

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented May 7, 2024

Description

This PR implements the glTF extension KHR_materials_specular 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. These are then attached to the material. See the updates to the Material class in ModelComponents.

Note that GltfLoader has been refactored to better handle multiple PBR extensions. See the methods loadMetallicRoughness and loadSpecularGlossiness which have been pulled out of loadMaterial, as well as the new method loadSpecular which handles the new extension. As part of this refactoring, I reworked many of the relevant private methods to use smaller function signatures (taking advantage of properties stored on the loader) and added docstrings.

Processing the extension properties in the pipeline

In MaterialPipelineStage, if material.specular is defined, its properties are processed by the new method processSpecularUniforms, which is very similar to the existing methods processSpecularGlossinessUniforms and processMetallicRoughnessUniforms.

The specular 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.

The specular extension has some naming conflicts with the old specular glossiness extension. To avoid confusion, I added "legacy" to the specular glossiness names. For example, the existing uniform u_specularFactor becomes u_legacySpecularFactor, and the corresponding define HAS_SPECULAR_FACTOR becomes HAS_LEGACY_SPECULAR_FACTOR.

Using the extension in the shader

Most of the shader changes are in MaterialStageFS. The specularColorFactor (computed from both specularColorFactor and specularColorTexture is used to modify the f0 value in the czm_modelMaterial (which is somewhat unfortunately named material.specular).

The specularWeight (computed from specularFactor) is then passed through to pbrLighting and ImageBasedLightingStageFS.

Note that the implementation in ImageBasedLightingStageFS is incomplete, as the specularWeight factor is only used to scale down the specular component. The specularWeight factor should also modify the diffuse component in a more complicated way, but this will require some reworking of our image-based lighting calculations.

Other code style updates

Reduced nesting of compiler directives

Some of the material and lighting shader code was difficult to read due to deeply nested compiler directives. The readability problem was exacerbated by inconsistent indentation. For example, in MaterialStageFS, we had:

void materialStage(...)
{
    // ...
    #if defined(LIGHTING_PBR) && defined(USE_SPECULAR_GLOSSINESS)
        #ifdef HAS_SPECULAR_GLOSSINESS_TEXTURE
        vec2 specularGlossinessTexCoords = TEXCOORD_SPECULAR_GLOSSINESS;
          #ifdef HAS_SPECULAR_GLOSSINESS_TEXTURE_TRANSFORM
          specularGlossinessTexCoords = computeTextureTransform(specularGlossinessTexCoords, u_specularGlossinessTextureTransform);
          #endif

        vec4 specularGlossiness = czm_srgbToLinear(texture(u_specularGlossinessTexture, specularGlossinessTexCoords));
        vec3 specular = specularGlossiness.rgb;
        float glossiness = specularGlossiness.a;
            #ifdef HAS_SPECULAR_FACTOR
            specular *= u_specularFactor;
            #endif
// ...

One popular style guide recommends that all preprocessor directives start at the beginning of the line, and that the directives should not affect the indentation of the rest of the code. This would be a significant change to all our shaders, so I did not try to make that change now. Instead, I followed the example of the reference implementation, and tried to limit nesting to 2 levels, by adding subroutines. The above example from MaterialStageFS now becomes:

void setSpecularGlossiness(inout czm_modelMaterial material)
{
    #ifdef HAS_SPECULAR_GLOSSINESS_TEXTURE
        vec2 specularGlossinessTexCoords = TEXCOORD_SPECULAR_GLOSSINESS;
        #ifdef HAS_SPECULAR_GLOSSINESS_TEXTURE_TRANSFORM
            specularGlossinessTexCoords = computeTextureTransform(specularGlossinessTexCoords, u_specularGlossinessTextureTransform);
        #endif

        vec4 specularGlossiness = czm_srgbToLinear(texture(u_specularGlossinessTexture, specularGlossinessTexCoords));
        vec3 specular = specularGlossiness.rgb;
        float glossiness = specularGlossiness.a;
        #ifdef HAS_LEGACY_SPECULAR_FACTOR
            specular *= u_legacySpecularFactor;
        #endif
//...
void materialStage(...)
{
    // ...
    #if defined(LIGHTING_PBR) && defined(USE_SPECULAR_GLOSSINESS)
        setSpecularGlossiness(material);
    #elif //...

This affects both MaterialStageFS and ImageBasedLightingStageFS.

Cleaner loop structure

Some loops with pre-declared counters were cleaned up to better conform to the Coding Guide. This primarily affects GltfLoader.

Options object destructuring

While reading some functions with the arguments supplied as options objects, I found it easier to understand the arguments if I rewrote this:

  const gltfResource = options.gltfResource;
  const typedArray = options.typedArray;
  const releaseGltfJson = defaultValue(options.releaseGltfJson, false);
  const asynchronous = defaultValue(options.asynchronous, true);
  const incrementallyLoadTextures = defaultValue(
    options.incrementallyLoadTextures,
    true
  );
  // ...

as an object destructuring with default values, like this:

  const {
    gltfResource,
    typedArray,
    releaseGltfJson = false,
    asynchronous = true,
    incrementallyLoadTextures = true,
    // ...
  } = options;

This primarily affects GltfLoader, ResourceCache, and ResourceCacheKey.

Issue number and link

Resolves #11938.

Testing plan

Load the development Sandcastle glTF PBR Specular and compare 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 Sandcastle from server.
  • Investigate color factor vs texture rendering differences
  • Add image comparisons

Copy link

github-actions bot commented May 7, 2024

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 9, 2024

Here are a few image comparisons.

With extension disabled (as it would be rendered before this PR):
image

Extension enabled: Note the bottom row becoming more mirror-like.
image

Using procedural IBL, scaled up to exaggerate effect, with extension disabled:
image

Using procedural IBL, scaled up to exaggerate effect, with extension enabled:
image

Compare to the reference implementation:
image

@jjhembd
Copy link
Contributor Author

jjhembd commented May 10, 2024

@ggetz this is ready for a first look.
I still need to load the Sandcastle dataset from a server, and also make a smaller dataset for the specs. But the code is all there.

@ggetz
Copy link
Contributor

ggetz commented May 10, 2024

Thanks @jjhembd, this appears to be working well! A few top-level comments:

  • Re: Reduced nesting of compiler directives, do you think it'd be appropriate to add this to the coding guide?
  • The GltfPipeline folder is actually copied from https://github.com/CesiumGS/gltf-pipeline. Any updates in that folder will need to be reflected back there.
  • Can you write up an issue (or link to an existing one) for the remaining IBL improvements we plan on making?

float VdotH = clamp(dot(v, h), 0.0, 1.0);

vec3 f0 = pbrParameters.f0;
float reflectance = max(max(f0.r, f0.g), f0.b);
vec3 f90 = vec3(clamp(reflectance * 25.0, 0.0, 1.0));
vec3 f90 = vec3(clamp(reflectance * 25.0, 0.0, 1.0)); // vec3(1.0) for dielectric.
Copy link
Contributor

Choose a reason for hiding this comment

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

This may just be for my benefit, but would you mind explaining this a bit more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just now learning this myself!
f0 is a property of the material, indicating the color of the specular reflection at normal incidence. In most cases it is only relevant for metallic materials. For example, gold will have a yellowish reflection.

f90 is the computed reflection at grazing incidence (90° from the normal).

For non-metallic (or 'dielectric') materials, the color comes from the diffuse reflection. The specular component is weaker, and assumed to be white. f0 is therefore typically set to a small constant, usually 0.04. This simplifying assumption is so pervasive (e.g., in tutorials) that f90 is also often set to a constant 1. See for example this comment in the Khronos reference implementation:

    // Compute reflectance.
    float reflectance = max(max(materialInfo.f0.r, materialInfo.f0.g), materialInfo.f0.b);

    // Anything less than 2% is physically impossible and is instead considered to be shadowing. Compare to "Real-Time-Rendering" 4th editon on page 325.
    materialInfo.f90 = vec3(1.0);

In our code, we have a formula that I haven't seen much elsewhere--I assume it is a less simplified approximation. But if f0 is the typical 0.04, the above formula will give f90 = 1.0, so in the common case we are consistent with the reference implementation.

I updated the comment to be a little more explanatory.

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

jjhembd commented May 17, 2024

@ggetz this is ready for another look. For some of the points you raised:

@lilleyse could you take a first look at the shaders and see if the strategy makes sense? The key changes are in MaterialStageFS, pbrLighting, and ImageBasedLightingStageFS.

Please note that the IBL implementation is incomplete: it scales the specular component correctly, but we are missing a corresponding adjustment to the diffuse component. But our diffuse component needs work, so I think it makes sense to wait until the bigger IBL update.

@jjhembd jjhembd requested a review from lilleyse May 17, 2024 00:02
@lilleyse
Copy link
Contributor

@jjhembd at a quick glance the shader code looks good, it looks like it closely matches the Khronos reference implementation.

@ggetz
Copy link
Contributor

ggetz commented May 20, 2024

Thanks @jjhembd! This looking good to me. Aside from the IBL modifications, any remaining work is summed up in your linked issues above.

Please open an issue for the IBL changes when you can.

As we talked about offline, once the initial extension support is complete for this, _materials_anisotropy, and KHR_materials_clearcoat, it makes sense to quickly follow up with the IBL improvements.

@ggetz ggetz merged commit 6c462b7 into main May 20, 2024
9 checks passed
@ggetz ggetz deleted the gltf-pbr-specular branch May 20, 2024 15:00
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.

Add support for glTF KHR_materials_specular extension
3 participants