Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2021.2] Adding _SURFACE_TYPE_TRANSPARENT keyword to URP and URP ShaderGraph shaders #4025

Merged
merged 5 commits into from Apr 7, 2021

Conversation

ellioman
Copy link
Contributor

@ellioman ellioman commented Mar 29, 2021

Purpose of this PR

Currently we have a bug in SSAO where it being applied to transparent objects.
In order to fix that I've added a new keyword (_SURFACE_TYPE_TRANSPARENT) to URP shaders. This is identical to the Keyword HDRP uses. A Material Upgrader was also added to properly set the keyword on materials.

Example of the bug

Original scene

Screen Shot 2021-03-26 at 11 33 28

Same Scene with a opaque plane above it

Screen Shot 2021-03-26 at 11 33 08

Same Scene with a transparent plane (alpha = 1.0) above it

Screen Shot 2021-03-26 at 11 32 01

Testing

What has been tested

  • Tested manually in the editor (OS X).

What needs testing

  • Make sure the fix works on all platforms

Automated testing

Yamato

-> when I take the PR out of draft mode

Copy link
Contributor

@phi-lira phi-lira left a comment

Choose a reason for hiding this comment

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

LGTM. Missing changelog.

@ellioman ellioman requested a review from a team March 30, 2021 08:50
@erikabar erikabar requested review from erikabar and a team March 30, 2021 09:43
Copy link

@erikabar erikabar left a comment

Choose a reason for hiding this comment

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

the issue is fixed. verified on Windows (DX11, DX12, Vulkan, OpenGLCore)

@aleksandrasdzikia aleksandrasdzikia requested review from aleksandrasdzikia and removed request for a team March 31, 2021 09:55
Copy link
Contributor

@eh-unity eh-unity left a comment

Choose a reason for hiding this comment

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

Looks good.

if (material.HasProperty(propertyID))
{
float surfaceType = material.GetFloat(propertyID);
if (surfaceType >= 1.0f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an enum instead of 1? If it's enable/disable i.e. 1/0 case, then I'd probably test against 0.

Copy link

@aleksandrasdzikia aleksandrasdzikia left a comment

Choose a reason for hiding this comment

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

Looks good, the issue is fixed. Ran on OnePlus Nord, Galaxy Note10, Huawei P20 and p40 Lite, iPhone SE.

Copy link
Contributor

@Verasl Verasl left a comment

Choose a reason for hiding this comment

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

LGTM

@ellioman ellioman marked this pull request as ready for review April 6, 2021 08:23
@ellioman ellioman requested a review from a team as a code owner April 6, 2021 08:23
# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants