Skip to content
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

[HDRP] Path tracer denoiser #6652

Merged
merged 57 commits into from
Feb 7, 2022
Merged

[HDRP] Path tracer denoiser #6652

merged 57 commits into from
Feb 7, 2022

Conversation

pmavridis
Copy link
Contributor

@pmavridis pmavridis commented Dec 17, 2021

Purpose of this PR

This PR exposes a set of denoising options for the HDRP path tracer. The implementation is using the "Unity Denoising Plugin" package that needs to be installed with the package manager. If the package is installed, then the new denoising options are available in the path tracer UI:
image

The number of the denoisers that are available in the options depends on the "Unity Denoiser Plugin" .
Currently we expose:

  • Intel Open Image Denoise
  • NVIDIA Optix denoiser

Example input image (32 samples):
image

Denoised output:
image

Temporal stability

When recording video sequences with the Unity Recorder, the "temporal" option should be enabled to maximize the temporal consistency of the output. From the available backends, currently only the NVIDIA Optix denoiser supports this option, with some limitations (using AOVs at the same time as temporal mode produces some artefacts).

Requirements

The Optix 7.4 backend requires an NVIDIA GPU with driver version R495.89 or newer.

For now you have to install manually the denoising package from this link [FOR INTERNAL USE ONLY]:
https://drive.google.com/file/d/1dxwHF-3iExDUUIVfYwsNJkiogR4jyZ3c/view?usp=sharing
(Unzip the archive and point the package manager to it)


Testing status

Testing the denoisers requires installing the denoising package that will be release separately (as Editor built-in package or in the Unity Registry)

  • Tested denoising in scene view, game view, scene+game view side-by-side
  • Tested denoising when recording sequences with the Unity recorder.

(TODO: need to add a test scene after we release the package)

@pmavridis pmavridis marked this pull request as ready for review January 25, 2022 08:30
@@ -40,6 +40,7 @@ Path tracing uses the [Volume](Volumes.md) framework, so to enable this feature,
| **Maximum Depth** | Set the maximum number of light bounces in each path. You can not set this to be lower than Minimum Depth.<br /> **Note**: You can set this and Minimum Depth to 1 if you only want to direct lighting. You can set them both to 2 if you only want to visualize indirect lighting (which is only visible on the second bounce). |
| **Maximum Intensity** | Set a value to clamp the intensity of the light value each bounce returns. This avoids very bright, isolated pixels in the final result.<br />**Note**: This property can make the final image dimmer, so if the result looks dark, increase the value of this property. |
| **Sky Importance Sampling** | Set the sky sampling mode. Importance sampling favors the brightest directions, which is beneficial when using a sky model with high contrast and very intense spots (like a sun, or street lights). On the other hand, it can be slightly detrimental when using a smooth, uniform sky. It is active by default for HDRI skies only, but can also be turned On and Off, regardless of the type of sky in use. |
| **Denoising** | Set the denoising mode for the path tracer. This option is only available when the "Unity Denoiser Package" is installed in your project and the available options might change based on the version of this package. The denoising operation can be adjusted by the following parameters: <br> - **Use AOVs**: improves the detail retention after denoising. <br> - **Temporal**: improves temporal consistency when denoising frame sequences.|
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe here would be a great time to clean the doc? According to our guidelines

  • max depth should use "Controls" instead of "Set"
  • max intensity should use "Controls"
  • Sky Importance Sampling => "Specifies"
  • Denoising should use "Specifies"
  • Maybe AOV and temporal should have their own lines starting with "When enabled,.. " as the rest of the checkboxes

Also we are missing the layer mask (which should use "specifies" as the rest of the dropdowns)

Copy link
Contributor Author

@pmavridis pmavridis Feb 1, 2022

Choose a reason for hiding this comment

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

@ValGrimm-U3D can you confirm these changes (the ones not related with denoising)? For denoising I have already applied your suggestions.

@remi-chapelain
Copy link
Contributor

remi-chapelain commented Jan 31, 2022

I did a second pass and here's a few concerns I'd like to talk about before shipping; We probably can't do much about some of those but worth a discussion :)

  • [FIXED] (waiting for c++ PR to include package) This is probably still expected but the fix button doesn't do anything (package not published yet?)
  • [WONT FIX] (temporal uses another different network so we can't really debug it) When enabling temporal on the nvidia denoiser, the image become darker for some reasons? Is there anything we can do? video
  • When enabling temporal there seems to be some sort of duplication of the previous / current image superimposed on the screen video1 video2
  • Most of the time switching from one denoiser to the other (and to none) seems to work without re-accumulating, but not everytime, (still not able to find the repro when that doesn't work)
  • [WONT FIX] (AOV needs to reaccumulate too) Same for the checkboxes use AOVs and temporal, sometimes it works but not everytime.
  • [FIXED] Using denoiser with scene and game view next to each other makes scene view glitchy video
  • [FIXED] In some situations, (try the inside of the template like in the gif), the denoising will pick what it would seem improper data outside the frame leaving you with some border on top and right of the frame video
  • Also would be cool to add multiple test for thoses if possible (I believe they are deterministic, but I'm worried about the package that could cause problem here)
  • [NOT RELATED TO THIS PR] Somehow, now the pathtracer becomes all green when using it in play mode. video
  • [WILL BE DONE LATER] Still think we should complete our documentation with:
    --- The goal of this feature which would be to clean small remaining noise when the pathtracer is not able to converge (not to have real-time denoised pathtracing).
    --- Providing an example gif with noisy / denoised picture would be great too (I can help with that)
    --- Also providing which filter works best in what conditions (i.e what is the reason we provide two filter instead of one)

@@ -40,6 +40,7 @@ Path tracing uses the [Volume](Volumes.md) framework, so to enable this feature,
| **Maximum Depth** | Set the maximum number of light bounces in each path. You can not set this to be lower than Minimum Depth.<br /> **Note**: You can set this and Minimum Depth to 1 if you only want to direct lighting. You can set them both to 2 if you only want to visualize indirect lighting (which is only visible on the second bounce). |
| **Maximum Intensity** | Set a value to clamp the intensity of the light value each bounce returns. This avoids very bright, isolated pixels in the final result.<br />**Note**: This property can make the final image dimmer, so if the result looks dark, increase the value of this property. |
| **Sky Importance Sampling** | Set the sky sampling mode. Importance sampling favors the brightest directions, which is beneficial when using a sky model with high contrast and very intense spots (like a sun, or street lights). On the other hand, it can be slightly detrimental when using a smooth, uniform sky. It is active by default for HDRI skies only, but can also be turned On and Off, regardless of the type of sky in use. |
| **Denoising** | Set the denoising mode for the path tracer. This option is only available when the "Unity Denoiser Package" is installed in your project and the available options might change based on the version of this package. The denoising operation can be adjusted by the following parameters: <br> - **Use AOVs**: improves the detail retention after denoising. <br> - **Temporal**: improves temporal consistency when denoising frame sequences.|
Copy link
Contributor

@ValGrimm-U3D ValGrimm-U3D Feb 1, 2022

Choose a reason for hiding this comment

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

Here is my revision proposal:

Denoising | Enable this feature to denoise the output of the the path tracer. This feature is only available if the Unity Denoiser Package is installed in your project. The parameters available may differ depending on the package version.

There are three denoising libraries available:

  • Intel Open Image Denoise
  • NVIDIA OptiX
  • Radeon Image Filtering (Experimental)

    The default is Intel Open Image Denoise.

    You can also enable two additional settings:
  • Use AOVs (Arbitrary Output Variables): Enable this option to increase detail retention after denoising.
  • Temporal: Enable this option to improve temporal consistency for denoised frame sequences.

Also I thought the Denoiser Package was planned to be included with Unity by default, as a core package? That doesn't mean you shouldn't mention that it needs to be installed, just wanted to check this. Also, later we should add a link to the docs for the denoiser package here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Altough, there's no Radeon filter anymore in the latest change so I think it has been removed for good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ValGrimm-U3D I have rephrased this a bit, because as Remi said we expose only the two backends in HDRP (for now) and there is also the None option in the dropdown (which is the default). Can you please check again? thanks.

}
else
{
EditorGUILayout.HelpBox($"The selected denoiser is not supported by this hardware configuration.", MessageType.Error, wide: true);
Copy link
Contributor

Choose a reason for hiding this comment

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

selected denoiser --> denoiser selected

}
}
#else
CoreEditorUtils.DrawFixMeBox("Path Tracing Denoising is not active in this project. To activate it, install the Unity Denoising Plugin package.", MessageType.Info, () =>
Copy link
Contributor

@ValGrimm-U3D ValGrimm-U3D Feb 1, 2022

Choose a reason for hiding this comment

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

Unity Denoising Plugin or Unity Denoiser Plugin?
I've seen both in this PR.
The first of these two options sounds more grammatical to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have Denoising in all publicly visible places right now. Before release we have the flexibility to change that.
In the scripting API, we do use "Denoiser" for the class that performs the denoising.

AlbedoAOV,
/// <summary>Path traced Normal AOV.</summary>
NormalAOV,
/// <summary>Path traced motion vecotr AOV.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

vecotr --> vector

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually wait, these are just comments, not API doc, huh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it's published as API doc in the package, so I'm fixing everything. Thanks for the comments.
(This is how we do scripting API docs in packages, hence the confusion in our meeting earlier today ;) )

NormalAOV,
/// <summary>Path traced motion vecotr AOV.</summary>
MotionVectorAOV,
/// <summary>Denoised path traced frame history.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the above:
path traced ---> path-traced

@@ -86,6 +114,33 @@ public sealed class PathTracing : VolumeComponent
[Tooltip("Defines the maximum, post-exposed luminance computed for indirect path segments. Lower values help against noise and fireflies (very bright pixels), but introduce bias by darkening the overall result. Increase this value if your image looks too dark.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

help against ---> help prevent

/// <summary>
/// Improves the detail retention after denoising by using albedo and normal AOVs.
/// </summary>
[Tooltip("Improves detail of the denoised image by using albedo and normal AOVs")]
Copy link
Contributor

Choose a reason for hiding this comment

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

detail --> the detail
by using --> with its

/// <summary>
/// Enables temporally stable denoising (not all denosing backends support this option)
/// </summary>
[Tooltip("Enables temporally stable denoising")]
Copy link
Contributor

Choose a reason for hiding this comment

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

temporally-stable

Copy link
Contributor

@ValGrimm-U3D ValGrimm-U3D left a comment

Choose a reason for hiding this comment

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

I have noted the changes requested in my comments.

@pmavridis pmavridis requested review from ValGrimm-U3D and removed request for remi-chapelain February 1, 2022 13:55
@pmavridis
Copy link
Contributor Author

@remi-chapelain Here are a few quick answers on the things that you have noted:
(for some of these fixes you need a new package)

  • The fix button needs to be verified with the Editor that will include the built-in package.
  • The bug regarding resetting iterations sometimes when switching the denoisers is now fixed (also fixes a related bug when increasing sample count)
  • It's expected to reset the path tracing iterations when toggling AOVs or Temporal mode.
  • The bug when game-view and scene-view are side-by-side is fixed (was related to rthandle scale).
  • The bug with the denoiser leaking information on the edges from outside the frame is fixed.
  • Regarding your comments on documentation, I agree that we need a general user guide regarding the denoising feature. We have discussed this with @ValGrimm-U3D and we plan to add some documentation in the Unity Manual (outside HDRP) since this is common infrastructure between HDRP and the lightmapper (so both can link to it).

I still need to fix the bug related to the temporal mode (motion vectors when rthandle scale is not 1). But we cannot control the color/exposure shift that happens when enabling this mode.


You can also enable two additional settings:
- **Use AOVs** (Arbitrary Output Variables): Enable this option to increase detail retention after denoising.
- **Temporal**: Enable this option to improve temporal consistency for denoised frame sequences.|
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good.

Two small punctuation/spacing things:

  • Just in case it causes a formatting issue, I'd add a space between the first table border and "Enable" and the other table border and "sequences."
  • For consistency, it would be good to add a period to the Open Image Denoise line.

Copy link
Contributor

@ValGrimm-U3D ValGrimm-U3D left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

noreply@unity3d.com and others added 5 commits February 2, 2022 11:47
# Conflicts:
#	com.unity.render-pipelines.high-definition/CHANGELOG.md
#	com.unity.render-pipelines.high-definition/Documentation~/Ray-Tracing-Path-Tracing.md
@sebastienlagarde sebastienlagarde merged commit 14c8dd3 into master Feb 7, 2022
@sebastienlagarde sebastienlagarde deleted the HDRP/pt_denoise branch February 7, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants