Fix depth buffer access in SSS pass & fix numerical robustness of packFloatInt16() #2553
Conversation
This file contains 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
sss_pass_y could not read from "_main" since the depth buffer was also still bound to the "tex" render target.
In the previous implementation, a matid of 2 and a metallic value of 0.0 would result in a packed float value of 0.125, which then got unpacked into matid = 1 and metallic = 1.0. The new implementation does not have this issue, although the principle is basically the same. Also removed the packFloatInt8 implementation which was not used (anymore?)
MoritzBrueckner
added
the
Release Notes: Fixes
|
Awesome! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR fixes two related issues mentioned in https://forums.armory3d.org/t/personal-website-made-with-armory/4723/35:
The SSS pass tried to read from the depth buffer that was still bound to a different render target. I'm not exactly sure which change caused this issue to appear (according to the forum post it didn't happen last year), but it works now.
The implementation of
packFloatInt16in gbuffer.glsl had some numerical robustness issues: a matid of 2 and a metallic value of 0.0 would result in a packed float value of 0.125, which then got unpacked into matid = 1 and metallic = 1.0 because both packed representations are very close to each other.My new implementation does not have this issue since the encoded output is basically an integer stored in a float. Unfortunately we cannot use
uintBitsToFloathere since possible NaN representations would result in undefined behaviour, so instead the packed bytes are converted back to a valid floating point value. As a result, the precision will be slightly worse for "large" integers than with the old implementation, but even then the worst case error should not be larger than ~0.008 if I'm not mistaken, so we should be fine (the largest currently used matID is 2 anyways I think). In a visual comparison of a color ramp that controls the metallic value, the color banding looks exactly the same for both implementations for a matID of 0. The packed integer value should be exactly preserved.I also removed the packFloatInt8 implementation which was not used (anymore?). If there was a reason for keeping it (or you think that my new packing implementation is not good), I can reverse this change. We need to find another solution then, however.
I tested the changes on html5, windows-krom and windows-hl (DirectX and OpenGL), with SSS materials, emission materials and regular materials.