Scene Shaders - Vertex Shading Fixes #100503
Conversation
Fixes two errors related to the normal, tangent, and bitangent vectors, namely normals not always being inverted on backfaces, and normalization being reversed from what MikkTSpace expects.
| // Uses slightly more FMA instructions (2x rate) to avoid special instructions (0.25x rate). | ||
| // Range is reduced to [0.64,4977] from [068,2,221,528] which makes mediump feasible for the rest of the shader. | ||
| // Converts GGX roughness to Blinn-Phong shininess. | ||
| // Roughly approximates `2.0 / roughness * roughness - 2.0` with a much lower high end. |
There was a problem hiding this comment.
Where does 2.0 / roughness * roughness - 2.0 come from? I don't think its a very good approximation for converting roughness to specular power.
Here is a desmos graph showing the different approaches (and our approximations) https://www.desmos.com/calculator/nzby8ouch4
There was a problem hiding this comment.
I believe here is where I first encountered it, and of the methods I've tried I consider it the most perceptually accurate for matching Blinn-Phong to GGX.
I'm also not sure why exp2(15.0 * (1.0 - roughness) + 1.0) * 0.25 was chosen originally, as shown in my comparison screenshots it's not a very close fit to what GGX produces. There are certainly other mappings we could use and I'm open to trying them, but the current method really leaves a lot to be desired.
There was a problem hiding this comment.
I thought it was from the Frostbite PBR slides, but looking quickly at them now I don't see it. AFAIK, this, or something like this has been the standard for a long time.
I guess if our goal is a visual approximation then it doesn't matter how close the curve is as long as the end result looks good.
I like the range of the new function, it does a good job of keeping the intermediate values in a sufficient range where you won't risk losing precision when using mediump. Its a shame the new approximation is slightly more expensive than the old, but if it looks better, then oh well.
| float size_A = 0.0; | ||
| if (omni_lights[idx].size > 0.0) { | ||
| float t = omni_lights[idx].size / max(0.001, light_length); | ||
| size_A = max(0.0, 1.0 - 1.0 / sqrt(1.0 + t * t)); | ||
| } |
There was a problem hiding this comment.
This was removed intentionally. The purpose of vertex lighting is twofold:
- To be very fast when you need fast but simple lighting (i.e for particles)
- To allow users to make old school looking games.
Neither of those cases require having area lights. so we dropped the code. The nature of specular reflections in vertex shaded materials also makes the effect barely visible except at extreme values, so its really not worth the cost IMO.
I wouldn't bring it back unless we have enough users saying that they absolutely need it.
There was a problem hiding this comment.
That's fair. I figured the cost wasn't much of a concern since it's only calculated when the user wants it to be.
That being said, if the only concern is performance, I can easily optimize this so it only uses one quarter-rate instruction instead of three (or even none, as size_A = max(0.0, 1.0 - 1.0 / sqrt(1.0 + t * t)) simply clamps the value below 1.0 with a very subtle rolloff, so can easily be dropped in favor of size_A = clamp(t, 0.0, 1.0), which would bring the area light code down to a multiply, a clamp, and three adds (two if we also drop the geometry term I added)).
There was a problem hiding this comment.
I would wait and see if users ask for it. There is no point in throwing away performance for something that may never get used.
There was a problem hiding this comment.
Speaking as a user, this is something that I want. Since area lights affect diffuse shading too, and our area lights aren't energy conserving (larger area lights simply emit more light than smaller ones), ignoring area lights causes a brightness discrepancy between per-vertex and per-pixel shaded materials. If I'm using per-vertex shading for particles, I want them to be lit consistently with the rest of their environment, regardless of my lighting setup.
Perhaps it's outside the scope of this discussion, but I don't think we should always wait for users to notice deficiencies before addressing them, especially when the performance cost for doing so can be reduced to as little as three instructions (two when not using specularity).
There was a problem hiding this comment.
I strongly agree with @StaydMcMuffin. Personally, I would expect and want this behavior too.
Approximating omni lights with a non-zero radius as point lights merely to save a few operations (per vertex!) is a bad idea. These approximations create noticeable visual inconsistencies for a negligible performance gain.
Particularly in both scenarios mentioned above: particle systems and retro-style games (as well as other use cases).
In general, per-vertex specular lightning looks better with larger light sources.
However, if desired, I could do some benchmarking to verify that this computation wouldn't be a problem performance-wise.
drivers/gles3/shaders/scene.glsl
Outdated
| float shininess = roughness_to_shininess(roughness); | ||
| float blinn = pow(cNdotH, shininess); | ||
| blinn *= (shininess + 2.0) * (1.0 / (8.0 * M_PI)) * cNdotL; | ||
| blinn *= (shininess + 8.0) * (1.0 / (8.0 * M_PI)) * (0.25 / max(0.0001, cLdotH * cLdotH)) * cNdotL * specular_amount; |
There was a problem hiding this comment.
Where does this extra term come from? I don't think it is necessary for a simple blinn NDF.
IIRC the current normalization factor comes from http://www.thetenthplanet.de/archives/255 and it is energy conserving. I wouldn't change it without a good reason
There was a problem hiding this comment.
This is the Geometry term mentioned in the fourth point, and is the same method we use for clearcoat specular. It was added to more closely match per-pixel shading with minimal extra cost, but we can drop this if that's not a priority.
There was a problem hiding this comment.
Oh, I see. I would drop it unless it makes a huge difference. Seeing as we already don't use the Fresnel term, I don't think the extra cost is worth it just to be slightly more correct. Fresnel makes way more of a difference, but you don't really miss it with vertex shading.
There was a problem hiding this comment.
It is relatively subtle so yeah, I'll go ahead and drop this.
You have this backwards |
Ah, right you are, I misremembered the table from here. I meant to refer to Blinn-Phong (modified), not Blinn-Phong (NDF), as the former is the case when multiplying specular by |
Its very strange that we do multiply by godot/drivers/gles3/shaders/scene.glsl Lines 239 to 241 in cd92ad0 I can't remember why I preferred the NDF over the RDF previously. Its probably worth checking what the visual difference is if you remove the |
Aha, that explains why the unmodified term was used in the first place. |
Fixes five errors related to per-vertex shading, with a focus on maintaining close visual parity between per-pixel and per-vertex shading modes.
StaydMcMuffin
force-pushed
the
scene-shaders-vertex-shading-fixes
branch
from
b6864fc to
1810135
Compare
|
What's the state of this PR? Are there any things that need a final decision (besides the specular computation for area lights)? |
I believe the area light handling is the only hold-up. Since the main concern is performance I could implement some trivial optimizations that I was originally saving for a dedicated optimization-focused PR, if that might help to move things along. |
|
In 4.4 stable I'm seeing just one of my models rendering extremely bright if the roughness is low (and non-0 metallic) on a per-vertex material. Is that relevant (and fixed?) here, or is there some other issue/PR or perhaps just a way to fix the model? |
See #103627. There are several bugs with metallic per-vertex shaded materials right now. |
Part three of: #100348
Requires the preceding: #100441
Addresses
sixfive errors found in the scene shader files that relate to per-vertex shading.In Forward+ and Compatibility, the geometry normals used for per-vertex shading were not normalized prior to their use, causing per-vertex shading to be either too bright or too dark depending on the overall scale of the model matrix. Specular shading in particular was strongly affected by this.
For per-vertex shading specifically, the Lambert Wrap diffuse model mistakenly used the clamped
cNdotLrather than the signedNdotL, meaning it could not, in fact, wrap.Per-vertex shading completely ignored both the contributing lights'
specular_amountandsizeparameters.Per-vertex specularity lacked a Geometry term entirely, resulting in overly bright rough highlights and a generally different "look" compared to per-pixel shading. It now uses the same approximate G term used for clearcoat specular.This may be considered as much of an improvement as it is a fix, but it's a fairly innocent one at that.
Dropped for performance.
The
roughness_to_shininessmapping from GGX roughness to Blinn-Phong shininess used for per-vertex shading resulted in very different specular responses between per-pixel and per-vertex shading, generally making materials look far more glossy despite having the same roughness. I've replaced it with a new formulation entirely, which loosely follows the curve of the common2 / roughness * roughness - 2remapping, but with a much lower maximum value more suitable for per-vertex shading.Per-vertex shading incorrectly used the unmodified Blinn-Phong normalization term
(shininess + 2.0) / (8.0 * PI)as opposed to the modified one(shininess + 8.0) / (8.0 * PI)(the modified term is the one you want when multiplying byNdotL).Here is a swatch of roughness values, from
0.0on the left to1.0on the right in intervals of0.2. The top row uses per-pixel shading while the bottom uses per-vertex.Here is a material tester using per-vertex shading with the Lambert Wrap diffuse mode and a roughness of
0.4. The mesh also has a uniform scale of0.8, which causes the specularity to disappear completely in the "Before" image.