Refactor high quality texture import

[reduz]

• Only two texture import modes for low/high quality now:
  • S3TC/BPTC
  • ETC2/ASTC
• Makes sense given this is the general preferred and most compatible combination of texture compression formats in most platforms.
• Removed lossy_quality from VRAM texture compression options. It was unused everywhere.
• Added a new "high_quality" option to texture import. When enabled, it uses BPTC/ASTC (BC7/ASTC4x4) instead of S3TC/ETC2 (DXT1-5/ETC2,ETCA).
• Changed MacOS export settings so required texture formats depend on the architecture selected.
• Also did some fixes in ASTC support (mipmaps not supported).

This solves the following problems:

• Makes it simpler to import textures as high quality, without having to worry about the specific format used.
• On Apple, compressed texture format support is very difficult both for the editor and for exporting because, depending on the hardware, its either one or the other. This makes exporting and running the editor on MacOS "Just Work"(tm).
• As the editor can now run on platforms such as web, Mac OS with Apple Silicion and Android, it should no longer be assumed that S3TC/BPTC is available by default for it.

[clayjohn]

I'm getting a crash when loading a scene.

I deleted the .godot folder before trying to import. Then after crashing the first time I deleted the .import files as well and I received the same crash (albeit pointing to a different line number in ResourceImporterTexture::import

ERROR: Trying to unreference a SafeRefCount which is already zero is wrong and a symptom of it being misused.
Upon a SafeRefCount reaching zero any object whose lifetime is tied to it, as well as the ref count itself, must be destroyed.
Moreover, to guarantee that, no multiple threads should be racing to do the final unreferencing to zero.
   at: _check_unref_sanity (./core/templates/safe_refcount.h:176)


================================================================
handle_crash: Program crashed with signal 4
Engine version: Godot Engine v4.0.beta.custom_build (dc5cdf4de4aa003de9e78780f87d7264aa4f9ec1)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7fd112c42520] (??:0)
[2] StringName::unref() (/home/clayjohn/dev/godot/./core/templates/safe_refcount.h:173)
[3] StringName::~StringName() (/home/clayjohn/dev/godot/./core/string/string_name.h:182)
[4] ResourceImporterTexture::import(String const&, String const&, HashMap<StringName, Variant, HashMapHasherDefault, HashMapComparatorDefault<StringName>, DefaultTypedAllocator<HashMapElement<StringName, Variant> > > const&, List<String, DefaultAllocator>*, List<String, DefaultAllocator>*, Variant*) (/home/clayjohn/dev/godot/editor/import/resource_importer_texture.cpp:454)
[5] EditorFileSystem::_reimport_file(String const&, HashMap<StringName, Variant, HashMapHasherDefault, HashMapComparatorDefault<StringName>, DefaultTypedAllocator<HashMapElement<StringName, Variant> > > const*, String const&) (/home/clayjohn/dev/godot/editor/editor_file_system.cpp:2001)
[6] EditorFileSystem::_reimport_thread(unsigned int, EditorFileSystem::ImportThreadData*) (/home/clayjohn/dev/godot/editor/editor_file_system.cpp:2159)
[7] WorkerThreadPool::GroupUserData<EditorFileSystem, void (EditorFileSystem::*)(unsigned int, EditorFileSystem::ImportThreadData*), EditorFileSystem::ImportThreadData*>::callback_indexed(unsigned int) (/home/clayjohn/dev/godot/./core/object/worker_thread_pool.h:152)
[8] WorkerThreadPool::_process_task(WorkerThreadPool::Task*) (/home/clayjohn/dev/godot/core/object/worker_thread_pool.cpp:72)
[9] WorkerThreadPool::_native_low_priority_thread_function(void*) (/home/clayjohn/dev/godot/core/object/worker_thread_pool.cpp:165)
[10] Thread::callback(Thread*, Thread::Settings const&, void (*)(void*), void*) (/home/clayjohn/dev/godot/core/os/thread.cpp:67)
[11] void std::__invoke_impl<void, void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*>(std::__invoke_other, void (*&&)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*&&, Thread::Settings&&, void (*&&)(void*), void*&&) (/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:61)
[12] std::__invoke_result<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*>::type std::__invoke<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*>(void (*&&)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*&&, Thread::Settings&&, void (*&&)(void*), void*&&) (/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:96)
[13] void std::thread::_Invoker<std::tuple<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*> >::_M_invoke<0ul, 1ul, 2ul, 3ul, 4ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul, 4ul>) (/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:252)
[14] std::thread::_Invoker<std::tuple<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*> >::operator()() (/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:259)
[15] std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(Thread*, Thread::Settings const&, void (*)(void*), void*), Thread*, Thread::Settings, void (*)(void*), void*> > >::_M_run() (/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:210)
[16] /home/clayjohn/dev/godot/bin/godot.linuxbsd.editor.dev.x86_64.llvm(+0xb2089f3) [0x5651a18e79f3] (msdf-error-correction.cpp:?)
[17] /lib/x86_64-linux-gnu/libc.so.6(+0x94b43) [0x7fd112c94b43] (??:0)
[18] /lib/x86_64-linux-gnu/libc.so.6(+0x126a00) [0x7fd112d26a00] (??:0)
-- END OF BACKTRACE --
============================

[reduz]

@clayjohn The crash and error seem unrelated to the PR, which is very strange. This PR forces the reimport of all textures, so it may be related to that?

[clayjohn]

@clayjohn The crash and error seem unrelated to the PR, which is very strange. This PR forces the reimport of all textures, so it may be related to that?

I'm not sure. I am testing on https://github.com/Rytelier/Godot-4-forest-benchmark and I can reliably reproduce the crash if I delete the .godot folder, or if import textures from an older version (I tried with Beta 15) and then try to import with this PR. Of note, after crashing, if I restart the editor it finishes importing the rest of the textures. So it seems like the issue could be due to the number of textures in the project?

I also tested with https://github.com/gdquest-demos/godot-4-3d-third-person-controller and it did not crash

Edit: Turns out in my testing project I had left "Import ETC" disabled. If I enable "Import ETC" I can reproduce the crash on master, so it is not caused by this PR

[clayjohn]

Couple notes:

1. When compressing Icon.png into ASTC I get the following error

astcenc: Encoding image size 64x64 to format ASTC_4x4, with mipmaps.
ERROR: Expected Image data size of 64x64x1 (with 6 mipmaps) = 5488 bytes, got 4096 bytes instead.
   at: initialize_data (core/io/image.cpp:2206)

It looks like the image size validator doesn't properly account for ASTC 2. I was able to work around the crash above by setting editor/import/use_multiple_threads to false. The crash happens more frequently when ETC ASTC is enabled. But it also happens on master with both S3TC and ETC2 enabled 3. The ASTC formats are missing from Image::format_names[] https://github.com/godotengine/godot/blob/a43db5afa4bbec4772be2f296931a6d44bb4cbb3/core/io/image.cpp#L79-L81 Need to add them to the end:

"ASTC_4x4",
"ASTC_4x4_HDR",
"ASTC_8x8",
"ASTC_8x8_HDR",

[clayjohn]

Here is the error log from running on an android device:

01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Unable to open file: res://.godot/imported/icon.svg-218a8f2b3041327d8a5756f3a245f83b.astc.ctex.
01-27 17:26:57.152 30718 30753 E godot   :    at: _load_data (scene/resources/texture.cpp:846)
01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Failed loading resource: res://.godot/imported/icon.svg-218a8f2b3041327d8a5756f3a245f83b.astc.ctex. Make sure resources have been imported by opening the project in the editor at least once.
01-27 17:26:57.152 30718 30753 E godot   :    at: _load (core/io/resource_loader.cpp:222)
01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Failed loading resource: res://icon.svg. Make sure resources have been imported by opening the project in the editor at least once.
01-27 17:26:57.152 30718 30753 E godot   :    at: _load (core/io/resource_loader.cpp:222)
01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Can't load dependency: res://icon.svg.
01-27 17:26:57.152 30718 30753 E godot   :    at: load (core/io/resource_format_binary.cpp:696)
01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Failed loading resource: res://.godot/exported/133200997/export-3070c538c03ee49b7677ff960a3f5195-main.scn. Make sure resources have been imported by opening the project in the editor at least once.
01-27 17:26:57.152 30718 30753 E godot   :    at: _load (core/io/resource_loader.cpp:222)
01-27 17:26:57.152 30718 30753 E godot   : USER ERROR: Failed loading scene: res://main.tscn
01-27 17:26:57.152 30718 30753 E godot   :    at: start (main/main.cpp:2921)
01-27 17:26:57.153 30718 30753 E godot   : USER ERROR: Condition "default_certs != nullptr" is true.
01-27 17:26:57.153 30718 30753 E godot   :    at: load_default_certificates (modules/mbedtls/crypto_mbedtls.cpp:302)

Running on a Pixel 4. The scene is just the default icon on a Sprite2D taking up most of the screen. The icon is set to use high quality import. and only "ETC ASTC" import is enabled in the project settings (edit: same results if both "S3TC BPTC" and "ETC ASTC" enabled in project settings) ASTC-test.zip

edit edit: Tested on Vulkan backend, same result. The project won't load and I get the above error endlessly. Unchecking the "high quality" flag on texture import makes the project work

[reduz]

@clayjohn Fixed some problems in the ASTC implementation, so hoping it works now. I can't manage to load ASTC on desktop (Godot does not recognize the format for some reason).

[clayjohn]

@clayjohn Fixed some problems in the ASTC implementation, so hoping it works now. I can't manage to load ASTC on desktop (Godot does not recognize the format for some reason).

Same error as before. The file it is referencing exists, its almost like its not getting included in the APK though. Do we have logic that selects what imported resources are included in the APK?

01-28 10:35:36.267 11948 11980 E godot   : USER ERROR: Unable to open file: res://.godot/imported/icon.svg-218a8f2b3041327d8a5756f3a245f83b.astc.ctex.
01-28 10:35:36.267 11948 11980 E godot   :    at: _load_data (scene/resources/texture.cpp:846)
01-28 10:35:36.267 11948 11980 E godot   : USER ERROR: Failed loading resource: res://.godot/imported/icon.svg-218a8f2b3041327d8a5756f3a245f83b.astc.ctex. Make sure resources have been imported by opening the project in the editor at least once.

[reduz]

Oh, my mistake, I forgot to add astc to Android and iOS, will do asap.

[reduz]

done

[clayjohn]

Tested again, the error is gone and the texture loads. But it is totally incorrect

Vulkan renderer on android

OpenGL renderer on android

How it looks on desktop (uses BPTC)

[reduz]

Damn it.. :unamused: The weird thing is that the texture encodes/decodes fine to ASTC because the preview shows fine on dekstop, but somehow it does not seem to work when loaded?

[reduz]

I will have to compile on my Mac M1 to test whats going on..

[clayjohn]

Damn it.. unamused The weird thing is that the texture encodes/decodes fine to ASTC because the preview shows fine on dekstop, but somehow it does not seem to work when loaded?

When I look at the preview on desktop it always reports DXT or BPTC

[reduz]

@clayjohn yeah I copied the .ctex file from the .godot/import folder to test but as Vulkan is not supported on my nvidia GPU I can't see it decoded.

[reduz]

@clayjohn Okay that took me a couple of hours to figure out, but it should be working well now. Make sure to test, but I tested on AMD (which supports ASTC) and it works fine.

[reduz]

@akien-mga I think its safe to merge at this point, seems to work well enough for me.

[clayjohn]

Just tested on my android device and I'm still getting the same output as before

[jcostello]

Dumb question but this has a visual impact?

[clayjohn]

Dumb question but this has a visual impact?

Yes, same as switching from s3tc to bptc in current betas. There won't be a visual change in the default case though

[akien-mga]

Thanks!