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

Improved TAA with Velocity Buffer #1213

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

HifiExperiments
Copy link
Member

@HifiExperiments HifiExperiments commented Oct 30, 2024

reworked #501 (which is reworked vircadia/vircadia-native-core#576, which is reworked yozlet/interface@master...Zvork:velgbuf) (by me)

I think I've fixed the jittering issues that were blocking the last iteration of this PR, hopefully it looks ok in VR too.

Funding

This project is funded through NGI0 Entrust, a fund established by NLnet with financial support from the European Commission's Next Generation Internet program. Learn more at the NLnet project page.

NLnet foundation logo
NGI Zero Logo

@HifiExperiments HifiExperiments added needs CR This pull request needs to be code reviewed needs QA This pull request needs to be tested NLnet labels Oct 30, 2024
@ksuprynowicz
Copy link
Member

I tested it in VR on PSVR2 on Windows and desktop mode on Linux, and it works perfectly. None of the artifacts happening before were showing up now.

@ksuprynowicz
Copy link
Member

Would there be a way to make it a bit less blurry or add blur/sharpness adjustment, even as a tradeoff for small amount of aliasing?

@HifiExperiments
Copy link
Member Author

if you run luci and open the “Antialiasing” section, there are various parameters that control it. these values aren’t settings-backed, but we could either:

  1. decide on new defaults (Julian came up with these defaults on the original PR, but maybe they should be different now with all of the changes)
  2. expose the settings in the Graphics options and make them persistent

@ksuprynowicz
Copy link
Member

I did side-by-side comparison of the old and new TAA and new one is actually really good. I'm not sure why it was blurry on the previous run. I think current settings are really good, and once review is ready we can merge it.
Here's a side-by-side comparison of old and new version:
image

@ksuprynowicz
Copy link
Member

Here's a closeup - it's a massive improvement:
image

@ksuprynowicz
Copy link
Member

It looks like it switches between sharp and blurry in the mirrors, and seems much blurrier in VR than in desktop mode

@ksuprynowicz
Copy link
Member

Mirrors seem to switch between sharp and blurry, depending on where I look:
Sharp:
overte-snap-by-X74hc595-on-2024-11-01_12-34-30

Blurry:
overte-snap-by-X74hc595-on-2024-11-01_12-34-23

@ksuprynowicz
Copy link
Member

The mirrors are blurrier than the rest of the environment in VR.
Here's side by side comparison with TAA enabled and disabled:
image

@ksuprynowicz
Copy link
Member

TAA also seems much sharper on desktop than in VR:
image

@ksuprynowicz
Copy link
Member

Another example:
overte-snap-by-X74hc595-on-2024-11-01_13-36-36

@ksuprynowicz
Copy link
Member

I also seem to remember that original TAA PR in Vircadia was as sharp in VR as in Desktop, so I'm wondering if there may be some regression there.

@ksuprynowicz
Copy link
Member

Maybe VR mode contains some extra blurring pass?
I noticed that transparent objects are not antialiased at all in Desktop mode (which is fine if it's a tradeoff for amazingly sharp TAA). In VR they are blurred:
image

@ksuprynowicz
Copy link
Member

Shadows on TAA PR seem to glitch in mirrors:
https://oaktown.pl/images/overte-snap-by-X74hc595-on-2024-11-01_14-37-29.gif

@ksuprynowicz
Copy link
Member

I tested the newest version, depending on the angle user looks at mirror, it's either good, or very blurry. Sharpness changes very noticeably based on camera angle.
Sharp image:
overte-snap-by-X74hc595-on-2025-01-22_20-42-15

Blurry image:
overte-snap-by-X74hc595-on-2025-01-22_20-41-53

@HifiExperiments
Copy link
Member Author

yeah this has some remaining issues that I will track down at some point. I don’t think it will be done in time for the end of the grant, but this is like 95% of the way there

@ksuprynowicz
Copy link
Member

In desktop mode it already does look amazing :)
I hope sometime we can look into VR part and see why it's blurry currently, but if grant is running out maybe we could merge it in current shape and switch it off by default until we can figure out blurriness issue in VR? I'd love to take a look at it too once grant work is over.

