[DRAFT] A Collection of Scene Shader Fixes

[StaydMcMuffin]

The result of a thorough examination of the main Forward+ scene shader and its lighting includes, this is a collection of fixes for 29 errors found therein. In particular I would like to stress that these are only the things I consider straight fixes, not improvements or enhancements.
Also included are many low-hanging fruit optimizations; in particular there were, for one reason or another, many instances where a quarter-rate instruction or two could be shaved off with clever use of inversesqrt.
Along the way I also made a point of correcting comment formatting and adding comments where it seemed prudent, in the interest of making maintenance easier for anyone delving into these not-insubstantial shaders in the future. These comment/formatting changes are contained in the first commit, while the fixes proper are in the second.

I leave this as a Draft for the time being, as I've only done the Forward+ shaders for now. Once any of my own mistakes get ironed out I'll move on to porting these fixes to the other renderers.

ISSUES FIXED (in no particular order):

• The normalization of the TBN vectors was reversed from what MikkTSpace expects: the vectors were left unnormalized in the vertex shader and then normalized early in the pixel shader, whereas MikkTSpace assumes the vectors will be normalized before vertex interpolation and left unnormalized for the normal map's tangent-space transformation.
  Ref: http://www.mikktspace.com/

• The faux-anisotropic bent_normal vector, used to sample reflection cubemaps when anisotropy is enabled, mistakenly used linear roughness as opposed to perceptual roughness like in Filament.
  Ref: https://github.com/google/filament/blob/6b7080e8b69f808df37eb6493cc73a2925e5f987/shaders/src/common_lighting.fs#L95

• The faux-anisotropic bent_normal vector used for anisotropic indirect reflections was not consistently used in place of the isotropic reflection vector, which it now always is. It was also a bit too strong, causing reflections to become visibly reversed over anisotropy values of ~0.75.

• When calculating the anisotropic tangent and bitangent, the normal-mapped normal was mistakenly used to construct the TBN matrix, rather than the geometric normal. Additionally, these tangents were only calculated when anisotropy > 0.01 rather than abs(anisotropy) > 0.01, which is important given that anisotropy is a [-1,1] value.

• The mapping of the anisotropy parameter to X and Y roughness was not symmetrical, meaning it only produced correctly stretched highlights for positive anisotropy values, while negative values produced significantly less stretch. This is because the formulation in question, from Disney's BSDF, was designed for [0,1] anisotropy values, not [-1,1].
  By using the absolute value of anisotropy and simply swapping X and Y roughness based on its sign, highlights are now stretched the same amount for both negative and positive values.

• The anisotropic tangent and bitangent were reversed in several places, causing the anisotropic flow direction to be off by 90 degrees for omni/spot lights.
  The corrected behaviour now consistently matches Filament (anisotropy set to 1.0 is along U, -1.0 is along V).
  Note that some assets will need to be adjusted depending on the lighting conditions they were originally configured in, but this is simply a matter of negating their anisotropy parameter if their highlight is going the wrong direction.

• The clearcoat_roughness parameter was mistakenly used as-is (ie. not remapped to the intended [0.001,0.1] range) in several places, most notably when calculating the dominant light direction used to sample reflection cubemaps, causing obvious distortion of clearcoat reflections with any clearcoat_roughness above zero.

• The clearcoat_roughness parameter was remapped differently between direct and indirect lighting.
  In scene_forward_clustered.glsl it was remapped to perceptual roughness as mix(0.001, 0.1, sqrt(clearcoat_roughness)), while in scene_forward_lights_inc.glsl it was remapped as linear roughness mix(0.001, 0.1, clearcoat_roughness). The latter behaviour has been made the consistent behaviour in both files.

• Clearcoat specular did not properly account for light size, causing a discrepancy between primary specular reflections and clearcoat ones.

• The Geometry term used for clearcoat specular would sometimes divide by 0, producing NaNs visible as bright flickering pixels.

• Indirect clearcoat reflections were being multiplied by the same indirect BRDF as primary reflections due to them being added to specular_light beforehand. Additionally, the clearcoat fresnel-based attenuation term multiplied into diffuse_light and specular_light was applied too early, meaning it only affected some sources of indirect light and not others.
  Indirect clearcoat reflections are now accumulated separately from primary reflections, and have their own BRDF and attenuation evaluated and applied after all other indirect light has been integrated, at which point they are finally added to specular_light.

• As noted in a comment, direct clearcoat specular was not energy conserving, as the underlying diffuse and specular needed to be attenuated by the clearcoat layer's fresnel.
  Normally I would consider this more of an improvement than a fix, however this energy conservation was already being done for indirect lighting, so I'm inclined to consider this a fix in the name of consistency.

• The global radiance cubemap, when sampled for clearcoat reflections, was never multiplied by scene_data.IBL_exposure_normalization, meaning clearcoat reflections would have a different brightness from primary reflections when exposure normalization was used.

• Local reflection probes were completely absent from clearcoat reflections on account of simply never being sampled. These are now properly accounted for.

• SSAO only affected indirect reflections relative to its Light Affect property, resulting in highly metallic materials appearing flatter and more "glowy" under bright cubemaps compared to less reflective materials when this property was low or 0.

• The screen-space roughness limiter used the derivatives of the normal-mapped normal to calculate shading variance rather than the geometric normal, resulting in visible 2x2 quad artifacts on normal mapped surfaces, especially with strong limiter settings and a strong normal map.
  It is important to note that the algorithm in question was only designed to handle geometric specular aliasing to begin with, and Godot already has a separate system for handling specular aliasing due to normal-mapping.

