Skip to content

Conversation

anisunity
Copy link
Contributor

@anisunity anisunity commented Aug 9, 2021

This PR adds the support of the volumetric clouds in the baking of the static sky, dynamic sky and ambient probe. It also generated a second probe without the clouds that is used to light the clouds themselfs as an environement lighting.
The clouds can be evaluated either at full resolution or quarter resolution and that can be controlled through the frame settings of the camera.

Testing status

  • Added new tests to cover the new functionnality.
  • Updated the screenshots with the new changed.
  • Made sure that the resolution of the dynamic sky correctly appears based on the frame setting.
  • Made sure that the tint of the ambient probe is now correct on top and at the bottom of the clouds

@anisunity anisunity added the HDRP label Aug 9, 2021
@anisunity anisunity requested review from a team, JulienIgnace-Unity and iM0ve August 9, 2021 11:06
@anisunity anisunity self-assigned this Aug 9, 2021
@github-actions
Copy link

github-actions bot commented Aug 9, 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.

@RemyUnity
Copy link
Contributor

RemyUnity commented Aug 9, 2021

How/What was tested ?
Isn't it also worth adding a graphic test ?

@anisunity
Copy link
Contributor Author

@RemyUnity it's a draft, so the PR is not ready for QA review

@anisunity
Copy link
Contributor Author

image
Now the interpolation between top and bottom of the sky works just fine

@anisunity
Copy link
Contributor Author

image
The static and ambient probes work just fine

@anisunity anisunity force-pushed the HDRP/volumetric-clouds-sky branch 2 times, most recently from ab52a5d to ce4b9db Compare August 11, 2021 15:29
@anisunity anisunity marked this pull request as ready for review August 12, 2021 11:47

static void TraceVolumetricClouds_Accumulation(CommandBuffer cmd, VolumetricCloudsParameters_Accumulation parameters,
RTHandle colorBuffer, RTHandle depthPyramid, TextureHandle motionVectors, TextureHandle volumetricLightingTexture, TextureHandle scatteringFallbackTexture,
RTHandle colorBuffer, RTHandle depthPyramid, RTHandle motionVectors, RTHandle volumetricLightingTexture, RTHandle scatteringFallbackTexture,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change to RTHandle here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was a mistake, should have been rthandle all long


static void TraceVolumetricClouds_FullResolution(CommandBuffer cmd, VolumetricCloudsParameters_FullResolution parameters,
RTHandle colorBuffer, RTHandle depthPyramid, TextureHandle volumetricLightingTexture, TextureHandle scatteringFallbackTexture,
RTHandle colorBuffer, RTHandle depthPyramid, RTHandle volumetricLightingTexture, RTHandle scatteringFallbackTexture,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above.

{
if (m_Asset.currentPlatformRenderPipelineSettings.supportVolumetricClouds)
{
int skyResolution = (int)m_Asset.currentPlatformRenderPipelineSettings.lightLoopSettings.skyReflectionSize;
Copy link
Contributor

@JulienIgnace-Unity JulienIgnace-Unity Aug 12, 2021

Choose a reason for hiding this comment

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

These buffers are only used to render the sky in order to compute ambient probe for the clouds right?
If that's the case, I think we should not use skyReflectionSize here because it can be huge (2k or 4k) and it would be very wasteful, even when this goes through render graph.
There are plans to add a new resolution setting for ambient only when possible but in the meantime, can we use a much smaller hardcoded fixed size here?

Copy link
Contributor

@RemyUnity RemyUnity left a comment

Choose a reason for hiding this comment

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

I did a test scene to check if the clouds were visible in the ambiant probe, as well as in the sky reflections of the different rendering objects (camera/static&dynamic reflection probes/plana probes), and I all these I checked if the full res option was working.

@iM0ve iM0ve removed their request for review August 16, 2021 10:44
@anisunity anisunity force-pushed the HDRP/volumetric-clouds-sky branch from def9897 to 49baaf3 Compare August 19, 2021 00:08
@anisunity
Copy link
Contributor Author

Managed to fix the metal issue, however an other PR needs to be merged first for us to generate the new screenshots.

@anisunity anisunity force-pushed the HDRP/volumetric-clouds-sky branch from f8644ff to c77915f Compare August 19, 2021 08:38
@anisunity anisunity force-pushed the HDRP/volumetric-clouds-sky branch from bb3edd8 to 0c55536 Compare August 19, 2021 11:52
@anisunity
Copy link
Contributor Author

Updated all the screenshots on all 5 platforms (OSX, Linux, WinDx11, WinDx12, WinVulkan) Standalone and playmode

@JulienIgnace-Unity JulienIgnace-Unity merged commit 0c02b74 into master Aug 19, 2021
@JulienIgnace-Unity JulienIgnace-Unity deleted the HDRP/volumetric-clouds-sky branch August 19, 2021 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants