Skip to content

Conversation

adrien-de-tocqueville
Copy link
Contributor

@adrien-de-tocqueville adrien-de-tocqueville commented Aug 5, 2021

Purpose of this PR

This PR is updating the algorithm for specular occlusion when using bent normals + AO.
This new version reduces specular occlusion sharpness for high smoothness materials, and reduces over darkenning of surfaces when looking at grazing angles.

Also changed default value for specular occlusion for new shader graphs. Previous was "Off", now it's "From AO" to match default value of the Lit shader

Comparison of the different options:
Before this PR is "From Bent Normals", new algorithm is "From Spherical Gaussians"
Both models have high smoothness in the gif below, all lighting is from HDRI Sky
spec_occlusion2

Added a section in Shader Code of upgrade guide about function renaming.
Added screenshot in Whats new guide

specularocclusion

Testing status

https://confluence.unity3d.com/pages/viewpage.action?spaceKey=HDRP&title=Specular+Occlusion

@github-actions
Copy link

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

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:
/.yamato%252F_abv.yml%2523all_project_ci_2021.2
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
Copy link
Contributor

Note for reviever: Before merging we want to know what method make sense and then we will remove the other one. Also be sure it works with all our shader type.

@sebastienlagarde sebastienlagarde requested review from a team and sebastienlagarde and removed request for a team August 24, 2021 08:26
@adrien-de-tocqueville adrien-de-tocqueville changed the title Add two methods for ambien occlusion based on spherical gaussians Update algorithm for specular occlusion Aug 31, 2021
@adrien-de-tocqueville adrien-de-tocqueville marked this pull request as ready for review August 31, 2021 16:45
@adrien-de-tocqueville adrien-de-tocqueville marked this pull request as draft August 31, 2021 16:45
@adrien-de-tocqueville adrien-de-tocqueville marked this pull request as ready for review September 2, 2021 13:11
@adrien-de-tocqueville adrien-de-tocqueville requested a review from a team September 3, 2021 11:41
@adrien-de-tocqueville
Copy link
Contributor Author

adding QA for awareness but already tested by artists 🙂

@sebastienlagarde
Copy link
Contributor

PR tested by the artsits. No need for futher QA. Merging

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.

2 participants