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

Improve normal map VRAM Compression with RGTC #85842

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

BlueCube3310
Copy link
Contributor

Fixes #57981

This PR extends etcpak with an RGTC compression module, as well as changes the compression type used by normal maps to RGTC_RG, instead of DXT5. This fixes artifacts caused by the R channel being quantized.
Also sets the compression type to RGTC_R for textures which only use the R channel.

Comparison:

master PR
aa bb

@akien-mga
Copy link
Member

Have you considered proposing these changes upstream to https://github.com/wolfpld/etcpak/?

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 (rebased on top of master 2f73a05), it works as expected. Normal maps in https://github.com/godotengine/tps-demo no longer exhibit artifacts when viewed from a distance or at oblique angles.

Before After (this PR)
Screenshot_20231206_152310 Screenshot_20231206_152149

@BlueCube3310
Copy link
Contributor Author

Have you considered proposing these changes upstream to https://github.com/wolfpld/etcpak/?

I did, but first I would like to implement SSE4 variants of the code for faster compression when available. I may do it sometime in the near future.

@Saul2022
Copy link

Saul2022 commented Dec 6, 2023

Sorry if i it is a dumb question, but will this be enabled by default on vram compresion? Anyways great job, i’vbeen waiting for this.

@Calinou
Copy link
Member

Calinou commented Dec 6, 2023

Sorry if i it is a dumb question, but will this be enabled by default on vram compresion? Anyways great job, i’vbeen waiting for this.

This is enabled by default – I didn't have to change any settings to fix the appearance of normal maps in the TPS demo screenshot above. However, as with all changes that affect imported files, you'll need to reimport the normal maps or remove the .godot/ folder to see any changes to imported normal maps.

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.

Looks good to me.

As mentioned above, it would be really nice to have SSE4 versions of the code, and for this to be upstreamed. But I think we should merge what we have in the meantime as the quality improvement is really important

@clayjohn clayjohn added this to the 4.3 milestone Dec 6, 2023
@BlueCube3310
Copy link
Contributor Author

As mentioned above, it would be really nice to have SSE4 versions of the code, and for this to be upstreamed. But I think we should merge what we have in the meantime as the quality improvement is really important

Perhaps the Godot-specific code from this and the modified squish decompressor could be moved to its own module one day, since RGTC isn't really considered to be part of the S3TC family?

@YuriSizov YuriSizov merged commit 671f59f into godotengine:master Dec 16, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

As far as I understand the added files are not technically thirdparty and are fully original? Not sure if this is ideal, but I guess it lets us keep everything nicely organized (and yeah, maybe it can be upstreamed at some point). And since they are completely original, we don't need to mention them in /thirdparty/README.

@BlueCube3310
Copy link
Contributor Author

As far as I understand the added files are not technically thirdparty and are fully original? Not sure if this is ideal, but I guess it lets us keep everything nicely organized (and yeah, maybe it can be upstreamed at some point). And since they are completely original, we don't need to mention them in /thirdparty/README.

The files aren't completely original as they use the code from the library's DXT5 alpha compressor, so maybe that should be mentioned?

@YuriSizov
Copy link
Contributor

YuriSizov commented Dec 16, 2023

In that case, I guess yes. After the "Files extracted from upstream source" list you could add something like:

Two files (ProcessRgtc.{cpp,hpp}) have been added to provide RGTC compression implementation, based on library's ProcessDxtc.{cpp,hpp}.

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.

Vulkan: Normal map mipmaps suffer from artifacts when VRAM compression is used
6 participants