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.

Sign up for GitHub

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

Jump to bottom

Fix lossless formats in PortableCompressedTexture2D #77712

Merged
merged 1 commit into from Jan 5, 2024
Merged

Fix lossless formats in PortableCompressedTexture2D #77712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions core/io/image.cpp
View file
Open in desktop
Expand Up @@ -3013,6 +3013,7 @@ void Image::fill_rect(const Rect2i &p_rect, const Color &p_color) {
}

ImageMemLoadFunc Image::_png_mem_loader_func = nullptr;
ImageMemLoadFunc Image::_png_mem_unpacker_func = nullptr;
ImageMemLoadFunc Image::_jpg_mem_loader_func = nullptr;
ImageMemLoadFunc Image::_webp_mem_loader_func = nullptr;
ImageMemLoadFunc Image::_tga_mem_loader_func = nullptr;
Expand Down
1 change: 1 addition & 0 deletions core/io/image.h
View file
Open in desktop
Expand Up @@ -145,6 +145,7 @@ class Image : public Resource {
};

static ImageMemLoadFunc _png_mem_loader_func;
static ImageMemLoadFunc _png_mem_unpacker_func;
static ImageMemLoadFunc _jpg_mem_loader_func;
static ImageMemLoadFunc _webp_mem_loader_func;
static ImageMemLoadFunc _tga_mem_loader_func;
Expand Down
13 changes: 8 additions & 5 deletions drivers/png/image_loader_png.cpp
View file
Open in desktop
Expand Up @@ -66,12 +66,14 @@ Ref<Image> ImageLoaderPNG::load_mem_png(const uint8_t *p_png, int p_size) {
return img;
}

Ref<Image> ImageLoaderPNG::unpack_mem_png(const uint8_t *p_png, int p_size) {
ERR_FAIL_COND_V(p_size < 4, Ref<Image>());
ERR_FAIL_COND_V(p_png[0] != 'P' || p_png[1] != 'N' || p_png[2] != 'G' || p_png[3] != ' ', Ref<Image>());
return load_mem_png(&p_png[4], p_size - 4);
}

Ref<Image> ImageLoaderPNG::lossless_unpack_png(const Vector<uint8_t> &p_data) {
const int len = p_data.size();
ERR_FAIL_COND_V(len < 4, Ref<Image>());
const uint8_t *r = p_data.ptr();
ERR_FAIL_COND_V(r[0] != 'P' || r[1] != 'N' || r[2] != 'G' || r[3] != ' ', Ref<Image>());
return load_mem_png(&r[4], len - 4);
return unpack_mem_png(p_data.ptr(), p_data.size());
}

Vector<uint8_t> ImageLoaderPNG::lossless_pack_png(const Ref<Image> &p_image) {
Expand Down Expand Up @@ -99,6 +101,7 @@ Vector<uint8_t> ImageLoaderPNG::lossless_pack_png(const Ref<Image> &p_image) {

ImageLoaderPNG::ImageLoaderPNG() {
Image::_png_mem_loader_func = load_mem_png;
Image::_png_mem_unpacker_func = unpack_mem_png;
Image::png_unpacker = lossless_unpack_png;
Image::png_packer = lossless_pack_png;
}
1 change: 1 addition & 0 deletions drivers/png/image_loader_png.h
View file
Open in desktop
Expand Up @@ -37,6 +37,7 @@ class ImageLoaderPNG : public ImageFormatLoader {
private:
static Vector<uint8_t> lossless_pack_png(const Ref<Image> &p_image);
static Ref<Image> lossless_unpack_png(const Vector<uint8_t> &p_data);
static Ref<Image> unpack_mem_png(const uint8_t *p_png, int p_size);
static Ref<Image> load_mem_png(const uint8_t *p_png, int p_size);

public:
Expand Down
3 changes: 2 additions & 1 deletion editor/plugins/texture_editor_plugin.cpp
View file
Open in desktop
Expand Up @@ -38,6 +38,7 @@
#include "scene/resources/atlas_texture.h"
#include "scene/resources/compressed_texture.h"
#include "scene/resources/image_texture.h"
#include "scene/resources/portable_compressed_texture.h"

TextureRect *TexturePreview::get_texture_display() {
return texture_display;
Expand Down Expand Up @@ -158,7 +159,7 @@ TexturePreview::TexturePreview(Ref<Texture2D> p_texture, bool p_show_metadata) {
}

bool EditorInspectorPluginTexture::can_handle(Object *p_object) {
return Object::cast_to<ImageTexture>(p_object) != nullptr || Object::cast_to<AtlasTexture>(p_object) != nullptr || Object::cast_to<CompressedTexture2D>(p_object) != nullptr || Object::cast_to<AnimatedTexture>(p_object) != nullptr || Object::cast_to<Image>(p_object) != nullptr;
return Object::cast_to<ImageTexture>(p_object) != nullptr || Object::cast_to<AtlasTexture>(p_object) != nullptr || Object::cast_to<CompressedTexture2D>(p_object) != nullptr || Object::cast_to<PortableCompressedTexture2D>(p_object) != nullptr || Object::cast_to<AnimatedTexture>(p_object) != nullptr || Object::cast_to<Image>(p_object) != nullptr;
}

void EditorInspectorPluginTexture::parse_begin(Object *p_object) {
Expand Down
1 change: 1 addition & 0 deletions scene/resources/compressed_texture.h
View file
Open in desktop
Expand Up @@ -40,6 +40,7 @@ class CompressedTexture2D : public Texture2D {

public:
enum DataFormat {
DATA_FORMAT_UNDEFINED,
Copy link
Member

Choose a reason for hiding this comment

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

This apparently broke compatibility for existing compressed textures, as it's reordering all enum values. New enum values should go to the end of the enum to avoid breaking compatibility.

(Haven't tested yet to confirm but I got multiple reports of a compat breakage on compressed textures introduced in today's merges.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check it now

Copy link
Contributor Author

@nklbdev nklbdev Jan 5, 2024 •
edited

Choose a reason for hiding this comment

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

@akien-mga, how can I push a fix?
I made it in the same branch of my fork and now I’m checking how it works.
nklbdev@73d3bcd

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

Copy link
Member

@akien-mga akien-mga Jan 5, 2024 •
edited

Choose a reason for hiding this comment

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

You would have to make a new PR, you can reuse the same branch if you want but it needs to be rebased on upstream master. I'd suggest using a new branch with a name that matches the fix being made.

As for the fix, this seems simpler, no?

diff --git a/scene/resources/compressed_texture.h b/scene/resources/compressed_texture.h
index 932109617f..bb40ee8c2b 100644
--- a/scene/resources/compressed_texture.h
+++ b/scene/resources/compressed_texture.h
@@ -40,11 +40,11 @@ class CompressedTexture2D : public Texture2D {
 
 public:
 	enum DataFormat {
-		DATA_FORMAT_UNDEFINED,
 		DATA_FORMAT_IMAGE,
 		DATA_FORMAT_PNG,
 		DATA_FORMAT_WEBP,
 		DATA_FORMAT_BASIS_UNIVERSAL,
+		DATA_FORMAT_UNDEFINED,
 	};
 
 	enum {

If new format types are added in the future, they would also be added at the end. So UNDEFINED would be in the middle but it's not a big deal I think?

Alternatively we can give it a high enough value so that there's room for more formats:

diff --git a/scene/resources/compressed_texture.h b/scene/resources/compressed_texture.h
index 932109617f..eaa7414f1c 100644
--- a/scene/resources/compressed_texture.h
+++ b/scene/resources/compressed_texture.h
@@ -40,11 +40,12 @@ class CompressedTexture2D : public Texture2D {
 
 public:
 	enum DataFormat {
-		DATA_FORMAT_UNDEFINED,
 		DATA_FORMAT_IMAGE,
 		DATA_FORMAT_PNG,
 		DATA_FORMAT_WEBP,
 		DATA_FORMAT_BASIS_UNIVERSAL,
+
+		DATA_FORMAT_UNDEFINED = 100,
 	};
 
 	enum {

or something like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the best way to go about it (since this PR has been merged) is to make any fixes to a separate branch and PR them separately

Copy link
Contributor Author

@nklbdev nklbdev Jan 5, 2024 •
edited

Choose a reason for hiding this comment

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

Okay, I'll make a new pull request now.

The problem is that 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a new pull-request: #86835

Copy link
Contributor Author

@nklbdev nklbdev Jan 5, 2024 •
edited

Choose a reason for hiding this comment

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

I'm very sorry that this happened. I want to do everything to minimize the consequences. Perhaps even rollback this pull request if necessary.

DATA_FORMAT_IMAGE,
DATA_FORMAT_PNG,
DATA_FORMAT_WEBP,
Expand Down
37 changes: 33 additions & 4 deletions scene/resources/portable_compressed_texture.cpp
View file
Open in desktop
Expand Up @@ -30,8 +30,10 @@

#include "portable_compressed_texture.h"

#include "core/config/project_settings.h"
#include "core/io/marshalls.h"
#include "scene/resources/bit_map.h"
#include "scene/resources/compressed_texture.h"

void PortableCompressedTexture2D::_set_data(const Vector<uint8_t> &p_data) {
if (p_data.size() == 0) {
Expand All @@ -41,7 +43,8 @@ void PortableCompressedTexture2D::_set_data(const Vector<uint8_t> &p_data) {
const uint8_t *data = p_data.ptr();
uint32_t data_size = p_data.size();
ERR_FAIL_COND(data_size < 20);
compression_mode = CompressionMode(decode_uint32(data + 0));
compression_mode = CompressionMode(decode_uint16(data));
CompressedTexture2D::DataFormat data_format = CompressedTexture2D::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);
Expand All @@ -56,6 +59,16 @@ void PortableCompressedTexture2D::_set_data(const Vector<uint8_t> &p_data) {
switch (compression_mode) {
case COMPRESSION_MODE_LOSSLESS:
case COMPRESSION_MODE_LOSSY: {
ImageMemLoadFunc loader_func;
if (data_format == CompressedTexture2D::DATA_FORMAT_UNDEFINED) {
loader_func = nullptr;
} else if (data_format == CompressedTexture2D::DATA_FORMAT_PNG) {
loader_func = Image::_png_mem_unpacker_func;
} else if (data_format == CompressedTexture2D::DATA_FORMAT_WEBP) {
loader_func = Image::_webp_mem_loader_func;
} else {
ERR_FAIL();
}
Vector<uint8_t> image_data;

ERR_FAIL_COND(data_size < 4);
Expand All @@ -64,7 +77,9 @@ void PortableCompressedTexture2D::_set_data(const Vector<uint8_t> &p_data) {
data += 4;
data_size -= 4;
ERR_FAIL_COND(mipsize < data_size);
Ref<Image> img = memnew(Image(data, data_size));
Ref<Image> img = loader_func == nullptr
? memnew(Image(data, data_size))
: Ref<Image>(loader_func(data, data_size));
ERR_FAIL_COND(img->is_empty());
if (img->get_format() != format) { // May happen due to webp/png in the tiny mipmaps.
img->convert(format);
Expand Down Expand Up @@ -99,6 +114,7 @@ void PortableCompressedTexture2D::_set_data(const Vector<uint8_t> &p_data) {
}

image_stored = true;
size_override = size;
lyuma marked this conversation as resolved.
Show resolved Hide resolved
RenderingServer::get_singleton()->texture_set_size_override(texture, size_override.width, size_override.height);
alpha_cache.unref();

Expand All @@ -122,7 +138,8 @@ void PortableCompressedTexture2D::create_from_image(const Ref<Image> &p_image, C
Vector<uint8_t> buffer;

buffer.resize(20);
encode_uint32(p_compression_mode, buffer.ptrw());
encode_uint16(p_compression_mode, buffer.ptrw());
lyuma marked this conversation as resolved.
Show resolved Hide resolved
encode_uint16(CompressedTexture2D::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);
Expand All @@ -131,12 +148,22 @@ void PortableCompressedTexture2D::create_from_image(const Ref<Image> &p_image, C
switch (p_compression_mode) {
case COMPRESSION_MODE_LOSSLESS:
case COMPRESSION_MODE_LOSSY: {
bool lossless_force_png = GLOBAL_GET("rendering/textures/lossless_compression/force_png") ||
!Image::_webp_mem_loader_func; // WebP module disabled.
bool use_webp = !lossless_force_png && p_image->get_width() <= 16383 && p_image->get_height() <= 16383; // WebP has a size limit.
for (int i = 0; i < p_image->get_mipmap_count() + 1; i++) {
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);
} else {
data = Image::webp_lossless_packer(p_image->get_image_from_mipmap(i));
if (use_webp) {
data = Image::webp_lossless_packer(p_image->get_image_from_mipmap(i));
encode_uint16(CompressedTexture2D::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);
}
}
int data_len = data.size();
buffer.resize(buffer.size() + 4);
Expand All @@ -145,6 +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);
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);
Expand All @@ -153,6 +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);
Ref<Image> copy = p_image->duplicate();
switch (p_compression_mode) {
case COMPRESSION_MODE_S3TC:
Expand Down