Skip to content

[HDRP] AOV API improvements #2769

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 38 commits into from
Feb 12, 2021
Merged

[HDRP] AOV API improvements #2769

merged 38 commits into from
Feb 12, 2021

Conversation

pmavridis
Copy link
Contributor

@pmavridis pmavridis commented Nov 27, 2020

Purpose of this PR

This PR introduces some improvements for the AOV API:

  • Fresnel0 output buffer
  • WorldSpacePosition output buffer
  • Internal rendering of the debug views is now using the buffer format provided by the user (no change in the public AOV API).

Bugfix: in the rendergraph path we have broken the "custom pass" AOV output, because we were not testing it :(


Testing status

The new features (and the custom pass AOV output) are tested with a new graphics test (ID 9902):
image


Comments to reviewers

Many new files in this PR are related to the graphics test.

@pmavridis pmavridis requested review from sebastienlagarde and a team November 27, 2020 10:22
@pmavridis pmavridis added the HDRP label Nov 27, 2020
# Conflicts:
#	TestProjects/HDRP_Tests/ProjectSettings/EditorBuildSettings.asset
#	com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.RenderGraph.cs
@pmavridis pmavridis removed the request for review from a team December 1, 2020 22:03
@pmavridis
Copy link
Contributor Author

Note: I'm in the process of resolving the merge conflicts, so until then this is not ready for review yet!

@sebastienlagarde sebastienlagarde requested a review from a team January 15, 2021 13:02
# Conflicts:
#	TestProjects/HDRP_Tests/ProjectSettings/EditorBuildSettings.asset
#	com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.RenderGraph.cs
#	com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs
#	com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/RenderPass/AOV/AOVRequestData.cs
@pmavridis
Copy link
Contributor Author

pmavridis commented Jan 25, 2021

I forgot to say that this PR is ready for reviewing now.
Changes from previous version:

  • For Fresnel zero, I'm now using the MaterialSharedProperty.Specular output, and I'm making sure that we get sensible information for materials that use the metalness workflow (convert metalness to f0)
  • We can now override the render format for the internal HDRP rendering when outputting AOVs (so this will affect lighting calculations too, not just full screen debug). To do this, the user needs to call a new public API:
    aovRequest.SetOverrideRenderFormat(true);
    In this case we will use the format of the user-allocated output buffer.
  • The docs have been updated to include this information

@pmavridis pmavridis marked this pull request as ready for review January 25, 2021 14:12
@sebastienlagarde
Copy link
Contributor

This PR is ready and green ABV, waiting for QA approval. @RemyUnity

@sebastienlagarde sebastienlagarde requested a review from a team January 28, 2021 17:36
@sebastienlagarde
Copy link
Contributor

Adding release management per request

@sebastienlagarde
Copy link
Contributor

Note: the failure on Mac and XR are from trunk and master.

@pmavridis
Copy link
Contributor Author

I have updated one last test image to make Nightly VFX_HDRP green (after discussing with Paul).

# Conflicts:
#	TestProjects/HDRP_Tests/ProjectSettings/EditorBuildSettings.asset
Copy link
Contributor

@JMargevics JMargevics left a comment

Choose a reason for hiding this comment

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

Made a pass. Things I've checked:

  • Tested new AOV buffers with HDRP shaders and all available master nodes (except Unlit for specular) ✅
  • Set the AOV buffer format to GraphicsFormat.R32G32B32A32 and verify in RenderDoc. Anything after the LorResTransparent pass uses the correct format. ✅ (some passes like LowResTransparent are still on 16bit and it's a requirement. Pavlos investigates which ones should stay on 16bit)
  • Wrangle vertex positions in SG and see if it affects World Position AOV. ✅
  • Checked if VFX mesh outputs appear correctly in new AOV buffers ✅
  • Verified on MacOS ✅

I have no breaking issues so far, all good on my side ✅

@pmavridis
Copy link
Contributor Author

pmavridis commented Feb 11, 2021

Just to give more context regarding the format override:
We don't override the format of certain passes than need a specific format, like the g-buffer. Also passes like LowResTransparent always needs an alpha channel, so we also don't override that.

Having said that, I'm looking at a RenderDoc capture again to see if I have missed something.
(And I will fix the merge conflicts, since this branch is a bit old now)

@pmavridis
Copy link
Contributor Author

pmavridis commented Feb 11, 2021

There was an additional issue with the format of the color pyramid generation that I have discovered after @JMargevics comments. I'm submitting the fix in a separate PR (#3470 ) because it might affect users outside of the AOV API (just in case we want to backport).

@sebastienlagarde sebastienlagarde merged commit b7e95f8 into master Feb 12, 2021
@sebastienlagarde sebastienlagarde deleted the HDRP/aov_improvements branch February 12, 2021 21:40
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.

4 participants