Skip to content
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

Fix Specular Blinn function #33836

Merged
merged 1 commit into from
Dec 3, 2019
Merged

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Nov 23, 2019

I noticed an issue with the Specular Blinn code path while working on a fix to the vertex-lit code path. Previously our normalized blinn function was divided by 4.0 * cNdotV * cNdotL. I am not sure why that term was included. It causes a weird circular artifact to appear, and it causes the specular lobe to slide around the entire mesh. None of the normalized-blinn NDFs and RDFs I could find used that term (not even Ogre3D's). However, I found that one of the current popular RDFs the modified-Blinn uses pow(cNdotH, shininess) * cNdotL;.

Using this change and removing the above scaling factor results in a much nicer Blinn Specular. The specular lobe looks a lot like the SCHLICK_GGX, only it is a little smaller on smooth materials and a little brighter on rough materials.

Because vertex-lit materials are forced to use Blinn Specular, it is especially important that it look nice.

This also resolves another bug I found where IBL was being scaled too dark on vertex-lit materials.

MASTER
Blinn on left, GGX on right
Screenshot (24)

This PR
Blinn on left, GGX on right
Screenshot (26)

Fixes: #33543

@@ -1547,157 +1545,157 @@ FRAGMENT_SHADER_CODE
#endif // !USE_SHADOW_TO_OPACITY

#ifdef BASE_PASS
{
Copy link
Member

Choose a reason for hiding this comment

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

Didn't you add this extra scope a few commits ago in another PR? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it made the code messier, and it is unnecessary now that I removed the environment scaling from the vertex_lighting code path

@akien-mga
Copy link
Member

Previously our normalized blinn function was divided by 4.0 * cNdotV * cNdotL. I am not sure why that term was included. It causes a weird circular artifact to appear, and it causes the specular lobe to slide around the entire mesh. None of the normalized-blinn NDFs and RDFs I could find used that term (not even Ogre3D's).

Seems like it was added by @reduz in 65fd37c. From a quick look at this commit it looks like he also added it for Phong, might be worth reviewing that too.

BTW, I did a very quick test scenes, probably not with the best environment to see specular effects, but it seems that Specular Mode SchlickGXX and Disable give the exact same output.

@clayjohn
Copy link
Member Author

Yea, the Phong lighting definitely appears strange, but it is not as obviously incorrect as the blinn is.

As for the specular modes, I fixed that in #33694, it just needs to be merged. :) The important part for that bug is the change to material.h. I could remove that part from the PR and make another one.

@akien-mga
Copy link
Member

I could remove that part from the PR and make another one.

Might be worth doing since there was no clear OK from @reduz on the .glsl changes in #33694.

@reduz
Copy link
Member

reduz commented Dec 3, 2019

Seems good, please do a similar PR for the Vulkan brach

@akien-mga akien-mga merged commit 65e6efa into godotengine:master Dec 3, 2019
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DirectionalLight is not work as expected on mobile device
3 participants