KhronosGroup
Changing the masking-shadowing function referenced in the 2.0 specifications
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
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?
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!
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.
