Add support for KTX image format so that we can use Basis Universal for GLTF

[acazuc]

Bugsquad Edit: This implements KTX support with Basisu gltf extension and fixes: godotengine/godot-proposals#5873 The final binary size is ~100KB added.

Hello,

This merge request allows KTX file format texture loading
There are some limitations to supported KTX files: 2d images only, no cubemap, no array, no de-padding, only a limited list of godot Image formats
Decoding is done using libktx
I tested ktx files from ktxtext, most useful formats are working (rgb8, rgba8, astc, bc1-7, and some others), invalid / unsupported files are handled with error messages
KTX uses 4-bytes padding, and no de-padding is done here:

• if padding is used on non-first mipmap, only the first mipmap is used
• if padding is used on the first mipmap, file isn't loaded

I guess it does help for godotengine/godot-proposals#5873

Compilation & tests has been done in amd64 target

Regards,

[Riteo]

Is every single header of the library needed? 45k new lines of stuff are a lot and I think a bit too much for loading a new image container. Can you trim a bit the thirdparty/libktx directory? I see also some example stuff into thirdparty/libktx/utils, so I think that you could start there.

[acazuc]

The biggest remaining file is vulkan_core.h (11k lines), which is also located in thirdparty/vulkan
Would it be ok for you to add thirdparty/vulkan/include as an include path to build thirdparty/libktx in modules/ktx/SCsub ?

[fire]

I will try to review. Trying get some more opinions from the godot engine team members

[fire]

Error: D:\a\godot\godot\thirdparty\libktx\lib\gl_format.h(95): error C2220: the following warning is treated as an error
Warning: D:\a\godot\godot\thirdparty\libktx\lib\gl_format.h(95): warning C4005: 'NOMINMAX': macro redefinition

Weird error. Might have to hotpatch and send upstream?

[RevoluPowered]

Hey, I was asked to review this by @fire and just wanted to add some supporting info:

Slides from Khronos
https://www.khronos.org/assets/uploads/apis/KTX-2.0-Launch-Overview-Apr21_.pdf

The goal here as far as I can understand is that we want to make exported games smaller and also we would like to reduce the amount of resource duplication.

Basically apple/android uses ETC2 or ASTC.
Windows/Linux use BTPC, DXT5 formats.

This means you export your game it includes them on windows both of them, potentially making things huge.

So that means you import one texture: and it becomes 3 potentially. This unifies in a single format "basis universal" which can easily transcode into those formats. This moves texture import time outside of the editor time, without compromising performance/quality.

For non subject matter experts: smaller game export, faster editor import time, faster boot time because the dataset for the export is smaller.

[RevoluPowered]

LGTM

The only portion I dislike is the massive switch case but it looks simple enough and is OK that it is explicit, it's also fast so, I should not complain really 😅

Maybe recommend writing a very simple unit test for this if you can.

[fire]

Some errors in the cicd and the pr isn’t squashed

[RevoluPowered]

• Needs some docs on how to use this

[Calinou]

Does this PR allow loading arbitrary KTX images from the filesystem or a buffer in an exported project, similar to Image.load_png_from_buffer()? As far as I know, this is required to work with the run-time glTF loader.

[acazuc]

It doesn't add arbitrary loading like png, it only adds support for KTX file the same way DDS files are handled (as imported texture files)
I guess KTX2 with basisu loading for glTF could be handled another way, maybe using ktx2_transcoder class from thirdparty/basis_universal the same way modules/basis_universal does handle basisu files

[fire]

Do you know how much effort it is to load_ktx_from_buffer and load_ktx_from_file? Was curious. We'd need to add the api as @Calinou said for the basisu in gltf usecase

[acazuc]

A commit was added for KHR_texture_basisu support in glTF module
I tested the import of https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/StainedGlassLamp/glTF-KTX-BasisU with success

NB: there is a shadow dependency in modules/ktx/SCsub for basisu transcoder: thirdparty/basis_universal/transcoder/basisu_transcoder.cpp is not added to source because it does collide with modules/basis_universal/SCsub target and I don't know how to solve it gracefully.

Sorry for the back and forthes of this review

[fire]

I'll try asking for support. Aside from that, there are some whitespace cicd errors.

[aaronfranke]

@acazuc With that test asset you posted, when I try to load StainedGlassLamp_base_basecolor.ktx2 (not as part of a GLTF, just by double-clicking on the texture by itself) I get this error:

 Unsupported format 0 of KTX2 texture file 'res://StainedGlassLamp/glTF-KTX-BasisU/StainedGlassLamp_base_basecolor.ktx2'.
  Invalid image
  Failed loading resource: res://StainedGlassLamp/glTF-KTX-BasisU/StainedGlassLamp_base_basecolor.ktx2. Make sure resources have been imported by opening the project in the editor at least once.
  editor/editor_node.cpp:1263 - Condition "!res.is_valid()" is true. Returning: ERR_CANT_OPEN

[Calinou]

It doesn't add arbitrary loading like png, it only adds support for KTX file the same way DDS files are handled (as imported texture files)

Note that #69085 implements run-time DDS loading.

[acazuc]

@aaronfranke that is strange, did you used the last commit (at least 6271fdd) ?
This error did happen before this commit because basisu transcode wasn't supported

[aaronfranke]

@acazuc I just re-tested, this time with 8ca3bbc, and it works. Although interestingly, the PNG and JPG version does not work. It might be the same issue as #76691.

---

Well, now that I've tested that this works... As for giving this PR a review, I'm not really sold on this being in core due to the extremely large amount of code required. 30k lines is a lot.

Regardless of whether we want this to be in core or not, I want to modify the code in GLTFDocument to allow GLTFDocumentExtension classes to extend the importer with arbitrary image formats, instead of just adding KTX to the current hard-coded code. As a first test I would like to implement EXT_texture_webp via GLTFDocumentExtension since Godot already includes all of the code required to save and load WebP files, then afterwards we can copy-paste that code as a template for KTX (or possibly just port that code to GDExtension if we decide to not put KTX in core). I spent today writing the code to do this and it seems to load WebP files into memory fine. However, I am unable to fully test it because I can't even get PNG files to work on master, see #76691.

[fire]

Is it correct that thirdparty/libktx/lib/dfdutils/vulkan/vulkan_core.h is a duplicate because it costs 10,000 lines of code?

[fire]

@acazuc Do you require some help with removing the "GHA / 🐧 Linux / Editor w/ Mono (target=editor) (pull_request) Failing after 4m" Github action test failure?

[acazuc]

@fire yes, it was almost a duplicate (there was some ASTC VkFormat enum that aren't defined in thirdparty/vulkan/include/vulkan/vulkan_core.h, but commenting the code where they're used isn't a problem)
linux editor pipeline should be ok

[fire]

Can you update this pr based on #76895? I am not sure how long the merge window for 4.1 lasts, but it's arriving soon.

[fire]

@Calinou What's the process for checking binary size comparisons?

[acazuc]

those are the binary sizes for platform=linuxbsd optimize=speed compiled on my machine:

commit	debug_symbols	size (Bytes)
ba74c0b	yes	1,709,191,632
28cca66	yes	1,707,476,192
ba74c0b	no	135,827,240
28cca66	no	135,703,912

[aaronfranke]

In terms of the code organization: This looks good to me, the code in the GLTF module is clean, the module can be disabled just with a scons option, and the Image code is similar to the code for the other image formats.

In terms of whether we want this in the engine or not: I don't know, 100 KiB of optional binary size is not that bad though. To me it depends on how often this will be used. I will leave the decision to others like @fire and @akien-mga. In the meantime I'll hit the approve button myself.

[fire]

https://github.com/KhronosGroup/glTF-Sample-Models/tree/4ca06672ce15d6a27bfb5cf14459bc52fd9044d1/2.0/FlightHelmet/glTF-KTX-BasisU

Tested with this basisu gltf file.

It's the scope of this pr to allow ktx2 import, but it's outside the scope of this pr to enforce things like only basisu for a gltf.

Great work!

Note:

In my view 100KB is ok to use for this, thoughts?

[fire]

Can you refresh the pr?

[Calinou]

Binary size comparison for a Linux x86_64 stripped release export template with LTO:

70,341,920  godot.linuxbsd.template_release.x86_64.master
70,448,800  godot.linuxbsd.template_release.x86_64.pr

This PR adds about 107 KB.

[akien-mga]

Just noting for reviewers that I still need to check the thirdparty code integration, but otherwise this should be good to merge once I've done that.

[akien-mga]

Did a quick review pass, the integration of the thirdparty library is pretty good! Great job trimming down all the files we don't need to keep it minimal.

Here's a diff with some further changes to properly document the copyright, and handle the fact that Godot can be compiled without basis_universal or without Vulkan.

diff --git a/COPYRIGHT.txt b/COPYRIGHT.txt
index 582784d78e..904b2adca2 100644
--- a/COPYRIGHT.txt
+++ b/COPYRIGHT.txt
@@ -256,6 +256,12 @@ Comment: jpeg-compressor
 Copyright: 2012, Rich Geldreich
 License: public-domain or Apache-2.0
 
+Files: ./thirdparty/libktx/
+Comment: KTX
+Copyright: 2013-2020, Mark Callow
+  2010-2020 The Khronos Group, Inc.
+License: Apache-2.0
+
 Files: ./thirdparty/libogg/
 Comment: OggVorbis
 Copyright: 2002, Xiph.org Foundation
diff --git a/modules/ktx/SCsub b/modules/ktx/SCsub
index 9507d5c4c8..9e45313701 100644
--- a/modules/ktx/SCsub
+++ b/modules/ktx/SCsub
@@ -11,7 +11,6 @@ thirdparty_obj = []
 
 thirdparty_dir = "#thirdparty/libktx/"
 thirdparty_sources = [
-    "lib/basis_transcode.cpp",
     "lib/checkheader.c",
     "lib/filestream.c",
     "lib/hashlist.c",
@@ -32,11 +31,19 @@ thirdparty_sources = [thirdparty_dir + file for file in thirdparty_sources]
 env_ktx.Prepend(CPPPATH=[thirdparty_dir + "include"])
 env_ktx.Prepend(CPPPATH=[thirdparty_dir + "utils"])
 env_ktx.Prepend(CPPPATH=[thirdparty_dir + "lib"])
-env_ktx.Prepend(CPPPATH=[thirdparty_dir + "lib/dfdutils"])
 env_ktx.Prepend(CPPPATH=[thirdparty_dir + "other_include"])
-env_ktx.Prepend(CPPPATH=["#thirdparty/basis_universal"])
-env_ktx.Prepend(CPPPATH=["#thirdparty/vulkan/include"])
-env_ktx.Prepend(CPPFLAGS=["-DKHRONOS_STATIC=1"])
+
+if env["module_basis_universal_enabled"]:
+    thirdparty_sources += [thirdparty_dir + "lib/basis_transcode.cpp"]
+    env_ktx.Prepend(CPPPATH=["#thirdparty/basis_universal"])
+
+if env["vulkan"]:
+    env_ktx.Prepend(CPPPATH=["#thirdparty/vulkan/include"])
+else:
+    # Falls back on bundled `vkformat_enum.h`.
+    env_ktx.Append(CPPDEFINES=["LIBKTX"])
+
+env_ktx.Append(CPPDEFINES=[("KHRONOS_STATIC", 1)])
 
 env_thirdparty = env_ktx.Clone()
 env_thirdparty.disable_warnings()
diff --git a/thirdparty/README.md b/thirdparty/README.md
index 611f63b02a..a4e4294a77 100644
--- a/thirdparty/README.md
+++ b/thirdparty/README.md
@@ -303,7 +303,7 @@ Files extracted from upstream source:
 
 - Upstream: https://github.com/KhronosGroup/KTX-Software
 - Version: 4.1.0 (d7255fe73cd53b856731ceb9f2c279181d0dbbca, 2023)
-- License: MIT
+- License: Apache-2.0
 
 Files extracted from upstream source:
 
@@ -315,7 +315,7 @@ Files extracted from upstream source:
 - `other_include/KHR/*`
 - ifndef-protect NOMINMAX define in `lib/gl_format.h`
 - remove `basisu/` prefix from `thirdparty/libktx/lib/basis_transcode.cpp` basisu includes
-- comment VK\_FORMAT\_ASTC_\*x\*x\*\_UNORM\_BLOCK\_EXT cases in lib/dfdutils/vk2dfd.inl
+- comment `VK_FORMAT_ASTC_*x*x*_UNORM_BLOCK_EXT` cases in `lib/dfdutils/vk2dfd.inl`
 
 
 ## libogg

The instructions in thirdparty/README.md are incomplete, as they suggest to copy over all files from the upstream repo, which is not what you ended up doing after trimming things down.

We need to have precise documentation on which files to copy or not, so that future updates can be done easily. (E.g. there's already version 4.2.1 upstream which we might want to update too eventually).

And this part might be best described as a .patch file so that it can be re-applied on updates:

- ifndef-protect NOMINMAX define in `lib/gl_format.h`
- remove `basisu/` prefix from `thirdparty/libktx/lib/basis_transcode.cpp` basisu includes
- comment `VK_FORMAT_ASTC_*x*x*_UNORM_BLOCK_EXT` cases in `lib/dfdutils/vk2dfd.inl

It's also a bit uncommon IMO to include files from a lib folder, especially when there's a clear distinction between lib (private API) and include (public API). Seems like we only need this for vk_format.h.

[akien-mga]

Looks really good now!

The only bit I'm missing from my earlier review is improving the update instructions in thirdparty/README.md, to know which files to copy and which ones to leave out.

[akien-mga]

Amazing work, thanks! Congrats for your first merged Godot contribution 🎉