• The screen-space roughness limiter's effect did not apply to clearcoat roughness, meaning clearcoat specular was particularly prone to aliasing.

• The horizon-occlusion term was mistakenly calculated using the dot product of the dominant specular vector and the normal-mapped normal, as opposed to be the dot product of the pure reflection vector and the geometric normal.
  Ref: https://marmosetco.tumblr.com/post/81245981087

• The specular microshadowing factor used for indirect reflections did not match that used for direct specular, using f0.g rather than the average f0. The latter is now the consistent behaviour.

• There was a redundant if (roughness > 0.0) around the direct specular code in light_process (complete with comment that it shouldn't be there), which did nothing because roughness never actually reached 0.
  This has been replaced by a check that skips direct specular if both f0 and metalness are zero (excluding clearcoat specular, which has its own f0).

• The rim effect was completely unaffected by shadows cast onto the material. This may have been intentional to allow the rim effect to ignore self-shadowing, but if so I think this was a poor decision, as the light leaking it allows is far worse than the rim light sometimes being obscured by self-shadowing.

• The rim effect's tint parameter (ie. multiplication by albedo) was handled incorrectly due to an order of operations error, causing the tint to be applied twice. Additionally, compared to the reference Disney implementation the effect was Pi times too bright, again due to order of operations.

• The TBN vectors (normal_interp, tangent_interp, and binormal_interp) were not being inverted on backfaces, resulting in incorrect shading, particularly on normal-mapped materials.

• For per-vertex lighting specifically, the wrapped-Lambert diffuse model mistakenly used clamped NdotL rather than signed NdotL, meaning it could not, in fact, wrap.

• Per-vertex shading completely ignored the contributing lights' specular_amount and size parameters.

• 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.

• The roughness_to_shininess mapping from GGX roughness to Blinn-Phong shininess used for per-vertex shading resulted in wildly different specular responses between per-pixel and per-vertex shading. A new remapping has been made to replace it, which notably has a much lower high-end more suitable for per-vertex shading.

• Per-vertex shading incorrectly used the Blinn-Phong RDF normalization term (shininess + 2.0) / (8.0 * PI) as opposed to the NDF one (shininess + 8.0) / (8.0 * PI) (the NDF term is the one you want when multiplying by NdotL).

• The geometry normals used for per-vertex shading were not normalized prior to their use, causing vertex shading to be either too bright or too dark depending on the overall scale of the model matrix, especially specular shading.

Before	This PR

TODO:

• Port fixes to Mobile renderer.
• Port fixes to Compatibility renderer.

[arkology]

Suggested change

	dp_clip = vertex_interp.z; // This attempts to avoid noise caused by objects sent to the other parabolloid side due to bias.
	dp_clip = vertex_interp.z; // This attempts to avoid noise caused by objects sent to the other paraboloid side due to bias.

Tiny correction 👀

[StaydMcMuffin]

Appreciated. 👍

[clayjohn]

Wow! Great work! There is a lot to unpack here. There are some really nice fixes in here that would be great to have. However, there is also just way too much packed into one PR for anyone to meaningfully review this. Typically we suggest doing one PR per bug fix, or one PR per enhancement as the case may be. That allows maintainers to evaluate the change on its own and allows us later meaningfully identify regressions if they arise.

To ensure a quick review process I would split this into a few chunks:

1. Style fixes + comment changes. These should be safe and easy to make on their own. But they make the substantive changes harder to review, so its best to split them out.
2. Anisotropy changes
3. Clearcoat changes
4. BRDF changes
5. TBN changes
6. misc performance changes (all performance changes should be split off so we can profile them and confirm that they actually improve performance. If you haven't profiled on a GPU you are as likely to reduce performance as you are to increase performance even with changes that seem to obviously improve things)

Doing so will allow reviewers to focus on just the relevant changes in each area. And then issues with one area won't block us merging clear improvements in another area.

[StaydMcMuffin]

Wow! Great work! There is a lot to unpack here. There are some really nice fixes in here that would be great to have. However, there is also just way too much packed into one PR for anyone to meaningfully review this. Typically we suggest doing one PR per bug fix, or one PR per enhancement as the case may be. That allows maintainers to evaluate the change on its own and allows us later meaningfully identify regressions if they arise.

To ensure a quick review process I would split this into a few chunks:

1. Style fixes + comment changes. These should be safe and easy to make on their own. But they make the substantive changes harder to review, so its best to split them out.

2. Anisotropy changes

3. Clearcoat changes

4. BRDF changes

5. TBN changes

6. misc performance changes (all performance changes should be split off so we can profile them and confirm that they actually improve performance. If you haven't profiled on a GPU you are as likely to reduce performance as you are to increase performance even with changes that seem to obviously improve things)

Doing so will allow reviewers to focus on just the relevant changes in each area. And then issues with one area won't block us merging clear improvements in another area.

I see. Should I make a new PR entirely or just update this one so it's only the first group of changes?

[clayjohn]

I see. Should I make a new PR entirely or just update this one so it's only the first group of changes?

I think you should make a new PR for each group of changes. Then once you have everything up you can close this one

[StaydMcMuffin]

I see. Should I make a new PR entirely or just update this one so it's only the first group of changes?

I think you should make a new PR for each group of changes. Then once you have everything up you can close this one

Ah, figures I would see this just after force-pushing the first group of changes. I can re-commit the original full-fat files here for posterity if desired, but I've opened up a new PR for the first round of fixes (just comment style and formatting): #100383