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

AMD FidelityFX Super Resolution #51679

Merged
merged 1 commit into from
Nov 24, 2021
Merged

AMD FidelityFX Super Resolution #51679

merged 1 commit into from
Nov 24, 2021

Conversation

Je06jm
Copy link
Contributor

@Je06jm Je06jm commented Aug 14, 2021

AMD FidelityFX Super Resolution, which will be referred to as FSR, can be applied to any viewport through code or in the editor. FSR is applied after all post processing effects. In it's current form, it's only implemented in a compute shader so it will not work with the mobile renderer.

FSR has a few advanced settings in the project settings menu under rendering/scaling 3d. In addition, these properties can be accessed in the SubViewport node under upscaling. Finally, a few new viewport properties have been exposed, namely scaling_3d_mode, scaling_3d_quality, fsr_mipmap_bias, and fsr_sharpness

Bugsquad edit: This closes godotengine/godot-proposals#2809.

@Calinou
Copy link
Member

Calinou commented Aug 14, 2021

Things that needs to be done before ready for a pull request are:

  • Adjust mipmap bias

See #38569, but it needs to be done in a different way to allow run-time adjustments. We could then have the FSR setting apply an automatic additional bias to the LOD bias project setting.

@Calinou Calinou added this to the 4.0 milestone Aug 14, 2021
@fire
Copy link
Member

fire commented Aug 14, 2021

I noticed you're a first time contributor.

If you need with the cicd errors, shout. Also see https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html

@Je06jm
Copy link
Contributor Author

Je06jm commented Aug 14, 2021

@fire Thanks for the offer, but I got it. They are mostly silly errors on my part involving not running clang-format and not running --doctool

@Je06jm
Copy link
Contributor Author

Je06jm commented Aug 14, 2021

At the moment, I'm having trouble figuring out how to apply FSR to the main 3d viewport. If anyone has any ideas, I would appreciate it. I can enable it through gdscript at runtime, I want to be able to apply the default settings from the project settings

@clayjohn
Copy link
Member

clayjohn commented Aug 14, 2021

@Je06jm maybe in main.cpp?

bool Main::start() {

edit: actually probably during initialization of the scene_tree

SceneTree::SceneTree() {

@Je06jm
Copy link
Contributor Author

Je06jm commented Aug 14, 2021

@clayjohn That does work for 3d scenes, but not for 2d scenes. I'm not sure how to tell if a scene is 3d or 2d from the initialization of the scene_tree

@Calinou
Copy link
Member

Calinou commented Aug 15, 2021

@clayjohn That does work for 3d scenes, but not for 2d scenes. I'm not sure how to tell if a scene is 3d or 2d from the initialization of the scene_tree

Shaders in the tonemap pass like FXAA and debanding are only effective on 3D scenes. 2D elements are not affected by those shaders, even if they are enabled. This is a good thing in FSR's context, because FSR should not be applied on 2D elements like the GUI. As for 2D games, it's less common for them to run into performance issues because they are run at high resolutions compared to 3D games.

@Je06jm
Copy link
Contributor Author

Je06jm commented Aug 15, 2021

@Calinou Sorry for the confusion. I need to detect when the root is a 2d scene so that I can disable FSR

@mrjustaguy
Copy link
Contributor

mrjustaguy commented Aug 15, 2021

AMD's FSR has issues with Godot's SDFGI

UP - DOWN arrows for switching between FSR Disabled and FSR Performance
SDFGI.zip

Native
SDFGI Native

FSR
SDFGI FSR

Aside from This Issue I Haven't Found any other Problems, and I must say, AMD really impressed me, in my testing, I've seen a performance uplift anywhere from 50% to 200% (from 10 to 30 FPS in my Stress tests, from 40 to 60 in less demanding tests) depending on The Scene when using Performance Mode and upscaling from 540p to 1080p on an antique low end potato of a GPU (Quadro K2000 - a Slightly weaker 650 with 2GB memory), which is well worth the slight image quality degradation

@Chaosus
Copy link
Member

Chaosus commented Aug 15, 2021

I've rebased this to fix a conflict + fixed a newline at the end of some files

@fire
Copy link
Member

fire commented Aug 18, 2021

@@@ -2524,6 -2575,11 +2576,14 @@@ EffectsRD::~EffectsRD() 
        RD::get_singleton()->free(index_buffer); //array gets freed as dependency
        RD::get_singleton()->free(filter.coefficient_buffer);
  
++<<<<<<< HEAD
++=======
+       RD::get_singleton()->free(ssao.mirror_sampler);
+       RD::get_singleton()->free(ssao.gather_constants_buffer);
+       RD::get_singleton()->free(ssao.importance_map_load_counter);
+ 
+       FSR_upscale.shader.version_free(FSR_upscale.shader_version);
++>>>>>>> remotes/Je06jm/fsr
        if (prefer_raster_effects) {
                blur_raster.shader.version_free(blur_raster.shader_version);
                bokeh.raster_shader.version_free(blur_raster.shader_version);
~

@@ -1353,7 +1353,15 @@ SceneTree::SceneTree() {
const int ssaa_mode = GLOBAL_DEF("rendering/anti_aliasing/quality/screen_space_aa", 0);
ProjectSettings::get_singleton()->set_custom_property_info("rendering/anti_aliasing/quality/screen_space_aa", PropertyInfo(Variant::INT, "rendering/anti_aliasing/quality/screen_space_aa", PROPERTY_HINT_ENUM, "Disabled (Fastest),FXAA (Fast)"));
root->set_screen_space_aa(Viewport::ScreenSpaceAA(ssaa_mode));
/*
Copy link
Member

Choose a reason for hiding this comment

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

Unused code should be deleted rather than commented out.

Also use // instead of /* */

@Je06jm
Copy link
Contributor Author

Je06jm commented Aug 23, 2021

Everything is now working except for changing the mipmap lod bias

@Je06jm Je06jm marked this pull request as ready for review August 24, 2021 15:51
@Je06jm Je06jm requested review from a team as code owners August 24, 2021 15:51
Copy link
Contributor

@Ansraer Ansraer left a comment

Choose a reason for hiding this comment

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

Honestly, I am not familiar enough with godots renderer to properly evaluate this, but from what I can tell this looks very promising. Some small nitpicks I noticed while reading this on my phone, nothing too major.

doc/classes/Viewport.xml Outdated Show resolved Hide resolved
glsl_builders.py Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_rd/effects_rd.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_viewport.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@Ansraer Ansraer left a comment

Choose a reason for hiding this comment

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

I am no expert, but this looks good to me. Thanks for spending the time to implement this!

@Je06jm
Copy link
Contributor Author

Je06jm commented Nov 23, 2021

On macOS/MoltenVK shader compilation fails with the following errors:

ERROR:  - Message Id Number: 0 | Message Id Name:
	VK_ERROR_INITIALIZATION_FAILED: Shader library compile failed (Error code 3):
program_source:223:12: error: as_type cast from 'int' to 'half' is not allowed
    return as_type<half>(AW1_x(param) - (as_type<ushort>(a) >> AW1_x(param_1)));
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
program_source:230:12: error: as_type cast from 'int' to 'half' is not allowed
    return as_type<half>(AW1_x(param) - as_type<ushort>(a));
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
program_source:584:14: error: as_type cast from 'int' to 'half' is not allowed
    half b = as_type<half>(AW1_x(param) - as_type<ushort>(a));
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.
	Objects - 1
		Object[0] - VK_OBJECT_TYPE_SHADER_MODULE, Handle 5510021216
   at: _debug_messenger_callback (drivers/vulkan/vulkan_context.cpp:157)
[mvk-error] VK_ERROR_INVALID_SHADER_NV: Compute shader function could not be compiled into pipeline. See previous logged error.

@bruvzg
That's interesting. Does the checks not test MoltenVK? What would happen if you changed servers/rendering/renderer_rd/effects_rd.cpp line 1934 from \n#define MODE_FSR_UPSCALE_NORMAL\n" to \n#define MODE_FSR_UPSCALE_FALLBACK\n?

@bruvzg
Copy link
Member

bruvzg commented Nov 23, 2021

That's interesting. Does the checks not test MoltenVK? What would happen if you changed servers/rendering/renderer_rd/effects_rd.cpp line 1934 from \n#define MODE_FSR_UPSCALE_NORMAL\n" to \n#define MODE_FSR_UPSCALE_FALLBACK\n?

It is compiling with MODE_FSR_UPSCALE_FALLBACK.

@Je06jm
Copy link
Contributor Author

Je06jm commented Nov 23, 2021

That's interesting. Does the checks not test MoltenVK? What would happen if you changed servers/rendering/renderer_rd/effects_rd.cpp line 1934 from \n#define MODE_FSR_UPSCALE_NORMAL\n" to \n#define MODE_FSR_UPSCALE_FALLBACK\n?

It is compiling with MODE_FSR_UPSCALE_FALLBACK.

Alright. I'll work on forcing the fallback mode when using MoltenVK. I'll also create an issue reporting this problem

@Je06jm
Copy link
Contributor Author

Je06jm commented Nov 23, 2021

Also @bruvzg, did this error happen when compiling source or when running Godot?

@bruvzg
Copy link
Member

bruvzg commented Nov 23, 2021

Also @bruvzg, did this error happen when compiling source or when running Godot?

When running.

@Je06jm
Copy link
Contributor Author

Je06jm commented Nov 23, 2021

@bruvzg I just updated the PR so it should now work on MoltenVK

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Re-approving given the changes since my last review

@Calinou
Copy link
Member

Calinou commented Nov 23, 2021

I made a testing project: test_fsr.zip

Tests below were run on a GeForce GTX 1080 with NVIDIA 470.74 driver.

Bilinear scaling works as expected, although the aforementioned FSR mipmap bias setting doesn't seem to have an effect (see my above review comment). Ideally, the mipmap bias setting should always work, even if the scale factor is 1.0 (it can be used to make textures appear sharper at a distance).

This is likely controlled here: https://github.com/Je06jm/godot/blob/20deb0917d466ca9dd1bf435dfb326c72f73e3c0/servers/rendering/renderer_rd/renderer_scene_render_rd.cpp#L2673-L2677

2021-11-24_00 13 51

FSR appears to be broken in this project until Glow is disabled. I only get a black screen with 2D elements not being cleared after every frame is rendered:

2021-11-24_00 14 52

After restarting the editor, I get this error spam:

ERROR: Uniforms were never supplied for set (0) at the time of drawing, which are required by the pipeline
   at: compute_list_dispatch (drivers/vulkan/rendering_device_vulkan.cpp:8086)
ERROR: Texture (binding: 0, index 0) is not a valid texture.
   at: uniform_set_create (drivers/vulkan/rendering_device_vulkan.cpp:5603)
ERROR: Condition "!uniform_set" is true.
   at: draw_list_bind_uniform_set (drivers/vulkan/rendering_device_vulkan.cpp:7271)
ERROR: Uniforms were never supplied for set (2) at the time of drawing, which are required by the pipeline
   at: draw_list_draw (drivers/vulkan/rendering_device_vulkan.cpp:7426)

Edit: It looks like disabling Glow in WorldEnvironment fixes this. Rendering stops working again as soon as I enable glow again.

Either way, kudos for sticking around and implementing FSR all the way 🙂

@Je06jm
Copy link
Contributor Author

Je06jm commented Nov 23, 2021

@Calinou. That is an interesting bug. I'll start working on that. As far as mipmap mipmap bias, I agree that it would be useful outside of FSR. That will be in the next PR as well.

@Je06jm
Copy link
Contributor Author

Je06jm commented Nov 24, 2021

@Calinou After some investigating, I discovered that the problem is that when the render texture's resolution is too low, the blur texture does not get allocated. The error I'm getting there is that Too many mipmaps requested for texture format and dimensions (10), maximum allowed: (9). It looks like the code that calculates how many mipmaps to request has a bug in it that only affects really low resolutions. Should this be fixed here or in a separate PR?

@Calinou
Copy link
Member

Calinou commented Nov 24, 2021

Should this be fixed here or in a separate PR?

I guess it should be fixed in a separate PR 🙂

@akien-mga akien-mga merged commit 547c270 into godotengine:master Nov 24, 2021
@akien-mga
Copy link
Member

Thanks for the great feature, and all the edits to match our requirements! And congrats for your first merged Godot contribution 🎉

@Je06jm
Copy link
Contributor Author

Je06jm commented Nov 24, 2021

@akien-mga Thanks! Unfortunately, there was one last thing I was going to change. As me and @Calinou discussed, I was going to make mipmaps bias apply even if FSR is disabled. Should I open a new PR for this?

@akien-mga
Copy link
Member

Yes, a new PR would be fine :)

@Calinou
Copy link
Member

Calinou commented Dec 22, 2021

Note that if anyone is interested on bringing FSR to mobile/low-end platforms, it would be best to add a "FSR Lite" scaling option as demonstrated here: https://atyuwen.github.io/posts/optimizing-fsr/
See the commits that added FSR Lite to the original FSR sample: https://github.com/atyuwen/FidelityFX-FSR/commits/master

FSR Lite looks a bit worse than vanilla FSR, but it performs much better on mobile GPUs and integrated graphics. It's fast enough to be usable on mid-range/high-end mobile.

@unfa
Copy link

unfa commented Jan 18, 2022

I am still having issues with FSR. It doesn't work at all when Glow is enabled (produces a weird glitches with transparent 2D on top of the 3D layer).

Even then it doesn't seem to work, and some particle effects are glitched out, rendering in a 2x smaller plane mismatched to the rest of the viewport. That's at 50% scale. At 25% scale the display breaks entirely as if the texture mapping to the viewport quad is completely broken.

@clayjohn
Copy link
Member

@unfa please make a bug report. Comments made on PRs will get lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement AMD FidelityFX Super Resolution (FSR) in the Vulkan renderer