Distortion optimizations #1961

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.

Sign up for GitHub

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

Jump to bottom

Merged

sebastienlagarde merged 11 commits into master from HDRP/SmoothDistortionSmoothPlanar

Oct 20, 2020

Contributor

anisunity commented Sep 22, 2020 •

edited by sebastienlagarde

Loading

Added a rough distortion frame setting and and info box on distortion materials (On the Frame settings, disabled by default)
Added tests to cover the new feature.
Did the rendergraph support.

Tested locally, all the test were green except an unrelated one.
I added one new tests to cover the feature.

Made sure that the rough distortion was unchked by default.
Made sure that the info message only displayed when distortion was checked.
Made sure when checked the distortion only did the offset part.
Made sure when unchked, the other color pyramid was not built

anisunity added the HDRP label

anisunity requested review from sebastienlagarde, JulienIgnace-Unity and a team

September 22, 2020 16:10

anisunity self-assigned this

github-actions bot added SRP testing labels

github-actions bot commented Sep 22, 2020

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page)
and cancel any jobs that started on Yamato.
See the PR template for more information.
Thank you!

JulienIgnace-Unity reviewed

View reviewed changes

com.unity.render-pipelines.high-definition/Runtime/Material/GGXConvolution/IBLFilterGGX.cs Outdated Show resolved Hide resolved

JulienIgnace-Unity reviewed

View reviewed changes

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.RenderGraph.cs Outdated Show resolved Hide resolved

JulienIgnace-Unity suggested changes

View reviewed changes

Contributor

JulienIgnace-Unity left a comment

There seem to be an implementation mistake in the render graph path.

Contributor Author

anisunity commented Sep 28, 2020

Fixed both @JulienIgnace-Unity

JulienIgnace-Unity approved these changes

View reviewed changes

sebastienlagarde changed the title ~~Planar and Distortion optimizations~~ Distortion optimizations [Thinking]

sebastienlagarde removed the request for review from a team

October 6, 2020 13:40

sebastienlagarde mentioned this pull request

Add option to enable pure specular planar reflection (optimization) #2105

Merged

anisunity force-pushed the HDRP/SmoothDistortionSmoothPlanar branch from 58e7c4d to c13117e Compare

October 13, 2020 10:43

JulienIgnace-Unity reviewed

View reviewed changes

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.RenderGraph.cs Outdated

-                          // because the output texture is imported from outside of render graph (as it is persistent)
-                          // and in this case the pass is considered as having side effect and cannot be culled.
-                          if (isPreRefraction)
+                          if (!isPreRefraction)

Contributor

JulienIgnace-Unity Oct 13, 2020

This logic here is becoming too complicated IMO.
We have logic outside the function (cf above RenderDistorsion) and also insde the function.
In term of RenderGraph logic, GenerateColorPyramid should do just what it says it does and all logic calling it or not should be outside. I think this would make things much clearer here (both on caller side, and inside the function)

Contributor Author

anisunity Oct 14, 2020

fixed

JulienIgnace-Unity reviewed

View reviewed changes

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.RenderGraph.cs Outdated

+                              }
+                              else
+                              {
+                                  distortionColorPyramid = currentColorPyramid;

Contributor

JulienIgnace-Unity Oct 13, 2020

Just to make sure I get it. Here we use the previous pyramid and just read mip 0 for smooth disto right?

Contributor Author

anisunity Oct 13, 2020 •

edited

Loading

Actually if smooth, we do not read the previous color pyramid, we copy the color buffer into an other buffer to use it as an input and an output

JulienIgnace-Unity reviewed

View reviewed changes

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.RenderGraph.cs Outdated

+                                      });
+                                  GenerateColorPyramid(m_RenderGraph, hdCamera, colorBuffer, distortionColorPyramid, false);
+                              }
+                              else

Contributor

JulienIgnace-Unity Oct 13, 2020 •

edited

Loading

I think we don't ever use currentColorPyramid after this code so we could just skip the else statement and store result of first branch in currentColorPyramid if needed by rough disto.

Contributor Author

anisunity Oct 13, 2020

done

JulienIgnace-Unity reviewed

View reviewed changes

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.RenderGraph.cs Outdated

+                              TextureHandle distortionColorPyramid;
+                              if (hdCamera.frameSettings.IsEnabled(FrameSettingsField.Distortion) && hdCamera.frameSettings.IsEnabled(FrameSettingsField.RoughDistortion))
+                              {
+                                  distortionColorPyramid = m_RenderGraph.CreateTexture(

Contributor

JulienIgnace-Unity Oct 13, 2020

One thing to note here.
Before those changes, I think we'd just use a double buffered history pyramid from the hdCamera.
Here we create a new one. Since we have no other (I think) textures with mips in the pipeline, this will actually allocate more memory than before. Can't we just reuse one of the history buffers?

Contributor Author

anisunity Oct 14, 2020

discussed, not relevant in this case

sebastienlagarde reviewed

View reviewed changes

...ipelines.high-definition/Runtime/RenderPipeline/RenderPass/Distortion/ApplyDistortion.shader Outdated

    
            @@ -12,6 +12,8 @@ Shader "Hidden/HDRP/ApplyDistortion"
          
                      #pragma only_renderers d3d11 playstation xboxone vulkan metal switch

                      #pragma editor_sync_compilation

                      #pragma multi_compile _ SMOOTH_DISTORTION

Contributor

sebastienlagarde Oct 14, 2020

change for a dynamic branch , no need for more variant here given the cost

Contributor

JulienIgnace-Unity Oct 14, 2020

I'd argue that this is a pretty simple shader here. Compilation cost is probably meaningless and maintenance complexity is very low so might as well have the most performant code (even if it's just a tiny bit).

Contributor

sebastienlagarde Oct 14, 2020 •

edited

Loading

There is no perf improvement here, but I think the
CoreUtils.SetKeyword(context.cmd, "SMOOTH_DISTORTION", data.smoothDistortion);
could easily pollute the global space, so better so keep everything simple for such cases.

Contributor

sebastienlagarde Oct 14, 2020

side note: use saturate (free) and not clamp (min / max ALU) for clamp(distortionBlur, 0.0, 1.0);

Contributor

sebastienlagarde Oct 14, 2020

and if you want to avoid branch at all you can do

float mip = (_ColorPyramidLodCount - 1) * clamp(distortionBlur, 0.0, 1.0);

=> float mip = (_ColorPyramidLodCount - 1) * saturate(distortionBlur) * RoughDistortion;
with RoughDistortion == 0 if smooth
it cost just an extra mul

Contributor Author

anisunity Oct 14, 2020

All done, relaunching the tests locally and pushing

anisunity force-pushed the HDRP/SmoothDistortionSmoothPlanar branch from 0af937f to b74efdb Compare

October 14, 2020 08:55

sebastienlagarde approved these changes

View reviewed changes

sebastienlagarde changed the title ~~Distortion optimizations [Thinking]~~ Distortion optimizations

anisunity force-pushed the HDRP/SmoothDistortionSmoothPlanar branch from 8def4b3 to e07a704 Compare

October 15, 2020 13:32

anisunity added 4 commits

October 15, 2020 18:36


          Added a rough distortion frame setting and and info box on distortion…

410a1d2

… materials.

Proper support for the migration process
Chaging the default value for rough distortion frame setting
SSR now uses the pre-refraction color pyramid.


          Review fixes, updating screenshot and adding capture delay

089f9b3


          additional review fixes

c001879


          review fixes

ee7d68d

anisunity added 3 commits

October 15, 2020 18:36


          missing meta file

28e0f64


          minor fix

831af9d


          Update screenshots

cc24358

anisunity force-pushed the HDRP/SmoothDistortionSmoothPlanar branch from df17c3c to cc24358 Compare

October 15, 2020 16:37


          update windows vulkan screenshots

a435e03

sebastienlagarde changed the base branch from HDRP/staging to master

October 19, 2020 22:13


          update documentation

28539bf

sebastienlagarde requested a review from JordanL8

October 20, 2020 08:54

JordanL8 reviewed

View reviewed changes

com.unity.render-pipelines.high-definition/Documentation~/Upgrading-from-2020.1-to-2020.2.md Outdated Show resolved Hide resolved

JordanL8 reviewed

View reviewed changes

com.unity.render-pipelines.high-definition/Documentation~/whats-new-10-0.md

               Planar reflection probe filtering is a process that combines the result of planar reflection and surfaces smoothness. Up until this version, the implementation for planar reflection probe filtering did not always produce results of fantastic quality. This version of HDRP includes a new implementation that is closer to being physically-based and improves on the image quality significantly.
+              ### Screen space reflection
+              [Screen Space Reflection](Override-Screen-Space-Reflection.md) effect always use the color pyramid generate after the Before Refraction transparent pass. Thus the color buffer only includes transparent GameObjects that use the **BeforeRefraction** [Rendering Pass](Surface-Type.md).

Contributor

JordanL8 Oct 20, 2020

Screen Space Reflection now uses the color pyramid HDRP generates after the Before Refraction transparent pass. This means the color buffer only includes transparent GameObjects that use the BeforeRefraction Rendering Pass.

JordanL8 reviewed

View reviewed changes

com.unity.render-pipelines.high-definition/Documentation~/whats-new-10-0.md Outdated Show resolved Hide resolved

JordanL8 suggested changes

View reviewed changes

Contributor

JordanL8 left a comment

Reworded a few bits

sebastienlagarde added 2 commits

October 20, 2020 18:33


          update doc

0506a7d


          Merge branch 'master' into HDRP/SmoothDistortionSmoothPlanar

a4762b1

sebastienlagarde merged commit ccdccef into master

sebastienlagarde deleted the HDRP/SmoothDistortionSmoothPlanar branch

October 20, 2020 16:36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

HDRP SRP testing