-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Add FidelityFX Super Resolution 2.2 (FSR 2.2.1) support. #81197
Conversation
My initial report on FSR 2.2 vs TAA FSR 2.2 is not compatible with MSAA, as it breaks rendering at lower resolutions, though it is usable with Native resolution if TAA is also enabled (else it turns into a jittering mess) Aside From that, Comparing with TAA, There is no Motion pixelization with FSR2, However static scenes with high frequency detail are a little less stable, It also seems to be properly resolving Transparencies unlike TAA which breaks with transparencies causing anything affected by transparent surfaces to go crazy (say Quad effects show the underlying quad moving with TAA, not so with FSR 2.2) FSR 2.2 costs 2x as TAA, However looks significantly better IMHO and is well worth it, but All other Anti Aliasing should probably be forced to off when FSR 2.2 is on |
Note that this PR includes changes to optimize motion vector generation (by deriving from the depth buffer when possible), which may improve performance substantially, even with traditional native TAA. Therefore, this PR might be enough to close #61905 (I haven't tested yet).
I'd recommend adding PS: The |
We'll probably see the biggest gains when the render list is sorted in a way that only renders static geometry first and dynamic geometry afterwards. This PR doesn't take care of that nor does it add that behavior to the existing TAA shader, but we can very well take advantage of the implementation here.
Sounds good to me. |
Well someone has to ask about this. |
The FSR 3 code isn't available yet. This will likely be worked on after it's open sourced and has a Vulkan + GLSL port, but we can't give an ETA. |
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.
Most of my remarks are more general ones, except for some of the FSR logic that ended up in RenderSceneBuffers
, but either should be in RenderBufferDataForwardClustered
or handled by the FSR effect itself like other effects do.
All in all this looks amazing!
servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp
Show resolved
Hide resolved
servers/rendering/renderer_rd/storage_rd/render_scene_buffers_rd.cpp
Outdated
Show resolved
Hide resolved
@@ -732,6 +735,11 @@ void RendererSceneRenderRD::_render_buffers_debug_draw(const RenderDataRD *p_ren | |||
} | |||
} | |||
|
|||
if (debug_draw == RS::VIEWPORT_DEBUG_DRAW_INTERNAL_BUFFER) { |
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.
Should probably have made this remark earlier on, but we need a better name then internal buffer. Internal buffer is far to non-descript. Which internal buffer? We have dozens of internal buffers...
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.
Do you think we should solve this in this particular PR? It matches the name of the internal texture on Render Buffers so that's pretty much why I used that.
Best alternatives I can think of is something like Color Buffer or related.
servers/rendering/renderer_rd/shaders/effects/fsr2_accumulate_pass.glsl
Outdated
Show resolved
Hide resolved
This fails to build with MinGW from Linux (Fedora 38) for me:
Command line: Also, the Viewport constant for FSR2 is missing or not exposed: Benchmark
All tests done with this PR. I couldn't notice a performance difference with
Binary sizeStripped Linux x86_64
Android APK size (release export template, arm64 only):
Given the increase in binary size, we should look into excluding as much FSR2 code as possible on mobile/web platforms, as it won't be used there. Visual comparisonStill sceneOutput resolution: 3840×2160
Scene in motionOutput resolution: 1920×1080 Camera is rotating towards the left. This means newly discovered pixels are on the left of the image. These are the ones to watch out for disocclusion artifacts 🙂
@DarioSamo I've noticed the disoccluded pixels look pin-sharp, almost like the image is using nearest-neighbor filtering before being upscaled. Is this expected? I recall most games looking softer with newly disoccluded pixels, even if fizzle does occur in more complex scenes. VideosAll videos are in 1920×1080 (matching the rendering resolution used to record those videos), with high-quality 4:4:4 (full chroma) VP9 encoding. Each video is about 40 MB, so previews have been disabled. Command used to record videos: path/to/godot.binary --path /path/to/godot-reflection/ --resolution 1920x1080 --write-movie "$PWD/fsr2_balanced/input.png" --quit-after 367 -- --fsr2 --balanced --spin
ffmpeg -r 60 -i input%08d.png -c:v libvpx-vp9 -pix_fmt yuv444p -crf 15 fsr2_balanced.webm Footnotes
|
there are some that don't look smooth and exhibit very similar FSR 2 behaviors, such as Star Wars Jedi Survivor mentioned above, though that is the only FSR 2 game where I've heard of such complaints so could be just bad implementation there too, and not an actual FSR 2 issue. |
That sounds about right to me. You can check the sample in https://github.com/GPUOpen-Effects/FidelityFX-FSR2 as well if you'd like a reference to compare to from AMD themselves, but I don't think anything in the implementation could cause that. |
Yea though if you plan to add it will there be a sething where we choose if an object is static or dynamic ?, kind of similar to bastian pr, but for geometry. |
Nope, the plan would be to auto-detect it based on whether the element has moved or not. |
Did all the changes that I marked on the TODO list as well as change the logic and possibly fixing that compilation error pointed out for MINGW. |
Alright, but it can have an effect even dissabling taa? I mostly ask, because it could fix this issue indirectly #73411 (comment) |
No, there would be no effect when TAA or FSR2 isn't used as the performance degradation comes from writing motion vector information. If motion vectors aren't required, then there's no point in splitting the render lists. |
594e4b3
to
b1944df
Compare
I've added the optimization we talked about, a new type of render list for geometry that requires motion vectors will be used when FSR2 is used and motion is deemed as necessary. This should lead to significant savings in bandwidth since only geometry that moves will write motion vectors. This improvement could be extended to the TAA shader once it's able to derive motion vectors from depth, but that'll probably be best separated into its own PR as there are some design changes that are required on the algorithm to support it correctly. |
349b4df
to
14ab457
Compare
DoF in Godot is dependent on input resolution (i.e. before upscaling), given how the DoF strength is currently never scaled according to resolution. #57210 needs to be salvaged to prevent this. |
I'm not sure there's much to investigate here, it's not gonna grab motion vectors from a flat animated surface. Godot doesn't support sampling the motion vectors from your other render target. The best we can hope for is to allow people to be able to output to the reactivity mask somehow, but that's not a feature that Godot currently has as a render target. I'm fairly sure you'd find the same problems with TAA as well. An specific draw mode for surfaces like this might be a good idea, like proposed here #77523. |
I've verified DOF is broken when using FSR2 as it was suggested on a comment before, will look into fixing it. |
@mrjustaguy DoF should be working now, thanks for the report. |
I've tested performance again on https://github.com/Calinou/godot-reflection/tree/fsr2-test with the latest revision of this PR (cf3fd3b1a). All tests done in 3840×2160 on a GeForce RTX 4090.
PS: Is #81350 incorporated in this PR? I've tested the latest revision of this PR, but mipmap bias isn't adjusted according to AMD's formula when FSR2 is enabled. This is noticeable if you decrease resolution scale all the way down near a checkerboard texture. Further manual changes of the Texture Mipmap Bias project settings aren't visible either. |
Testing out this PR with Screen Space Reflections enabled and it seems that using FSR2 causes them to be even more jittery than when using the built-in taa. Is this a bug or just a downside of them being less blurry? |
I think this is just a consequence of SSR looking less blurry, making the jitter more obvious. SSR needs to be redesigned to take advantage of TAA jitter into account at some point. |
Cool, just wanted to be sure in case it was a bug. I've seen the sdfgi lighting jittering a bit too but that might also be a downside of using a sharper taa solution. |
Nope it's not incorporated. Now that it's merged I'll rebase it based on that (although I'll go a bit less aggressive than AMD's formula by using -0.5 instead of -1 as it causes some visible jittering) |
Rebased and now mipmap bias works properly. Texture quality should now be maintained better when using lower scales. |
I don't remember if this was mentioned anywhere before, so here goes: |
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 to me! I left one nitpick on an error message, but overall the code looks very good.
I tested locally with the TPS demo and tried various post FX (DoF, Glow, Auto exposure, etc) and it appears to work well.
Let's get this merged before the next dev release so we can get some testing before beta.
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.
Should be good to merge after a rebase to squash the commits
Done. |
Introduces support for FSR2 as a new upscaler option available from the project settings. Also introduces an specific render list for surfaces that require motion and the ability to derive motion vectors from depth buffer and camera motion.
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.
Third-party code integration looks good.
Thanks! |
FidelityFX Super Resolution 2.2
Introduces support for FSR2 as a new upscaler option available from the project settings.
It's worth noting that while I've done my best effort in making improvements to the pipeline to provide the information as well as possible to FSR2 with the minimal performance impact, some artifacts inherent to the algorithm are to be expected. This is a pretty competitive area at the moment where both DLSS and FSR aren't perfect in all cases and it'll vary a lot on the type of content that is used. While I can address artifacts introduced by my own implementation due to errors, if the contents of the render fit what FSR2 expects, there's not much else I can do. Therefore, we should probably limit reporting to noticeable issues (e.g. broken results) that aren't FSR2 problems.
To verify
There's some UX issues where it'd be appreciated to gather some feedback on how they should be solved.
While the FSR version included in the PR mostly matches the public GitHub version, some changes have been made to better accommodate for Godot's needs.
TODO
get_texture_slice_view
function for public-facing API🍀 This work has been financed and kindly donated to the Godot Engine project by W4 Games. 🍀