Fix lossless formats in PortableCompressedTexture2D #77712

nklbdev · 2023-05-31T20:28:47Z

This is a right copy of my previous wrong pull-request

Sorry for my bad English(((

PortableCompressedTexture2D have a problem with creating from PNG images and some other little problems.
I want to use this resource type as the way to import animations from Aseprite in my Aseprite importers plugin bundle

For now I am creating one PNG image near every *.aseprite-file that imported. Then call some methods to update filesystem and import the image. Then plugin is continuing it's work and embedding new image in an imported resource.

PortableCompressedTexture2D is a beautiful way to embed PNG data silently in SpriteFrames or other resources. But it is not usable now. Method create_from_image cannot load PNG image and shows errors in console:

WARNING: Not a PNG file
     at: PNGDriverCommon::check_error (drivers\png\png_driver_common.cpp:56)
ERROR: Condition "!success" is true. Returning: ERR_FILE_CORRUPT
   at: PNGDriverCommon::png_to_image (drivers\png\png_driver_common.cpp:69)
ERROR: Condition "err" is true. Returning: Ref<Image>()
   at: ImageLoaderPNG::load_mem_png (drivers\png\image_loader_png.cpp:64)
ERROR: It's not a reference to a valid Image object.
   at: (.\core/io/image.h:426)
ERROR: Condition "err" is true. Returning: Ref<Image>()
   at: _jpegd_mem_loader_func (modules\jpg\image_loader_jpegd.cpp:131)
ERROR: It's not a reference to a valid Image object.
   at: (.\core/io/image.h:426)
ERROR: Condition "p_width <= 0 || p_width > 16384" is true.
   at: GLES3::TextureStorage::texture_set_size_override (drivers\gles3\storage\texture_storage.cpp:1053)

This is because PNG image have not only PNG prefix ‰PNG, but also Godot's own PNG prefix

I understand that this solution is not ideal.
Theoretically, there could be such a sequence of bytes PNG in this memory location for some other reason. Not because Godot added it there. I don't know how to be completely sure of this. If you know such a criteria, please tell me and I will correct the code.

Result: It works! Now I use PortableCompressedTexture2D and save it directly in imported resource - SpriteFrames or PackedScene!

aseprite_importers_showcase.mp4

scene/resources/texture.cpp

nklbdev · 2023-06-02T22:03:35Z

I'll need add some changes to save PortableCompressedTexture2D to *.res-file

I was finally set up my IDE to debug Godot C++ code after scons build. I plan to finish this pull request in a day or two.

nklbdev · 2023-06-03T23:03:32Z

Done.

I've added an image file format detection to avoid trying to load unsuitable formats, red console messages, and exceptions.
This code has been thoroughly tested and conforms to the specifications of these file formats.

It's strange, I couldn't find the reason, but without this, Godot can't save the PortableCompressedTexture2D to the .godot/imported folder as a separate resource file (*.res or *.pctex).

P.S.: And I fixed author name and email in my commits))

nklbdev · 2023-06-04T15:24:36Z

I found an error in servers/rendering_server.cpp on line 442 and many other lines:
An integer value in the range 0 to 65535 is casted to int16_t, which will accommodate the range from -32767 to 32767. This causes the application to crash.

Now I'll try to fix it.

core/io/image.cpp

nklbdev · 2023-06-04T21:03:29Z

Done

fire · 2023-09-21T22:22:48Z

Commentary from an impromptu 3d imports discussion:

Concerns that this needs to keep backwards compatibility
Feature freeze is upcoming.
Improves the situation

scene/resources/compressed_texture.h

fire · 2023-12-12T21:24:07Z

Hiya, sorry this got lost. I'll see what I can do to nudge this.

nklbdev · 2023-12-12T21:39:20Z

Hiya, sorry this got lost. I'll see what I can do to nudge this.

Thank you!
If anyone has any ideas on how to implement this better, I will be very happy.

lyuma

I think it looks great. The changes made in September address the original concern about backwards compatibility breakage ("2 bytes for compression mode and 2 bytes for data type") so I see no problems with it. Just one nitpick.

Sorry we didn't get to this before the 4.2 feature freeze, but now that the 4.3 merge window is open, I see no problem with merging this.

scene/resources/portable_compressed_texture.cpp

nklbdev · 2023-12-24T02:02:44Z

@lyuma, wonderful! Now I will rebase and make your edits. I should warn you that I am not a C++ professional and usually work with C#. Therefore, be attentive to the code that I made. I may not know some subtleties.

lyuma · 2023-12-24T02:36:00Z

I also removed breaks compat label since this does not break backwards compatibility. I don't think any of the concerns raised prior to september are still valid. Do you think you can double check the past review comments and click Resolve Conversation on the discussions which are no longer applicable?

nklbdev · 2023-12-24T02:53:35Z

@lyuma, I checked and resolved all the discussions that I could resolve.

Keep in mind:

Textures created by a previous version of the engine will be able to be opened by this version.
But textures created by this version of the engine will no longer be able to open normally with the previous version.

scene/resources/portable_compressed_texture.cpp

akien-mga · 2024-01-04T18:18:50Z

Looks good! Could you squash the commits? See PR workflow for instructions.

nklbdev · 2024-01-04T19:06:11Z

I was a little mistaken. Wait a little, now I'll fix everything.

AThousandShips · 2024-01-04T19:07:42Z

You need to push with git push -f to overwrite, after you've squashed

Update scene/resources/portable_compressed_texture.cpp Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>

nklbdev · 2024-01-04T19:15:00Z

Yes, I just rushed a little and GitExtensions executed a command that I didn't want. Now everything is fine: the branch is inherited from the most recent commit in the master branch.
I usually use the recommended parameter --force-with-lease

akien-mga · 2024-01-05T11:50:13Z

Thanks!

nklbdev · 2024-01-05T12:10:47Z

Thanks, too! Happy New Year!

akien-mga · 2024-01-05T14:06:00Z

scene/resources/compressed_texture.h

@@ -40,6 +40,7 @@ class CompressedTexture2D : public Texture2D {

 public:
 	enum DataFormat {
+		DATA_FORMAT_UNDEFINED,


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.)

I'll check it now

@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

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.

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

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.

I created a new pull-request: #86835

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

nklbdev commented May 31, 2023

View reviewed changes

scene/resources/texture.cpp Outdated Show resolved Hide resolved

nklbdev commented May 31, 2023

View reviewed changes

scene/resources/texture.cpp Outdated Show resolved Hide resolved

nklbdev commented May 31, 2023

View reviewed changes

scene/resources/texture.cpp Outdated Show resolved Hide resolved

AThousandShips reviewed May 31, 2023

View reviewed changes

scene/resources/texture.cpp Outdated Show resolved Hide resolved

nklbdev force-pushed the Fix_lossless_formats_in_PortableCompressedTexture2D branch from 69c27ff to f64a24a Compare May 31, 2023 20:52

Chaosus added bug topic:core labels Jun 1, 2023

Chaosus added this to the 4.1 milestone Jun 1, 2023

nklbdev force-pushed the Fix_lossless_formats_in_PortableCompressedTexture2D branch 3 times, most recently from cb77602 to c25a27d Compare June 1, 2023 19:37

nklbdev requested a review from a team as a code owner June 3, 2023 22:58

nklbdev force-pushed the Fix_lossless_formats_in_PortableCompressedTexture2D branch 6 times, most recently from efdfaf8 to 803256d Compare June 4, 2023 01:40

nklbdev requested a review from a team as a code owner June 4, 2023 15:44

nklbdev force-pushed the Fix_lossless_formats_in_PortableCompressedTexture2D branch 5 times, most recently from 9562828 to a7b16b0 Compare June 4, 2023 20:07

nklbdev commented Jun 4, 2023

View reviewed changes

core/io/image.cpp Outdated Show resolved Hide resolved

nklbdev force-pushed the Fix_lossless_formats_in_PortableCompressedTexture2D branch from a7b16b0 to 5f345da Compare June 4, 2023 20:13

AThousandShips reviewed Sep 22, 2023

View reviewed changes

scene/resources/compressed_texture.h Outdated Show resolved Hide resolved

nklbdev force-pushed the Fix_lossless_formats_in_PortableCompressedTexture2D branch from 9aff56b to 3f8c5f4 Compare September 22, 2023 16:31

nklbdev force-pushed the Fix_lossless_formats_in_PortableCompressedTexture2D branch from bd5d990 to 89e6b35 Compare October 10, 2023 08:33

lyuma approved these changes Dec 24, 2023

View reviewed changes

scene/resources/portable_compressed_texture.cpp Show resolved Hide resolved

scene/resources/portable_compressed_texture.cpp Show resolved Hide resolved

nklbdev force-pushed the Fix_lossless_formats_in_PortableCompressedTexture2D branch from 89e6b35 to fc77cfd Compare December 24, 2023 02:27

lyuma added topic:import and removed breaks compat labels Dec 24, 2023

akien-mga reviewed Jan 4, 2024

View reviewed changes

scene/resources/portable_compressed_texture.cpp Outdated Show resolved Hide resolved

akien-mga requested review from reduz and a team January 4, 2024 11:25

reduz approved these changes Jan 4, 2024

View reviewed changes

Fix lossless formats in PortableCompressedTexture2D

47d9916

Update scene/resources/portable_compressed_texture.cpp Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>

nklbdev force-pushed the Fix_lossless_formats_in_PortableCompressedTexture2D branch from 8ef7809 to 47d9916 Compare January 4, 2024 19:12

akien-mga merged commit d1b7c60 into godotengine:master Jan 5, 2024
15 checks passed

akien-mga modified the milestones: 4.x, 4.3 Jan 5, 2024

akien-mga reviewed Jan 5, 2024

View reviewed changes

nklbdev mentioned this pull request Jan 5, 2024

Fix data format storage method for PortableCompressedTexture2D #86835

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lossless formats in PortableCompressedTexture2D #77712

Fix lossless formats in PortableCompressedTexture2D #77712

nklbdev commented May 31, 2023 •

edited

nklbdev commented Jun 2, 2023 •

edited

nklbdev commented Jun 3, 2023 •

edited

nklbdev commented Jun 4, 2023

nklbdev commented Jun 4, 2023

fire commented Sep 21, 2023 •

edited

fire commented Dec 12, 2023 •

edited

nklbdev commented Dec 12, 2023

lyuma left a comment

nklbdev commented Dec 24, 2023

lyuma commented Dec 24, 2023

nklbdev commented Dec 24, 2023

akien-mga commented Jan 4, 2024

nklbdev commented Jan 4, 2024

AThousandShips commented Jan 4, 2024

nklbdev commented Jan 4, 2024 •

edited

akien-mga commented Jan 5, 2024

nklbdev commented Jan 5, 2024

akien-mga Jan 5, 2024

nklbdev Jan 5, 2024

nklbdev Jan 5, 2024 •

edited

akien-mga Jan 5, 2024 •

edited

Zireael07 Jan 5, 2024

nklbdev Jan 5, 2024 •

edited

nklbdev Jan 5, 2024

nklbdev Jan 5, 2024 •

edited

Fix lossless formats in PortableCompressedTexture2D #77712

Fix lossless formats in PortableCompressedTexture2D #77712

Conversation

nklbdev commented May 31, 2023 • edited

nklbdev commented Jun 2, 2023 • edited

nklbdev commented Jun 3, 2023 • edited

nklbdev commented Jun 4, 2023

nklbdev commented Jun 4, 2023

fire commented Sep 21, 2023 • edited

fire commented Dec 12, 2023 • edited

nklbdev commented Dec 12, 2023

lyuma left a comment

Choose a reason for hiding this comment

nklbdev commented Dec 24, 2023

lyuma commented Dec 24, 2023

nklbdev commented Dec 24, 2023

akien-mga commented Jan 4, 2024

nklbdev commented Jan 4, 2024

AThousandShips commented Jan 4, 2024

nklbdev commented Jan 4, 2024 • edited

akien-mga commented Jan 5, 2024

nklbdev commented Jan 5, 2024

akien-mga Jan 5, 2024

Choose a reason for hiding this comment

nklbdev Jan 5, 2024

Choose a reason for hiding this comment

nklbdev Jan 5, 2024 • edited

Choose a reason for hiding this comment

akien-mga Jan 5, 2024 • edited

Choose a reason for hiding this comment

Zireael07 Jan 5, 2024

Choose a reason for hiding this comment

nklbdev Jan 5, 2024 • edited

Choose a reason for hiding this comment

nklbdev Jan 5, 2024

Choose a reason for hiding this comment

nklbdev Jan 5, 2024 • edited

Choose a reason for hiding this comment

nklbdev commented May 31, 2023 •

edited

nklbdev commented Jun 2, 2023 •

edited

nklbdev commented Jun 3, 2023 •

edited

fire commented Sep 21, 2023 •

edited

fire commented Dec 12, 2023 •

edited

nklbdev commented Jan 4, 2024 •

edited

nklbdev Jan 5, 2024 •

edited

akien-mga Jan 5, 2024 •

edited

nklbdev Jan 5, 2024 •

edited

nklbdev Jan 5, 2024 •

edited