-
-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
Fix regression in SSS with reverse-z #99220
Conversation
Welcome and thanks for contributing! Could you amend the commit message to remove the "This reverts commit dd74865f0e6d860932f6c084bceb92ab6d6c6103." line? |
4bc3f12
to
1bc5961
Compare
Thanks @akien-mga ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would simplify this and just do the same change we did for every other shader that reconstructs depth. You can fix this issue with some very minor shader changes. Take a look at servers/rendering/renderer_rd/shaders/effects/bokeh_dof.glsl
for example, it was modified in the reverse z PR
Overall, I prefer that all the screen space effects use the same code so that we can fix them all at the same time when they need to be fixed.
Makes sense to have same code everywhere. I would then suggest to move instead to the depth reconstruction code of this PR, for all other effects. The reason is it doesn't depend on any camera value extraction method ( This is related to the work I'm carrying out on supporting a wider range of zfar / znear values to untap the potential of reverse-z in single precision. What I can propose to separate the concerns is twofold :
How does it sound ? |
That sounds great! |
1bc5961
to
3376b91
Compare
Completed this PR simplification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
The separate PR to change the depth reconstruction code in all effects is here : #99755 |
Thanks! |
This PR fixes how the Subsurface Scattering shader fetches depth from the now reverse-z buffer.
It seems like SSS was missed in the original reverse-z implementation #88328.
The change is very subtle, so I had to set the depth scale to
1.0
in the settings to make it obvious in the screenshots below (rendering/environment/subsurface_scattering/subsurface_scattering_depth_scale
).Note that the depth value is not used in orthographic mode, hence no difference.
I also took this opportunity to rewrite the depth reconstruction code in a less convoluted way. It should allow easier maintenance in the future.