Skip to content

Comments

Cloud System and Cloud Layer#2711

Merged
sebastienlagarde merged 32 commits intomasterfrom
HDRP/cloud-layer
Dec 16, 2020
Merged

Cloud System and Cloud Layer#2711
sebastienlagarde merged 32 commits intomasterfrom
HDRP/cloud-layer

Conversation

@adrien-de-tocqueville
Copy link
Contributor

@adrien-de-tocqueville adrien-de-tocqueville commented Nov 23, 2020

Purpose of this PR

This PR introduces a cloud API to HDRP that can be used to render clouds between the sky and the fog.
The clouds are baked in the sky reflection probe and are included in the static lighting skies

It uses the volume system in a similar way to the Sky System and is acessible through the Visual Environnement override
VisualEnvironment

The PR also includes a type of clouds called CloudLayer which renders a cloud texture on top of the sky with options for a simple lighting simulation and for shadow casting by the sun light.

CloudLayer_UI

Default cloud map example (with lighting option on the right)
Lighting

Shadow casting on the template
shadows-ld


Testing status

  • Launched yamato
  • Added a graphics test (5010)
  • Tested with a few different cloud textures
  • Tested static sky lighting

Comments to reviewers

  • Shadow texture is rendered as a cookie, which means rotating the sun around it's direction axis will rotate the shadow texture.
  • Also should the shadow texture be put in the cookie atlas ?
  • The lighting parameters don't seem very intuitive (step count / thickness)

…ud-layer

# Conflicts:
#	com.unity.render-pipelines.high-definition/Documentation~/Override-Visual-Environment.md
#	com.unity.render-pipelines.high-definition/Documentation~/TableOfContents.md
@adrien-de-tocqueville adrien-de-tocqueville changed the title Hdrp/cloud layer Cloud System and Cloud Layer Nov 23, 2020
@RemyUnity
Copy link
Contributor

Thanks Adrien for the great description !

Shouldn't the new "nested parameters" have their own dedicated PR ?

Also, what do you think that QA should look at for reviewing ?

@AlixMi
Copy link
Contributor

AlixMi commented Nov 24, 2020

Hello, great improvements, the lighting is promising for a non volumetric solution :)
My feedback :

  • this would be nice to have the DefaultCloudMap already populating the Cloud Map parameter
  • I think it would be better if we wouldn't have to copy the exposure value inside, when creating the clouds they appears black with a daylight scene for example before we rise the exposure. It would be nice if the clouds would already fetch the intensity of the sun so that they are bright enough and working with the overall exposure.
  • When lighting is enabled, it would be nice to be able to tint shadows
  • The blend between layers seems to be a max (that comes from me sorry) I think we should choose another type of blend so that it doesn't eat the shapes of cloud when layering them
  • Have a contrast parameter on the shadows casted on the scene
  • Have a speed parameter on the shadow casted on the scene when there is distortion enabled, because depending on the chosen tiling, it can be too fast. (Or make it work depending on the tiling so that it has a good parallel with the sky movement)
    When cast shadows is disabled in the lighting tab, the sun is no longer taken into account for the lighting of clouds (last position storred)
  • sometimes the scroll direction of the casted shadows seems to differ from what we can see around the sun

@adrien-de-tocqueville
Copy link
Contributor Author

adrien-de-tocqueville commented Nov 25, 2020

Shouldn't the new "nested parameters" have their own dedicated PR ?

Probably, moved it to this PR: #2745

Also, what do you think that QA should look at for reviewing ?

Maybe some testing on the static lighting sky or baked lighting, I am not 100% sure I tested that correclty.

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.

Review status: Done
Last updated: 2020/12/14

WHATS TESTED

  • Windows and mac
  • Usability and default values
  • Typical use cases
  • Checked that clouds are included in the bake
  • Influence on new project (HDRP defaults)
  • Tooltips
  • Automation - Adrien added graphic tests

1) FIXED As Alix mentioned, Cloud map should be already assigned as the default value, because:

  • Without it, when you add the override it does nothing.
  • Without it, if you search for cloud in the texture search, you will not find anything because it doesnt search in packages folder. So already need a lot of knowledge jus to try out the feature.

I would consider this an UX bug.

ux

2) FIXED Cloud Map has a default value but it doesnt work until the property is enabled.

a6Gxo9RJm9

3) FIXED Maybe worth specifying that the exposure is using Directional light. Not entirely clear what is it relative to

image

4) FIXED Could we make the tooltip a little more beginner-friendly, also does it simulate directional light absorption or more complex?

image (33)

5) FIXED Changing settings produce blinking when shadows are on.

7kU4EAEu5U

6) FIXED When shadow is enabled. The tiling setting becomes unhidden on Directional light, this is a bit hard to find. We currently have an info box for it. But maybe its possible to also dislplay these values on the Volume override too? Even if grayed out

image

7) FIXED There is a very noticeable seam when shadows are enabled. Is there something we can improve?

Screenshot 2020-12-03 at 15 20 39

8) FIXED With the default multiplier shadows do not match what you would expect to see from a cloudy sky. Making it unintuitive to use, we should tweak the default value or mutliplier code if we can.

shadows

9) FIXED We might want to remove the Cloud Layer component from the HDRP default settings since it no longer serves its purpose

image

@adrien-de-tocqueville
Copy link
Contributor Author

adrien-de-tocqueville commented Nov 25, 2020

1) As Alix mentioned, Cloud map should be already assigned as the default value

I agree but it's not possible to have it as a default value. I can propose a workaround:

  • Define a CloudLayer override in the HDRP default settings volume with the texture set to DefaultCloudMap
  • Copy the cloudmap in the project folder like we do for HDRI sky ?

@iM0ve iM0ve self-requested a review November 30, 2020 11:49
@iM0ve
Copy link
Contributor

iM0ve commented Dec 4, 2020

Updated the review above

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.

All issues were resolved. For test details (doc) see above comment.

In conclusion: A very nice addition. Allows you with barely any knowledge to add some nice looking clouds in the scene, but also leaves a lot of space for further customisation.

clouds

@sebastienlagarde sebastienlagarde merged commit 361ea9c into master Dec 16, 2020
@sebastienlagarde sebastienlagarde deleted the HDRP/cloud-layer branch December 16, 2020 15:36
Arrqh pushed a commit to Arrqh/Graphics that referenced this pull request Dec 21, 2020
* CloudSystem

* Nested volume components

* CloudLayer

* Test and doc

* Refactor

* Revert some modifications

* Bake resolution

* Nested volume components

* feedback

* Use directional cookie

* update doc

* Add Init callback to volume overrides

* whats new 11

* Test screenshots and bug fixes

* Reviewed the cloud layer documentation

* Update Override-Visual-Environment.md

* Doc and tooltips

* Final pass for docs

Reworded a bit, removed a dead link, and redirected a couple of links because git isn't picking up file name changes that only change the case of some characters

* Fix texture size

* fix atan2 compute

* Remove from default settings, fix shadow orientation and boost opacity

* Updated screenshots

* whats new

Co-authored-by: Lewis Jordan <lewisjordan@unity3d.com>
Co-authored-by: Lewis Jordan <lewis.jordan@hotmail.co.uk>
Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
# Conflicts:
#	com.unity.render-pipelines.high-definition/CHANGELOG.md
#	com.unity.render-pipelines.high-definition/Documentation~/whats-new-11.md
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.

7 participants