search

mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.52k stars 35.37k forks source link

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

Closed erichlof closed 1 month ago

erichlof commented 8 years ago
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
bhouston commented 8 years ago

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" notifications@github.com 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 https://github.com/mrdoob/three.js/issues/8348.

erichlof commented 8 years ago

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 commented 8 years ago

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

erichlof commented 8 years ago

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