Possible 'roughness-squared' typo and other bugs in BRDF_Specular_GGX function

[erichlof]

Description of the problem

@bhouston Hi Ben, I know you said you were going to be off for a couple of weeks due to another deadline, but I wanted to bring these possible typos/bugs to your attention. I'm pinging you because it appears that you are the one who maintains PBR part of the codebase.

On this line,
https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L87
alpha is set to roughness squared, which is fine, but later in these lines,
https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L98-L100
the already squared alpha is sent to these two functions as a parameter. Both of these functions again square the already squared alpha parameter here,
https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L59
and here,
https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L75

By the time alpha arrives at these methods, 'a2' is now pow2(alpha2) which is essentially 'a4'. Is this what is intended, or just a typo?

The other issue is that inside the G_GGX_Smith method, if either of the gl or the gv terms are 0.0, we will have a divide by zero issue on certain platforms:
https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L65

On my Samsung Galaxy 6 mobile for instance, the materials come out Black. Inserting a minimum epsilon to adhere to, avoids the possible divide by zero. Here's an example of what worked for me:
float gl = max( 0.00001, dotNL + pow( a2 + ( 1.0 - a2 ) * pow2( dotNL ), 0.5 ) );
float gv = max( 0.00001, dotNV + pow( a2 + ( 1.0 - a2 ) * pow2( dotNV ), 0.5 ) );
return 1.0 / ( gl * gv );
...

Three.js version

• Dev
• r74
• ...

Browser

• All of them
• Chrome
• Firefox
• Internet Explorer

OS

• All of them
• Windows
• Linux
• Android
• IOS

Hardware Requirements (graphics card, VR Device, ...)

[bhouston]

The roughness ^4 is a model choice. Frostbite 3 uses that. Can you make a
PR for the divide by zero issue though, that should clearly be fixed. I
actually already fixed that in a previous threejs fork.

Best regards,
Ben Houston
http://Clara.io Online 3d modeling and rendering
On Mar 12, 2016 4:11 PM, "erichlof" [email protected] wrote:

Description of the problem

@bhouston https://github.com/bhouston Hi Ben, I know you said you were
going to be off for a couple of weeks due to another deadline, but I wanted
to bring these possible typos/bugs to your attention. I'm pinging you
because it appears that you are the one who maintains PBR part of the
codebase.

On this line,

https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L87
alpha is set to roughness squared, which is fine, but later in these lines,

https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L98-L100
the already squared alpha is sent to these two functions as a parameter.
Both of these functions again square the already squared alpha parameter
here,

https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L59
and here,

https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L75

By the time alpha arrives at these methods, it is now essentially
pow4(alpha). Is this what is intended, or just a typo?

The other issue is that inside the G_GGX_Smith method, if either of the gl
or the gv terms are 0.0, we will have a divide by zero issue on certain
platforms:

https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L65

On my Samsung Galaxy 6 mobile for instance, the materials come out Black.
Inserting a minimum epsilon to adhere to, avoids the possible divide by
zero. Here's an example of what worked for me:
float gl = max( 0.00001, dotNL + pow( a2 + ( 1.0 - a2 ) * pow2( dotNL ),
0.5 ) );
float gv = max( 0.00001, dotNV + pow( a2 + ( 1.0 - a2 ) * pow2( dotNV ),
0.5 ) );
return 1.0 / ( gl * gv );
...
Three.js version

• Dev
• r74
• ...

Browser

• All of them
• Chrome
• Firefox
• Internet Explorer

OS

• All of them
• Windows
• Linux
• Android
• IOS

Hardware Requirements (graphics card, VR Device, ...)

—
Reply to this email directly or view it on GitHub
#8348.

[erichlof]

Hi Ben, ok roughness^4 understood now as a choice. Yes I'll make a PR for the divide by zero issue. Btw, thanks for all your PBR contributions to three.js - it's been a joy to see them in action! :)

[bhouston]

BTW you may just be allowed to add Epsilon to the denominator rather than using two max statements.

[erichlof]

Ahh yes, a better solution. I'll do the PR right now :)