Skip to content

Conversation

anisunity
Copy link
Contributor

  • Added a toggle to allow to include or exclude smooth surfaces from ray traced reflection denoising.
  • Fixed issues in geometry buffer reading for half res RTR and RTGI.
  • Removed the upscale radius from the RTR.
  • How the history validity is properly tracked for RTR.
  • Changed the denoising strategy for very rough RTR surfaces.

Testing status
Tested on various scenes
Ran the tests locally

This PR needs to be tested quite extensively before being merged. I already discussed what needs to be tested with @remi-chapelain .

@anisunity
Copy link
Contributor Author

@JordanL8 FYI

@remi-chapelain
Copy link
Contributor

[Not finished testing]
Since parts of the denoisers are shared between RTR perf mode and RTGI perf mode and Upscale radius was removed for RTR in perf mode, would it make sense to also remove it for RTGI ? Either by removing the effect completely (since it can be very subtle especially with both denoisers ON) or hardcode the value somewhere if the cost can be negligible?

fb4993a419323014a0306fc7ad8278a6

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.

Tried multiple scene from out Full Test pass project with different reflective setup with different smoothness transitions. ✔️

Tested with different viewport size with scene and game view, deferred and forward. ✔️
Took a look at the updated doc ✔️

Also tested RTGI in perf (because part of denoiser is shared) and difference is very subtle but can be seen as "better" but definitely not worse. ✔️
See comment above to also remove upscale radius in the RTGI also to match what is done with SSR in perf. ❗

Couldn't find a case/setup where the PR is worse than what was previously done.
Biggest improvement can be seen using half resolution in performance mode.
Checkbox that force to affect perfectly smooth surface definitely helps reducing aliasing and small glitches that attracts the eye ✔️

Example of improvement at half resolution (Left is before / right is after)

@sebastienlagarde sebastienlagarde marked this pull request as ready for review December 14, 2020 18:42

#endregion

#region Legacy Pipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "Legacy pipeline"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non-render graph

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this pr took too much time to get merged. @JulienIgnace-Unity removed this part of the code i think


GGXSample GenerateGGXSampleDirection(uint2 currentCoord, float3 normalWS, float3 viewWS, float roughness, int frameIndex)
{
// Create the local ortho basis
Copy link
Contributor

Choose a reason for hiding this comment

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

is this function really specialise to this files? Shouldn't this be share.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exact version is only used for perf rtr

…ray traced reflection denoising.

- Fixed issues in geometry buffer reading for half res RTR and RTGI.
- Removed the upscale radius from the RTR.
- How the history validity is properly tracked for RTR.
- Changed the denoising strategy for very rough RTR surfaces.
- Updated tooltip and removed documentation for a property that no longer exists
- Added informatiom about the Affects Smooth Surfaces property
@anisunity anisunity force-pushed the HDRP/rtr-improvements-hr branch from 623e68a to d880e69 Compare December 15, 2020 11:06
@anisunity
Copy link
Contributor Author

Merging and updating screenshots

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.

4 participants