Skip to content

Conversation

anisunity
Copy link
Contributor

@anisunity anisunity commented Aug 5, 2021

https://fogbugz.unity3d.com/f/cases/1350590/
Added a control for the fallback for the last bounce (rtgi/rtr/rr) to keep a previously existing side effect on user demand (case 1350590).

  • Fixed an inconsistance between perf mode and quality mode for sky lighting (case 1350590).
  • Fixed an inconsistance between perf quand quality mode for material simplification in rtgi (case 1350590).
    This PR adds a lot of tests to significantly increase the coverage.
    image
    image
    image

Testing status
Tests have been updated and are all green locally.

…to keep a previously existing side effect on user demand (case 1350590).

- Fixed an inconsistance between perf mode and quality mode for sky lighting (case 1350590).
- Fixed an inconsistance between perf quand quality mode for material simplification in rtgi (case 1350590).
@anisunity anisunity requested review from a team, iM0ve and sebastienlagarde August 5, 2021 10:05
@anisunity anisunity self-assigned this Aug 5, 2021
@github-actions
Copy link

github-actions bot commented Aug 5, 2021

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

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.

@anisunity anisunity marked this pull request as ready for review August 5, 2021 10:11
Copy link
Contributor

@iM0ve iM0ve left a comment

Choose a reason for hiding this comment

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

Whats tested:

  • Used the new parameters to see if they function as expected
  • Upgraded 2 projects to see that the result with default values is unchanged (except bug fix)
  • Verified that SSGI, SSR and Recursive rendering is still working properly

Minor concerns:

Demo:

bPJYHOCYwB.mp4

@anisunity anisunity requested a review from iM0ve August 9, 2021 07:46
Copy link
Contributor

@iM0ve iM0ve left a comment

Choose a reason for hiding this comment

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

Verified the latest fixes and quickly tested properties still work

Copy link
Contributor

@Vic-Cooper Vic-Cooper left a comment

Choose a reason for hiding this comment

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

Docs reviewed, please check for accuracy.

@@ -81,6 +81,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Added warning for when a light is not fitting in the cached shadow atlas and added option to set maximum resolution that would fit.
- Added a custom post process injection point AfterPostProcessBlurs executing after depth of field and motion blur.
- Added the support of volumetric clouds for baked and realtime reflection probes.
- Added a control for the fallback for the last bounce (rtgi/rtr/rr) to keep a previously existing side effect on user demand (case 1350590).
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Added a property to control the fallback of the last bounce of a RTGI, RTR, RR ray to keep a previously existing side effect on user demand (case 1350590).

@@ -355,6 +356,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Fixed the camera near plane not being taken into account when rendering the clouds (case 1353548).
- Fixed controls for clouds fade in (case 1353548).
- Reduced the number shader variants for the volumetric clouds.
- Fixed an inconsistance between perf mode and quality mode for sky lighting (case 1350590).
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: inconsistance > inconsistency :D

Fixed an inconsistency between perf mode and quality mode for sky lighting (case 1350590).

@@ -355,6 +356,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Fixed the camera near plane not being taken into account when rendering the clouds (case 1353548).
- Fixed controls for clouds fade in (case 1353548).
- Reduced the number shader variants for the volumetric clouds.
- Fixed an inconsistance between perf mode and quality mode for sky lighting (case 1350590).
- Fixed an inconsistance between perf quand quality mode for material simplification in rtgi (case 1350590).
Copy link
Contributor

Choose a reason for hiding this comment

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

Typos, also caps acronyms:

  • Fixed an inconsistency between perf mode and quality mode for material simplification in RTGI (case 1350590).

@@ -46,13 +46,16 @@ HDRP uses the [Volume](Volumes.md) framework to calculate SSGI, so to enable and
| - **Denoiser Radius** | Set the radius of the spatio-temporal filter. |
| - **Second Denoiser Pass** | Enable this feature to process a second denoiser pass. This helps to remove noise from the effect. |
| **Depth Tolerance** | Use the slider to control the tolerance when comparing the depth of the GameObjects on screen and the depth buffer. Because the SSR algorithm can not distinguish thin GameObjects from thick ones, this property helps trace rays behind GameObjects. The algorithm applies this property to every GameObject uniformly. |
| **Ray Miss** | Defines if HDRP should use the reflection probes, the sky, both or nothing as a fall-back for screen space global illumination when a ray doesn't find an intersection. |
Copy link
Contributor

@Vic-Cooper Vic-Cooper Aug 10, 2021

Choose a reason for hiding this comment

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

Determines what HDRP does when a screen space global illumination (SSGI) ray doesn't find an intersection. Choose from one of the following options:
Reflection probes: HDRP uses reflection probes in your scene to calculate the missing SSGI intersection.
Sky: HDRP uses the sky defined by the current Volume settings to calculate the missing SSGI intersection.
Both: HDRP uses both reflection probes and the sky defined by the current Volume settings to calculate the missing SSGI intersection.
Nothing: HDRP does not calculate indirect lighting when SSGI doesn't find an intersection.


This property is set to Both by default.


### Ray-traced

![](Images/Override-ScreenSpaceGlobalIllumination2.png)

| Property | Description |
| ------------------------------ | ------------------------------------------------------------ |
| **Ray Miss** | Defines if HDRP should use the reflection probes, the sky, both or nothing as a fall-back for ray-traced global illumination when a ray doesn't find an intersection. |
Copy link
Contributor

@Vic-Cooper Vic-Cooper Aug 11, 2021

Choose a reason for hiding this comment

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

Determines what HDRP does when ray-traced global illumination (RTGI) doesn't find an intersection. . Choose from one of the following options:
Reflection probes: HDRP uses reflection probes in your scene to calculate the last RTGI bounce.
Sky: HDRP uses the sky defined by the current Volume settings to calculate the last SSGI bounce.
Both : HDRP uses both reflection probes and the sky defined by the current Volume settings to calculate the last SSGI bounce.
Nothing: HDRP does not calculate indirect lighting when RTGI doesn't find an intersection.

This property is set to Both by default


### Ray-traced

![](Images/Override-ScreenSpaceGlobalIllumination2.png)

| Property | Description |
| ------------------------------ | ------------------------------------------------------------ |
| **Ray Miss** | Defines if HDRP should use the reflection probes, the sky, both or nothing as a fall-back for ray-traced global illumination when a ray doesn't find an intersection. |
| **Last Bounce** | Defines if HDRP should use the reflection probes, the sky, both or nothing as a fall-back for ray-traced global illumination when lighting the last bounce. |
Copy link
Contributor

@Vic-Cooper Vic-Cooper Aug 11, 2021

Choose a reason for hiding this comment

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

Determines what HDRP does when ray-traced global illumination (RTGI) lights the last bounce. Choose from one of the following options:
Reflection probes: HDRP uses reflection probes in your scene to calculate the last RTGI bounce.
Sky:** HDRP uses the sky defined by the current Volume settings to calculate the last RTGI bounce.
Both : HDRP uses both reflection probes and the sky defined by the current Volume settings to calculate the last RTGI bounce.
Nothing: HDRP does not calculate indirect lighting when it evaluates the last bounce.

This property is set to Both by default.

@anisunity anisunity requested a review from Vic-Cooper August 11, 2021 15:52
@anisunity
Copy link
Contributor Author

Just pushed the docs updates

Copy link
Contributor

@Vic-Cooper Vic-Cooper left a comment

Choose a reason for hiding this comment

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

SSGI > RTGI typos.


### Ray-traced

![](Images/Override-ScreenSpaceGlobalIllumination2.png)

| Property | Description |
| ------------------------------ | ------------------------------------------------------------ |
| **Ray Miss** | Determines what HDRP does when ray-traced global illumination (RTGI) doesn't find an intersection. Choose from one of the following options: <br/>&#8226;**Reflection probes**: HDRP uses reflection probes in your scene to calculate the last RTGI bounce.<br/>&#8226;**Sky**: HDRP uses the sky defined by the current [Volume](Volumes.md) settings to calculate the last SSGI bounce.<br/>&#8226;**Both** : HDRP uses both reflection probes and the the sky defined by the current [Volume](Volumes.md) settings to calculate the last SSGI bounce.<br/>&#8226;**Nothing**: HDRP does not calculate indirect lighting when RTGI doesn't find an intersection.<br/><br/>This property is set to **Both** by default|
Copy link
Contributor

Choose a reason for hiding this comment

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

SSGI should be RTGI here, apologies for missing that!:

Determines what HDRP does when ray-traced global illumination (RTGI) doesn't find an intersection. Choose from one of the following options:
Reflection probes: HDRP uses reflection probes in your scene to calculate the last RTGI bounce.
Sky: HDRP uses the sky defined by the current Volume settings to calculate the last RTGI bounce.
Both : HDRP uses both reflection probes and the the sky defined by the current Volume settings to calculate the last RTGI bounce.
Nothing: HDRP does not calculate indirect lighting when RTGI doesn't find an intersection.

This property is set to Both by default

Copy link
Contributor

@Vic-Cooper Vic-Cooper left a comment

Choose a reason for hiding this comment

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

Small changes in ray-tracing-recursive-rendering.

@@ -29,7 +29,7 @@ You can also do this for Shader Graph master nodes:
1. In the Project window, double-click on the Shader to open it in Shader Graph.
2. In the **Graph Settings** tab of the **Graph Inspector**, go to **Surface Options** and enable **Recursive Rendering (Preview)**.

It is best practice to use recursive rendering in situations that require multi-bounced reflection and refraction, for example car headlights. Although recursive rendering also produces ray-traced reflections in more simple scenarios, like for a mirror or a puddle, it is best practice to use [ray-traced reflections](Ray-Traced-Reflections.md) here for performance reasons.
It is best practice to use recursive rendering in situations that require multi-bounced reflection and refraction, for example car headlights. Although recursive rendering also produces recursive rendering in more simple scenarios, like for a mirror or a puddle, it is best practice to use [ray-traced reflections](Ray-Traced-Reflections.md) here for performance reasons.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's improve the phrasing here:

