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

TAA off screen reprojection FXAA fallback #8847

Closed
wants to merge 17 commits into from

Conversation

DGriffin91
Copy link
Contributor

@DGriffin91 DGriffin91 commented Jun 15, 2023

Update

I'm abandoning this in favor of using https://github.com/DGriffin91/bevy_mod_taa which addresses many issues with the current TAA implementation.
I also don't think falling back to FXAA is a great approach.

Objective

  • TAA reprojection clamps to the screen edges causing smearing when reprojection is off screen.

Solution

  • If the reprojection is off screen, fall back to FXAA.

This PR also updates the anti_aliasing example with a noise texture and camera movement needed to illustrate the issue.

These images show the camera under motion. View at full resolution and note right side of image.

Current TAA:
current_taa_clamping

FXAA fallback:
fxaa_fallback

No FXAA fallback:
no_fxaa_fallback

@JMS55 JMS55 self-requested a review June 15, 2023 01:02
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior labels Jun 15, 2023
Copy link
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

Code looks good, just the PR description needs a changelog/migration guide for the prepass changes.

@JMS55 JMS55 added this to the 0.11 milestone Jun 18, 2023
Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com>
// Compare current and previous depth
let current_depth = 1.0 / current_depth_nonlinear;
let last_frame_depth = 1.0 / textureSampleLevel(last_frame_depth, nearest_sampler, history_uv, 0.0);
if abs(current_depth - last_frame_depth) > 0.05 {
Copy link
Contributor Author

@DGriffin91 DGriffin91 Jun 26, 2023

Choose a reason for hiding this comment

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

@JMS55 I don't think we should use the linear depth here. It will cause TAA to act very differently depending on how close the camera is to things. Things far in the distance will now be much more likely to trigger disocclusion. I think we should probably use the pixel radius, or non linear depth somehow. I haven't looked yet at what is typically used for TAA.

I noticed in this version that there's a lot more aliasing under motion than before on the edges of the boxes as well as on the noisy plane.

Copy link
Contributor

Choose a reason for hiding this comment

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

Typically you check for disocclusion by checking if the difference in depth is greater than ~10%. I thought the depth of the texture was already non-linear. We should probably be using world/view space depth. I always forget what the depth texture is stored as though... (we could really use some docs about the prepass formats).

There's also a plane distance check that has less positive disocclusions, but I've left that for future work.

I noticed in this version that there's a lot more aliasing under motion than before on the edges of the boxes as well as on the noisy plane.

Can you try plugging in closest_depth instead of d_mm into the disocclusion checks and see if that fixes it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The texture is non-linear but you convert it to linear by dividing it by 1.0. (for linear view space it would be near / depth, and default near is 0.1).

We should probably be using world/view space depth.

Using world/view space depth implies we would be using linear depth.

I always forget what the depth texture is stored as though

NDC, same as frag_coord.z

Can you try plugging in closest_depth instead of d_mm into the disocclusion checks and see if that fixes it?

Sure, I'll play around with it.

Copy link
Contributor

@JMS55 JMS55 Jun 26, 2023

Choose a reason for hiding this comment

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

Sounds like we should just use linear depth (so that 0.1 and 0.2 are the same distance apart as 0.9 and 1.0, right?) then. View space vs NDC space doesn't really matter, as either way we just want to check for a ~10% difference.

But you said

I don't think we should use the linear depth here. It will cause TAA to act very differently depending on how close the camera is to things.

which I don't understand why. Did you mean non-linear depth?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, not sure how well we'll actually be able to handle the noisy pixelated plane, that's kinda a torture test. I'm more concerned about regular geometry and materials.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should use linear depth. Yes 0.1 and 0.2 are the same distance apart as 0.9 and 1.0, but depending how close the camera is to something it will dramatically change the distances involved. Things in the far distance will often much more easily have significantly larger distance differences, while appearing close together on screen.

One example of how to think about this it to think about a normal sized scene, say the indoor part of bistro. If you scale the whole scene up 10x and make the camera move 10x faster, every thing will look and feel the same, but the distances will be a lot further and TAA will totally break. On the other hand if you scale everything down to miniature size, make the camera move slower, it will again look the same, but TAA will not disocclude nearly as much as at normal scale.

I think your initial comment about "checking if the difference in depth is greater than ~10%" is probably closer to what we want and would require either not using linear depth or taking into account the pixel radius.

@DGriffin91 DGriffin91 marked this pull request as draft June 27, 2023 00:54
@DGriffin91 DGriffin91 mentioned this pull request Jul 5, 2023
@DGriffin91 DGriffin91 marked this pull request as ready for review July 7, 2023 03:11
@cart
Copy link
Member

cart commented Jul 9, 2023

In light of #8974 (comment) I think we should hold off on this and discuss our options for 0.12. If this behavior is only present for worst case "super noisy" textures, adding in more complexity here might actually make the common case worse (given that the seams on the FXAA fallback line are pretty stark ... at least for me while in motion).

@cart cart modified the milestones: 0.11, 0.12 Jul 9, 2023
@JMS55
Copy link
Contributor

JMS55 commented Aug 26, 2023

@DGriffin91 could you open a new PR / modify this one to make FXAA a function in order to allow FXAA to be able to be called from TAA? I'll then integrate it into my PR.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Sep 29, 2023
@alice-i-cecile alice-i-cecile removed this from the 0.12 milestone Sep 29, 2023
// Read the color at the new UV coordinates, and use it.
var finalColor = textureSampleLevel(view_target, linear_sampler, finalUv, 0.0).rgb;
return vec4<f32>(finalColor, centerSample.a);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

I haven't checked that the FXAA code move matches exactly. I think I'll have to do that locally.

Comment on lines +23 to +25
// Settings for FXAA EXTREME
const EDGE_THRESHOLD_MIN: f32 = 0.0078;
const EDGE_THRESHOLD_MAX: f32 = 0.031;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these here and not in the FXAA shader modules?

@DGriffin91 DGriffin91 marked this pull request as draft October 19, 2023 02:38
@DGriffin91
Copy link
Contributor Author

I'm abandoning this in favor of using https://github.com/DGriffin91/bevy_mod_taa which addresses many issues with the current TAA implementation.
I also don't think falling back to FXAA is a great approach.

@DGriffin91 DGriffin91 closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants