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

Disable VRAM compression by default for small textures in Detect 3D #62023

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 14, 2022

More flexible redone version of #45587.

This is done to prevent reducing texture quality when it doesn't save much video memory, especially for pixel art.

The size threshold can be adjusted in the project settings. To get the previous behavior where textures detected to be used in 3D had their compression mode always set to VRAM, set this to the lowest value (16).

Already imported textures are not affected by this change unless you remove the associated *.import file for a given texture.

I reordered the normal map detection logic to be after the compression mode logic, as the compression mode affects whether normal map compression can be used.

This closes godotengine/godot-proposals#4669.

Testing project: test_vram_compress_small.zip
To test reimport behavior, remember to remove all *.import files and .godot/ in the project folder. The testing project already has those files removed, so it will perform 3D detection the first time you open it.

@Calinou Calinou requested review from a team as code owners June 14, 2022 10:57
@Calinou Calinou added this to the 4.0 milestone Jun 14, 2022
@Calinou Calinou force-pushed the detect-3d-small-textures-no-vram-compress branch from a2f34b3 to 5a67306 Compare June 14, 2022 11:01
This is done to prevent reducing texture quality when it doesn't save
much video memory, especially for pixel art.

The size threshold can be adjusted in the project settings.
To get the previous behavior where textures detected to be used in 3D
had their compression mode always set to VRAM, set this to the lowest value
(16).
@Calinou Calinou force-pushed the detect-3d-small-textures-no-vram-compress branch from 5a67306 to 04d5626 Compare June 14, 2022 11:08
@clayjohn
Copy link
Member

I'm not sure I understand the reasoning behind this PR. Have there been reports of very sub-optimal quality of small textures in 3D?

What is the problem that this PR is trying to solve?

While this is better than #45587 which disabled compression entirely, I am not sure that this PR is actually solving an existing problem.

@Calinou
Copy link
Member Author

Calinou commented Jun 14, 2022

Have there been reports of very sub-optimal quality of small textures in 3D?

I've seen several reports on Discord about this over the years, although nobody opened an issue on GitHub about this to my knowledge. I've opened a proposal: godotengine/godot-proposals#4669

@lyuma
Copy link
Contributor

lyuma commented Jun 16, 2022

Have there been reports of very sub-optimal quality of small textures in 3D?

Yes, if VRAM compression is used. Compression is fixed-bitrate, so the percentage of texture size with artifacts will grow significantly as the texture size is small (especially 64x64 or smaller). Furthermore, pixel art can be affected, for example small icons displayed in 3D.

Another cause: Many users use Godot in 2D. Of those usecases, some involve use as a uniform in 2D shader materials, which has a bug ( #62087 ) that triggers the "Detect 3D" code with the following message when used in a CanvasItem material:

res://random_texture.png: Texture detected as used in 3D. Enabling mipmap generation and setting the texture compression mode to VRAM Compressed (S3TC/ETC/BPTC).

This is a bug yes, but it might contribute to some of the reported issues with VRAM compression being inadvertently enabled in a 2D project.

Anyway... seeing as this commit only changes default settings, namely Detect 3D, and doesn't prevent the user from forcing a particular compression mode... I don't see a problem with making it just a bit harder to shoot yourself in the foot.

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.

Thanks for the extra detail in your proposal. It is very helpful to see the visual difference that this makes for small textures.

@akien-mga akien-mga merged commit 0daa868 into godotengine:master Jun 17, 2022
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the detect-3d-small-textures-no-vram-compress branch June 19, 2022 16:20
@reduz
Copy link
Member

reduz commented Jun 20, 2022

As I mentioned in the meeting, I am not in favor of this PR. It is bad for mobile and likely low end GPUS. I understand the problem but I don't think this is the right solution so I advise reverting and likely thinking of something else.

@Zireael07
Copy link
Contributor

@reduz: Can't this be somehow limited to desktop only by default?

@reduz
Copy link
Member

reduz commented Jun 20, 2022

@Zireael07 To me the right solution here is having a project template for PS1 style looking games that sets the settings properly for importing textures using regular compression (PNG or WebP lossless). You really don't want this for games that don't care about it.

@Zireael07
Copy link
Contributor

That would be a good idea too (or some other way to tell Godot "hey I want this and that import setting for this project by default")

@Calinou
Copy link
Member Author

Calinou commented Jun 20, 2022

That would be a good idea too (or some other way to tell Godot "hey I want this and that import setting for this project by default")

Since Godot 3.4, there's already an import defaults editor in the Project Settings. You can disable Detect 3D there, although doing so will also mean mipmaps won't be generated for textures used in 3D (unless you also enable mipmaps for all textures).

@Zireael07
Copy link
Contributor

@Calinou: I was aware of that editor, but only peripherally, and I had no idea it can disable Detect3D. Yet another thing that needs better documented...

@lyuma
Copy link
Contributor

lyuma commented Jun 20, 2022

Is there ever an instance where a 4x4 texture should be vram compressed at all. 8x8?
Surely there is some threshold where it makes sense to change what is in essence a default setting per-texture.

Also we need to reopen those issues that got closed when this was merged

@Calinou
Copy link
Member Author

Calinou commented Jun 20, 2022

Also we need to reopen those issues that got closed when this was merged

The proposal is technically considered rejected now, so I don't think we should reopen it (other than changing its labels).

Is there ever an instance where a 4x4 texture should be vram compressed at all. 8x8?

reduz isn't opposed to disabling VRAM compression for very small textures (16×16 = 256 pixels or smaller). Such textures are not very common anyway, even in pixel art projects, so I don't know how often this would help users. I know Minecraft uses 16×16 textures, but as soon as you'd use even a 17×17 texture, your textures would start looking ugly by default again 🙂

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.

Disable VRAM compression by default for small textures in Detect 3D
6 participants