Skip to content

Comments

Added shadowmask and shadowmask distance support#1594

Merged
phi-lira merged 81 commits intomasterfrom
universal/shadow-mask-support
Sep 23, 2020
Merged

Added shadowmask and shadowmask distance support#1594
phi-lira merged 81 commits intomasterfrom
universal/shadow-mask-support

Conversation

@lukaschod
Copy link
Contributor

@lukaschod lukaschod commented Aug 13, 2020

Checklist for PR maker

  • Have you added a backport label (if needed)? For example, the need-backport-* label. After you backport the PR, the label changes to backported-*.
  • Have you updated the changelog? Each package has a CHANGELOG.md file.
  • Have you updated or added the documentation for your PR? When you add a new feature, change a property name, or change the behavior of a feature, it's best practice to include related documentation changes in the same PR. If you do add documentation, make sure to add the relevant Graphics Docs team member as a reviewer of the PR. If you are not sure which person to add, see the Docs team contacts sheet.
  • Have you added a graphic test for your PR (if needed)? When you add a new feature, or discover a bug that tests don't cover, please add a graphic test.

Purpose of this PR

  • Added shadowmask and shadowmask distance support.
  • Renaming _MIXED_LIGHTING_SUBTRACTIVE into LIGHTMAP_SHADOW_MIXING. This gives us few advantages: consistency with builtin/hdrp, re-using existing builtin keyword, keyword stripping is automatically handled by core, we use keyword of core functionality.
  • Refactoring lightOcclusionChannel to work as lightShadowMaskSelector (Where we used mask). Sadly I can not renamed property as it would break public API.
  • Refactoring unity_ProbesOcclusion to be applied in MixRealtimeAndBakedShadows instead of light.distanceAttenuation. This way keeping similar functionality to builtin. Now light probes behaves more closely what we have in builtin.
  • Changing shadow fade to be calcualte inside MixRealtimeAndBakedShadows instead of in Light gathering. As this way required by realtime and baked shadow mixing.

For more information please check the card https://jira.unity3d.com/browse/URP-36!


Testing status

Manual Tests: What did you do?

  • Opened test project + Run graphic tests locally
  • Built a player
  • Checked new UI names with UX convention
  • Tested UI multi-edition + Undo/Redo + Prefab overrides + Alignment in Preset
  • C# and shader warnings (supress shader cache to see them)
  • Checked new resources path for the reloader (in developer mode, you have a button at end of resources that check the paths)
  • Other:

Comparing URP and Builtin mixed lighting. Simple scene with static walls, dynamic cube and lightprobes group.:

Builtin Mixed Lighting Subtractive + Probes

  • Static mesh keywords LIGHTMAP_ON LIGHTMAP_SHADOW_MIXING.
  • Static mesh effected by LIGHMAP + SCREENSPACE_SHADOWS.
  • Dynamic mesh keywords LIGHTMAP_SHADOW_MIXING.
  • Dynamic mesh effected by PROBES + SCREENSPACE_SHADOWS.
    image
    URP Mixed Lighting Subtractive + Probes
    image

Builtin Mixed Lighting ShadowMask + Probes

  • Static mesh keywords LIGHTMAP_ON LIGHTMAP_SHADOW_MIXING SHADOWS_SHADOWMASK.
  • Static mesh effected by SHADOWMASK + SCREENSPACE_SHADOWS.
  • Dynamic mesh keywords LIGHTMAP_SHADOW_MIXING.
  • Dynamic mesh effected by PROBES + SCREENSPACE_SHADOWS.
    image
    URP Mixed Lighting ShadowMask + Probes
    image

Builtin Mixed Lighting ShadowMask + Distance + Probes

  • Static mesh keywords LIGHTMAP_ON SHADOWS_SHADOWMASK.
  • Static mesh effected by SHADOWMASK + SCREENSPACE_SHADOWS.
  • Dynamic mesh keywords None.
  • Dynamic mesh effected by PROBES + SCREENSPACE_SHADOWS. (Probes turn only when realtime is out of distance)
    image
    URP Mixed Lighting ShadowMask + Probes (Has harder cut-off, but it is not part of shadowmask change. However it is something we should look into.)
    image

Automated Tests:
049_Lighting_Mixed_Subtractive
image

043_Lighting_Mixed_ShadowMask
image

134_Lighting_Mixed_ShadowMask_Distance
image

Yamato:
https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics/tree/universal%252Fshadow-mask-support

Any test projects to go with this to help reviewers?


For QA

  • Check if mixed lighting subtractive did not regressed in any way.
  • Check if mised lighting shadowmask and shadowmask distance works similar to builtin.
  • Make sure keywords are stripped if mixed lighting is not used (Either disabled in pipeline asset or not enabled in lighting settings or not baked).

sienaiwun and others added 30 commits April 3, 2020 11:49
* shadowmask support

* CHANGELOG.MD file

* add images

* Fix additional light shadowmask

* Updated CHANGELOG.md

* Shadowmask supports Vertex-Lighting

Co-authored-by: Sharlene Tan <43795986+sharlenet@users.noreply.github.com>
…ighting. Also none of the pipelines do it any ways
…ghtShadow from MixRealtimeAndBakedGI (Now only used by subtractive lighting), this way having realtime and baked shadow calculation in one place with fade. Adding new overloads of GetMainLight and GetAdditionalLight that will contain realtime and baked shadow logic.
Copy link

@w1shmast3r w1shmast3r left a comment

Choose a reason for hiding this comment

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

QA pass has revealed the following issue: No baked shadowmask is rendered after switching the shadowmask mode to Distance shadowmask, rebaking and setting shadow distance to 0.
For more details regarding the repro (project, steps, repro video, and images) please refer to the following link.
This bug needs to be fixed prior to merging this PR.

@lukaschod
Copy link
Contributor Author

QA pass has revealed the following issue: No baked shadowmask is rendered after switching the shadowmask mode to Distance shadowmask, rebaking and setting shadow distance to 0.
For more details regarding the repro (project, steps, repro video, and images) please refer to the following link.
This bug needs to be fixed prior to merging this PR.

Fixed in latest commit

@w1shmast3r w1shmast3r requested review from a team and w1shmast3r September 11, 2020 10:53
@w1shmast3r
Copy link

Added @Unity-Technologies/mobile-qa for test pass on mobiles and Linux. More information regarding platform testing could be found here: https://docs.google.com/spreadsheets/d/1OAmFn25Y4OyhQqGeZF-ZSPQPzhfWK_ZXw9-6MmgwtU8/edit#gid=868713657

@ghost ghost self-requested a review September 16, 2020 15:35
@ghost
Copy link

ghost commented Sep 16, 2020

Automated test 049_Lighting_Mixed_Subtractive seems to fail right out of the gate when run manually on this branch... any idea why?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Switched the PBR materials in the automated tests to shadergraph materials and ensured that the results were still identical visually after baking.

@vaidasma vaidasma requested review from vaidasma and removed request for a team September 21, 2020 09:07
@phi-lira
Copy link
Contributor

What's status on this PR?

@vaidasma
Copy link

vaidasma commented Sep 22, 2020

finishing manual shadowmask testing today

Copy link

@w1shmast3r w1shmast3r left a comment

Choose a reason for hiding this comment

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

Still waiting for the clarification for this recently found bug: https://confluence.unity3d.com/display/teamgi/%5BURP%5D+-+Shadowmask+Support#id-[URP]-ShadowmaskSupport-bug7 other than that the Lighting QA test pass is done.

Copy link

@w1shmast3r w1shmast3r left a comment

Choose a reason for hiding this comment

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

Lighting QA test pass has been completed on Windows and macOS (both using Editor and Standalone Player.
Link to the feature test doc: https://confluence.unity3d.com/display/teamgi/%5BURP%5D+-+Shadowmask+Support

Tested with using Unity Version 2020.2.0b3 (ef376aff2859)
Branch: trunk

Tested on the following projects:

  • URP Sample scene
  • CornellBox URP;
  • Sponza URP.

Testing included:

  • Checking that the shadowmask gets generated for all aforementioned projects;
  • Testing both Shadowmask and Distance Shadowmask modes and ensuring that the behaviour is consistent with the built-in RP;
  • Testing some complicated scenarios with baked shadowmask (overlapping lights, toggling shadow distance, switching between lighting modes at runtime, etc)
  • Integration with different lightmap backends (CPU/GPU PLM, Enlighten), different shaders and geometry (terrains).

Test outcomes:

  • Several UI and functional bugs found, addressed and verified;
  • 1 known UI issue: No Light Overlap mode scene view visualization filter in URP. Discussed that with Lukas and decided that due to the complexity of the potential fix, we need to fix this bug separately.
  • Several unrelated issues related to URP integration with certain shaders and mixed lighting modes found. All of them have been logged to FogBugz;
  • No other new regressions or ship-stoppers introduced by this PR have been found.

Approving as or revision 3f7f2fc. Great job!

Copy link

@vaidasma vaidasma left a comment

Choose a reason for hiding this comment

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

Feature looks good on Android and iOS, no artifacts or unexpected behaviours detected. Testing document: https://docs.google.com/document/d/1pshQpUoKHpe7bMmiWVCYzPrUGt44o6whiFMycNDWHIY/edit?usp=sharing

Additional project were not covered in order not to bottleneck the feature time-wise. If additional coverage needed, please contact me.

@lukaschod lukaschod added the ready-to-merge Add this tag whenever your PR is ready to be merged. i.e, non draft, all reviewers approved, ABV label Sep 23, 2020
@phi-lira
Copy link
Contributor

Cancelled yamato. Previous commit was green and new one just fixed a conflict in editor build settings.

@phi-lira phi-lira merged commit cbb7355 into master Sep 23, 2020
@phi-lira phi-lira deleted the universal/shadow-mask-support branch September 23, 2020 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Add this tag whenever your PR is ready to be merged. i.e, non draft, all reviewers approved, ABV testing universal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants