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.

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

Render graph support of RT and screen space shadows [Final] #1610

Merged
merged 9 commits into from
Aug 26, 2020
Merged

Render graph support of RT and screen space shadows [Final] #1610

merged 9 commits into from
Aug 26, 2020

Conversation

anisunity
Copy link
Contributor

@anisunity anisunity commented Aug 13, 2020 •
edited
Loading

Doing the Rendergraph implementation for RTAO and required denoisers. Enabling the render graph compatibiliity on RTAO tests.
Doing the Rendergraph implementation ray traced reflection support for the render graph version of the pipeline.
Doing the Rendergraph implementation for RTGI and required denoisers.
Doing the Rendergraph implementation for RTSSS and RR.
Doing the Rendergraph implementation for ray traced and screen space shadow.

Testing status
Render graph tests (DXR) that were enabled are green (All but Path tracing)
All the non-RG tests are also green (DXR)

@anisunity anisunity self-assigned this Aug 13, 2020
@github-actions
Copy link

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!

remi-chapelain
remi-chapelain approved these changes Aug 14, 2020
View reviewed changes
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.

Smoke tested directionnal light RT shadows
tested denoiser / semi transparent and colored shadows
✔️

@anisunity anisunity changed the title Render graph support of RT and screen space directional shadows Render graph support of RT and screen space shadows Aug 14, 2020
@anisunity
Copy link
Contributor Author

@remi-chapelain added the remaining shadows to this PR

@remi-chapelain remi-chapelain self-requested a review August 17, 2020 09:21
remi-chapelain
remi-chapelain approved these changes Aug 17, 2020
View reviewed changes
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.

Smoke tested the whole RT stack one last time + made a build with every RT effect
Tested denoisers, forward/deferred, fallbacks and deactivating raytracing custom frame settings
Everything seems to look as expected ✔️

@anisunity anisunity changed the title Render graph support of RT and screen space shadows Render graph support of RT and screen space shadows [Final] Aug 24, 2020
JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
com.unity.render-pipelines.high-definition/Runtime/Debug/DebugDisplay.cs Outdated
@@ -1586,7 +1590,7 @@ void RegisterRenderingDebug()
widgetList.Add(new DebugUI.BoolField { displayName = "XR single-pass test mode", getter = () => data.xrSinglePassTestMode, setter = value => data.xrSinglePassTestMode = value });
}

//widgetList.Add(new DebugUI.BoolField { displayName = "Enable Render Graph", getter = () => HDRenderPipeline.currentPipeline.IsRenderGraphEnabled(), setter = value => HDRenderPipeline.currentPipeline.EnableRenderGraph(value) });
widgetList.Add(new DebugUI.BoolField { displayName = "Enable Render Graph", getter = () => HDRenderPipeline.currentPipeline.IsRenderGraphEnabled(), setter = value => HDRenderPipeline.currentPipeline.EnableRenderGraph(value) });
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to revert back to commented before pushing the PR.

JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
...er-pipelines.high-definition/Runtime/Lighting/Shadow/ScreenSpaceShadowManager.RenderGraph.cs Outdated
dimension = TextureDimension.Tex2DArray,
filterMode = FilterMode.Point,
enableRandomWrite = true,
useDynamicScale = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

useDynamicScale is already set by the first param of the TextureDesc constructor so you can remove it as it is redundant in this case.

JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
...ity.render-pipelines.high-definition/Runtime/Lighting/Shadow/ScreenSpaceShadowManagerArea.cs Outdated
passData.motionVectorsBuffer = builder.ReadTexture(motionVectorsBuffer);
if (hdCamera.frameSettings.litShaderMode == LitShaderMode.Deferred)
{
passData.gbuffer0 = prepassOutput.gbuffer.mrt[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these textures read? Seems like it's missing a builder.ReadTexture here

JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
...der-pipelines.high-definition/Runtime/Lighting/Shadow/ScreenSpaceShadowManagerDirectional.cs Outdated
{
// If it is screen space but not ray traced, then we can rely on the shadow map
// WARNING: This pattern only works because we can only have one directional and the directional shadow is evaluated first.
CoreUtils.SetRenderTarget(cmd, textureArray, depthSlice: sssdParams.depthSlice);
cmd.SetGlobalTexture(HDShaderIDs._NormalBufferTexture, normalBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using SetGlobal is dangerous in the context of RenderGraph since it breaks the contract about lifetime of textures.
Please use a MaterialPropertyBlock along with the material to set this parameter.

JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
...lines.high-definition/Runtime/Material/SubsurfaceScattering/SubsurfaceScatteringManagerRT.cs Outdated
passData.depthStencilBuffer = builder.UseDepthBuffer(depthStencilBuffer, DepthAccess.Read);
passData.normalBuffer = builder.ReadTexture(normalBuffer);
passData.sssColor = builder.ReadTexture(sssColor);
passData.intermediateBuffer0 = builder.WriteTexture(renderGraph.CreateTexture(new TextureDesc(Vector2.one, true, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the intermediateBuffersX are only used in this pass. Shouldn't they use CreateTransientTexture?

JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
...lines.high-definition/Runtime/Material/SubsurfaceScattering/SubsurfaceScatteringManagerRT.cs Outdated
{ colorFormat = GraphicsFormat.R16G16B16A16_SFloat, enableRandomWrite = true, name = "Intermediate Texture 2" }));
passData.intermediateBuffer3 = builder.WriteTexture(renderGraph.CreateTexture(new TextureDesc(Vector2.one, true, true)
{ colorFormat = GraphicsFormat.R16G16B16A16_SFloat, enableRandomWrite = true, name = "Intermediate Texture 3" }));
passData.directionBuffer = builder.WriteTexture(renderGraph.CreateTexture(new TextureDesc(Vector2.one, true, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for directionBuffer

JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
...ender-pipelines.high-definition/Runtime/RenderPipeline/Raytracing/HDDiffuseShadowDenoiser.cs Outdated
int kernelSize, float angularDiameter, bool singleChannel = true)
{
// Request the intermediate buffer we need
RTHandle intermediateBuffer = m_RenderPipeline.GetRayTracingBuffer(InternalRayTracingBuffers.RGBA3);
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem to be used.

JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
...ender-pipelines.high-definition/Runtime/RenderPipeline/Raytracing/HDDiffuseShadowDenoiser.cs Outdated
int kernelSize, Vector3 lightPosition, float lightRadius)
{
// Request the intermediate buffer we need
RTHandle intermediateBuffer = m_RenderPipeline.GetRayTracingBuffer(InternalRayTracingBuffers.RGBA3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above (also, the format does not match the one in the render graph pass. Is that intended? Or is RGBA3 not what I think it is?)

JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
...pipelines.high-definition/Runtime/RenderPipeline/Raytracing/HDRaytracingRecursiveRenderer.cs Outdated
return renderGraph.CreateTexture(new TextureDesc(Vector2.one, true, true)
{
colorFormat = GraphicsFormat.R8_SNorm,
slices = TextureXR.slices ,
Copy link
Contributor

Choose a reason for hiding this comment

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

slices, dimension and useDynamicScale are already set by the 2 parameters of the TextureDesc constructor so they are redundant here.

JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
...unity.render-pipelines.high-definition/Runtime/RenderPipeline/Raytracing/HDSimpleDenoiser.cs Outdated
int kernelSize, bool singleChannel)
{
// Request the intermediate buffer we need
RTHandle intermediateBuffer = m_RenderPipeline.GetRayTracingBuffer(InternalRayTracingBuffers.RGBA3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover again?

JulienIgnace-Unity
JulienIgnace-Unity reviewed Aug 24, 2020
View reviewed changes
...-pipelines.high-definition/Runtime/RenderPipeline/Raytracing/HDTemporalFilter.RenderGraph.cs Outdated
passData.normalBuffer = builder.ReadTexture(normalBuffer);
passData.motionVectorBuffer = builder.ReadTexture(motionVectorBuffer);
passData.velocityBuffer = renderGraph.defaultResources.blackTextureXR;
passData.historyDepthTexture = renderGraph.ImportTexture(hdCamera.GetCurrentFrameRT((int)HDCameraFrameHistoryType.Depth));
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing read/write info on imported buffers here. It won't have an impact since imported buffer's lifetime is not handled by rendergraph but it can be used for debug display purpose so it's better to be consistent and specify it for imported buffers as well.

JulienIgnace-Unity
JulienIgnace-Unity suggested changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@JulienIgnace-Unity JulienIgnace-Unity 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 few comments.

@anisunity
Copy link
Contributor Author

Check Read/Writes

This was referenced Aug 26, 2020
Closed
Closed
Closed
Closed
@JulienIgnace-Unity JulienIgnace-Unity merged commit d6f22cc into HDRP/staging Aug 26, 2020
@JulienIgnace-Unity JulienIgnace-Unity deleted the HDRP/RTShadows_RG branch August 26, 2020 09:48
@sebastienlagarde sebastienlagarde mentioned this pull request Sep 3, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JulienIgnace-Unity JulienIgnace-Unity JulienIgnace-Unity approved these changes

@remi-chapelain remi-chapelain remi-chapelain approved these changes

@sebastienlagarde sebastienlagarde Awaiting requested review from sebastienlagarde

Assignees

@anisunity anisunity

Labels
dxr HDRP testing
Projects
None yet
Milestone
No milestone
3 participants
@anisunity @JulienIgnace-Unity @remi-chapelain