SSR clear coat improvement #5565
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed. HDRP Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure. |
TomasKiniulis
approved these changes
Sep 9, 2021
anisunity
changed the title
|
Fixed formatting and launching yamato |
|
@anisunity, @slunity should this fix also be applied to AxF? |
|
Added Stephane for awareness |
|
An other solution to avoid the inout would be to add a parameter, but that would mean breaking the shader api... so |
yeah, was afraid that inout may currently braek the API but it doesn, we will go with it. Don't like it too much but I haven't better alternative to propose without breaking the API so let's use that |
sebastienlagarde
approved these changes
Sep 12, 2021
|
There is various failure with Stacklit on test apparently |
|
Checking that |
|
Looking at AxF.hlsl, it really looks like the fix should be done there too |
|
Fixed stack lit, relaunching the tests |
|
there is still one test failing: 1302_StackLitSG_PixarLM.png |
|
After looking at the screenshots, the new screenshots make more sense than the previous ones. Updating them. |
…on the Lit and StackLit shaders (case 1352424).
anisunity
force-pushed
the
HDRP/clear-cloat-ssr
branch
from
ed99950 to
a402559
Compare
sebastienlagarde
pushed a commit
that referenced
this pull request
Sep 20, 2021
slunity
added a commit
that referenced
this pull request
Oct 21, 2021
Add support for more refined reflection hierarchy for SSR-RTR blending with IBL and to avoid double coat lighting. This is mainly based on #4968, but with the added feature to re-use the coat-traced light for similarly-rough bottom layer lobes, like done for Lit. This fixes several issues in the implementation in StackLit since #5565: -Similar issues fixed for Lit (see other PR for Lit) -StackLit can have a rough coat, but an arbitrary 0.9 smoothness value was used to decide if the coat-traced light would be re-used for the bottom lobes. -The wrong bottom lobe roughnesses were used (values that don't include the effect of BRDF vertical layering computations) -StackLit can have different lobe directions and the light hierarchy now account for this.
slunity
added a commit
that referenced
this pull request
Oct 21, 2021
…lback when a coat is present: 1) SSR lighting was applied using the coatMask in such a way that the bottom layer incorrectly gained coat-traced light as the coatMask went to 0, even prior #5565. #5565 added the usage of the coat lighting (irrespective of coatMask considerations, ie even when coatMask = 1) when the bottom layer has a roughness close enough to 0, ie to that of the coat lobe for which we traced the indirect light. However, even when the bottom layer roughess was very different (blendFactor = 1), coatMask could escape that logic as it went to 0. (In that case, the comment in the code "// On top of this above we also add an additional hack to limit the smoothness used on based layer depending on coat layer for SSR application (See below)" was wrong) 2) The reflectionHierarchyWeight (again even before #5565) was incorrectly including an F term, while the consumed / used-up light hierarchy weight is orthogonal to the specific material shading details (reflectance factors, ie pre-integrated FGD or coatF here). So regardless of if we used the ssr coat lighting for the bottom layer, as coatF grew, we would artificially remove total bottom layer light at grazing angles. Ergo, this was incorrectly darkening the grazing angle fresnel reflection because of darkening of the bottom layer. Moreover, with the separate coat hierarchy weight, the envlighting can no longer use the same probe volume intersection weight, and a separate update and capping must be done for the coat reflection hierarchy. 3) In #5565, the code states we lerp-in coat light contribution for the bottom layer when its smoothness is above 0.9 and lerp-out from 0.9 to 0.8, but the code was actually fading from 0.9 to 0.7, not 0.8. I assumed the code behavior was desired (since probably eyeballed) and used the same ranges but made the code cleaner. 4) More importantly (vs 3), the behavior since #5565 could actually erroneously eliminate the coat lobe and use only the bottom lobe with the coat-traced light: this happened when the bottom roughness was close to the coat roughness, as in that case we had a "blendfactor" that would lerp from coat F to bottom FGD, instead of actually combining both lobes with their respective wanted contributions.
This was referenced Oct 21, 2021
sebastienlagarde
added a commit
that referenced
this pull request
Nov 22, 2021
…ith IBL fallback (case 1380351) (#6106) * Coat SSR-RTR: Fix various issues with using SSR lighting with IBL fallback when a coat is present: 1) SSR lighting was applied using the coatMask in such a way that the bottom layer incorrectly gained coat-traced light as the coatMask went to 0, even prior #5565. #5565 added the usage of the coat lighting (irrespective of coatMask considerations, ie even when coatMask = 1) when the bottom layer has a roughness close enough to 0, ie to that of the coat lobe for which we traced the indirect light. However, even when the bottom layer roughess was very different (blendFactor = 1), coatMask could escape that logic as it went to 0. (In that case, the comment in the code "// On top of this above we also add an additional hack to limit the smoothness used on based layer depending on coat layer for SSR application (See below)" was wrong) 2) The reflectionHierarchyWeight (again even before #5565) was incorrectly including an F term, while the consumed / used-up light hierarchy weight is orthogonal to the specific material shading details (reflectance factors, ie pre-integrated FGD or coatF here). So regardless of if we used the ssr coat lighting for the bottom layer, as coatF grew, we would artificially remove total bottom layer light at grazing angles. Ergo, this was incorrectly darkening the grazing angle fresnel reflection because of darkening of the bottom layer. Moreover, with the separate coat hierarchy weight, the envlighting can no longer use the same probe volume intersection weight, and a separate update and capping must be done for the coat reflection hierarchy. 3) In #5565, the code states we lerp-in coat light contribution for the bottom layer when its smoothness is above 0.9 and lerp-out from 0.9 to 0.8, but the code was actually fading from 0.9 to 0.7, not 0.8. I assumed the code behavior was desired (since probably eyeballed) and used the same ranges but made the code cleaner. 4) More importantly (vs 3), the behavior since #5565 could actually erroneously eliminate the coat lobe and use only the bottom lobe with the coat-traced light: this happened when the bottom roughness was close to the coat roughness, as in that case we had a "blendfactor" that would lerp from coat F to bottom FGD, instead of actually combining both lobes with their respective wanted contributions. * Update CHANGELOG.md Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
sebastienlagarde
added a commit
that referenced
this pull request
Nov 22, 2021
Add support for more refined reflection hierarchy for SSR-RTR blending with IBL and to avoid double coat lighting. This is mainly based on #4968, but with the added feature to re-use the coat-traced light for similarly-rough bottom layer lobes, like done for Lit. This fixes several issues in the implementation in StackLit since #5565: -Similar issues fixed for Lit (see other PR for Lit) -StackLit can have a rough coat, but an arbitrary 0.9 smoothness value was used to decide if the coat-traced light would be re-used for the bottom lobes. -The wrong bottom lobe roughnesses were used (values that don't include the effect of BRDF vertical layering computations) -StackLit can have different lobe directions and the light hierarchy now account for this. Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://fogbugz.unity3d.com/f/cases/1352424/
A double contribution of the clear cloat was found when SSR/RTR was enabled on the Lit and Stack Lit shader. This fixes that behavior.
Testing status.
The solution was validated by the user.
Multiple tests will need screenshot updates