Skip to content

Fix a flickering issue related to moving shadow receivers (case 1302392) and a refactoring to avoid computing the same value for every effect. #3039

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

Merged
merged 5 commits into from
Feb 2, 2021

Conversation

anisunity
Copy link
Contributor

This PR includes twp seperate changes:

  • Rejecting the history when the receiver is moving (as the signal will be changing on the target obejct) (case 1302392)
  • Doing the history validation pass once for the whole frame and using it for every effect instead of doing it for every effect.

Testing status
I ran the DXR tests locally they are all green.
Opened couple scenes and made sure the reprojection looks fine.(Shadows, AO, RTGI)
Unfortunately, the tests do not capture temporal denoising this would require a bit more manual testing from @remi-chapelain to make sure it behaves normally after the PR.

@anisunity anisunity self-assigned this Jan 8, 2021
@anisunity anisunity marked this pull request as ready for review January 8, 2021 18:45
Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested multiple settings for the 3 raytraced effects concerned (RTGI, RT Shadows, RTAO) in the repro project and in another bigger real world project.

The ghosting seems to have completely disappeared when the "receiver" is moving in RTGI / RTAO and RT Shadows except RT Area Light shadow. It's the only case that does not seem affected by this fix. (see gif)
88beecb0e878e4fcabe75675c9561f52

Despite ghosting being fixed, I have the feeling it's maybe a bit to "agressive" on discarding the history, is this possible to fine tune the threshold for history validity ?
I've noticed that the history gets discarded instantly if the receiver is moving, even if we cannot see it moving with out eyes. I think it would be a bit better to increase a bit the tolerance to be able to NOT discard history if it's moving very slowly, some ghosting can be tolerated especially if it's moving really really slowly.
I tried fiddling around with the constant in TemporalFilter compute shader but not luck
You can see an example of what I'm saying HERE

Difference BEFORE & AFTER
Before
before_low

After
after_low

@anisunity
Copy link
Contributor Author

No, not possible to "tune" it, we don't have access to that piece of information

@remi-chapelain remi-chapelain self-requested a review January 19, 2021 14:16
Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerns about unaffected area lights won't be fixed in this PR, it will be the object of future work.
As for the very low moving threshold, it can introduce some new issues, (unwanted/unjustified noise if the receiver has a very low velocity basically) but I believe this is still an improvement compared to previous implementation.

Approving ✔️

@anisunity anisunity force-pushed the HDRP/rt-shadow-flicker-history-validation branch from d1c4f8b to 0cb8e6a Compare January 28, 2021 12:59
@sebastienlagarde sebastienlagarde marked this pull request as draft January 28, 2021 13:43
@sebastienlagarde sebastienlagarde marked this pull request as ready for review February 1, 2021 18:45
@sebastienlagarde sebastienlagarde merged commit 91b6557 into master Feb 2, 2021
@sebastienlagarde sebastienlagarde deleted the HDRP/rt-shadow-flicker-history-validation branch February 2, 2021 18:34
@anisunity
Copy link
Contributor Author

Unfortuantely can't be a trade off. Either it ghosts or it doesn't

sebastienlagarde pushed a commit that referenced this pull request Mar 2, 2021
…92) and a refactoring to avoid computing the same value for every effect. #3039
sebastienlagarde added a commit that referenced this pull request Mar 2, 2021
* Fix issue with register spilling in light list shaders #3016

* Fix a flickering issue related to moving shadow receivers (case 1302392) and a refactoring to avoid computing the same value for every effect. #3039

* Merge Hd/bugfix #3047

* [HDRP]Improvements and fixes on Volumes Components Inspectors #3072

* Fix XR depth copy and MSAA #3075

* [HDRP][Compositor] Fix issues with compositor's undo #3100

* Hdrp/docs/fix 1305538 (#3112)

* Added information about using recursive rendering and other effects

* Fixed formatting

* [HDRP] Merge Hd/bugfix #3120

* Formatting

Co-authored-by: FrancescoC-unity <43168857+FrancescoC-unity@users.noreply.github.com>
Co-authored-by: anisunity <42026998+anisunity@users.noreply.github.com>
Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>
Co-authored-by: Fabien Houlmann <44069206+fabien-unity@users.noreply.github.com>
Co-authored-by: Pavlos Mavridis <pavlos.mavridis@unity3d.com>
Co-authored-by: Lewis Jordan <lewis.jordan@hotmail.co.uk>
sebastienlagarde pushed a commit that referenced this pull request Mar 3, 2021
…92) and a refactoring to avoid computing the same value for every effect. #3039
sebastienlagarde added a commit that referenced this pull request Mar 3, 2021
* Fix a flickering issue related to moving shadow receivers (case 1302392) and a refactoring to avoid computing the same value for every effect. #3039

* The RTAO's history is now discarded if the occlusion caster was moving (case 1303418). #3343

* Update CHANGELOG.md

* Add new API to CachedShadowManager and fix WouldFitInAtlas #3387

* Added master stack documentation for HDRP (#3388)

* Added landing page and context/blocks overview page

* Added vertex context page and snippets

* Added the fragment context page and supporting snippets

* Added the decal master stack and the contexts intro

* Updated toc

* Added vertex context doc

* Update TableOfContents.md

* Added unlit doc

* Added lit master stack doc

* Added eye master stack doc

* Added fabric master stack

* added hair master stack

* Added stacklit master stack

* Added snippets to support shader stack work

* Fixed some formatting bugs

* Removed old links

* Updated links

* formatting

Co-authored-by: Lewis Jordan <lewisjordan@unity3d.com>
Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Fix Sub-shadow rendering with cached shadows + Fix PCSS filtering issues on first frame or cached shadows #3396

* Reset ambient probe upon switching to very different skies bis #3423

* HDRP Graphic Test Fixes for Metal (#3408)

* Fixed the metal pipeline creation error message and updated several gfx test images.

* Rearranged LightData definition to workaround area lights not rendering issue on Intel GPUs on Catalina

* Updated 2501 reference image for Metal with the area light fix

* Updated 1550 and 2010 on Metal as previous were incorrect due to area light issue

Co-authored-by: Jeannette Yu <jeannette.yu@unity3d.com>

* Custom Pass error message fix (#3424)

Typo in DrawRenderersCustomPass error message. Fixed a few other typos in this section too :D

* Add Unity version to what's new 11 (#3432)

* Update whats-new-11.md

* Updated the table of contents

* Shader abstraction cleanup #3443

* Some leftover caused by master merge (#3456)

* Fix multiple any hit occuring on transparent objects (case 1294927). #3471

* Colored Shadows update (#3486)

* Colored Shadows update

Included new information in the Color Shadow property description to tell the user that the Material's  Refraction Model has to be set to Thin, Box or Sphere for it to work.

* Fixed typo

Removed the "the" :D

* Revert "Fix multiple any hit occuring on transparent objects (case 1294927). #3471"

This reverts commit 0587177.

* Update 1219_Lit_Light_on_Tesselation.png

Co-authored-by: anisunity <42026998+anisunity@users.noreply.github.com>
Co-authored-by: FrancescoC-unity <43168857+FrancescoC-unity@users.noreply.github.com>
Co-authored-by: Lewis Jordan <lewis.jordan@hotmail.co.uk>
Co-authored-by: Lewis Jordan <lewisjordan@unity3d.com>
Co-authored-by: Brandon Fogerty <34072588+brandon-unity3d@users.noreply.github.com>
Co-authored-by: Jeannette Yu <jeannette.yu@unity3d.com>
Co-authored-by: Vic Cooper <63712500+Vic-Cooper@users.noreply.github.com>
Co-authored-by: robinb-u3d <robinb-u3d@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants