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

Tune TAA disocclusion scale to avoid rejecting all samples during motion. #86809

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Jan 5, 2024

Noticed while reviewing #61834

The values we use for the disocclusion scale were a bit problematic. Basically they mean the following:
disocclusion_threshold: If the difference in velocity of this pixel between this frame and last frame is less than this amount, we can happily use the reprojected color. If the difference is greater we weight this frame more heavily (basically we assume there is a good chance that this pixel is the result of disocclusion)
disocclusion_scale: This scales the disocclusion amount (which is measured in pixels (difference_in_velocity - disocclusion_threshold)) to arrive at a final "disocclusion" value.

The final disocclusion value is used to weight towards the current sample. A disocclusion value of 1 means "throw away all previous samples". 0 means, this isn't a disoccluded pixel, blend it as normal.

As a result of having such high values, we pretty much always had a disocclusion value clamped to 1 if there was any movement at all.

https://www.elopezr.com/temporal-aa-and-the-quest-for-the-holy-trail/ has a nice breakdown of this type of disocclusion factor. The author uses 0.001 for threshold and 10 for scale. But they measure disocclusion in uv-space and we measure in texel-space. Accordingly, I increased/decreased the values by a factor of 1,000 and the results look a lot better.

They are most noticable in shadows with #61834 applied.

In this screenshot the camera is moving slowly. On the left is master, on the right is this PR
Screenshot from 2024-01-04 12-16-50

I tested this PR in a couple of scenes, both with and without #61834 and cannot see any signs of new ghosting or other artifacts that can come from improper disocclusion handling.

@mrjustaguy
Copy link
Contributor

I think 2.5 pixels is a bit too much, as that'll allow pixels to bleed into neighbors up to 3 pixels away.

The optimal value is likely sqrt(2)/2 pixels as that is the distance between center of the pixel and corner of the pixel, and there's no point in going outside of the pixel's grid for reprojected samples, as they should be relevant for different pixels, but not the current one.

@clayjohn
Copy link
Member Author

clayjohn commented Jan 5, 2024

I think 2.5 pixels is a bit too much, as that'll allow pixels to bleed into neighbors up to 3 pixels away.

The optimal value is likely sqrt(2)/2 pixels as that is the distance between center of the pixel and corner of the pixel, and there's no point in going outside of the pixel's grid for reprojected samples, as they should be relevant for different pixels, but not the current one.

This isn't a pixel distance. Its a texel difference in velocities. Basically it says, if the velocity from last frame changes by less than this amount, treat it as if its the same pixel. IMO 2.5 is a really small amount and it would be safe to go up even higher

@mrjustaguy
Copy link
Contributor

ah, I misunderstood the above in that case.

@Calinou
Copy link
Member

Calinou commented Jan 10, 2024

Tested locally in https://github.com/Calinou/godot-reflection (CLI arguments: --write-movie output.avi --quit-after 600 -- --benchmark).

Video showcasing master 3524346 (left), PR rebased against master (right):

taa_comparison.mp4

TAA coverage seems a lot better with this PR, but it's blurrier and ghosting is a lot more common. Some stills showing ghosting:

On top of the staircase in the middle

taa1

Behind the ladder in the top-right corner

taa2

After fast camera movement, the PR has much better TAA coverage but it's also blurrier if you look at the sign or grass pot in the distance:

taa3 webp

@mrjustaguy
Copy link
Contributor

IIRC the adaptive box size is quite problematic, too high values and it'll ghost like crazy, too low values and it's a jittery mess.. Figuring out the proper function for calculating it would improve the ghosting seen in the stairs example, but figuring that out is a very big ask IMO

@clayjohn
Copy link
Member Author

TAA coverage seems a lot better with this PR, but it's blurrier and ghosting is a lot more common. Some stills showing ghosting:

As mrjustaguy says solving this problem will be pretty challenging. The reason we didn't have any ghosting before is because we had no reprojection at all. Same thing with the perceived blurriness in motion, there was no blurriness before because there was no TAA

@Ansraer
Copy link
Contributor

Ansraer commented Jan 12, 2024

Hmm, looking at the blurrier textures seen in Calinou's last screenshot, I am wondering if this is caused by the camera jitter (which we need for TAA to work).

If so, we might be able to alleviate the problem by adjusting the texture coordinates in the fragment shader. The idea would be to move it in the opposite direction of the camera jitter to ensure that an object's textures are stable across several frames, which would reduce the blurring when TAA looks at past frames. I think I read about this technique somewhere a while ago, but can't remember where it was right now.

EDIT: I spent some more time staring at the screenshots, and I am getting more and more convinced that this could be needed. When you compare the yellow sign or the side of the stairs facing the camera, you can see that the textures suddenly look lower res than before.
If someone is working on TAA and wants to give this a shot, you can find the technique described in more detail here under "Texture Blurring"

@clayjohn clayjohn modified the milestones: 4.3, 4.4 Jul 19, 2024
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

LGTM, seeing these values are sourced from the original implementation. From reading the reactions, it feels like we're just seeing TAA in action.

I suggest we merge this and see how people react to the visual change.

@akien-mga akien-mga merged commit 49ed6c5 into godotengine:master Sep 3, 2024
@akien-mga
Copy link
Member

Thanks!

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.

6 participants