Ps5/raytracing #5262

peterjohnlong · 2021-07-29T14:44:58Z

Purpose of this PR

The purpose of this PR is to activate HDRP ray tracing functionality for PS5, by enabling compilation of associated shaders and code paths.

Testing status

Tested locally on HDRP_DXR_Tests on the console against reference images https://github.cds.internal.unity3d.com/unity/ScriptableRenderPipelinePrivate/tree/ps5-dxr/ReferenceImages/HDRP_DXR_Tests/PS5/PlayStation5/None
Tested locally on HDRP_DXR_Tests are remain green on DX12.
Currently yamato SRP PS5-DXR test is broken due to unrelated issue.

github-actions · 2021-07-29T14:45:12Z

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

Shader Graph
/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_2021.2
Depending on your PR, you may also want
/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_2021.2
/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_2021.2

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs

sebastienlagarde

outside of the quetion about SystemInfo.supportsRayTracing (which could be done in a next PR), all looks good.

Other question: With this PR, can we trigger test on scriptableRenderPipelinePrivate on PS5 for raytrace?

Also I expect it need to be backport up to 20.3 ?

anisunity

I think we need to rework a bit the wizard ps5 code as the wizard was centered around the dx12 problem. I'll take it from here @peterjohnlong if you're fine with that. The ps5 related changed are fine (except my question on the max recursion).

anisunity · 2021-07-30T02:56:25Z

com.unity.render-pipelines.high-definition/Editor/RenderPipeline/HDRenderPipelineUI.cs

                ++EditorGUI.indentLevel;
                EditorGUILayout.PropertyField(serialized.renderPipelineSettings.supportedRayTracingMode, Styles.supportedRayTracingMode);
-                if (serialized.renderPipelineSettings.supportRayTracing.boolValue && !UnityEngine.SystemInfo.supportsRayTracing)
+                if (serialized.renderPipelineSettings.supportRayTracing.boolValue && !UnityEngine.SystemInfo.supportsRayTracing


I would argue this is the best way of doing it, let's discuss

anisunity · 2021-07-30T02:58:50Z

com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs

        bool IsArchitecture64Bits()
-            => EditorUserBuildSettings.activeBuildTarget == BuildTarget.StandaloneWindows64;
+        {
+            return (EditorUserBuildSettings.activeBuildTarget == BuildTarget.StandaloneWindows64) || (EditorUserBuildSettings.activeBuildTarget == BuildTarget.PS5);


same here, not the best way imo

anisunity · 2021-07-30T03:00:41Z

...elines.high-definition/Runtime/RenderPipeline/Raytracing/Shaders/RaytracingRenderer.raytrace


 // WARNING: This define must be kept in sync with the c# code
+#if defined(SHADER_API_PSSL)
+#define RAYTRACING_MAX_RECURSION 1


why this one only? RayTracingReflection.raytracing and RaytracingIndirectDiffuse need to have their max recursion force to 1 aswell

@anisunity I am fine with you reworking the wizard code. Good point about RaytracingRenderer.raytrace - mainly it was done there as a reaction to some HDRP_DXR tests e.g. 902_Materials_SG_Variants_xxxxx where a third of the screen is testing recursive raytrace, in the case of PS5 I wanted to achieve a consistent result of just the background. I will rethink best way to do this.

@anisunity I have reverted this change to RayTracingRenderer.raytrace, and added a scripting define in ProjectSettings NO_RAY_RECURSION, and tests to set the max recursion in the shader setup .cs code for this and the other cases

peterjohnlong · 2021-07-30T10:35:45Z

@sebastienlagarde
I am looking into running test on scriptableRenderPipelinePrivate on PS5 for raytrace again, which is broken ATM.
I will need to backport up to 20.3, but only once the C++ PR for the backports 2020.3 and 2021.1 are ready otherwise it will break PS5.

anisunity · 2021-08-03T12:16:31Z

@peterjohnlong i'll do a quick pass on the pr later this week to change the wizard, etc.

RSlysz

Looks good. But please rename Bobby

RSlysz · 2021-08-12T09:02:39Z

com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs


+
+        [InitializeOnLoadMethod]
+        static void Bobby()


Please find a meaningful name :)

Renamed Bobby to InitializeEntryList

RSlysz · 2021-08-12T09:05:09Z

com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs

+
+        internal void ReBuildEntryList()
+        {
+            m_Entries = null;


Not needed. The line below already replace it. Former will be cleaned later by garbage collection.

Note that alternately if you only nullify it, the list will be rebuild by the lazy constructor anyway in entries. But in this case the name of this method is not accurate anymore.

Removed the unneeded code

RSlysz · 2021-08-12T09:11:30Z

com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs

+            }
+        }
+
+        Entry[] BuildEntryList()


[Optional] I think that you can simplify things a little by removing BuildEntryList() and only keep ReBuildEntryList().

You should be able to make it work with only

internal void ReBuildEntryList() //current BuildEntryList() in your code { //compute List<Entry> entryList as below this comment m_Entries = BuildEntryList(); }

and then entries would become

Entry[] entries { get { // due to functor, cannot static link directly in an array and need lazy init if (m_Entries == null) ReBuildEntryList(); return m_Entries; } }

Especially because when cleaning current ReBuildEntryList, it will become a one liner method.

RSlysz · 2021-08-12T16:24:25Z

Above is a fix for error logged in the console when user switch platform and Wizard window is open.

(@alex-vazquez for awareness. If this PR don't land, we would still need to do this fix. It seams unrelated to current Wizard change and could have been introduced 2 month ago in 31b7b5d)

…racing

…erial/ShaderGraph/HDTarget.cs

peterjohnlong added 21 commits March 4, 2021 15:36

enable raytracing shader compiler for platform

7bee269

enabling raytracing for platform

55023e4

add platform scripting defines for hdrp dxr tests

904bc1a

Update ProjectSettings.asset

fdb2f84

Merge branch 'master' into ps5/raytracing

ded11aa

Merge branch 'master' into ps5/raytracing

f94ddd0

Merge branch 'master' into ps5/raytracing

eda837b

Merge branch 'master' into ps5/raytracing

451e6ff

Merge branch 'master' into ps5/raytracing

765889b

Merge branch 'master' into ps5/raytracing

403ac41

RayMarching.compute add ps5 to only_renderers pragma

8138930

limit RAYTRACING_MAX_RECURSION on some platforms

41ab5c7

Merge branch 'master' into ps5/raytracing

a90ccfb

add ps5 to pragma only_renderers

aa8f9ae

Merge branch 'master' into ps5/raytracing

65ec8c9

ProjectSettings.asset set static batching off for buildtarget PS5

db044e9

Fix fix ambiguous lerp(float3,float3,float4) does not compile on PSSL

e2fb99b

Merge branch 'master' into ps5/raytracing

2218f37

Merge branch 'master' into ps5/raytracing

b721449

fix formatting

8495291

Merge branch 'master' into ps5/raytracing

9a4267b

github-actions bot added HDRP Shader Graph testing labels Jul 29, 2021

sebastienlagarde reviewed Jul 29, 2021

View reviewed changes

com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs Show resolved Hide resolved

sebastienlagarde approved these changes Jul 29, 2021

View reviewed changes

sebastienlagarde requested review from anisunity and sambickley July 29, 2021 15:41

sebastienlagarde marked this pull request as ready for review July 29, 2021 15:41

sebastienlagarde requested a review from a team as a code owner July 29, 2021 15:41

anisunity reviewed Jul 30, 2021

View reviewed changes

peterjohnlong added 3 commits August 3, 2021 12:52

reverted previous change

cab87c6

added a test on NO_RAY_RECURSION for setting maxRecursion =1

93ef327

Update HDRP_DXR ProjectSettings.asset for console

96a7cb1

Reworking a bit the wizardf for dxr

9597db1

anisunity requested a review from RSlysz August 11, 2021 21:56

anisunity added 2 commits August 12, 2021 09:16

Fixformatting and optional check

851cf0e

Fix volume ui

b856984

RSlysz requested changes Aug 12, 2021

View reviewed changes

Fix HDWizard/HDProjectSettings issue when switching platform

4e21f28

peterjohnlong added 7 commits August 13, 2021 10:04

Merge branch 'master' into ps5/raytracing

2d361b9

Merge commit 'a8abe4c5d857cbada0535ec3a5ca770265b639ed' into ps5/rayt…

e6d94ae

…racing

Apply reformat changes

41db716

Merge commit '7d292932bec3b4257a4defaf698fc7d77e2027f5' into ps5/rayt…

22efbf9

…racing

Merge branch 'master' into ps5/raytracing

fd632ca

Address PR feedback from Remi Slysz

48cb7b7

add pragma OnlyRenderers platform validation exception for Editor/Mat…

e1507d8

…erial/ShaderGraph/HDTarget.cs

sebastienlagarde requested a review from RSlysz August 25, 2021 10:22

sebastienlagarde approved these changes Aug 25, 2021

View reviewed changes

sebastienlagarde merged commit 8714705 into master Aug 25, 2021

sebastienlagarde deleted the ps5/raytracing branch August 25, 2021 11:44

peterjohnlong mentioned this pull request Sep 6, 2021

2021.1/ps5/raytracing #5533

Merged

peterjohnlong mentioned this pull request Sep 13, 2021

10.x.x/ps5/raytracing #5634

Closed

Ps5/raytracing #5262

Ps5/raytracing #5262

Uh oh!

Conversation

peterjohnlong commented Jul 29, 2021

Purpose of this PR

Testing status

Uh oh!

github-actions bot commented Jul 29, 2021

Uh oh!

Uh oh!

sebastienlagarde left a comment

Choose a reason for hiding this comment

Uh oh!

anisunity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterjohnlong commented Jul 30, 2021

Uh oh!

anisunity commented Aug 3, 2021

Uh oh!

RSlysz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RSlysz Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RSlysz commented Aug 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RSlysz Aug 12, 2021 •

edited

Loading