-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
Thank you for the pull request, @jjhembd! ✅ We can confirm we have a CLA on file for you. |
Here are a few image comparisons. With extension disabled (as it would be rendered before this PR): Extension enabled: Note the bottom row becoming more mirror-like. Using procedural IBL, scaled up to exaggerate effect, with extension disabled: Using procedural IBL, scaled up to exaggerate effect, with extension enabled: Compare to the reference implementation: |
@ggetz this is ready for a first look. |
Thanks @jjhembd, this appears to be working well! A few top-level comments:
|
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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 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 at a quick glance the shader code looks good, it looks like it closely matches the Khronos reference implementation. |
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, |
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 theMaterial
class inModelComponents
.Note that
GltfLoader
has been refactored to better handle multiple PBR extensions. See the methodsloadMetallicRoughness
andloadSpecularGlossiness
which have been pulled out ofloadMaterial
, as well as the new methodloadSpecular
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
, ifmaterial.specular
is defined, its properties are processed by the new methodprocessSpecularUniforms
, which is very similar to the existing methodsprocessSpecularGlossinessUniforms
andprocessMetallicRoughnessUniforms
.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
becomesu_legacySpecularFactor
, and the corresponding defineHAS_SPECULAR_FACTOR
becomesHAS_LEGACY_SPECULAR_FACTOR
.Using the extension in the shader
Most of the shader changes are in
MaterialStageFS
. The specularColorFactor (computed from bothspecularColorFactor
andspecularColorTexture
is used to modify thef0
value in theczm_modelMaterial
(which is somewhat unfortunately namedmaterial.specular
).The
specularWeight
(computed fromspecularFactor
) is then passed through topbrLighting
andImageBasedLightingStageFS
.Note that the implementation in
ImageBasedLightingStageFS
is incomplete, as thespecularWeight
factor is only used to scale down the specular component. ThespecularWeight
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: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:This affects both
MaterialStageFS
andImageBasedLightingStageFS
.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:
as an object destructuring with default values, like this:
This primarily affects
GltfLoader
,ResourceCache
, andResourceCacheKey
.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
CONTRIBUTORS.md
CHANGES.md
with a short summary of my changeOther TODO: