Wrong pseudocode in BRDF reference implementation

[lisyarus]

I believe that the pseudocode for a reference BRDF implementation in Appendix B, in the end of section 3.5, is wrong.

The BRDF described prior to that in Appendix B can be formulated as

fresnel(f0) = f0 + (1 - f0) * (1 - abs(VdotH))^5

metal_fresnel = fresnel(color)
dielectric_fresnel = fresnel(0.04)

metal_brdf = specular * metal_fresnel
dielectric_brdf = mix(diffuse, specular, dielectric_fresnel)

brdf = mix(dielectric_brdf, metal_brdf, metallic)

However, the final pseudocode is doing something different. First, it notes that we can pretend that metallic brdf also has a fake diffuse component equal to zero, and rewrites

metal_brdf = mix(0, specular, metal_fresnel)

which is a correct thing to do. Then, the code rewrites the BRDF computation by first computing a "mixed" Fresnel term and then using it to interpolate between the "mixed" diffuse term and the common specular term:

mixed_diffuse = mix(0, diffuse, metallic)
mixed_fresnel = mix(metal_fresnel, dielectric_fresnel, metallic)
brdf = mix(mixed_diffuse, specular, mixed_fresnel)

Which is not equal to the original formulation. If we combine all the mixes together, the problem boils down to

mix(
    mix(diffuse, specular, dielectric_fresnel),
    mix(0, specular, metal_fresnel),
    metallic
)

being not equal to

mix(
    mix(diffuse, 0, metallic),
    specular,
    mix(dielectric_fresnel, metal_fresnel, metallic)
)

One simple way to see that these two expressions are not equivalent is to note that the latter contains terms proportional to metallic^2 after fully expanding the mixes (because metallic is in both the first and the third arguments of the outer mix), while the former only contains terms linear in metallic.

The difference is non-negligible: if the BRDF is implemented using the reference pseudocode, the scene appears darker (top image) compared to an implementation based directly on the material description in section 2.4 (bottom image), as demonstrated by this Cornell Box rendering using a Monte-Carlo path tracer:

(Please, ignore the occasional fireflies, they are irrelevant to the discussion).

Curiously, the difference disappears if one only uses materials with metallic = 0 or metallic = 1, suggesting that metallic^2 terms are indeed the root of the problem.

I propose to rewrite the pseudocode in the following way, which also more directly corresponds to the material description that precedes the code:

specular = D(α) * G(α) / (4 * abs(VdotN) * abs(LdotN))
diffuse = (1 / π) * baseColor.rgb

metal_fresnel = f0 + (1 - f0) * (1 - abs(VdotH))^5
dielectric_fresnel = 0.04 + (1 - 0.04) * (1 - abs(VdotH))^5

metal_brdf = specular * metal_fresnel
dielectric_brdf = mix(diffuse, specular, dielectric_brdf)

material = mix(metal_brdf, dielectric_brdf, metallic)

I'd be happy to make a pull request myself (in case I'm not misunderstanding things, of course!).

[proog128]

This is indeed a bug in the pseudocode which has been there for a very long time, thanks for your detailed report and derivations. Your proposal looks good, it's also more readable than the old version. However, it will change the look of the material, which might have to be discussed @emackey. This particular code snippet in the "new" Appendix B is identical to the one in the "old" Appendix B (before the rewrite), I copied this bug without noticing. Because it's only in the non-normative part, I suppose the change is fine. Might be a good idea to also check the sample viewer, it might also be affected.

I'd be happy to make a pull request myself (in case I'm not misunderstanding things, of course!).

Since you change the naming of some terms, it's required to adapt the extension too. I think KHR_materials_specular, _ior, _clearcoat, and maybe also _iridescence have to be updated.

[emackey]

Thanks for finding this and the great writeup @lisyarus! I think @bsdorra is going to take a swing at a fix that includes the updates to other extensions mentioned by @proog128. We'd love to hear feedback on the fix once the PR goes online.