-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Avoid possible 'divide by 0' in G_GGX_Smith function #8349
Conversation
@bhouston looks ok? |
This seems reasonable. I notice as well in the other D_GGX there is this:
If roughness (e.g. a2) is 0, and dotNH is also 1, denom can be zero as well here right? I am not sure if that is a common case though. |
Yes I was looking at that too, but I think there is a safeguard against this in the following line:
Is this clamp called no matter what the user sets roughness to? If it ends up being a minimum of 0.04, then there should be no cause for concern. However, if the user can dynamically change the roughness after this line can be called, there could be a potential divide by zero. That's how I'm reading this, could be mistaken though :-) |
Nice catch. Sweet. :) I think this is a fine PR to merge. |
BTW the GPU path tracing stuff of yours seems very promising - although I do not completely understand it if it can be competitive with renderers like Octane or RedShift. Did you notice I have contributed some physically-based lighting stuff as well that allows one to use real-world light units, probably useful. |
@@ -62,7 +62,9 @@ float G_GGX_Smith( const in float alpha, const in float dotNL, const in float do | |||
|
|||
float gv = dotNV + pow( a2 + ( 1.0 - a2 ) * pow2( dotNV ), 0.5 ); | |||
|
|||
return 1.0 / ( gl * gv ); | |||
float denominator = ( gl * gv ) + 0.00001; // avoid possible divide by 0 on the next line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I would arbitrarily add a constant to one term of the BRDF without studying how the product of all the terms behaves in the limit.
Besides, I was under the assumption that we specifically bounded roughness away from zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We bounded roughness, but a2 here is pow4( roughness) which on low precision mobile devices may be able to go to zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only seen this issue arise on low precision mobile devices. a2 = pow4(0.04) = 0.00000256. Mediump float range is [2^-14, 2^14] according to the spec. Many mobile devices are limited to mediump float precision. That means the smallest number is 0.00006103515, which is larger than pow4(0.04), which means pow4(0.04) will round to zero. I believe this explains the issue.
I would prefer to implement the models exactly as they are specified in the literature, and control the inputs, instead. If you have evidence the denominator is zero, we need to figure out why that is happening. |
Hi Ben, thanks for checking out my three.js path tracing renderer! It is more of an experiment for me at this stage as I don't think I can ever compete with Brigade, my inspiration. At this stage I am using raymarching distance fields so that all of the three.js core shapes (as well as new analytical shapes that are hard to classically ray trace) can be called by an inexperienced three.js user and just have it appear on their screen. Kind of how the current ray tracing canvas renderer works, but rendering 30 to 60 frames per second the same quality of scene. |
@WestLangley Yes the roughness is safely clamped, but like I mentioned, I don't know if the user can circumvent that clamp by dynamically changing the roughness while the program is running. As for adding the epsilon, the roughness or alpha is not the problem, the dotNV and dotNL are I believe. If one of those is zero for some reason, we will have a divide by zero later. We could clamp the dotNV and dotNL to 0.00001 earlier in the BRDF_Specular_GGX function (instead of 'saturate' to 0.0), or just stick this epsilon in once at this location (with a comment in the source code). I didn't want to clamp all of the dot products if we didn't have to - just this one denominator occurrence that has a potential divide by 0. I have seen other people clamp the dot products to 0.00001 or some small number elsewhere when using PBR, but I'd have to hunt an example down on Shadertoy.com or somewhere. |
@erichlof If you have evidence there is a problem (with symptoms), we need to determine the cause and fix it properly. Currently, we are bounding roughness away from absolute zero, which I believe is sufficient to prevent a zero divide. |
I finally found an occurrence of this clamping to avoid 'divide by 0' artifacts in literature: and here in a less-formal blog form: (bottom of article), 4th comment down, posted by blog author 'Simon' |
@WestLangley I had to add the first fix in my private ThreeJS fork over a year ago as well. Thus it is something that is needed on (low precision?) mobile devices. I am all for the most accurate way of doing this though. |
I'll post this here in the main thread to be clear: I have only seen this issue arise on low precision mobile devices. The limit of roughness if 0.04. But in the formula of interest to @erichlof the issue is actually caused by a2, which is pow4(roughness). The limit of a2 is pow(0.04) = 0.00000256. Mediump float range is [2^-14, 2^14] according to the spec. Many mobile devices are limited to mediump float precision. That means the smallest number is 0.00006103515, which is larger than pow4(0.04), which means pow4(0.04) will round to zero. I believe this explains the issue. It also means that the other equation I brought up can also go to zero on medium precision mobile devices because a2 can be rounded to zero. |
In my private fork I added minimums to both of these equations over a year ago, but I did it really quickly without actually working through the math. |
@bhouston Thank you for your insight. We are making some progress. First, an aside. The shader code as currently implemented is written to be clear. It has not been optimized for performance or for robustness. So the questions are:
(I would be glad to experiment myself, but I do not have a device with which I can replicate the problem.) Once we have the answers to these questions, I am sure we can come up with a modification that makes sense. I am hopeful that controlling the remapping of roughness is all that is required. Originally, I believe we were using the Disney formulation, and Then, @bhouston was not able to achieve perfect mirror reflections, and roughness was instead clamped to [0.04, 1]. The clamped value is still squared, however. Note: "remapping" means Note, we can remap roughness to a different range for the purpose of indexing into the environment map; we do not have to use the same remapping formula. We can do whatever is visually appealing. The only thing that is important is that |
In the Frostbite 3 paper (which deals with console games, thus they never have to deal with mediump precision), they do deal with artifacts. See page 12, table 2, line 32:
|
I've implemented a quick minimal fix for the ggx artifacts based on the Frostbite 3 solution: |
BTW I do not mind remapping roughness or clamping it to the range (0.04, 1.0). The difference between clamping versus remapping is likely visually equivalent. |
Agreed. It just matters if the 0.04 is significantly increased. Also, the squaring
is also optional. (edit: It is so the perceived roughness is visually linear.) In that case, the clamping can be after squaring, instead of before, if that helps. |
@erichlof Interesting. I didn't realize you has to implement everything analytically. That really reduces its applicability unfortunately - argh. Very few real-world scenes are able to be easily expressed analytically. I think that Brigade and Octane and the others use compute shaders (CUDA, OpenCL or the OpenGL compute shader) these days to do the complex intersection stuff. Unfortunately, not even WebGL 2 has the compute shader in it: https://www.khronos.org/webgl/public-mailing-list/archives/1406/msg00002.php |
@bhouston Yes the ray-marching distance field technique has its advantages and drawbacks versus classical ray tracing. I'm sure you've seen the great iq's (Inigo Quilez) and others' fragment shaders demonstrating the power of Ray-Marching: As great as Ray-Marching is however, classical Ray-Tracing still wins out when trying to intersect mesh triangle geometry (with almost every 3d model out there being composed of triangles). I might implement both in the future to compare them and see which is more useful, especially on top of three.js's framework. That's interesting about CUDA and compute shaders - so that's how Brigade did it! I went down the WebGL path for this project because I really like using three.js (inside browsers which are ubiquitous) as do so many other devs out there. I'll keep an eye on the compute shader for WebGL 2.1 (thanks for that link!). Maybe at that point I can add classical ray-triangle intersection and my renderer will actually be able to load an .obj model file for instance. Until then, it'll have to be analytical distance fields - however most can be used like a toolbox, and you just copy and paste them into your fragment shader one at a time. Here's a nice compilation from iq: I'll keep you guys posted if I hear of anything new or have any breakthroughs! :) |
@WestLangley What should we do with this PR? |
@mrdoob I'll be revisiting the shaders over the next few months. I'll make a note of it. |
Closing in favor of #8353. |
As discussed in #8348 , this PR adds a small epsilon to the denominator at the end of the G_GGX_Smith function. Previously, if either of the gl or gv terms were 0.0, on the following 'return' line, there would be a divide by 0 issue.