Wrong pseudocode in BRDF reference implementation
lisyarus opened this issue · 2 comments
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!).
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.