Skip to content

Fix stale data for main light directional cookie. #5828

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 30, 2021
Merged

Fix stale data for main light directional cookie. #5828

merged 1 commit into from
Sep 30, 2021

Conversation

eh-unity
Copy link
Contributor

@eh-unity eh-unity commented Sep 28, 2021 •
edited
Loading

Please read the Contributing guide before making a PR.

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

Fix using stale date for directional light cookies, when only additional lights have a cookie.
Fix bug case: 1354575

The main and additional lights used to have a different keyword to toggle them on/off. Currently they share a keyword to save variants. This caused the main light cookie to not turned off properly if there was additional lights and use stale data.
Now we clear the stale data and also test the main light cookie is actually turned on.

Backport: 21.2

Testing status

Ran local tests.

Comments to reviewers

Notes for the reviewers you have assigned.

@eh-unity eh-unity requested a review from ellioman September 28, 2021 16:45
@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://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)

URP
/.yamato%252Fall-urp.yml%2523PR_URP_trunk
With changes to URP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

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.

@eh-unity eh-unity marked this pull request as ready for review September 28, 2021 17:56
@eh-unity eh-unity requested review from a team as code owners September 28, 2021 17:56
ellioman
ellioman reviewed Sep 29, 2021
View reviewed changes
com.unity.render-pipelines.universal/ShaderLibrary/LightCookie/LightCookie.hlsl

real3 SampleMainLightCookie(float3 samplePositionWS)
{
if(!IsMainLightCookieEnabled())
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space between if and (

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:)

ellioman
ellioman reviewed Sep 29, 2021
View reviewed changes
com.unity.render-pipelines.universal/Runtime/LightCookieManager.cs
private enum LightCookieShaderFormat
{
None = -1,

Copy link
Contributor

Choose a reason for hiding this comment

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

The extra line done on purpose or by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On purpose, since it's a bit special and enums often do not have negative values.

@eh-unity
Copy link
Contributor Author

eh-unity commented Sep 29, 2021 •
edited
Loading

Test failures don't appear to be related to this PR or light cookies. This only touches light-cookie shaders and C#.

@eh-unity eh-unity mentioned this pull request Sep 29, 2021
Merged
simon-engelbrecht-soerensen
simon-engelbrecht-soerensen approved these changes Sep 30, 2021
View reviewed changes
Copy link

Choose a reason for hiding this comment

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

Looks good.

@eh-unity eh-unity merged commit 900b4ef into master Sep 30, 2021
@eh-unity eh-unity deleted the universal/light-cookies-fix-dir-light-toggle branch September 30, 2021 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@ellioman ellioman ellioman approved these changes

@simon-engelbrecht-soerensen simon-engelbrecht-soerensen simon-engelbrecht-soerensen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backported-2021.2 universal

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@eh-unity @ellioman @simon-engelbrecht-soerensen