Implementing a water system for HDRP #6109

anisunity · 2021-10-21T16:29:07Z

https://jira.unity3d.com/browse/HDRP-110

This PR implements an initial version of the Water System for HDRP. It still be significantly improved before
The design and scope document can be found here:
https://confluence.unity3d.com/display/HDRP/HDRP+Water+System

This system allows for the users to add ocean, rivers and pools to their scenes.

Testing status
Check the feedback document here: https://shorturl.at/efluM

github-actions · 2021-10-21T16:29:24Z

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://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, 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
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/jobDefinition/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

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.

sebastienlagarde · 2021-10-21T17:23:34Z

com.unity.render-pipelines.high-definition/Runtime/Lighting/HDRenderPipeline.ProbeHelpers.cs

are you sure we don't have all those function already available somewhere?

ok currently it was in cloud :)

Yeah i moved it from cloud specific to a more generic file

So those function should be moved into the file name: com.unity.render-pipelines.high-definition\Runtime\Lighting\SphericalHarmonics.cs . There is other function for SH there.

remi-chapelain

PR is in good shape for merging a v1 ✔️

We're still missing a few things to be able to consider it really finished like:

Documentation (will come later since it will evolve with the UX pass coming)
Metal support (has been disabled for now, but will make a fogbugz as soon as this is merged and hopefully will be fixed asap)

You can find more info in the confluence test document here.
As for the feedback document, most of the issues have been fixed and less serious issues/concerns will be fixed in the upcoming weeks.

anisunity · 2021-11-24T08:04:21Z

Also this needs screenshot updates on the other platforms, but I am going to wait for the branching to do it. @sebastienlagarde

anisunity · 2021-11-26T10:34:01Z

Fixed some issue with the normal management that improves significantly the rendering at high wind speeds

sebastienlagarde · 2021-11-26T12:23:20Z

....render-pipelines.high-definition/Editor/Material/ShaderGraph/Templates/Vertex.template.hlsl

    $VertexDescription.Position: input.positionOS = vertexDescription.Position;
    $VertexDescription.Normal:   input.normalOS = vertexDescription.Normal;
    $VertexDescription.Tangent:  input.tangentOS.xyz = vertexDescription.Tangent;
+    $VertexDescription.uv0:      input.uv0 = vertexDescription.uv0;


note for me

sebastienlagarde · 2021-11-26T12:29:05Z

...nity.render-pipelines.high-definition/Editor/Material/Water/ShaderGraph/Vertex.template.hlsl

@@ -0,0 +1,70 @@
+VertexDescriptionInputs AttributesMeshToVertexDescriptionInputs(AttributesMesh input)


note for me

sebastienlagarde · 2021-11-26T12:54:15Z

com.unity.render-pipelines.high-definition/Runtime/Material/Water/Water.hlsl

+
+    // This intends to simulate indirect specular "multi bounce"
+    float3 attenuation = 1.0f;
+    if (R.y < 0.0)


maybe add a comment that it assume Y is up? and assume water is flat

sebastienlagarde · 2021-11-26T12:55:47Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs

+            // Update the water surfaces
+            var commandBuffer = CommandBufferPool.Get("");
+            UpdateWaterSurfaces(commandBuffer);
+            renderContext.ExecuteCommandBuffer(commandBuffer);


Pure sanity check, @JulienIgnace-Unity any thought about calling update of water surface with command buffer like this?

sebastienlagarde · 2021-11-26T12:56:52Z

....render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipelineRuntimeResources.cs

+            // Water textures
+            [Reload("Runtime/RenderPipelineResources/Texture/Water/FoamSurface.png")]
+            public Texture2D foamSurface;
+            [Reload("Runtime/RenderPipelineResources/Texture/Water/FoamNormals.tga")]


Any reasons this isn't a png here?

sebastienlagarde · 2021-11-26T12:59:12Z

com.unity.render-pipelines.high-definition/Runtime/Water/HDRenderPipeline.WaterSystem.cs

+            numBands = nbBands;
+
+            // Allocate the buffers
+            phillipsSpectrumBuffer = RTHandles.Alloc(simulationResolution, simulationResolution, numBands, dimension: TextureDimension.Tex2DArray, colorFormat: GraphicsFormat.R16G16_SFloat, enableRandomWrite: true, wrapMode: TextureWrapMode.Repeat);


sanity check, is this always allocated even if water is disabled?

Only allocated if the system is on

sebastienlagarde · 2021-11-26T13:02:36Z

com.unity.render-pipelines.high-definition/Runtime/Water/HDRenderPipeline.WaterSystem.cs

+
+                using (var builder = renderGraph.AddRenderPass<WaterRenderingData>("Render Water Surfaces", out var passData, ProfilingSampler.Get(HDProfileId.WaterSurfaceRendering)))
+                {
+                    builder.EnableAsyncCompute(false);


? is it for future testing?

sebastienlagarde · 2021-11-26T13:03:49Z

com.unity.render-pipelines.high-definition/Runtime/Water/ShaderPassWaterForward.hlsl

+    UNITY_TRANSFER_INSTANCE_ID(input, output);
+
+    // Apply the mesh modifications that come from the shader graph
+    float3 positionOS;


note for me

Note for self: evaluate the need of duplicated the shaderpasswaterforward.hlsl instead of reusing regular file

sebastienlagarde · 2021-11-26T13:05:48Z

com.unity.render-pipelines.high-definition/Runtime/Water/ShaderPassWaterForward.hlsl

+    GetSurfaceAndBuiltinData(input, V, posInput, surfaceData, builtinData);
+
+    // Light layers need to be set manually here as there is no mesh renderer
+    builtinData.renderingLayers = DEFAULT_LIGHT_LAYERS;


Ah good point, light layers will need to works with water, so we will need to add them on the water volume control or the equivalent mesh

anisunity · 2021-11-26T14:03:43Z

Launching the tests, yay!

anisunity · 2021-11-27T22:13:03Z

Did the fixed asked by @sebastienlagarde, rebased master and relaunching the tests

anisunity · 2021-11-27T22:14:41Z

relaunched tests

anisunity · 2021-11-28T14:59:18Z

Fixed memory leak and relaunching tests

mariandev · 2021-11-29T11:32:41Z

com.unity.render-pipelines.high-definition/Runtime/Water/WaterSurface.cs

+
+            // Make sure to release the resources if they have been created (before HDRP destroys them)
+            if (simulation != null && simulation.AllocatedTextures())
+                simulation.ReleaseSmmulationResources();


I think this is a typo, it should be ReleaseSimulationResources

Note: Sorry if I shouldn't comment on the PR, I saw the comment you left a while back about this but I still thought I should point this out. Have a great day.

sebastienlagarde · 2021-11-30T12:15:27Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs

+            {
+                // Update the water surfaces
+                var commandBuffer = CommandBufferPool.Get("");
+                UpdateWaterSurfaces(commandBuffer);


note for me. Maybe run async in the future

anisunity added the HDRP label Oct 21, 2021

anisunity requested review from SeanPuller, jpg-unity, pierre-unity, remi-chapelain and sebastienlagarde October 21, 2021 16:29

anisunity self-assigned this Oct 21, 2021

github-actions bot added Shader Graph SRP labels Oct 21, 2021

anisunity force-pushed the HDRP/water-system branch from 59449eb to c9ff624 Compare October 21, 2021 17:03

sebastienlagarde reviewed Oct 21, 2021

View reviewed changes

anisunity force-pushed the HDRP/water-system branch from c9ff624 to 69b924f Compare October 21, 2021 18:49

Unity-Technologies deleted a comment from GokhanOzcan Oct 22, 2021

remi-chapelain requested a review from a team October 25, 2021 11:53

anisunity force-pushed the HDRP/water-system branch from 4fc8bb5 to 27fbaa2 Compare November 2, 2021 16:43

anisunity force-pushed the HDRP/water-system branch 3 times, most recently from 63d4717 to a7c43ac Compare November 10, 2021 09:21

anisunity changed the title ~~[WIP] Implementing a water system for HDRP~~ Implementing a water system for HDRP Nov 22, 2021

anisunity force-pushed the HDRP/water-system branch from 2caf220 to 7e3d2d6 Compare November 22, 2021 18:12

anisunity marked this pull request as ready for review November 23, 2021 09:15

anisunity requested a review from a team as a code owner November 23, 2021 09:15

github-actions bot added the testing label Nov 23, 2021

remi-chapelain approved these changes Nov 23, 2021

View reviewed changes

anisunity requested a review from sebastienlagarde November 26, 2021 10:35

sebastienlagarde reviewed Nov 26, 2021

View reviewed changes

anisunity force-pushed the HDRP/water-system branch from 81b8b5d to 8cde6b8 Compare November 27, 2021 22:12

anisunity force-pushed the HDRP/water-system branch from 86f8b70 to 9cbf2fc Compare November 28, 2021 14:57

anisunity added 5 commits November 28, 2021 22:21

- Implemented an initial version of the HDRP water system.

338e5bf

Fix memory leak command buffer

7e13205

Skip the memory checks as we cannot do much about it in this version

0cb1ca0

add test meta files

1bf608e

Avoid runtime memory allocations due to findallobjectsoftype

5874dc4

anisunity force-pushed the HDRP/water-system branch from 91b5f7a to 5874dc4 Compare November 28, 2021 21:30

anisunity added 2 commits November 29, 2021 01:05

small behaviour fix

da3d751

Update some of the screenshots

683315b

mariandev reviewed Nov 29, 2021

View reviewed changes

anisunity added 3 commits November 29, 2021 16:09

Trying things to get ride of the yamato instabilities

f1adac8

Adjust the test scene

a2a0caf

Remove shader graph modifications

cc0bf55

anisunity removed the request for review from a team November 30, 2021 11:46

sebastienlagarde reviewed Nov 30, 2021

View reviewed changes

sebastienlagarde merged commit 3f5f1cd into master Nov 30, 2021

sebastienlagarde deleted the HDRP/water-system branch November 30, 2021 12:17

ElmarJ mentioned this pull request Nov 9, 2022

Switch to the build-in water shader ElmarJ/waterlooplein-timemachine#134

Closed

		@@ -0,0 +1,70 @@
		VertexDescriptionInputs AttributesMeshToVertexDescriptionInputs(AttributesMesh input)

Implementing a water system for HDRP #6109

Implementing a water system for HDRP #6109

Uh oh!

Conversation

anisunity commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

remi-chapelain left a comment • 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!

anisunity commented Nov 24, 2021

Uh oh!

anisunity commented Nov 26, 2021

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!

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!

Choose a reason for hiding this comment

Uh oh!

anisunity commented Nov 26, 2021

Uh oh!

anisunity commented Nov 27, 2021

Uh oh!

anisunity commented Nov 27, 2021

Uh oh!

anisunity commented Nov 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anisunity Nov 30, 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anisunity commented Oct 21, 2021 •

edited

Loading

github-actions bot commented Oct 21, 2021 •

edited

Loading

remi-chapelain left a comment •

edited

Loading

anisunity Nov 30, 2021 •

edited

Loading