It is best practice to use recursive rendering in situations that require multi-bounced reflection and refraction, for example, car headlights. You can use recursive rendering in simple scenarios, like a mirror or a puddle, but for best performance, use ray-traced reflections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure when i changed this, probably a find and replace

@@ -41,3 +41,5 @@ Since recursive rendering uses an independent render pass, HDRP cannot render an
| **Max Depth** | Controls the maximum number of times a ray can reflect or refract before it stops and returns the final color. Increasing this value increases execution time exponentially. |
| **Max Ray Length** | Controls the length of the rays that HDRP uses for ray tracing. If a ray doesn't find an intersection, then the ray returns the color of the sky. |
| **Min Smoothness** | Defines the threshold at which reflection rays are not cast if the smoothness value of the target surface is inferior to the one defined by the parameter. |
| **Ray Miss** | Determines what HDRP does when recursive rendering (RR) doesn't find an intersection. Choose from one of the following options: <br/>&#8226;**Reflection probes**: HDRP uses reflection probes in your scene to calculate the last RR bounce.<br/>&#8226;**Sky**: HDRP uses the sky defined by the current [Volume](Volumes.md) settings to calculate the last RR bounce.<br/>&#8226;**Both** : HDRP uses both reflection probes and the the sky defined by the current [Volume](Volumes.md) settings to calculate the last RR bounce.<br/>&#8226;**Nothing**: HDRP does not calculate indirect lighting when RR doesn't find an intersection.<br/><br/>This property is set to **Both** by default|
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to abbreviate recursive rendering to RR here (also fixed double "the" typo):

| Ray Miss | Determines what HDRP does when recursive rendering doesn't find an intersection. Choose from one of the following options:
Reflection probes: HDRP uses reflection probes in your scene to calculate the last recursive rendering bounce.
Sky: HDRP uses the sky defined by the current Volume settings to calculate the last recursive rendering bounce.
Both : HDRP uses both reflection probes and the sky defined by the current Volume settings to calculate the last recursive rendering bounce.
Nothing: HDRP does not calculate indirect lighting when recursive rendering doesn't find an intersection.

This property is set to Both by default|

@@ -41,3 +41,5 @@ Since recursive rendering uses an independent render pass, HDRP cannot render an
| **Max Depth** | Controls the maximum number of times a ray can reflect or refract before it stops and returns the final color. Increasing this value increases execution time exponentially. |
| **Max Ray Length** | Controls the length of the rays that HDRP uses for ray tracing. If a ray doesn't find an intersection, then the ray returns the color of the sky. |
| **Min Smoothness** | Defines the threshold at which reflection rays are not cast if the smoothness value of the target surface is inferior to the one defined by the parameter. |
| **Ray Miss** | Determines what HDRP does when recursive rendering (RR) doesn't find an intersection. Choose from one of the following options: <br/>&#8226;**Reflection probes**: HDRP uses reflection probes in your scene to calculate the last RR bounce.<br/>&#8226;**Sky**: HDRP uses the sky defined by the current [Volume](Volumes.md) settings to calculate the last RR bounce.<br/>&#8226;**Both** : HDRP uses both reflection probes and the the sky defined by the current [Volume](Volumes.md) settings to calculate the last RR bounce.<br/>&#8226;**Nothing**: HDRP does not calculate indirect lighting when RR doesn't find an intersection.<br/><br/>This property is set to **Both** by default|
| **Last Bounce** | Determines what HDRP does when recursive rendering (RR) lights the last bounce. Choose from one of the following options: <br/>&#8226;**Reflection probes**: HDRP uses reflection probes in your scene to calculate the last RR bounce.<br/>&#8226;**Sky**: HDRP uses the sky defined by the current [Volume](Volumes.md) settings to calculate the last RR bounce.<br/>&#8226;**Both**: HDRP uses both reflection probes and the sky defined by the current [Volume](Volumes.md) settings to calculate the last RR bounce.<br/>&#8226;**Nothing**: HDRP does not calculate indirect lighting when it evaluates the last bounce.<br/><br/>This property is set to **Both** by default. |
Copy link
Contributor

Choose a reason for hiding this comment

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

As above:

| Last Bounce | Determines what HDRP does when recursive rendering lights the last bounce. Choose from one of the following options:
Reflection probes: HDRP uses reflection probes in your scene to calculate the last bounce.
Sky: HDRP uses the sky defined by the current Volume settings to calculate the last recursive rendering bounce.
Both: HDRP uses both reflection probes and the sky defined by the current Volume settings to calculate the last recursive rendering bounce.
Nothing: HDRP does not calculate indirect lighting when it evaluates the last bounce.

This property is set to Both by default. |

@anisunity anisunity requested a review from Vic-Cooper August 11, 2021 16:39
Copy link
Contributor

@Vic-Cooper Vic-Cooper left a comment

Choose a reason for hiding this comment

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

Docs done, thanks!

@JulienIgnace-Unity JulienIgnace-Unity merged commit ad9a961 into master Aug 12, 2021
@JulienIgnace-Unity JulienIgnace-Unity deleted the HDRP/fix-1350590 branch August 12, 2021 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants