Fix data format storage method for PortableCompressedTexture2D

[nklbdev]

This is a fix after previous merged PR: #77712

It fixes backwards compatibility after this PR:
The main idea is:

• Remove the problem enum value
• Encode data format as data_format + 1 to distinguish it from files, generated by previous versions of Godot.
• Subtract 1 while data_format decoding if data_format_code is greater than 0

Why i cannot just place the DATA_FORMAT_UNDEFINED in back of the enum or make it's value so big:
The DATA_FORMAT_UNDEFINED is the format that all the old images are in. That's why I can't put it at the end of the enum values list. It must be equal 0. Therefore, it conflicts with the DATA_FORMAT_IMAGE. That's why he had to be removed.

Alternative way:
Store the boolean “new data format” sign in a separate byte. And store data_format as is (without adding 1)

[AThousandShips]

When you say old data? Is this data from before #77712? Where was that handling added?

[nklbdev]

@AThousandShips, yes, I mean the resources created by the version of Godot before #77712. When the user downloads the new version of the engine and tries to open his old project, he should succeed.

The image data of that (old) version contains 0 at this address.
In the new version, 0 would indicate DATA_FORMAT_IMAGE. But we can’t do that. That's why I added 1 there.

An alternative is to put the format version mark (boolean or integer) in a separate byte.
The problem is that there are not many free bytes where this mark could be stored.
If you know such an address, tell it to me.

[AThousandShips]

You can still store a separate unknown, just as long as you keep it separate from the valid formats, that was the issue, as well as that the old values elsewhere were offset incorrectly

[nklbdev]

I'm afraid I didn't make myself clear enough.

The new version of the code will not store value DATА_FORMAT_UNDEFINED ever. This value was needed so that when reading the resource bytes of the old version, we could understand that for this image we do not need to use the loader_func, as in the old version - this is for backward compatibility.

Therefore, this enum value must be removed. Instead, we need to store the mark of the new data format in some other way.

[AThousandShips]

Then that works fine, but no need to encode the old format assuming the value is always 0 in those bytes for old encoded formats, assuming it's only used when loading old files, loaded data shouldn't have it when in memory, the issue was how other files were mutated

[nklbdev]

If we will read this byte (0) from old resource (created by previous version of Godot) with this enum contents, we will get DATA_FORMAT_IMAGE. This is wrong, because this will select loader_func == Image::_png_mem_unpacker_func:

enum DataFormat {
        DATA_FORMAT_IMAGE,           // 0 <- this value will be read incorrectly instead of DATA_FORMAT_UNDEFINED
	DATA_FORMAT_PNG,             // 1
	DATA_FORMAT_WEBP,            // 2
	DATA_FORMAT_BASIS_UNIVERSAL, // 3
	DATA_FORMAT_UNDEFINED,       // 4 <- this value at this place is useless
};

But this order of values is breaking compatibility too

enum DataFormat {
        DATA_FORMAT_UNDEFINED,       // 0
	DATA_FORMAT_IMAGE,           // 1
	DATA_FORMAT_PNG,             // 2
	DATA_FORMAT_WEBP,            // 3
	DATA_FORMAT_BASIS_UNIVERSAL, // 4
};

Therefore, the DATA_FORMAT_UNDEFINED value will have to be removed.

[AThousandShips]

Yes, that's why offsetting it solves it, that's what I said about the value being offset, I agreed that the value has to be removed from the enumeration, but you don't need to indicate that it's new or old format except by it either having or not having a specified format, the problem isn't there but that the compressed texture data is broken, not the portable one

[nklbdev]

OK. How can we differentiate between the old version (which requires loader_function = nullptr) and the new version, where the user explicitly specified the data type - DATA_FORMAT_IMAGE?
This format requires a loader_func = Image::_png_mem_unpacker_func. <- P.S. no, not this func

Both of these options are encoded exactly the same (value 0) 😢

[AThousandShips]

No, because you offset it by 1 🙂, just like your code here, you don't need to store it being in the old format any other way:

Then that works fine, but no need to encode the old format assuming the value is always 0 in those bytes for old encoded formats

The original PR didn't break PortableCompressedTexture2D really, it broke CompressedTexture2D/3D which depended on the image values

[nklbdev]

Then I probably don’t understand what exactly broken.
Do you have any ideas on how to fix this?

