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

Introduce concept of falling back textures to black in rendergraph + ReadWrite tag #2762

Merged
merged 6 commits into from Dec 3, 2020

Conversation

FrancescoC-unity
Copy link
Contributor

This among a more general thing fixes: https://fogbugz.unity3d.com/f/cases/1295272/

The issue was that because normal buffer is set globally is marked as read in the transparency pass. However, if no opaque objeccts were defined, the texture handle would be valid (we create it outside passes), but the content won't be as it was never written to.

Instead of fixing the specific case, I introduced the concept of write count and fall back to black texture whenever the above case happens and that should happen automagically whenever a texture is declared to have such behaviour. This logic needs to be on the Rendergraph resource to avoid issues when an handle is copied around but is not updated.

On top of that, since it is important for the mechanism that all Reads are defined after a Write, I replaced the somewhat ugly pattern of builder.ReadTexture(builder.WriteTexture(...)) with a compact builder.ReadWriteTexture(...)

What did I test: the repro case and all the DXR tests (most of the ReadWrite code was in DXR)

RenderGraphDebugParams m_RenderGraphDebug;
RenderGraphLogger m_Logger;
int m_CurrentFrameIndex;
TexturePool m_TexturePool = new TexturePool();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes visual studio, gonna revert this spacing change

@RemyUnity
Copy link
Contributor

I don't really know what those changes might do or affect, do you have any hint about what additional tests of check QA should do ?

@@ -798,7 +799,10 @@ void RenderDBuffer(RenderGraph renderGraph, HDCamera hdCamera, TextureHandle dec
passData.parameters = PrepareRenderDBufferParameters(hdCamera);
passData.meshDecalsRendererList = builder.UseRendererList(renderGraph.CreateRendererList(PrepareMeshDecalsRendererList(cullingResults, hdCamera, use4RTs)));
SetupDBufferTargets(renderGraph, passData, use4RTs, ref output, builder);
passData.decalBuffer = builder.ReadTexture(decalBuffer);
if (hdCamera.frameSettings.IsEnabled(FrameSettingsField.DecalLayers))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leftovers from previous tests?
We talked about the dbuffer pass directly returning black if possible to remove this logic (and in the render pass itself)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will change that, was planning on doing a sanity check on stuff like that later, but might as well fix this one now.

Copy link
Collaborator

@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

@FrancescoC-unity
Copy link
Contributor Author

@RemyUnity not much aside the repro case I think, there is nothing user facing that won't be caught by automation I think

@anisunity
Copy link
Contributor

From what I read, it looks good to me, didn't test it locally though

Copy link
Contributor

@TomasKiniulis TomasKiniulis 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.

What I've tested:
Forward and Deferred mode
Opaque Objects enabling/disabling for Cameras, Realtime Probes, Custom Reflection
Opaque Objects enabling/disabling in Player debug mode

# Conflicts:
#	com.unity.render-pipelines.core/Runtime/RenderGraph/RenderGraphResourceRegistry.cs
#	com.unity.render-pipelines.core/Runtime/RenderGraph/RenderGraphResources.cs
#	com.unity.render-pipelines.high-definition/CHANGELOG.md
#	com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants