Changing the masking-shadowing function referenced in the 2.0 specifications

[49iohcy]

Hello,

It has recently dawned on me that the GGX visibility term implemented in the glTF-Sample-Renderer as of now uses the height-correlated form of the Smith joint masking-shadowing function, instead of the alleged separable form in the spec.

I believe this would be a good revision if the intention is to maintain reasonable consistency between the sample implementation in the specification and the glTF-Sample-Renderer project.

Provided are links to the most up-to-date version of glTF-Sample-Renderer/source/Renderer/shaders/brdf.glsl, as well as the full expansions of both the height-correlated and separable forms from Heitz (2014).

Please let me know if you have any questions. Thank you.

• https://github.com/KhronosGroup/glTF-Sample-Renderer/blob/e6b052db89fb2adbaf31da4565a08265c96c2b9f/source/Renderer/shaders/brdf.glsl#L74
• https://observablehq.com/@n-a33/smith-joint-masking-shadowing-function

[emackey]

Hi @49iohcy, thanks for submitting this! We talked about this PR a little on today's PBR TSG call, but some of the members wanted more time to review it. The group agrees that if the glTF Sample Viewer is doing a better job than section B.3 here, then a change like this is likely warranted, particularly if it brings the two into better alignment.

I notice that the height-correlated masking and shadowing function is also mentioned in section B.3.6.1. Masking-Shadowing Term and Multiple Scattering. Does this section need a tweak as well?

[proog128]

In KHR_materials_transmission there is another mention of the G term which should be changed as well.

I did a bit of digging to find out when and why this diverged, but didn't find anything useful. The glTF Sample Viewer implemented the separable function until April 2019, then changed it to height-correlated (3359d5). The rewrite of Appendix B started off with height-correlated in December 2019 (d4319f), but switched to separable in August 2020 (f513c9), without mentioning this specifically, neither in the Git commit nor in the pull request (#1717). Most likely an accident.

Anyways, looks good and thanks for bringing it up!

[UX3D-haertl]

It makes sense to adjust this to reflect the current sample viewer implementation.
Filament also defines a simplified version of the height-correlated form. This could be added additionally as another alternative.

[emackey]

I got the CI to run by pushing a copy of this branch to the main repo. The section in question is B.3.2. Specular BRDF. (Also note, the currently-published version is not up-to-date with main, and fixes such as #2403 were merged but not yet published. The diffs shown here are comparing the top of main to what's in this PR, not what's currently published.)

Diff 1

The first formula changes from this, the current version:

To this new version:

Diff 2

The second formula changes from this, the current version:

To this new version:

(Also note, the above are screenshots of the rendered HTML. I also looked at the rendered PDF versions, and they look the same as their HTML counterparts, no rendering errors.)

Also, much thanks to @proog128 for digging through the history of this! Do the newly-rendered formulas look OK?