I don't want problems for the development of the engine. Therefore, you can always roll back my previous pull request. I will not be against.

[AThousandShips]

Just do this way that you already are, using 0 to mean "this is an old file, update it" and the rest to be explicit, the problem is that CompressedTexture uses the same DataFormat, so files saved before your PR stored 0 to mean DATA_FORMAT_IMAGE, but now that one isn't valid, and so on, that's why changing enums is dangerous 🙂

The problem is here, and other places in CompressedTexture, which uses this format:

godot/scene/resources/compressed_texture.cpp

Lines 304 to 337 in 89cc635

	Image::Format format = Image::Format(f->get_32());
	if (data_format == DATA_FORMAT_PNG || data_format == DATA_FORMAT_WEBP) {
	//look for a PNG or WebP file inside
	int sw = w;
	int sh = h;
	//mipmaps need to be read independently, they will be later combined
	Vector<Ref<Image>> mipmap_images;
	uint64_t total_size = 0;
	bool first = true;
	for (uint32_t i = 0; i < mipmaps + 1; i++) {
	uint32_t size = f->get_32();
	if (p_size_limit > 0 && i < (mipmaps - 1) && (sw > p_size_limit || sh > p_size_limit)) {
	//can't load this due to size limit
	sw = MAX(sw >> 1, 1);
	sh = MAX(sh >> 1, 1);
	f->seek(f->get_position() + size);
	continue;
	}
	Vector<uint8_t> pv;
	pv.resize(size);
	{
	uint8_t *wr = pv.ptrw();
	f->get_buffer(wr, size);
	}
	Ref<Image> img;
	if (data_format == DATA_FORMAT_PNG && Image::png_unpacker) {

So now it will read a 2, which used to mean DATA_FORMAT_PNG, but now means DATA_FORMAT_IMAGE and fail to load

[nklbdev]

@AThousandShips, tell me please, does the solution I present in the current PR solve this problem? It seems that it should.

[AThousandShips]

I think so yes 🙂 sorry said so several times I'm sorry if I'm unclear, others will have to review it but to my eyes this should solve it, the issue was the added entry in the enum really, the other data is unchanged from your previous PR, it just has a manual addition and subtraction to make it work the same way

[nklbdev]

Then thank you!
I apologize for my poor understanding: I don't speak English very well and often use Google translator.

[AThousandShips]

No problem! I understand 🙂

[AThousandShips]

I'll come back to look later today but I realized that you should probably add a new enum to PortableCompressedTexture to handle this, to prevent future problems, as you might not know that the one in CompressedTexture is used in an unrelated class, just copy the one that's there, with the undefined one intact, and leave the code you originally added as it was, but restore the code in CompressedTexture

It's good generally to avoid data from unrelated types, "cousins" if that makes sense, it risks errors 🙂

(Please don't hesitate to ask me to say something in a different way to help understanding 🙂)

diff --git a/scene/resources/compressed_texture.h b/scene/resources/compressed_texture.h
index 932109617f..5297d79cfe 100644
--- a/scene/resources/compressed_texture.h
+++ b/scene/resources/compressed_texture.h
@@ -40,7 +40,6 @@ class CompressedTexture2D : public Texture2D {

 public:
        enum DataFormat {
-               DATA_FORMAT_UNDEFINED,
                DATA_FORMAT_IMAGE,
                DATA_FORMAT_PNG,
                DATA_FORMAT_WEBP,
diff --git a/scene/resources/portable_compressed_texture.cpp b/scene/resources/portable_compressed_texture.cpp
index a67f32f67e..b926e3c690 100644
--- a/scene/resources/portable_compressed_texture.cpp
+++ b/scene/resources/portable_compressed_texture.cpp
@@ -44,7 +44,7 @@ void PortableCompressedTexture2D::_set_data(const Vector<uint8_t> &p_data) {
        uint32_t data_size = p_data.size();
        ERR_FAIL_COND(data_size < 20);
        compression_mode = CompressionMode(decode_uint16(data));
-       CompressedTexture2D::DataFormat data_format = CompressedTexture2D::DataFormat(decode_uint16(data + 2));
+       DataFormat data_format = DataFormat(decode_uint16(data + 2));
        format = Image::Format(decode_uint32(data + 4));
        uint32_t mipmap_count = decode_uint32(data + 8);
        size.width = decode_uint32(data + 12);
@@ -60,11 +60,11 @@ void PortableCompressedTexture2D::_set_data(const Vector<uint8_t> &p_data) {
                case COMPRESSION_MODE_LOSSLESS:
                case COMPRESSION_MODE_LOSSY: {
                        ImageMemLoadFunc loader_func;
-                       if (data_format == CompressedTexture2D::DATA_FORMAT_UNDEFINED) {
+                       if (data_format == DATA_FORMAT_UNDEFINED) {
                                loader_func = nullptr;
-                       } else if (data_format == CompressedTexture2D::DATA_FORMAT_PNG) {
+                       } else if (data_format == DATA_FORMAT_PNG) {
                                loader_func = Image::_png_mem_unpacker_func;
-                       } else if (data_format == CompressedTexture2D::DATA_FORMAT_WEBP) {
+                       } else if (data_format == DATA_FORMAT_WEBP) {
                                loader_func = Image::_webp_mem_loader_func;
                        } else {
                                ERR_FAIL();
@@ -139,7 +139,7 @@ void PortableCompressedTexture2D::create_from_image(const Ref<Image> &p_image, C

        buffer.resize(20);
        encode_uint16(p_compression_mode, buffer.ptrw());
-       encode_uint16(CompressedTexture2D::DATA_FORMAT_UNDEFINED, buffer.ptrw() + 2);
+       encode_uint16(DATA_FORMAT_UNDEFINED, buffer.ptrw() + 2);
        encode_uint32(p_image->get_format(), buffer.ptrw() + 4);
        encode_uint32(p_image->get_mipmap_count() + 1, buffer.ptrw() + 8);
        encode_uint32(p_image->get_width(), buffer.ptrw() + 12);
@@ -155,14 +155,14 @@ void PortableCompressedTexture2D::create_from_image(const Ref<Image> &p_image, C
                                Vector<uint8_t> data;
                                if (p_compression_mode == COMPRESSION_MODE_LOSSY) {
                                        data = Image::webp_lossy_packer(p_image->get_image_from_mipmap(i), p_lossy_quality);
-                                       encode_uint16(CompressedTexture2D::DATA_FORMAT_WEBP, buffer.ptrw() + 2);
+                                       encode_uint16(DATA_FORMAT_WEBP, buffer.ptrw() + 2);
                                } else {
                                        if (use_webp) {
                                                data = Image::webp_lossless_packer(p_image->get_image_from_mipmap(i));
-                                               encode_uint16(CompressedTexture2D::DATA_FORMAT_WEBP, buffer.ptrw() + 2);
+                                               encode_uint16(DATA_FORMAT_WEBP, buffer.ptrw() + 2);
                                        } else {
                                                data = Image::png_packer(p_image->get_image_from_mipmap(i));
-                                               encode_uint16(CompressedTexture2D::DATA_FORMAT_PNG, buffer.ptrw() + 2);
+                                               encode_uint16(DATA_FORMAT_PNG, buffer.ptrw() + 2);
                                        }
                                }
                                int data_len = data.size();
@@ -172,7 +172,7 @@ void PortableCompressedTexture2D::create_from_image(const Ref<Image> &p_image, C
                        }
                } break;
                case COMPRESSION_MODE_BASIS_UNIVERSAL: {
...skipping...
-                       } else if (data_format == CompressedTexture2D::DATA_FORMAT_WEBP) {
+                       } else if (data_format == DATA_FORMAT_WEBP) {
                                loader_func = Image::_webp_mem_loader_func;
                        } else {
                                ERR_FAIL();
@@ -139,7 +139,7 @@ void PortableCompressedTexture2D::create_from_image(const Ref<Image> &p_image, C

        buffer.resize(20);
        encode_uint16(p_compression_mode, buffer.ptrw());
-       encode_uint16(CompressedTexture2D::DATA_FORMAT_UNDEFINED, buffer.ptrw() + 2);
+       encode_uint16(DATA_FORMAT_UNDEFINED, buffer.ptrw() + 2);
        encode_uint32(p_image->get_format(), buffer.ptrw() + 4);
        encode_uint32(p_image->get_mipmap_count() + 1, buffer.ptrw() + 8);
        encode_uint32(p_image->get_width(), buffer.ptrw() + 12);
@@ -155,14 +155,14 @@ void PortableCompressedTexture2D::create_from_image(const Ref<Image> &p_image, C
                                Vector<uint8_t> data;
                                if (p_compression_mode == COMPRESSION_MODE_LOSSY) {
                                        data = Image::webp_lossy_packer(p_image->get_image_from_mipmap(i), p_lossy_quality);
-                                       encode_uint16(CompressedTexture2D::DATA_FORMAT_WEBP, buffer.ptrw() + 2);
+                                       encode_uint16(DATA_FORMAT_WEBP, buffer.ptrw() + 2);
                                } else {
                                        if (use_webp) {
                                                data = Image::webp_lossless_packer(p_image->get_image_from_mipmap(i));
-                                               encode_uint16(CompressedTexture2D::DATA_FORMAT_WEBP, buffer.ptrw() + 2);
+                                               encode_uint16(DATA_FORMAT_WEBP, buffer.ptrw() + 2);
                                        } else {
                                                data = Image::png_packer(p_image->get_image_from_mipmap(i));
-                                               encode_uint16(CompressedTexture2D::DATA_FORMAT_PNG, buffer.ptrw() + 2);
+                                               encode_uint16(DATA_FORMAT_PNG, buffer.ptrw() + 2);
                                        }
                                }
                                int data_len = data.size();
@@ -172,7 +172,7 @@ void PortableCompressedTexture2D::create_from_image(const Ref<Image> &p_image, C
                        }
                } break;
                case COMPRESSION_MODE_BASIS_UNIVERSAL: {
-                       encode_uint16(CompressedTexture2D::DATA_FORMAT_BASIS_UNIVERSAL, buffer.ptrw() + 2);
+                       encode_uint16(DATA_FORMAT_BASIS_UNIVERSAL, buffer.ptrw() + 2);
                        Image::UsedChannels uc = p_image->detect_used_channels(p_normal_map ? Image::COMPRESS_SOURCE_NORMAL : Image::COMPRESS_SOURCE_GENERIC);
                        Vector<uint8_t> budata = Image::basis_universal_packer(p_image, uc);
                        buffer.append_array(budata);
@@ -181,7 +181,7 @@ void PortableCompressedTexture2D::create_from_image(const Ref<Image> &p_image, C
                case COMPRESSION_MODE_S3TC:
                case COMPRESSION_MODE_ETC2:
                case COMPRESSION_MODE_BPTC: {
-                       encode_uint16(CompressedTexture2D::DATA_FORMAT_IMAGE, buffer.ptrw() + 2);
+                       encode_uint16(DATA_FORMAT_IMAGE, buffer.ptrw() + 2);
                        Ref<Image> copy = p_image->duplicate();
                        switch (p_compression_mode) {
                                case COMPRESSION_MODE_S3TC:
diff --git a/scene/resources/portable_compressed_texture.h b/scene/resources/portable_compressed_texture.h
index 86d80e39f7..3103c2daba 100644
--- a/scene/resources/portable_compressed_texture.h
+++ b/scene/resources/portable_compressed_texture.h
@@ -39,6 +39,14 @@ class PortableCompressedTexture2D : public Texture2D {
        GDCLASS(PortableCompressedTexture2D, Texture2D);

 public:
+       enum DataFormat {
+               DATA_FORMAT_UNDEFINED,
+               DATA_FORMAT_IMAGE,
+               DATA_FORMAT_PNG,
+               DATA_FORMAT_WEBP,
+               DATA_FORMAT_BASIS_UNIVERSAL,
+       };
+
        enum CompressionMode {
                COMPRESSION_MODE_LOSSLESS,
                COMPRESSION_MODE_LOSSY,

This way the code works correctly in both cases 🙂, and you don't need to do a lot of additions and subtractions

Edit: Apply this on top of master, not on top of your changes

Edit: Tested this and it fixes the CompressedTexture errors reported, but haven't tested PortableCompressedTexture2D

[AThousandShips]

You didn't remove it from CompressedTexture2D, please fix that 🙂

[nklbdev]

@AThousandShips, thank you, you are a genius!
I replaced my commit with your suggestions.
I was too timid. I was afraid to influence too much a codebase that is not mine.
And it didn’t even occur to me that I could introduce a new enumeration into the code!

[AThousandShips]

This solves the regression, haven't tested the portable format changes, but they are unchanged from the code in the previous PR

[akien-mga]

Thanks!