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

Add SMAA 1x to screenspace AA options #102330

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RGDTAB
Copy link

@RGDTAB RGDTAB commented Feb 2, 2025

The quality settings used for edge detection and blending weight calculation are the defaults for SweetFX's ReShade implementation. I didn't add any way to change these settings as the very meager performance improvement between the highest and lowest settings didn't justify the visual difference between them.

@RGDTAB RGDTAB requested review from a team as code owners February 2, 2025 17:11
@RGDTAB RGDTAB marked this pull request as draft February 2, 2025 17:13
@Chaosus Chaosus added this to the 4.x milestone Feb 2, 2025
@RGDTAB RGDTAB marked this pull request as ready for review February 2, 2025 20:02
@tetrapod00

This comment was marked as resolved.

@RGDTAB
Copy link
Author

RGDTAB commented Feb 2, 2025

There is a proposal for SMAA already: godotengine/godot-proposals#2779

Right. I forgot to mention that in my first message. This is aimed at implementing that proposal.

@fire
Copy link
Member

fire commented Feb 4, 2025

Are you able to run pre-commit -a and godot --doctool so tests can proceed? I'll try to get some instructions written in the future.

@RGDTAB
Copy link
Author

RGDTAB commented Feb 4, 2025

Sure thing, but it looks like the only thing pre-commit wants to change is the last name of one of the authors of SMAA. Are you sure that's OK?

@tetrapod00
Copy link
Contributor

It's definitely not right to misspell an authors name so the CI passes.

Looks like COPYRIGHT.txt is already skipped by codespell:

"COPYRIGHT.txt",

But since the author name is in multiple files, the right fix might be to add "Masia" to the ignored word list:
ignore-words-list = [

@RGDTAB
Copy link
Author

RGDTAB commented Feb 4, 2025

Thanks for the clarification. It should be fixed now.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally on macOS with Metal, it works as expected.

Testing project: test_pr_89582.zip
This uses the approach described in #89582 (comment) for testing.

Click to view at full size.

No AA FXAA master FXAA #89582 SMAA
No AA FXAA master FXAA PR SMAA

SMAA does a better job at preserving detail compared to FXAA, while having slightly better edge coverage.

SMAA is not affected by #83089 either:

No AA FXAA master SMAA
image image image

@mrjustaguy
Copy link
Contributor

mrjustaguy commented Feb 10, 2025

While I know were in the beta stage as is right now and that's a feature freeze, I personally don't see much harm in getting this in 4.4 if it's ready, given there's practically no compat breakage, and our current screen space aa is bad (until FXAA 3.11 gets merged if it gets merged in the future which will break compatibility given significantly different behavior and performance of it compared to our current FXAA) and there are cases where only Screen Space AA is usable

Example: when using https://github.com/TokisanGames/Terrain3D breaking TAA/FSR2 due to Motion Vector issues (FSR2's somewhat usable, TAA however is not) and MSAA isn't an option due to Vulkan Synchronization issues in Godot when used with other effects like in #102646

Though do keep in mind Compositor SMAA (https://github.com/RGDTAB/Godot-SMAA) is also an option, though it's much harder to work with it, making a setting to turn it off/on

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Feb 17, 2025

Testing on the TPS demo (Native resolution, SMAA enabled, other AAs disabled). Edge detection seems a bit broken.

image

image

This also happened in Godot-SMAA back then

@mrjustaguy
Copy link
Contributor

No, this is the SMAA settings.. the edge detection threshold needs to get really really low to catch some of the aliasing, and some of the aliasing just is due to a lack of samples, that SMAA and FXAA cannot help with.

@Calinou
Copy link
Member

Calinou commented Feb 19, 2025

I suggest comparing the output of this PR with ReShade-injected SMAA (with all forms of in-game AA disabled), so you can be sure of this.

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Feb 19, 2025

I suggest comparing the output of this PR with ReShade-injected SMAA (with all forms of in-game AA disabled), so you can be sure of this.

Had to use vkBasalt for this. SMAA is the only effect enabled:
image

For reference, this is the default config file which includes SMAA settings. I only changed the effects to smaa

I observed the threshold in this PR is 0.1 (Medium/High), while in vkBasalt is 0.05 (Ultra). Increasing the value in vkBasalt to 0.1 seems to yield the same result as the first screenshots.

Maybe decreasing the threshold to 0.05 fixes the issue?

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Feb 19, 2025

Here is what it looks in a build with SMAA_THRESHOLD lowered to 0.05:

image

I didn't add any way to change these settings as the very meager performance improvement between the highest and lowest settings didn't justify the visual difference between them.

I genuinely believe this one does. On the other hand, it would be 1:1 with SMAA_PRESET_ULTRA.

@mrjustaguy
Copy link
Contributor

In the Addon I modified mine to use 0.01 (below even ultra) as the game I'm working on is quite dark and it wasn't quite picking up the aliasing that well..

Honestly I think this may be a good idea to keep that as an option and default it to 0.05, as that's the one thing that affects the AA the most, both performance and visuals wise (though to my surprise the difference on an rx 6600 at 1440p doesn't seem to be much between 0.2 and 0.01, but it is quite a powerful GPU for a little post processing work)

@RGDTAB
Copy link
Author

RGDTAB commented Feb 20, 2025

The edge detection threshold can now be controlled from the project settings (default is 0.05).

@clayjohn
Copy link
Member

Is there a way to we can compress the AreaTex.h file? I'm not super keen on including a 1.1 mb LUT in the source that appears to be mostly zeroes.

Also, have you looked into supporting the Compatibility renderer? If it's not possible, then we need to provide a warning when users try to enable SMAA when using the compatibility renderer

@fire
Copy link
Member

fire commented Mar 13, 2025

Would it be possible to encode AreaTex.h into something like base64?

@mrjustaguy
Copy link
Contributor

https://github.com/RGDTAB/Godot-SMAA/tree/main/SMAA has the textures stored as DDS, and about 1/10th the size.
I don't know if there's some limitation that's preventing that approach here.

@BlueCube3310
Copy link
Contributor

https://github.com/RGDTAB/Godot-SMAA/tree/main/SMAA has the textures stored as DDS, and about 1/10th the size. I don't know if there's some limitation that's preventing that approach here.

AreaTex.dds is compressed as RGTC_RG, which isn't available across all devices

@DeeJayLSP
Copy link
Contributor

DeeJayLSP commented Mar 13, 2025

Is compressing as PNG an option? The format is part of core and pretty much available in all devices.

Maybe WebP if the module is on, as its compression is much better than PNG.

@RGDTAB
Copy link
Author

RGDTAB commented Mar 15, 2025

I'll start experimenting with base64 encoding over PNG or some general purpose compression to see what gets the best results.

Compatibility support will probably have to be saved for its own PR. I think that it will be possible, I just don't have the necessary time to commit to it at the moment.
For now I'll add a warning for anyone on the Compatibility renderer.

@Calinou
Copy link
Member

Calinou commented Mar 15, 2025

Compatibility support will probably have to be saved for its own PR. I think that it will be possible, I just don't have the necessary time to commit to it at the moment.
For now I'll add a warning for anyone on the Compatibility renderer.

See also #69463.

@RGDTAB RGDTAB force-pushed the add_partial_smaa branch from 286e861 to a248b4b Compare March 15, 2025 19:13
@RGDTAB
Copy link
Author

RGDTAB commented Mar 15, 2025

AreaTex.h is now only 42.1KB. Base64 over PNG did the trick nicely, thank you all for the help.

See also #69463.

I ended up adding a warning for FXAA too since it didn't seem like there was one.

@RGDTAB RGDTAB force-pushed the add_partial_smaa branch from a248b4b to c368b4c Compare March 15, 2025 23:18
@akien-mga akien-mga requested review from a team and removed request for a team March 19, 2025 10:48
Copy link
Contributor

@BlueCube3310 BlueCube3310 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've tested it on several scenes and the results look as expected. The code itself also seems fine, though I think it could use a more thorough review

@RGDTAB RGDTAB force-pushed the add_partial_smaa branch from c368b4c to 7214db3 Compare March 25, 2025 02:17
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it fails to compile:

Compiling core/os/scu/scu_core_os.gen.cpp ...
In file included from servers/rendering/renderer_rd/effects/scu/scu_servers_rendering_renderer_rd_effects_1.gen.cpp:2:
./servers/rendering/renderer_rd/effects/smaa.cpp: In constructor 'RendererRD::SMAA::SMAA()':
./servers/rendering/renderer_rd/effects/smaa.cpp:126:45: error: 'core_bind' has not been declared
  126 |                 Vector<uint8_t> png_bytes = core_bind::Marshalls::get_singleton()->base64_to_raw(areaTexPNG);
      |                                             ^~~~~~~~~
scons: *** [bin/obj/servers/rendering/renderer_rd/effects/scu/scu_servers_rendering_renderer_rd_effects_1.gen.linuxbsd.editor.x86_64.o] Error 1
scons: building terminated because of errors.
INFO: Time elapsed: 00:01:02.26 

Replacing core_bind with CoreBind fixes compilation.

Comparison between the previous state and the current state of SMAA:

Old SMAA Current SMAA
Old SMAA Current SMAA

It looks much better now, likely thanks to the edge detection threshold tweak. It's much closer to Tesseract's SMAA implementation:

Current SMAA Tesseract SMAA
Current SMAA Tesseract SMAA

@Calinou
Copy link
Member

Calinou commented Mar 25, 2025

Edit: Disregard the accidental force-push above (I accidentally committed the second commit from https://github.com/Calinou/godot/tree/add_partial_smaa-webp to your fork instead of mine).


It's possible to reduce binary size by converting the AreaTex texture to lossless WebP. The result is visually identical compared to before.

Here's a branch in my fork with a commit applied on top of your PR. Feel free to cherry-pick it: https://github.com/Calinou/godot/tree/add_partial_smaa-webp

Unfortunately, it fails to run due to the following:

ERROR: Parameter "p_loader" is null.
   at: _load_from_buffer (core/io/image.cpp:4177)

I don't know why, maybe I messed up the Base64 conversion or WebP loading can't be done this early in the engine (since it's a module and not core). The Base64 appears to be valid, as removing the == at the end of the Base64 strings causes this error instead:

ERROR: Condition "CryptoCore::b64_decode(&w[0], buf.size(), &arr_len, (unsigned char *)cstr.get_data(), strlen) != OK" is true. Returning: Vector<uint8_t>()
   at: base64_to_raw (core/core_bind.cpp:1236)

The source texture (it's a WebP file despite its .png extension):

AreaTex webp

Note that it modifies a thirdparty/ file, but given SMAA is unlikely to get future updates, it's probably not much of an issue.

Binary size of a stripped Linux x86_64 release export template (production=yes lto=full):

  • master: 72,777,624 bytes
  • This PR: 72,859,544 bytes
  • This PR with WebP texture: 72,843,160 bytes

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.

Integrate sub-pixel morphological antialiasing (SMAA) in the Vulkan renderer
10 participants