这是indexloc提供的服务，不要输入任何密码
Skip to content

Fixed a bug where NdotV can be >1.0 #80

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 22, 2018
Merged

Fixed a bug where NdotV can be >1.0 #80

merged 1 commit into from
May 22, 2018

Conversation

corporateshark
Copy link
Contributor

Fixed a bug where NdotV can be >1.0 resulting in a black spot if used with the diffuse term from "Disney implementation of diffuse from Physically-Based Shading at Disney" by Brent Burley.

vec3 diffuse(PBRInfo pbrInputs)
{
    float f90 = 2.0 * pbrInputs.LdotH * pbrInputs.LdotH * pbrInputs.alphaRoughness - 0.5;

    return (pbrInputs.diffuseColor / M_PI) * (1.0 + f90 * pow((1.0 - pbrInputs.NdotL), 5.0)) * (1.0 + f90 * pow((1.0 - pbrInputs.NdotV), 5.0));
}

The term 1.0-pbrInputs.NdotV will become negative.

@emackey
Copy link
Member

emackey commented May 15, 2018

I think I've seen that black spot... Do you know how to reproduce it?

emackey
emackey reviewed May 15, 2018
View reviewed changes
shaders/pbr-frag.glsl Outdated
@@ -254,7 +254,7 @@ void main()
vec3 reflection = -normalize(reflect(v, n));

float NdotL = clamp(dot(n, l), 0.001, 1.0);
float NdotV = abs(dot(n, v)) + 0.001;
float NdotV = min(abs(dot(n, v)) + 0.001, 1.0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to not use clamp here, just to be consistent with the surrounding code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To preserve existing behavior and keep the changes to a minimum.
Should it be clamp(abs(dot(n, v))+0.001, 0.0, 1.0); or clamp(abs(dot(n, v)), 0.001, 1.0);?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the 0.001 is there to prevent a divide-by-zero, so yes I would think that clamp(abs(dot(n, v)), 0.001, 1.0); is the correct one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to clamp(abs(dot(n, v)), 0.001, 1.0);. That's actually what I decided to use in my project after thinking why that 0.001 is here.

@emackey
Copy link
Member

emackey commented May 15, 2018

/cc @snagy NdotV seems odd with its lack of clamping when a number of parameters surrounding it are clamped. Was this intentional?

@corporateshark
Copy link
Contributor Author

Current implementation uses this code for diffuse:

vec3 diffuse(PBRInfo pbrInputs)
{
    return pbrInputs.diffuseColor / M_PI;
}

If we go to https://github.com/KhronosGroup/glTF-WebGL-PBR and replace the code above with this fragment

vec3 diffuse(PBRInfo pbrInputs)
{
    float f90 = 2.0 * pbrInputs.LdotH * pbrInputs.LdotH * pbrInputs.alphaRoughness - 0.5;

    return (pbrInputs.diffuseColor / M_PI) * (1.0 + f90 * pow((1.0 - pbrInputs.NdotL), 5.0)) * (1.0 + f90 * pow((1.0 - pbrInputs.NdotV), 5.0));
}

The black circle will appear.

emackey
emackey reviewed May 17, 2018
View reviewed changes
shaders/pbr-frag.glsl Outdated
@@ -147,7 +147,11 @@ vec3 getIBLContribution(PBRInfo pbrInputs, vec3 n, vec3 reflection)
float mipCount = 9.0; // resolution of 512x512
float lod = (pbrInputs.perceptualRoughness * mipCount);
// retrieve a scale and bias to F0. See [1], Figure 3
#ifdef USE_TEX_LOD
vec3 brdf = SRGBtoLINEAR(texture2DLodEXT(u_brdfLUT, vec2(pbrInputs.NdotV, 1.0 - pbrInputs.perceptualRoughness), 0)).rgb;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a separate bug, not sure if you intended to address it as part of this PR. Is there some better way to do this, maybe turn off mipmapping at the WebGL level, rather than just always asking for a particular mip level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Removed from here and will be put into a separate PR.

Setting filtering to GL_LINEAR should solve the problem as well and sounds like a better solution.

emackey
emackey approved these changes May 17, 2018
View reviewed changes
Copy link
Member

@emackey emackey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @snagy Do you want to take a peek before merge?

@emackey emackey merged commit 0a4008b into KhronosGroup:master May 22, 2018
@ziriax ziriax mentioned this pull request May 26, 2018
Open
@corporateshark corporateshark mentioned this pull request Jun 20, 2018
Closed
bwasty added a commit to bwasty/gltf-viewer that referenced this pull request Aug 17, 2018
@bwasty
partially update reference shader
01aa6d5 
KhronosGroup/glTF-Sample-Viewer#69
KhronosGroup/glTF-Sample-Viewer#80
bwasty added a commit to bwasty/gltf-viewer that referenced this pull request Aug 17, 2018
@bwasty
partially update reference shader
e4af2b1 
KhronosGroup/glTF-Sample-Viewer#69
KhronosGroup/glTF-Sample-Viewer#80
emackey pushed a commit that referenced this pull request Mar 3, 2021
@UX3D-becher
Merge pull request #80 from ux3d/feature/xmpUI
9275380 
added initial version of xmp UI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emackey emackey emackey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@corporateshark @emackey