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

Rename Raytracing node to Raytracing quality node + update documentation #1479

Merged
merged 34 commits into from Sep 21, 2020

Conversation

sebastienlagarde
Copy link
Collaborator

@sebastienlagarde sebastienlagarde commented Aug 2, 2020

Purpose of this PR

  • Rename Raytracing node to Raytracing quality node + update documentation
  • Change low and high to Default and Raytraced as well for the inputs of this node
  • Now all raytrace effects but path tracer will use the raytracing input version while rendering from a shader graph

Testing status

Manual Tests: What did you do?

  • Opened test project + Run graphic tests locally
  • Built a player
  • Checked new UI names with UX convention
  • Tested UI multi-edition + Undo/Redo + Prefab overrides + Alignment in Preset
  • C# and shader warnings (supress shader cache to see them)
  • Checked new resources path for the reloader (in developer mode, you have a button at end of resources that check the paths)
  • Other:

Automated Tests: What did you setup? (Add a screenshot or the reference image of the test please)

Yamato: (Select your branch):
https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics

Any test projects to go with this to help reviewers?


Comments to reviewers

Notes for the reviewers you have assigned.

@github-actions
Copy link

github-actions bot commented Aug 2, 2020

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!

@sebastienlagarde
Copy link
Collaborator Author

@remi-chapelain we will need to add a test to check that raytracing quality node works correctly for the given effect (which mean we need to validate that it doesn't change for recursive rendering for example or shadow (can be done with alpha test);
But we also need to add a test that Material Quality + Raytracing Quality work correctly together.
i.e need a test where we chain them.

@@ -0,0 +1,17 @@
# Raytracing Quality Node

The Raytracing Quality Node allows you to provide a fast implementation of your Shader Graph to be use in GPU heavy raytraced effect to tradeoff accuracy for speed: [Ray-Traced Reflections](Ray-Traced-Reflections.md), [Ray-Traced-Global-Illumination](Ray-Traced-Global-Illumination.md) and [Ray-Traced-Subsurface-Scattering](Ray-Traced-Subsurface-Scattering.md).
Copy link
Contributor

@JordanL8 JordanL8 Aug 3, 2020

Choose a reason for hiding this comment

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

The Raytracing Quality Node allows you to provide a fast implementation of your Shader Graph to use in GPU-intensive ray-traced effects to tradeoff accuracy for speed. The GPU-intensive effects are:
* [Ray-Traced Reflections](Ray-Traced-Reflections.md).
* [Ray-Traced-Global-Illumination](Ray-Traced-Global-Illumination.md).
* [Ray-Traced-Subsurface-Scattering](Ray-Traced-Subsurface-Scattering.md).


## Setup

The Raytracing Quality Node is a builtin keyword and thus need to be created in the Dashboard **Keyword -> Raytracing Quality** of the shader graph then drag and dropped in the working area.
Copy link
Contributor

@JordanL8 JordanL8 Aug 3, 2020

Choose a reason for hiding this comment

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

The Raytracing Quality Node represents a built-in keyword which means, to create one:

  1. In the [Blackboard](https://docs.unity3d.com/Packages/com.unity.shadergraph@latest/index.html?subfolder=/manual/Blackboard.html), click the plus (**+**) button.
  2. Select Keyword > Raytracing Quality. This creates the keyword and makes it visible on the Blackboard.
  3. Now, to use the keyword in your graph, drag it from the Blackboard into the workspace.


| Name | Direction | Type | Description |
| :------------ | :-------- | :------------- | :----------------------------------------------------------- |
| **default** | Input | Vector4 | Sets the value to use for the normal shader graph. This is the default path use to render this shader graph. |
Copy link
Contributor

Choose a reason for hiding this comment

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

The value to use for the normal Shader Graph. This is the default path Unity uses to render this Shader Graph.

| Name | Direction | Type | Description |
| :------------ | :-------- | :------------- | :----------------------------------------------------------- |
| **default** | Input | Vector4 | Sets the value to use for the normal shader graph. This is the default path use to render this shader graph. |
| **optimized** | Input | Vector4 | Sets the value to use for the fast shader graph to use with the GPU heavy raytraced effect.|
Copy link
Contributor

@JordanL8 JordanL8 Aug 3, 2020

Choose a reason for hiding this comment

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

The value to use for the fast Shader Graph to use with the GPU-intensive ray-traced effects.

@@ -103,6 +103,9 @@ From Unity 2020.2,SHADERPASS for TransparentDepthPrepass and TransparentDepthPos
Unity 2020.2 introduces a new multi-compile for Depth Prepass and Motion vector pass to allow for support of the Decal Layers feature. These passes now require you to add #pragma multi_compile _ WRITE_DECAL_BUFFER.

From Unity 2020.2, the shader code for the Decal.shader has changed. Previously, the code used around 16 passes to handle the rendering of different decal attributes. It now only uses four passes: DBufferProjector, DecalProjectorForwardEmissive, DBufferMesh, DecalMeshForwardEmissive. Some pass names are also different. DBufferProjector and DBufferMesh now use a multi_compile DECALS_3RT DECALS_4RT to handle the differents variants and the shader stripper has been updated as well. Various Shader Decal Properties have been renamed/changed to match a new set of AffectXXX properties (_AlbedoMode, _MaskBlendMode, _MaskmapMetal, _MaskmapAO, _MaskmapSmoothness, _Emissive have been changed to _AffectAlbedo, _AffectNormal, _AffectAO, _AffectMetal, _AffectSmoothness, _AffectEmission - Keyword _ALBEDOCONTRIBUTION is now _MATERIAL_AFFECTS_ALBEDO and two new keywords, _MATERIAL_AFFECTS_NORMAL, _MATERIAL_AFFECTS_MASKMAP, have been added). These new properties now match with properties from the Decal Shader Graph which are now exposed in the Material. A Material upgrade process automatically upgrades all the Decal Materials. However, if your project includes any C# scripts that create or manipulate a Decal Material, you need to update the scripts to use the new properties and keyword; the migration does not work on procedurally generated Decal Materials.

From Unity 2020.2, Raytracing Node for Raytracing in ShaderGraph have been rename Raytracing Quality Node and the define RAYTRACING_SHADER_GRAPH_LOW and RAYTRACING_SHADER_GRAPH_HIGH respectively rename RAYTRACING_SHADER_GRAPH_DEFAULT and RAYTRACING_SHADER_GRAPH_OPTIMIZED. Unless you have use those define in custom code, there is nothing to do and shader graph will automatically regenerate the shader with correct define at load.
Copy link
Contributor

@JordanL8 JordanL8 Aug 3, 2020

Choose a reason for hiding this comment

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

From Unity 2020.2, the Raytracing keyword Node in Shader Graph has been renamed to Raytracing Quality Node and the RAYTRACING_SHADER_GRAPH_LOW and RAYTRACING_SHADER_GRAPH_HIGH defines are now RAYTRACING_SHADER_GRAPH_DEFAULT and RAYTRACING_SHADER_GRAPH_OPTIMIZED respectively. Unless you used these defines in custom Shader code, you do not need to do anything because Shader Graph automatically regenerates its Shaders with the correct defines when you load the Project.

Copy link
Contributor

@JordanL8 JordanL8 left a comment

Choose a reason for hiding this comment

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

Oops, meant to request changes. Just left a few comments.

@alindmanUnity alindmanUnity self-requested a review August 5, 2020 15:24
@alindmanUnity
Copy link
Collaborator

i'm going to take an depth look at this today -- there's some inconsistencies with docs here that can create confusion when cross referencing with shader graph docs, and I want to try to identify the issues that seb had brought up on slack yesterday

Copy link
Collaborator

@alindmanUnity alindmanUnity left a comment

Choose a reason for hiding this comment

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

please ping for review of these docs changes, but it's important to be clear on the distinction between available keywords and nodes.

JordanL8 and others added 6 commits August 11, 2020 09:32
…ability-Manual.md

Co-authored-by: Alex Lindman <39529353+alindmanUnity@users.noreply.github.com>
…ability-Manual.md

Co-authored-by: Alex Lindman <39529353+alindmanUnity@users.noreply.github.com>
…de-Raytracing-Quality.md

Co-authored-by: Alex Lindman <39529353+alindmanUnity@users.noreply.github.com>
Co-authored-by: Alex Lindman <39529353+alindmanUnity@users.noreply.github.com>
…ading-from-2020.1-to-2020.2.md

Co-authored-by: Alex Lindman <39529353+alindmanUnity@users.noreply.github.com>
…eOfContents.md

Co-authored-by: Alex Lindman <39529353+alindmanUnity@users.noreply.github.com>
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.

Doc and new naming is good for me.
But, this PR make the test 1000_RaytracingQualityKeyword_Indirect fail. (displays only the optimized version)
The old raytracing keyword is not upgraded properly with the new name and only work when deleting the keyword in the list and re-adding it.

I've commit the new SG that fix this test but the issue still persists in other projects that use raytracing keyword and want to update

@anisunity
Copy link
Contributor

anisunity commented Aug 12, 2020

I was pointed to this discussion today, i wasn't aware of it. I don't think the ray tracing node should be named "optimized" vs default. Sure, optimization is the usage you would be going for if you were thinking about real-time constraint, but given that most the users that will be using ray tracing will not be in the game constraint, it should probably be named "rasterization" vs "ray tracing" or "default" vs "ray tracing". Also the ray tracing node can be useful in RTR, RTGI, RTAO and RTShadow (in case there is alpha testing that needs to be avoided due to performance reasons), Rercursive Rendering and NOT in RTSSS (The way it is implemented, the value is very low). The fact that the current shader graph code doesn't mimic this is just because during the recent upgrades of the shader graph pacakge no automatic tests were there to make sure that the behavior was not broken in the last months.

If you are using recursive rendering, you are clearly not in the real-time mindset and are aiming for high visual quality. Then it doesn't make any sense to have low quality reflections given the little that is going to save compared to the giant cost of the recursive rendering effect.

Copy link
Contributor

@anisunity anisunity left a comment

Choose a reason for hiding this comment

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

Please check the comment above

@remi-chapelain
Copy link
Contributor

Updated the tests scene to increase coverage (RTGI, RT color shadow) and check rendergraph compatible

There might be an issue when switching to Unlit SG, on the test scene 1000_RaytracingQualityKeyword_MaterialQuality_Indirect, if you change the master node to UNLIT, the "default" version take the raytraced color.
image

Also, re-launched yamato : https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/HDRP%252Frename-raytracing-node/.yamato%252Fhdrp_dxr-win-dx12.yml%2523HDRP_DXR_Win_DX12_playmode_trunk/3494979/job

@sebastienlagarde sebastienlagarde merged commit b30de00 into HDRP/staging Sep 21, 2020
@sebastienlagarde sebastienlagarde deleted the HDRP/rename-raytracing-node branch September 21, 2020 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants