bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Improve TAA

Open JMS55 opened this issue 2 years ago • 25 comments

Objective

  • Improve TAA quality
  • Prevent smearing from motion vectors that point to pixels no longer on-screen
  • Use textureSampleLevel() which is probably faster than textureSample() (Thanks to @DGriffin91)
  • Use a uniform for reset, instead of a shaderdef (sadly WebGPU doesn't support push constants :/)
  • Fix https://github.com/bevyengine/bevy/issues/9721
  • Adopt parts of the improved AA example from @DGriffin91

Followup

  • Apply FXAA when accumuluated_samples = 1 (#8847)

Changelog

  • Improve TAA quality
  • Fixed TAA smearing when reprojecting pixels that are no longer on screen
  • Fixed a bug where despawning an entity in PreUpdate while using MotionVectorPrepass would cause a panic
  • Made TemporalAntiAliasNode public

JMS55 avatar Jun 27 '23 20:06 JMS55

@JMS55 Just tested this PR with the anti_aliasing example from my PR (https://github.com/bevyengine/bevy/pull/8847) and it appears that it does not fix the smearing.

image

DGriffin91 avatar Jul 05 '23 22:07 DGriffin91

@DGriffin91 I can only get the smearing when running your example with the noise texture specifically. Using any other texture/material (I tested a few) works fine.

I think the reason it smears on your example is due to neighborhood color clamping. Reprojection succeeds, but it's not 100% perfectly accurate, and with such a high frequency, multi-colored texture, the color clamping (which is based on neighboring pixels) performs very poorly. I could be wrong though.

I'm guessing that the reason it works in #8847 is that in my previous TAA code (what's currently in main, and what that PR is based off of), I reset the accumulation/blend factor when any motion (non-zero motion vector) is detected. Which in hindsight, wasn't a great idea. However, that did mean you didn't get any ghosting even when neighborhood clamping failed. This PR tries to keep the history even under motion.

A possible solution would be to reset/reduce accumulation when color clips by a lot or is near clipping, which iirc is what Brian Karis/Unreal did. That can be very finicky and introduce more artifacts if we're not careful. Doesn't seem like the kind of thing we should try right before 0.11.

JMS55 avatar Jul 07 '23 01:07 JMS55

Removing this from 0.11 as theres too much controversy here.

cart avatar Jul 08 '23 00:07 cart

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions[bot] avatar Aug 15 '23 18:08 github-actions[bot]

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions[bot] avatar Aug 15 '23 18:08 github-actions[bot]

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions[bot] avatar Aug 15 '23 19:08 github-actions[bot]

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions[bot] avatar Aug 15 '23 20:08 github-actions[bot]

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions[bot] avatar Aug 15 '23 20:08 github-actions[bot]

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions[bot] avatar Aug 15 '23 23:08 github-actions[bot]

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions[bot] avatar Aug 16 '23 01:08 github-actions[bot]

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions[bot] avatar Aug 16 '23 01:08 github-actions[bot]

~~Well, after doing some more search, I've come to the conclusion my Mitchell-Netravali calculations were wrong, but fixing that made 0 difference and still had the same issue. Not sure how to actually fix it. May just remove that part for this PR.~~

JMS55 avatar Sep 18 '23 04:09 JMS55

Could you explain the solutions to the objectives in the PR description? It's difficult to know what the PR is doing otherwise.

superdump avatar Oct 01 '23 20:10 superdump

  • Code is overall cleaned up a lot
  • Instead of the weird history confidence thing I was doing, I'm now using the standard "keep a count of successful reprojections, blend at a rate of 1 / count, clamped between [1, 8]" (could arguably be 16, and increasing the jitter sequence length to 16).
  • Prevent blurring from motion vectors pointing off-screen: accumulated_samples *= f32(all(saturate(history_uv) == history_uv));
  • Mitchell–Netravali filter over the 3x3 neighborhood of the input pixel to TAA, which increases sharpness vs a straight average (box filter).

JMS55 avatar Oct 01 '23 20:10 JMS55

@cart thoughts on how we should fix this issue: https://github.com/bevyengine/bevy/discussions/9719

JMS55 avatar Oct 01 '23 21:10 JMS55

image Shadows are clipping as you move the camera :/. I hate messing with shadows, can someone else try and fix this? @DGriffin91 :)

JMS55 avatar Oct 12 '23 03:10 JMS55

Shadows are clipping as you move the camera :/. I hate messing with shadows, can someone else try and fix this?

You just need to extend the maximum_distance of the cascade_shadow_config to be eg. 5m instead of 3m.

Elabajaba avatar Oct 15 '23 03:10 Elabajaba

Thanks, that fixed it!

JMS55 avatar Oct 15 '23 03:10 JMS55

I've mentioned this in the discord but want to comment here so it doesn't get lost. I think the state of the TAA in the PR currently has much more smearing/blurring under motion than the current implementation in bevy main. And still does not address the edge smearing issue that https://github.com/bevyengine/bevy/pull/8847 fixes.

Main as of 5733d2403e9a8dc86e39757fad41db169764cc2e main

https://github.com/bevyengine/bevy/pull/8847 8847

https://github.com/bevyengine/bevy/pull/8974 (this PR) 8974

Main | https://github.com/bevyengine/bevy/pull/8847 | https://github.com/bevyengine/bevy/pull/8974 compare_large

DGriffin91 avatar Oct 17 '23 02:10 DGriffin91

I don't think this can be merged until the NaN issues are solved.

image

Potentially related is that the history texture has negative RGB values for some things for some reason.

Elabajaba avatar Oct 17 '23 02:10 Elabajaba

Nor because of the ghosting/smearing that @DGriffin91 showed above.

superdump avatar Oct 17 '23 03:10 superdump

In addition to ghosting/smearing things are just blurry in general:

https://github.com/bevyengine/bevy/pull/8847 | https://github.com/bevyengine/bevy/pull/8974 (this PR) combined2

With some areas highlighted in case it's not initially obvious: combined3

DGriffin91 avatar Oct 17 '23 04:10 DGriffin91

I just realized that I had done these tests with vsync at 165hz and high framerate minimizes the issue. Here's setting my monitor for vsync at 60hz:

https://github.com/bevyengine/bevy/pull/8847 8847_60fps

https://github.com/bevyengine/bevy/pull/8974 (this PR) 8974_60fps

DGriffin91 avatar Oct 17 '23 04:10 DGriffin91

@DGriffin91 what does that last example at 60Hz look like on main?

OK, so, we all want to make progress on TAA. None of what I am writing is intended to offend anyone, and all efforts and contributions are sincerely appreciated.

It looks to me like this PR causes significant regressions in visual quality in its current form. As this PR currently combines multiple changes into one PR, it makes it difficult to understand what part of the changes introduces what improvements and what problems.

I think we need better in-repo test cases for common problems, such as a moving / rotating camera, perhaps if heuristics for when disocclusions are detected are based on depth differences then we need to test disocclusions at different depths, etc. Then this PR could be split into multiple smaller steps to be able to verify that each in isolation is as good or better. That will also make reviewing much easier which increases the likelihood of faster merges. When situations with PRs become unclear, it is difficult to prioritise reviewing them over other PRs that have a clear state.

I looked through #8847 and aside from needing to check the FXAA code move line-by-line, it looks like it adds camera movement and a noise texture to stress test TAA functionality. That change to example code should probably be a separate PR so we can first see how it looks on main and then how it looks with the introduced changes from any of the PRs modifying TAA.

  • @DGriffin91 could you separate out the example changes from #8847 so we can review and merge those first?
  • @JMS55 could you then split this PR into separate PRs for each logical change, and then we can review them step by step to make sure we're consistently improving things?

superdump avatar Oct 17 '23 13:10 superdump

@superdump I've created a PR with just the example from https://github.com/bevyengine/bevy/pull/8847: https://github.com/bevyengine/bevy/pull/10183

I've also updated it with a few things:

  1. The noisiness of the noise texture was a bit excessive without mipmaps so it now generates those.
  2. I used deterministic PRNG for the white noise so it's consistent (ported from some gpu code)
  3. I made the UV scale follow the window height so the density of the noise on screen is consistent. (without this a small window would render solid grey, for example)
  4. I added a screenshot mode that animates the camera based on the current frame instead of time delta and takes a screenshot at a specific frame. This allows the TAA response and camera motion to be consistent regardless of the frame rate. I tested this at 60hz and 165hz and the resulting image looked the same.
  5. I added a second helmet to give a bit more context for seeing detail. Here's the result (preferably view at full 1:1 resolution):

Bevy main: main

This pr: 8974

I've abandon https://github.com/bevyengine/bevy/pull/8847 in favor of using a 3rd party plugin: https://github.com/DGriffin91/bevy_mod_taa taa_screenshot-0

DGriffin91 avatar Oct 19 '23 03:10 DGriffin91