-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Initial TAA implementation #61319
Initial TAA implementation #61319
Conversation
I found some jittering in some cases, is that expected? jitter.mp4 |
Looks great so far! The implementation is nice and clean and should provide a great base to build off of. I am noticing the same jittering artifacts as jcostello, predominently on thin objects, but also on the edges of objects that strongly exhibit aliasing. |
I have noticed extreme jittering artifacts on Humans (Females especially) from the side when in motion (No animations) and other objects with highly curved silhouettes also seem exhibit this behavior in motion, and movement of shadow casters at some speeds causes shadows to get horizontal lines appear at equal distances. Aside from that, I'm loving it, it's Great at Transitioning between different Split Resolutions fairly seamlessly, and plays interestingly with MSAA (in a good way) |
Edit: I tested the current revision of this PR and can confirm it doesn't force redrawing until TAA has fully converged. Otherwise, you'll need to enable Update Continuously in the Editor Settings to ensure TAA can converge in still scenes in the editor. See also #54892. As for jittering issues, I'm not sure how other engines tackle this. Do they decrease jittering over time when the camera doesn't move? This would make aliasing more noticeable over time though. Footnotes |
I spent the last days going over the implementation and double-checking that everything was working as intended. I force-pushed a new version which adds a way to disable subgroups and thus allows for shader debugging with RenderDoc. I've been comparing our implementation with other open-source engines and also played around with some of them, and it seems like the jittering of thin bright lines is just something you have to live with. I added a bit of code from (Anki3D)[https://github.com/godlikepanos/anki-3d-engine/blob/master/AnKi/Shaders/TemporalAA.glsl] to reduce jittering, and it improves things slightly but the jittering is still noticeable. @clayjohn Do you have a MRP or test scene for the jittering on aliasy edges you mention? I haven't really been able to find any clear examples myself. @Calinou do you know how the editor forces continuous updates when the information panel is enabled? I was just checking so I can keep it consistent, but I can't seem to find how it's done. Once I add the editor update code, I think this is good to review/merge. We can look for improvements and fixes later, but it's a pretty large PR and I would like to get it merged before it creates a massive conflict :) |
The editor doesn't actually force constant redrawing in that situation. Instead, constant redrawing happens when View Frame Time is enabled simply because the frametime reported changes every frame since #54808 was merged. (That might be a good reason to revert this change, since it increases CPU/GPU usage a lot whenever this panel is enabled. Think of laptops' battery life 🙂) Edit: I've tried to revert #54808 locally and constant redrawing still occurs whenever the frame time panel is enabled, regardles of the current framerate. I don't know why. If you want to force constant redrawing, I'd look at volumetric fog temporal reprojection which does actually force constant redrawing in this case (in an explicit manner). That said, I think doing so without limiting the number of frames that will be redrawn is a bad idea as it increases CPU/GPU usage without good reason: #54893 |
Is TAA Planned for the Mobile Renderer? |
We discussed it on the There are no plans to add TAA to the OpenGL renderer, given the legacy/low-end focus of that rendering backend. |
What I used before was a sphere with a strong directional light on it from one side. With your recent pushed changes I can no longer reproduce the issue. Also, the change you made to reduce the swimming artifacts appears to have done quite a bit! While there is still some swimming, it isn't as obvious and jarring as before. @jcostello Can you test again in your test scene that you posted earlier? I'll start an in-depth review now! |
The jittering improved a bit 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I appreciate how simple you kept it, it shouldn't be too much work to iterate on
Just a few comments to clean things up before it is ready to merge.
I put together a little testing project to test reprojection and I think reprojection isn't quite working perfectly yet. My test case has three objects, one is translated a little bit each frame, the other one grows and shrinks using vertex animation and the last is stationary. For slow movements like this (with simple objects) I expect that the reprojection would be able to keep up and maintain moderate levels of anti aliasing. However, you can see that the moving objects are just as aliased and there appears to be a small amount of ghosting |
@clayjohn The Label3D also appears to be quite affected by ghosting. I wonder if there should be a material property or render mode to disable TAA on specific materials, including the one used on Label3D and SpriteBase3D by default. Alpha-blended materials in general don't benefit much from TAA in most cases. Alpha scissor materials do benefit from TAA, but these are not the default for Label3D and SpriteBase3D. |
Alright, I had introduced a regression in a last-minute change. In order to save an extra buffer I used the same image as source and destination, which should work fine for local groups as they load to shared memory at the beginning, but I failed to realize that they also access neighboring group's data, which may have already been processed. This should be fixed now. I also tweaked the disocclusion threshold and scale factors, so they are not as aggressive. I managed to track and fix an upstream bug, only to realize it had already been fixed upstream in the meantime (oh well). Plus, I applied @clayjohn's suggestions. With all that done, there's still some minor aliasing in moving edges, but as I understand it arises from history clipping, which is essential to TAA. Most games and engines deal with it by adding a small amount of motion blur. I can't really think of anything else to improve right now, so if the code and style look good, this should be good to merge. |
The Issues I've had (#61319 (comment)) have been resolved in the latest branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! It looks much better in my test scene, I also tried it out on Sponza and it looks amazing.
The last peice is to add the Licence attribution to COPYRIGHT.txt
Looks like here will be the best spot.
Line 120 in 204f260
So in this case, what should I list the license as? MIT or Expat? Poke @akien-mga |
It should be |
@JFonS when I create a triplanar material, I get a crash on your TAA branch |
Force-pushed the license attribution and the fix for @jcostello's crash. |
Initial TAA support based on the implementation in Spartan Engine. Motion vectors are correctly generated for camera and mesh movement, but there is no support for other things like particles or skeleton deformations.
Fixed the license. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Thanks! |
It's awesome! Sadly it makes VoxelGI flicker (I also toggle on and off FXAA to show that does not affect the flickering): 2022-06-15.20-22-53.mp4: |
@MisfitVillage Please open a new issue with a minimal reproduction project attached. Also test this with SDFGI, just to make sure. Edit: Issue opened: #62080 |
Initial TAA support based on the implementation in Spartan Engine.
Motion vectors are correctly generated for camera and mesh movement, but there is no support for other things like particles or skinned mesh deformations.
Here's a still image comparison: https://imgsli.com/MTA5MDA5
Bugsquad edit: This closes godotengine/godot-proposals#3401.
TAA Enabled
TAA Disabled
@Calinou You mentioned adding a project setting for the TAA blend factor. I played around with it for a bit, but I didn't see much use for it. Here's the patch for my changes with a hard-coded blend factor, if you want to take a look yourself. Feel free to open a PR if you think users could benefit from such a project setting:
Blend factor patch