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

[3.x] Make blinn and phong specular consider albedo and specular amount #51410

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Aug 8, 2021

Fixes: #50455 and fixes #34257

4.0 version: #51411

This PR adds in the specular amount and albedo into the calculation of specular lighting for Blinn and Phong modes. This is not physically based, however, it comes close to mirroring the old Specular/Diffuse lighting that was around before PBR. In discussions with other contributors it is clear that we want to maintain the Blinn and Phong modes for performance reasons and for compatibility with older workflows. The main difference between this and the old Specular/Diffuse lighting model is that this only provides one albedo colour. The older workflow always specified a specular colour and a diffuse colour, here we use albedo for specular and diffuse. Essentially we have gone from:

diffuse_color = albedo;
specular_color = vec3(1,1,1);

to:

diffuse_color = specular_color = albedo;

This PR should be slightly improve performance in theory. But in practice there will be a negligible difference.

Visual comparison

All comparisons use the same layout. GGX in the upper right, Blinn in the lower left, and Phong in the lower right

Smooth metal

old
smooth-old

new: note the color around the specular highlight
smooth-newnew

0.5 rough metal

old
mid-old

new: note the colored specular highlight
mid-newnew

rough metal

old
rough-old

new
rough-newnew

This PR should result in as little change as possible for existing materials while allowing more cohesiveness when using Blinn and Phong. Users that appreciated having the overblown specular on darker materials can still achieve the same look by boosting specular with the specular property.

Old notes This PR adds a Fresnel term and geometric term to both the Blinn and Phong specular modes. The Fresnel term scales the specular intensity quite strongly and removes the overbright specular reflections when roughness is high.

By way of background, BRDF's typically share 3 terms D (normalized density function) G (geometric term) F (Fresnel term). The Blinn BRDF is D*G*F / 4 * NdotL * nDotV (see https://www.cs.cornell.edu/~srm/publications/EGSR07-btdf.pdf) . Godot has been using a more simplified model which ignored the G and F terms (and had a mistake in the D term): D` / 4 * NdotL * nDotV. This simplified model is naturally simpler to compute, but was totally broken (hence #33836 which didn't actually fully fix Blinn).

This PR restores the G and F terms. The G term for Blinn-Phong is equal to NdotL * nDotV and so it cancels out the NdotL * nDotV in the BRDF. We are left with D * F / 4 which is what this PR uses.

Visual comparison

All comparisons use the same layout. GGX in the upper right, Blinn in the lower left, and Phong in the lower right

Smooth metal

old
smooth-old

new: note the color around the specular highlight
smooth-new

0.5 rough metal

old
mid-old

new: note the colored specular highlight
mid-new

rough metal

old
rough-old

new
rough-new

edit: references for future reference
http://www.thetenthplanet.de/archives/255
https://seblagarde.wordpress.com/2011/08/17/hello-world/
https://www.cs.cornell.edu/~srm/publications/EGSR07-btdf.pdf
https://slideplayer.com/slide/1507561/

@clayjohn clayjohn requested a review from a team as a code owner August 8, 2021 22:07
@Calinou Calinou added this to the 3.4 milestone Aug 8, 2021
@clayjohn clayjohn changed the title Make blinn and phong specular use full pbr [3.x] Make blinn and phong specular use full pbr Aug 8, 2021
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

I don't know enough about the internals of PBR so I'll take you're word on this being more accurate.

As for the choice whether to do this or not, I'm all for it.

@lawnjelly
Copy link
Member

I missed the discussion last night as it was late here, but I'd also say give it a try in some betas, and see if we get anyone complaining.

Aside from the backward compatibility, my only concern (being a performance guy) is when you say it runs slower, is to make sure we always have a cheap and cheerful diffuse and specular mode available that runs as fast as humanly possible, and doesn't care about PBR accuracy etc. Because that's what it boils down to in most cases, people want to be able to choose between 'looks good' and 'runs fast'. Myself I try and tweak the settings to have as fast as possible, or often just run an entirely custom shader for this reason.

@charlesmlamb
Copy link

From my perspective, having shaders that are more realistic, or do things as normal light, is quite important . .

  • if they become exactly the same, in terms of visuals, why even have them . .
  • if speed becomes significantly lower, that might be a draw-back . .

I often wondered why they were there, I just use the default one, assuming it's best . . . maybe they are important shaders, for historical purposes . . reg. speed, it's an issue whether current hardware is optimized for one, or the other, about speed . . .

I sort-of rather wish, there was an explanation for why even to use them . . . one down-side for them looking is identical is, why not just delete the two slower one's, and have the fastest / best one . . .

The pre-view images are amazing, one can see how the color of the reflection light, becomes more like GGX ( accurate ? ) . . Amazing, hope you can make it run fast, and look good . . . .

@clayjohn
Copy link
Member Author

clayjohn commented Aug 9, 2021

Aside from the backward compatibility, my only concern (being a performance guy) is when you say it runs slower, is to make sure we always have a cheap and cheerful diffuse and specular mode available that runs as fast as humanly possible, and doesn't care about PBR accuracy etc. Because that's what it boils down to in most cases, people want to be able to choose between 'looks good' and 'runs fast'. Myself I try and tweak the settings to have as fast as possible, or often just run an entirely custom shader for this reason.

Blinn and Phong are still cheaper than the GGX mode, they just aren't as cheap as they were before (this PR adds a handful of multiplications and 2 lerps). For most cases, they probably won't be slower because the difference is so small, but in theory more instructions leads to worse performance.

One problem with the current implementation is that it is halfway between old Diffuse/Specular and PBR. Without this change, specular ignores albedo. Since Godot materials cannot specify diffuse_color and specular_color, all materials using Blinn/Phong implicitly use a pure white specular_color. Which is of course a huge problem both for a PBR workflow and for a Diffuse/Specular workflow.

IMO the workflow that makes sense is having Blinn and Phong look like simplified versions of GGX (but cheaper of course). That is the workflow that this PR enforces.

@clayjohn
Copy link
Member Author

clayjohn commented Aug 9, 2021

@lawnjelly Another option would be to meet in the middle. We could remove the Fresnel term completely and make the assumption that diffuse_color = specular_color = albedo.

Actually, that may be the best option here. If we do that we can substantially reduce the instruction count while maintaining a consistent behaviour

Code below for reference (tested in the online editor while I am at work :P)

void light() {
//Blinn
	vec3 H = normalize(VIEW + LIGHT);
	float cNdotH = max(dot(NORMAL, H), 0.0);
	float shininess = exp2(15.0 * (1.0 - ROUGHNESS) + 1.0) * 0.25;
	float blinn = pow(cNdotH, shininess);
	blinn *= (shininess + 2.0) * (1.0 / (8.0 * 3.14159)); // Normalized NDF and Geometric term
	SPECULAR_LIGHT += LIGHT_COLOR * ATTENUATION * blinn *ALBEDO * specular;

        // Lambertian
	float NdotL = dot(NORMAL, LIGHT);
	float cNdotL = max(NdotL, 0.0); // clamped NdotL
	DIFFUSE_LIGHT = cNdotL * (1.0 / 3.14159) * LIGHT_COLOR * ALBEDO;
}

void light() {
        // Phong
	vec3 R = normalize(-reflect(LIGHT, NORMAL));
	float cRdotV = max(0.0, dot(R, VIEW));
	float shininess = exp2(15.0 * (1.0 - ROUGHNESS) + 1.0) * 0.25;
	float phong = pow(cRdotV, shininess);
	phong *= (shininess + 1.0) * (1.0 / (8.0 * 3.14159));
	SPECULAR_LIGHT += LIGHT_COLOR * ATTENUATION * phong *ALBEDO * specular;

        // Lambertian
	float NdotL = dot(NORMAL, LIGHT);
	float cNdotL = max(NdotL, 0.0); // clamped NdotL
	DIFFUSE_LIGHT = cNdotL * (1.0 / 3.14159) * LIGHT_COLOR * ALBEDO;
}

This removes the Fresnel term completely (the Fresnel term is responsible for making all surfaces appear reflective at a glancing angle) and introduces albedo and specular amount into the terms. This way the materials will behave consistently (but not physically correct) and it introduces minimal calculations, thus keeping them as cheap as possible.

@clayjohn clayjohn requested a review from BastiaanOlij August 10, 2021 03:46
@clayjohn clayjohn changed the title [3.x] Make blinn and phong specular use full pbr [3.x] Make blinn and phong specular consider albedo and specular amount Aug 10, 2021
@clayjohn
Copy link
Member Author

Okay, I have updated the PR to not go full-PBR. Instead it maintains some aspects of the old Diffuse/Specular model. Results are similar but not quite as close to GGX as they were before. I suggest we approve and merge like this as it should impact existing users less and still provides the same benefits.

@akien-mga akien-mga merged commit 6518a61 into godotengine:3.x Aug 10, 2021
@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.

6 participants