-
Notifications
You must be signed in to change notification settings - Fork 24
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
Specular weight > 1 can cause energy gain in metals, and NaNs in dielectrics #224
Comments
Also, we currently say the |
Just to note, the implementation would be simpler if we just removed the facility to set That also avoids the issue described here, i.e. having to insert awkward clamping logic to avoid unphysical energy gain. |
The suggestion above is implemented in #228 |
Here are a few observations on our side: Specular weight for metalI don't have any feedback on this part and I suspect (to be confirmed) it is not a use case for us. As for the solution proposed to avoid energy gain, I agree with the suggestion of clamping per channel. It could result in slightly unintuitive behaviour due to the hue shift, but as this happens when stretching the domain of the material, I think that's an acceptable tradeoff. Specular weight for dielectricsI am still gathering feedback and, more importantly, actual use cases, so I don't have a definitive answer yet. Here are some elements I've noted so far.
From those observations, it looks like it might be harmless to clamp the weight, since scanned materials could always be processed. |
We decided to allow the specular weight > 1 case, and added the clamping logic to deal with it properly, in #238. So this can be closed. |
On testing we noticed that if
specular_weight > 1
and the material is metallic, then there is energy gain, e.g.:specular_weight
1specular_weight
6This happens because the
specular_weight
multiplies the metallic lobe as below:We forgot to add a clamp to prevent the Fresnel factor exceeding 1 when the weight exceeds 1 though, which IMO is a mistake in the spec itself. We could either clamp the
specular_weight
before multiplying, or clamp each component of the final metallic Fresnel factor. The latter is closer to what we do for the dielectric case, so probably best.Another issue we noted is that the formula below from the spec, for the dielectric Fresnel IOR ratio, produces$\epsilon=1$ then results (due to the clamp) which produces zero in the denominator (and this will later propagate into NaNs). This needs to be checked for and avoided, e.g. by clamping the denominator to be < 1.
inf
ifspecular_weight
is high, sinceThis is not really an error in the spec (though arguably it is, since an infinite IOR doesn't make physical sense), but it's a rather easy mistake to make in implementing it (and thus generate NaNs in the render), so I suggest we point out the need to clamp explicitly in the spec.
NB, I think this bug is also present in the current MaterialX implementation.
The text was updated successfully, but these errors were encountered: