-
Notifications
You must be signed in to change notification settings - Fork 857
[ShaderGraph] Fullscreen ShaderGraph Feature #6110
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
Conversation
… is compatible with current RP
# Conflicts: # com.unity.shadergraph/Editor/Generation/Targets/Fullscreen/FullscreenSubTarget.cs
# Conflicts: # TestProjects/HDRP_Tests/ProjectSettings/EditorBuildSettings.asset
…llscreen-subtarget-pr # Conflicts: # TestProjects/HDRP_Tests/Assets/GraphicTests/Scenes/8x_ShaderGraph/8210_Fullscreen/8210_CustomPP2.shadergraph # com.unity.render-pipelines.high-definition/Editor/Material/Fullscreen/ShaderGraph/HDFullscreenSubtarget.cs # com.unity.render-pipelines.high-definition/Editor/Material/ShaderGraph/Nodes/HDSampleBufferNode.cs # com.unity.render-pipelines.high-definition/Editor/Material/ShaderGraph/Nodes/HDSceneDepthNode.cs # com.unity.render-pipelines.high-definition/Editor/RenderPipeline/CustomPass/DrawRenderersCustomPassDrawer.cs # com.unity.render-pipelines.high-definition/Editor/RenderPipeline/CustomPass/FullScreenCustomPassDrawer.cs # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.RenderGraph.cs # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/RenderPass/CustomPass/CustomPassCommon.hlsl # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/RenderPass/CustomPass/CustomPassSampling.hlsl # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/RenderPass/DrawRenderersCustomPass.cs # com.unity.render-pipelines.universal/Editor/ShaderGraph/Nodes/UniversalSampleBufferNode.cs # com.unity.render-pipelines.universal/Runtime/RendererFeatures/DrawFullscreenPass.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I think I see what this is doing now. It's trying to set up the data injection you have in HDRP, where you store the datas at the Target level, but inject them into the SubTargets so that it the datas can be shared between multiple SubTargets. I'm wondering if there is an easier way to do this, if we're going to be making it a standard across all of the Targets..
Could we invert it, and have the SubTargets request shared data from the target? That would get rid of the reflection based data injection, and make it easier to read:
i.e. in the SubTarget, replace:
SystemData IRequiresData<SystemData>.data
{
get => m_SystemData;
set => m_SystemData = value;
}
with:
SystemData systemData => m_systemData ?? (m_systemData = target.GetSharedData<SystemData>());
@cdxntchou Initially I wanted to add the same data saving system that we have in HDRP (it's about sharing the data between different subtargets, but not between targets). Right now the data is not shared. Maybe it's something we can investigate later on since all this API is internal? |
I'm confused. As you have it written in the Builtin Target, the data is being serialized on the Target and injected into the SubTarget, so it's potentially shared between SubTargets of a Target (but not between different Targets). I was just suggesting we use a different method to implement that sharing that doesn't rely on reflection. It's true though that it is something that could be changed in a later PR though. |
Ah yes sorry, I misunderstood what you were talking about with the code snippet. I think we can definitely get rid of the reflection just by using standard generic types. I can plan this work and replace the current HDRP and Builtin data system with this in a future PR, but it would land for 2022.2. Is that okay with you? |
Yes, that would work. Let me finish looking at the rest of the PR real quick. So far the rest looks good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
} | ||
|
||
var customEditor = context.defaultShaderGUI; | ||
if (customEditor != null && m_Targets[i].WorksWithSRP(GraphicsSettings.currentRenderPipeline)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this check removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, there was a problem if this condition is true: The "CustomEditor" instruction in the shader was added twice resulting in a conflict. (see below line 233, the CustomEditor is added regardless of the condition above).
With the new code, the override custom editor is only added when customEditor != null
otherwise we use the builtin SG inspector.
internal static class BuiltInMaterialInspectorUtilities | ||
{ | ||
internal static void AddFloatProperty(this PropertyCollector collector, string referenceName, float defaultValue, HLSLDeclaration declarationType = HLSLDeclaration.DoNotDeclare) | ||
internal static void AddFloatProperty(this PropertyCollector collector, string referenceName, float defaultValue, HLSLDeclaration declarationType = HLSLDeclaration.DoNotDeclare, bool generatePropertyBlock = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the default behavior? Wouldn't it have been default false before because of default
boolean being false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because the default value of the generatePropertyBlock
boolean is true in the ShaderInput class: https://github.com/Unity-Technologies/Graphics/blob/master/com.unity.shadergraph/Editor/Data/Graphs/ShaderInput.cs#L212
Setting this to false would have broken all the other properties.
|
||
namespace UnityEditor.Rendering.Fullscreen.ShaderGraph | ||
{ | ||
internal class FullscreenData : JsonObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this class done as a JsonObject? I dont see any refs that couldn't be serialized in a normal fashion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need the JsonObject
because the m_SubTargetData
in the Target is a list of JsonData<JsonObject>
. JsonData
only accepts a JsonObject
as a template, so unless I remove completely the JsonData
and use something else, I don't see how I can get rid of JsonObject
on the FullscreenData
.
The only alternative I know regarding polymorphic serialization besides JSON is [SerializeReference] but I don't think it works in ShaderGraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add graphics tests in the regular shadergraph test project as well?-
If we are moving for deprecation of the built in renderer, can we remove the built in target from this PR? |
Replaced by #6943 |
Purpose of this PR
Adding the basis to support for Fullscreen effects and Blit: https://unity.productboard.com/portal/1-unity-platform-rendering-visual-effects/tabs/373d7e5c-077c-421a-b0e8-7c2dbadaf31f/features/4181551/portal/expanded
This PR includes:
Fixes:
Documentation for ShaderGraph Fullscreen: https://docs.google.com/document/d/1hnuSH4uOZt5GLNNr_i1ZShQOPn9FoKwu6ZU437OijZo/edit#
Documentation for CRT: https://docs.google.com/document/d/1Fh_1VgqWWBR4fRDwdzxlcezD6USJU6dOsxVWGa_4Tcs/edit#
If you have any questions regarding this feature, you can ask in #devs-fullscreen-master-node.
Note for QA:
This work is split into 3 PRs for ease of reviewing. If you want to test everything (URP, HDRP, and builtin implementation) I'd recommend to use the sg/fullscreen-target branch
Testing status
All implementations have been tested with both URP and HDRP.
Automated tests have been added for HDRP (in a separate PR).
Custom Render Texture still needs verification, as well as the Builtin custom post processes.
Test document: https://confluence.unity3d.com/plugins/servlet/samlsso?redirectTo=%2Fpages%2Fviewpage.action%3FspaceKey%3DHDRP%26title%3DFullscreen%2BTarget
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffullscreen-subtarget-pr/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk/9743653/job