Support higher-quality S3TC compression modes

[elasota]

Switches Squish compression mode to cluster fit at >0.75 and iterative cluster fit at >0.85 lossy quality. Both get substantially better quality than range fit.

[groud]

This seems compatibility breaking, unless if the squish module was added in 3.1.

Also, I don't understand why using an obscure float number instead of an enum. The float does not seem to be used anywhere but in selecting the compression mode.

[elasota]

Squish has been in for years, but it's always been using its lowest-quality compression mode (range fit). The float parameter is already used by the ETC importer to control for import speed/quality tradeoffs.

[groud]

OK, I un understand why you used a float value but I am not sure the image_compress_squish is the best place to do the float -> compression mode conversion. For a cleaner API, I would have an enum mode parameter for this function and select the correct mode in the Image compress function. But that's my humble opinion on that.

Squish has been in for years, but it's always been using its lowest-quality compression mode (range fit).

So unless if you move the parameter to the end and give it a default value, this is thus compatibility breaking change.

[akien-mga]

@groud The method is not bound to scripting languages, so it doesn't break compat per se for projects. But it does mean that the previously ignored lossy ratio will now be used and produce better compressions at high ratios - but I think that's fine for 3.1.

[akien-mga]

And the float lossy_quality is not being added by @elasota, so I think it's out of scope of this PR to ask them to find a better design:

$ rg lossy_quality
core/image.cpp
1633:Error Image::compress(CompressMode p_mode, CompressSource p_source, float p_lossy_quality) {
1655:                   _image_compress_etc1_func(this, p_lossy_quality);
1660:                   _image_compress_etc2_func(this, p_lossy_quality, p_source);
2431:   ClassDB::bind_method(D_METHOD("compress", "mode", "source", "lossy_quality"), &Image::compress);

core/image.h
287:    Error compress(CompressMode p_mode = COMPRESS_S3TC, CompressSource p_source = COMPRESS_SOURCE_GENERIC, float p_lossy_quality = 0.7);

scene/resources/texture.cpp
339:    set_lossy_storage_quality(p_data["lossy_quality"]);
362:    ADD_PROPERTY(PropertyInfo(Variant::REAL, "lossy_quality", PROPERTY_HINT_RANGE, "0.0,1.0,0.01"), "set_lossy_storage_quality", "get_lossy_storage_quality");
1289:   } else if (p_name == "lossy_quality") {
1313:   } else if (p_name == "lossy_quality") {

modules/etc/image_etc.cpp
97:static void _compress_etc(Image *p_img, float p_lossy_quality, bool force_etc1_format, Image::CompressSource p_source) {
190:    if (p_lossy_quality > 0.75)
192:    else if (p_lossy_quality > 0.85)
194:    else if (p_lossy_quality > 0.95)
235:static void _compress_etc1(Image *p_img, float p_lossy_quality) {
236:    _compress_etc(p_img, p_lossy_quality, true, Image::COMPRESS_SOURCE_GENERIC);
239:static void _compress_etc2(Image *p_img, float p_lossy_quality, Image::CompressSource p_source) {
240:    _compress_etc(p_img, p_lossy_quality, false, p_source);

editor/import/resource_importer_texture.h
86:     void _save_stex(const Ref<Image> &p_image, const String &p_to_path, int p_compress_mode, float p_lossy_quality, Image::CompressMode p_vram_compression, bool p_mipmaps, int p_texture_flags, bool p_streamable, bool p_detect_3d, bool p_detect_srgb, bool p_force_rgbe, bool p_detect_normal, bool p_force_normal);

editor/import/resource_importer_texture.cpp
162:    if (p_option == "compress/lossy_quality") {
190:    r_options->push_back(ImportOption(PropertyInfo(Variant::REAL, "compress/lossy_quality", PROPERTY_HINT_RANGE, "0,1,0.01"), 0.7));
208:void ResourceImporterTexture::_save_stex(const Ref<Image> &p_image, const String &p_to_path, int p_compress_mode, float p_lossy_quality, Image::CompressMode p_vram_compression, bool p_mipmaps, int p_texture_flags, bool p_streamable, bool p_detect_3d, bool p_detect_srgb, bool p_force_rgbe, bool p_detect_normal, bool p_force_normal) {
288:                            PoolVector<uint8_t> data = Image::lossy_packer(image, p_lossy_quality);
311:                            image->compress(p_vram_compression, csource, p_lossy_quality);
350:    float lossy = p_options["compress/lossy_quality"];

doc/classes/ImageTexture.xml
78:             <member name="lossy_quality" type="float" setter="set_lossy_storage_quality" getter="get_lossy_storage_quality">

doc/classes/Image.xml
92:                     <argument index="2" name="lossy_quality" type="float">

[groud]

OK I though it was bound. It's not compatibility breaking then :).

If the p_lossy_quality is used everywhere else, I agree thus change might be out of the scope of this PR. Still I find weird having an API where you can provide any float value between 0 and 0.75 and which does not changes the output result by any mean. That is why I suggested moving the float value outside of the squish API and do the conversion outside.

[elasota]

I think there are simplicity benefits to having a single "quality" parameter, but possibly also drawbacks of the same setting not making sense everywhere. (And range fit as default isn't something I'm thrilled with anyway - The quality is poor.)

[groud]

I think there are simplicity benefits to having a single "quality" parameter, but possibly also drawbacks of the same setting not making sense everywhere.

What I meant is that, indeed, the single parameter in the Image::compress function totally makes sense. But in the image_compress_squish function, it does not.

Anyway, this is a cosmetic change. As the PR does not involves any compatibility issue it can be merged as is. ;)

[akien-mga]

This needs a rebase after the BPTC merge.

[elasota]

Rebase done.

[akien-mga]

Thanks!