Skip to content

Conversation

@FrancescoC-unity
Copy link
Contributor

@FrancescoC-unity FrancescoC-unity commented Oct 15, 2021

This PR implements mixed cached shadows (allow always draw dynamic) for directional lights too. Turned out to be less trivial than I originally thought :D It was a bit tricky to get the 2 set of rendered data to agree with the fact that they are drawn with different view positions. To solve this we warp the dynamic ones to be rendered in the camera space of the cached shadows.

In this Pic, the shadows are set to OnDemand, the dancing avatar and a spinning cube are dynamic so the shadow is updated. The other spinning cube is marked as static and so it doesn't update/render.

MixedDirCachedShadows.mp4

This PR fixes also an issue we had with area light and mixed cached shadows.

Also, given that it comes with an extra memory cost, it is an option in the HDRP asset.

For QA: Please give a spin with other light types for the OnDemand/OnEnable with/without Always Draw Dynamic option. Also a bit of playing with normal shadows in directional wouldn't hurt.

@github-actions
Copy link

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%23HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_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.

@github-actions github-actions bot added the SRP label Oct 15, 2021
@github-actions
Copy link

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page).
See the PR template for more information.
Thank you!

Copy link
Contributor

@sebastienlagarde sebastienlagarde left a comment

Choose a reason for hiding this comment

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

This PR fixes also an issue we had with area light and mixed cached shadows.
can you make a separte backport PR for 21.2 for this fix? thanks

@FrancescoC-unity
Copy link
Contributor Author

This PR fixes also an issue we had with area light and mixed cached shadows.
can you make a separte backport PR for 21.2 for this fix? thanks

Done! #6121

# Conflicts:
#	com.unity.render-pipelines.high-definition/CHANGELOG.md
#	com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/LightLoopDef.hlsl
@kecho kecho self-requested a review October 25, 2021 13:14
@kecho
Copy link
Contributor

kecho commented Oct 26, 2021

ping me when QA & yamato are good. We should be able to merge it.

Copy link
Contributor

@kecho kecho left a comment

Choose a reason for hiding this comment

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

Very cool! Any chance you could add a test?

@FrancescoC-unity
Copy link
Contributor Author

I can try, cannot be fully tested as it will require precise camera movements to test all aspects, but will try to add something

@FrancescoC-unity
Copy link
Contributor Author

Added and run tests, but got some seemingly unrelated fails. Re-running

Copy link
Contributor

@kecho kecho left a comment

Choose a reason for hiding this comment

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

Fantastic. Thank you so much. Checked the tests, looks rock solid. Waiting on QA now and this should be ready to merge in.

@FrancescoC-unity FrancescoC-unity merged commit c83cb31 into master Oct 29, 2021
@FrancescoC-unity FrancescoC-unity deleted the HDRP/mixed-cached-dir-shadows branch October 29, 2021 12:59
@FrancescoC-unity
Copy link
Contributor Author

@Unity-Technologies/gfx-qa-hdrp Got easy on the merge button, we had automated testing work, but still worth looking a bit and lemme know if something got bad.

@remi-chapelain
Copy link
Contributor

Tested again the feature from scratch to made sure I understand.
Seems to be working the same as with punctuals and area lights ✔️

Only thing I'd like changed is the typo on the tooltip on the HDRP Asset. Here's the proposed version:
"When enabled, HDRP allows mixed cached shadows for directional lights, it will incur additional memory costs."

@sgribushin
Copy link

Hi, this is a feature we definitely need in our game to hit our performance target. I have tried back-porting it to 2021.2.0f1 and it works but only in the scene edit view.
In playmode in both the game and scene view it renders badly (the static shadows are shifted randomly everywhere, dynamic ones draw normally) depending on which view you use, it changes. It looks like the issue is a wrong transformation matrix or something similiar since they kind of keep their shape but their position is skewed.
We are using HDRP 12.1.

Do you have an idea about where the problem could be?

@FrancescoC-unity
Copy link
Contributor Author

FrancescoC-unity commented Nov 9, 2021

Hi, this is a feature we definitely need in our game to hit our performance target. I have tried back-porting it to 2021.2.0f1 and it works but only in the scene edit view. In playmode in both the game and scene view it renders badly (the static shadows are shifted randomly everywhere, dynamic ones draw normally) depending on which view you use, it changes. It looks like the issue is a wrong transformation matrix or something similiar since they kind of keep their shape but their position is skewed. We are using HDRP 12.1.

Do you have an idea about where the problem could be?

The video captured in this PR is in playmode and I tested again locally playmode and all is fine. When it comes to cached shadows and directional lights is really important to know that the shadows are view dependent. For the cached component this is really important as when switching the view a lot the cached component is invalidated.

I assume you have scene and game view together at the same time? (This will confuse one of the two as the view)

Generally the cached component needs to be updated every time you have significant change on the main view.

If you have only one view and no other change in view, it might be that your backport was not successful. One thing that is fundamental to get things working properly is this https://github.com/Unity-Technologies/Graphics/pull/6030/files#diff-f649de66adbf0e482453589d1bb42e446bdb7df4af577f41355a71dc72caca50R333 in case you missed this step.

@sgribushin
Copy link

More info: In the build it works but only up to a certain world position, then the shadows get "noodlified" as i move and stop working as i move further, if i go back it "de-noodlifies" and works again. In the editor in playmode it is not right no matter what i do. I did check the changes you linked to (even before posting here as it was what i thought too).

Could it be that im on an older version of HDRP (2021.2 + 12.1)? This was tested on 2022 alpha right?

gmitrano-unity added a commit that referenced this pull request Nov 24, 2021
This change fixes an issue where the shadow atlas could end up with an
invalid texture handle after the render pipeline asset is modified and
reloaded.

The previous code was invalidating the output texture handle in some
particular cases, but a new handle was recently added to the atlas
code without updating the invalidation logic. This change simply adds
the new handle to the existing invalidation logic.

The new handle was added as part of #6030 and merged into master as:
c83cb31

This fix may need to be backported to other branches.
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