Skip to content

[HDRP] Fix incorrect light list indexing when TAA is enabled - #5287

Merged
sebastienlagarde merged 3 commits into
hd/bugfixfrom
HDRP/fix_taa_lightlist
Aug 3, 2021
Merged

sebastienlagarde merged 3 commits into
hd/bugfixfrom
HDRP/fix_taa_lightlist

Conversation

@pmavridis

@pmavridis pmavridis commented Aug 3, 2021 •

Copy link
Copy Markdown
Contributor

Purpose of this PR

https://fogbugz.unity3d.com/f/cases/1352444/

When indexing into the light list, we need to take into account the jittered projection matrix when TAA is enabled.

Before this PR, some tiles in the planar reflection do not have the correct number of reflection probes and part of the reflection is missing (and flickers because on every frame the error is different based on the TAA jitter):
image

After this PR, all tiles in the planar reflection have the correct number of reflection probes and the reflection is not flickering:
image


Testing status

The ticket does not include a repro scene, so I've built a very simple scene as you see above.
After this PR, the reflection does not flicker.

Note: I have not tested XR, which might be affected by this PR.

@ghost ghost added the HDRP label Aug 3, 2021

ghost commented Aug 3, 2021

Copy link
Copy Markdown

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

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.

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see we are only changing the DeferredUseComputeAsPixel version of deferred. Isn't there similar code in other versions?

ghost commented Aug 3, 2021

Copy link
Copy Markdown

I see we are only changing the DeferredUseComputeAsPixel version of deferred. Isn't there similar code in other versions?

I think you might have been tricked by github diff viewer, the change is the prepare light function

ghost commented Aug 3, 2021

Copy link
Copy Markdown

good catch!

ghost commented Aug 3, 2021

Copy link
Copy Markdown

I think you might have been tricked by github diff viewer, the change is the prepare light function

I have been tricked by my own bad eye sight :D
All is good then!

@ghost
ghost self-requested a review August 3, 2021 15:37

ghost commented Aug 3, 2021

Copy link
Copy Markdown

For QA: merging, test should be enough

@ghost
ghost marked this pull request as ready for review August 3, 2021 21:23
@ghost
ghost merged commit b7d7011 into hd/bugfix Aug 3, 2021
@ghost
ghost deleted the HDRP/fix_taa_lightlist branch August 3, 2021 21:23
@ghost ghost mentioned this pull request Aug 5, 2021
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.

4 participants