Improving the denoiser for RTGI (case 1346383) #5078

anisunity · 2021-07-06T19:03:01Z

This PR improves the upscaling process as well as the denoising of RTGI (and RTAO), it is one of the two PRs to solve this bug
https://fogbugz.unity3d.com/f/cases/1346383/

Removes the upscaling parameter for RTGI (forced to 3 for half res, 4 for full res).
Reduces signficantly the cost of the upscaling pass.
Resolves bleeding (and ghosting artifacts) that come from the upscale.
Change the default parameters for quality settings for RTGI.
Fix edges issues with the half resolution denoiser.
Deduce automatically the world space denoiser radius for surfaces (instead of fixed for the whole scene)

https://youtu.be/J5UJRT5fF6E
https://youtu.be/GZwg9ditAzk

Testing status
DXR Tests are green after updating the screenshots.

github-actions · 2021-07-06T19:03:18Z

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.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

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.

anisunity · 2021-07-06T19:14:21Z

https://youtu.be/JNtsRVlo6Iw

anisunity · 2021-07-08T08:53:28Z

After running the test a thousand times, all green

remi-chapelain

I quickly tested performance on the ray tracing apartment scene in quality mode and in performance medium mode and -surprisingly, I found performance a bit worse than the current master. Needs to be discussed later.
I honestly don't know how we can solve this but since values are serialized in the HDRP asset, the only way to get the new values from the quality settings is to create a new HDRP Asset, which is a terrible pain for upgrading. It should at least be mentionned in the upgrade guide or something.

Also, only managed to repro those issues on those specifics projects, but they didn't repro on master so they must be linked with the improvement in a way.

Found some NaNs when we use the perf mode denoiser in this project. Doesn't repro in quality mode and on master.
https://user-images.githubusercontent.com/57442369/125320240-cc97ad00-e33b-11eb-8241-d9c489ab7ff5.mp4
And in perf mode again, I can see some green appearing on the right side of the view. Doesn't repro in quality mode and on master.
https://user-images.githubusercontent.com/57442369/125320427-fc46b500-e33b-11eb-9c30-d7f94826b1e7.mp4

Hit me up when you are getting back on this to either pair debug it or for me to send you repros.

anisunity · 2021-07-18T19:43:34Z

@remi-chapelain The counters for the perf are not working properly and cannot be trusted. I'll check the perf comparison using a GPU capture to make sure that there is no major regression.

Let's sync for the issues that you found, so that you give me a repro.

FrancescoC-unity · 2021-07-19T11:39:27Z

...ity.render-pipelines.high-definition/Runtime/RenderPipeline/GlobalLightingQualitySettings.cs

-            RTGIDenoiserRadius[(int)ScalableSettingLevelParameter.Level.Low] = 0.75f;
-            RTGIDenoiserRadius[(int)ScalableSettingLevelParameter.Level.Medium] = 0.5f;
-            RTGIDenoiserRadius[(int)ScalableSettingLevelParameter.Level.High] = 0.25f;
+            RTGIDenoiserRadius[(int)ScalableSettingLevelParameter.Level.Low] = 1.0f;


Given it is all the same value, does it make sense to keep as a scalability setting? (is it beneficial if user change this defaults or should they always be the same value?)

Users can chose to lower it for lower perf modes it as it can affect performance a lot

If it has an high impact, shouldn't this have different values by default too though? Out of the box the scalability does nothing here.

To be disccused

anisunity · 2021-07-19T11:39:46Z

The perf is equal at worst (full res, but higher visual quality), much better at best (half res)

FrancescoC-unity · 2021-07-19T11:44:39Z

....high-definition/Runtime/RenderPipeline/Raytracing/Shaders/Denoising/DiffuseDenoiser.compute

+{
+    // Compute the tap coordinate in the 8x8 grid
+    uint2 tapAddress = (uint2)((int2)(groupThreadId) + offset);
+    return clamp((uint)(tapAddress.x) + tapAddress.y * GATHER_REGION_SIZE, 0, GATHER_REGION_SIZE_2 - 1);


No need for cast here, tapAddress.x is already a uint

FrancescoC-unity · 2021-07-19T11:49:05Z

....high-definition/Runtime/RenderPipeline/Raytracing/Shaders/Denoising/DiffuseDenoiser.compute

+    uint2 cornerGroupThread = corner - groupId * DIFFUSE_DENOISER_TILE_SIZE;
+
+    // Grab the indices of the sub-region to use
+    int ldsIdx0 = OffsetToLDSAdress(cornerGroupThread, int2(0, 0));


So I see you don't load a safety border so it is clamp at group border which I guess can give problems if we have signficiant depth discontinuities happening at the edge of the group? Might be ok, but leaving comment here to see if needs further discussion or not.

Not sure to get what you are describing, let's chat

FrancescoC-unity · 2021-07-19T11:51:01Z

.../Runtime/RenderPipeline/Raytracing/Shaders/IndirectDiffuse/RaytracingIndirectDiffuse.compute

+
+    // If the normals of both pixels are too different, we cannot use it
+    if(dot(neighborData.normalWS, normalWS) < NORMAL_REJECTION_THRESHOLD)
+        weight *= 0.1f;


Would it make sense to soft reject this? Lerping towards 0.1 (or close to) multiplier as we approach the threshold?

I guess we can have a transition region. But the idea here is to not use these unless there are not better samples

anisunity · 2021-07-24T09:34:46Z

Everything is green

anisunity · 2021-07-24T12:00:44Z

Conflits, need to merge

anisunity · 2021-07-24T20:31:57Z

Everything is green @remi-chapelain depends on your review

remi-chapelain

Couldn't repro any of the issues I reported above. ✔️

For the rest, I could only see improvements, couldn't find any more regression on the scenes I tested. ✔️

Also tested animation because we don't have coverage for moving objects. ✔️

Pushed the changes regarding the upscale filter to make the documentation match.

anisunity added HDRP dxr labels Jul 6, 2021

anisunity requested review from sebastienlagarde and remi-chapelain July 6, 2021 19:03

anisunity self-assigned this Jul 6, 2021

github-actions bot added the testing label Jul 6, 2021

anisunity force-pushed the HDRP/rtgi-denoise-improv branch from 4f97ecb to a4b8457 Compare July 7, 2021 18:17

anisunity requested a review from FrancescoC-unity July 7, 2021 18:26

anisunity changed the title ~~[Draft] Improving the denoiser for RTGI~~ Improving the denoiser for RTGI (case 1346383) Jul 7, 2021

anisunity marked this pull request as ready for review July 7, 2021 18:27

anisunity force-pushed the HDRP/rtgi-denoise-improv branch from a4b8457 to cf4110b Compare July 8, 2021 07:07

remi-chapelain requested a review from a team July 12, 2021 12:02

remi-chapelain requested changes Jul 12, 2021

View reviewed changes

anisunity force-pushed the HDRP/rtgi-denoise-improv branch from 86ff6e4 to 4787412 Compare July 19, 2021 09:39

FrancescoC-unity reviewed Jul 19, 2021

View reviewed changes

FrancescoC-unity approved these changes Jul 19, 2021

View reviewed changes

anisunity requested a review from remi-chapelain July 19, 2021 13:13

sebastienlagarde approved these changes Jul 20, 2021

View reviewed changes

anisunity force-pushed the HDRP/rtgi-denoise-improv branch 3 times, most recently from 597a548 to 1062c75 Compare July 23, 2021 21:15

anisunity added 7 commits July 24, 2021 14:02

Improvements to the RTGI denoising.

fa5377e

Update the screenshots

f298240

Fix formatting

ed6d9f9

Fix green border bug

5ae58b2

Update screenshots

9205992

fix formatting

591e4f9

review fix

397751d

anisunity force-pushed the HDRP/rtgi-denoise-improv branch from 1062c75 to 397751d Compare July 24, 2021 12:04

Update screenshot

7e1a05a

Documentation update

c4988fd

remi-chapelain approved these changes Jul 26, 2021

View reviewed changes

anisunity merged commit 09e1584 into master Jul 26, 2021

anisunity deleted the HDRP/rtgi-denoise-improv branch July 26, 2021 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving the denoiser for RTGI (case 1346383) #5078

Improving the denoiser for RTGI (case 1346383) #5078

anisunity commented Jul 6, 2021 •

edited

github-actions bot commented Jul 6, 2021

anisunity commented Jul 6, 2021

anisunity commented Jul 8, 2021

remi-chapelain left a comment •

edited

anisunity commented Jul 18, 2021

FrancescoC-unity Jul 19, 2021

anisunity Jul 19, 2021

FrancescoC-unity Jul 19, 2021

anisunity Jul 19, 2021

anisunity commented Jul 19, 2021

FrancescoC-unity Jul 19, 2021

FrancescoC-unity Jul 19, 2021

anisunity Jul 19, 2021

FrancescoC-unity Jul 19, 2021

anisunity Jul 19, 2021

anisunity commented Jul 24, 2021

anisunity commented Jul 24, 2021

anisunity commented Jul 24, 2021

remi-chapelain left a comment •

edited

Improving the denoiser for RTGI (case 1346383) #5078

Improving the denoiser for RTGI (case 1346383) #5078

Conversation

anisunity commented Jul 6, 2021 • edited

github-actions bot commented Jul 6, 2021

anisunity commented Jul 6, 2021

anisunity commented Jul 8, 2021

remi-chapelain left a comment • edited

Choose a reason for hiding this comment

anisunity commented Jul 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anisunity commented Jul 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anisunity commented Jul 24, 2021

anisunity commented Jul 24, 2021

anisunity commented Jul 24, 2021

remi-chapelain left a comment • edited

Choose a reason for hiding this comment

anisunity commented Jul 6, 2021 •

edited

remi-chapelain left a comment •

edited

remi-chapelain left a comment •

edited