@JulianGro
Copy link
Member

JulianGro commented Jan 22, 2025

As long as this doesn't cause any regressions, I am down for merging this. I haven't tested the most recent version, but it sounds like I will be using this as soon as it lands in master. Can't wait for crisp graphics without having to set 200% resolution scale.

@HifiExperiments
Copy link
Member Author

ok, I can rebase and make sure it’s disabled by default (I think it is now) and then we can re-evaluate.

the remaining issues are:

  • blurry in VR (this is probably easy to fix, I think we have to half…or double?…the horizontal jitter while in VR)
  • sometimes blurry in mirrors
  • shadows in mirrors glitch

I think the latter two issues are related to the saved transform slots which are just tricky to debug

@ksuprynowicz
Copy link
Member

I think it's a really good idea. Let me know when I can review it :)

@HifiExperiments
Copy link
Member Author

@ksuprynowicz I've rebased this so it should be ready for review! I've confirmed that on fresh settings, AA still defaults to off. I also found where we were modifying the horizontal jitter for VR; I tried disabling it (see the last commit) to see if that helps. the other issues are still present but only in mirrors so they aren't a huge deal, hopefully we can fix them in a follow up

@JulianGro I'd be curious to hear how it looks for you too if you can find some time to test it.

zvork had some other branches related to this that we could try to resurrect after this is merged, some of them might help with the bright bloom flashes or reduce ghosting

@JulianGro
Copy link
Member

I am looking at it.
So far I ran into one issue I can put my finger on:

  • It seems like sometimes on emissive materials (I have only seen this coming from emissive materials so far), some of the TAA pixels fly away either horizontally or vertically, causing artifacts. I say sometimes because while I always see it after looking for it for a while, the strength of the effect feels random. In one picture you can see a really strong effect, which persisted (switching between horizontal, vertical, and not being there) until I restarted. I marked the areas of interest in red. (I don't think this issue appears in master, though the TAA in master is so blurry that I wouldn't be surprised if you just couldn't see it.)
Pictures

Bildschirmfoto vom 2025-01-25 19-02-24
Bildschirmfoto vom 2025-01-25 19-08-17
Bildschirmfoto vom 2025-01-25 19-08-55
Bildschirmfoto vom 2025-01-25 19-10-04

As a note: There are thin black edges around emissive objects, though this also happens in master. I just thought it was noteworthy because the other issue also happens in conjunction with emissive materials.

Pictures

Bildschirmfoto vom 2025-01-25 19-10-02
Bildschirmfoto vom 2025-01-25 19-35-22
Bildschirmfoto vom 2025-01-25 19-35-33

@ksuprynowicz
Copy link
Member

@ksuprynowicz I've rebased this so it should be ready for review!

Awesome! I'm halfway through review, should be ready tomorrow morning

Copy link
Member

@ksuprynowicz ksuprynowicz left a comment

Choose a reason for hiding this comment

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

Everything looks good :)

vec3 ctc = taa_fetchSourceMap(fragUV - dv).rgb;
vec3 ctr = taa_fetchSourceMap(fragUV - dv + du).rgb;
vec3 cml = taa_fetchSourceMap(fragUV - du).rgb;
vec3 cmc = sourceColor; //taa_fetchSourceMap(fragUV).rgb; // could resuse the same osurce sample isn't it ?
Copy link
Member

Choose a reason for hiding this comment

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

Typos in the comment

@ksuprynowicz
Copy link
Member

IMO it's ready to merge, but can we wait with merging this PR until OpenXR is merged?
It makes a lot of changes to shaders and rendering backend, and I'm worried that with rebasing Vulkan PR on top of it I might not have time left to complete all Vulkan grant tasks.

@ksuprynowicz ksuprynowicz added CR approved This pull request has been successfully code reviewed and removed needs CR This pull request needs to be code reviewed labels Jan 26, 2025
@HifiExperiments
Copy link
Member Author

@ksuprynowicz yeah no prob! happy to wait. I’m also happy to handle merging this into the vulkan branch if that would be helpful for you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR approved This pull request has been successfully code reviewed needs QA This pull request needs to be tested NLnet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants