Trade: KTX importer #103
Trade: KTX importer #103
Conversation
|
The dependency on Also, what Vulkan formats should we support? 1.0 only? |
| else | ||
| f->dimensions = 1; | ||
|
|
||
| /** @todo Assert that 3D images can't have depth format */ |
There was a problem hiding this comment.
I thought it would be handy to have a function that tells you if a PixelFormat is a depth and/or stencil format. Might have to do this manually in this plugin to support all Vulkan formats, but maybe an idea for your ever-growing todo list 👀
There was a problem hiding this comment.
Yes, this makes sense to have directly in Magnum, isDepthStencilPixelFormat() for example. I have many such utils for VertexFormat already, so a similar set could be for pixel formats.
Codecov Report
@@ Coverage Diff @@
## master #103 +/- ##
==========================================
+ Coverage 94.70% 94.97% +0.26%
==========================================
Files 104 112 +8
Lines 7673 8917 +1244
==========================================
+ Hits 7267 8469 +1202
- Misses 406 448 +42
Continue to review full report at Codecov.
|
Copy the enum from Magnum's own
If you copy from the
Huh, not sure if that was a good decision back then. I'd import cube maps as 3D images instead, one reason is that the Actually, the KTX file is always just one image, right? It can have multiple layers (in which case it's a 1D / 2D array, 3D or a cubemap) and multiple mip levels (which are exposed under via the
Yeah, the weird GL coordinate system. For now (until the above-mentioned
Honestly I have no idea how it is in DDS, I suspect nobody ever tested that. So don't take that as the ground truth :)
A problem is that the
TODO |
Every single one of 1D, 2D, 3D, cubemap can have array levels in a KTX file, so for cubemap arrays and 3D arrays you'd still have
I'm okay with this option, at least a few tests should use external data to make sure the converter and importer don't have some shared faulty assumption about the format, writing and then accepting broken files 👀
If that isn't more work than me just getting that data in a switch, that would be handy. Do any other importers have a use for this? |
|
Feel free to ignore commits for now, mainly just pushing things for backup reasons. When I have tests and/or the converter working, I'll ping you again. |
Cube map arrays are a thing though, and GL/Vulkan implements them simply as a bunch of cubemaps following each other in a 3D image. So in case of these I'd still make them a single 4D textures, on the other hand... were a thing in 1997 but I don't remember any recent driver / platform bothering with these anymore. Since these don't map to hardware anyway, I'd say having them special-cased with multiple
That's not really mapping to hardware either, TextureData primarily specifies texture type and sampler options in order to form a texture out of ImageData. Why forcing the users to split the ImageData3D to multiple 3D textures to be able to use them at all if the importer can do that directly?
Great point, yep, that definitely needs to be done :)
Right now I got stuck in a "fixing a lightbulb" loop while adding mip level support to the image converter, hopefully getting unstuck soon. After that I'll see how involved this is. |
|
Made a bit of progress in the last few days 🤠
Apart from what I mentioned, the major missing pain point is cubemaps. They're imported just fine, but there are still a few oddities with orientations and coordinate space I don't full understand. Going to have to do a bit more reading and testing to see how this interacts with GL and what the official Khronos tools do. |
|
I was bored and spent a few minutes implementing the frame animation data reading, so I'll just put the snippet here if one of us goes back to implementing it on top of a new animation type:
/* KTX2 animation frame */
struct KtxAnimationFrame {
UnsignedInt duration; /* Number of time units per frame */
UnsignedInt timescale; /* Number of time units per 1 second */
UnsignedInt loopCount; /* Loop count, 0 = infinite */
};
static_assert(sizeof(KtxAnimationFrame) == 12, "Improper size of KtxAnimationFrame struct");
/* Read animation data */
if(isLayered) {
const auto found = keyValueMap.find("KTXanimData"_s);
if(found != keyValueMap.end()) {
if(numLayers*sizeof(Implementation::KtxAnimationFrame) <= found->second.size()) {
const auto frames = Containers::arrayCast<const Implementation::KtxAnimationFrame>(found->second);
Float key = 0.0f;
for(UnsignedInt layer = 0; layer != numLayers; ++layer) {
/** @todo import as animation track with {key, layer} */
Implementation::KtxAnimationFrame frame = frames[layer];
Utility::Endianness::littleEndianInPlace(frame.duration, frame.timescale);
Float duration = frame.duration;
if(frame.timescale > 0)
duration /= frame.timescale;
key += duration;
}
}
else
Warning{} << "Trade::KtxImporter::openData(): invalid animation data, ignoring";
}
} |
I'd say "just" import (and store) them in a way that makes them work properly with the expaded cubemap example (link to the code above)? This will need to get rethought for Vulkan and the flipping flags later anyway, but until I have something tangible to test this with I don't think you should need to care beyond what GL needs. Or am I missing something? |
|
The new
Opinions / ideas? :) |
| UnsignedInt leastCommonMultiple(UnsignedInt a, UnsignedInt b) { | ||
| const UnsignedInt product = a*b; | ||
|
|
||
| /* Greatest common divisor */ | ||
| while(b != 0) { | ||
| const UnsignedInt t = a % b; | ||
| a = b; | ||
| b = t; | ||
| } | ||
| const UnsignedInt gcd = a; | ||
|
|
||
| return product/gcd; | ||
| } |
There was a problem hiding this comment.
Interesting that I still don't have such utils in the Math lib. TODO for me.
|
Thanks for reviewing 👍 I adressed all the minor items locally, they'll be pushed along with the completed tests once I finished those.
Mostly I need to make sure that image flips don't affect the coordinate space. The KTX spec mentioned e.g. having to flip faces for mismatching y-orientation. But there's no major code to write, just needs testing with GL.
I'm a fan of the last option. Ideally, there'd also be a way to query those flags per image. Then we could even scrap exposing the image type through |
Yep, I settled on the same conclusion, this is essential to have otherwise dealing with GL/Vulkan coordinate system differences would quickly turn into a nightmare. So basically:
I still need to think about backwards compatibility to avoid breaking everyone's projects, but most probably the defaults would be "import with Y up" and "save with Y down", and somehow begging people to set these explicitly. |
|
Starting to get to the finish line 🥉 I converted the cubemap example files to KTX and everything just seemlessly works when loaded in GL, so no special code needed there. If you want to try yourself, here's the output of PVRTexTool: Apart from that, I'm just finishing tests, a few esoteric cases need covering, but the majority is there. For compressed formats, how do I best go about getting ground truth data? From a quick glance, CIs are failing because I forgot to update the test files in CMake, but they do seem to compile, so there's that. |
There was a problem hiding this comment.
Great work, again.
Is this finishable without me implementing the CubeMap / Array / ... image and converter flags first? Anything else this is being blocked on?
I probably won't be able to finish CompressedImage::blocks() and related APIs anytime soon, but also I don't want to put this on hold just because of these -- I think it should be doable without these, if you won't support the pixel storage parameters for now.
|
That was a great review round, mostly just deleting things 😆
Absolutely, once those are added it shouldn't take a whole lot more than setting the correct values in the KTX header. The layout of the level data won't change. Until then all data is exported as 1D/2D/3D. Nothing blocking I'm aware of.
Yeah, I just fixed the last remaining issue with block-compressed image export, and if I ignore the storage parameters, this is 99% done. Just need some more code for depth/stencil format DFD writing, and then tests. |
WIP and missing tests
praying that GCC 4 doesn't explode
friendly reminder to implement support
Similarly to uncompressed mipmapped 3D images, we have to generate them with the converter tests. We, however, have to manually generate the .bin data with compressed blocks. Not ideal, but better than not testing these at all.
Raw DFD data is bundled with the test, generated from a patch to a dfdutils program, see README.md
We only needed that many to get some DFD test coverage, but we have a separate DFD test for all formats now
Needed for Emscripten tests
|
I'm going to strangle GCC 4.8 one of these days 🤡 |
|
I addressed all your review comments and fixed a few remaining DFD bugs, but the DFD is now tested against dfdutils output for all formats.
|
|
Okay, I did a final pass over the latest changes and this looks exceptionally good and the test coverage is amazing as well. I'll do a bunch of tests locally (playing with One remaining thing -- I was thinking to to put all the dfd blobs into a single file, having them separately feels a bit excessive 😆 Thinking of rewriting the patch to open the file for append instead and then prefix each DFD blob with size + format ID as two 32bit numbers, that could hopefully do the trick. If you have time to do it yourself, great, if not, I'll do it during merge. |
I can do that, yeah. That would also fix an interesting error on Windows with Ninja about the max command line length being exceeded 👀 Ran into that when testing Emscripten and I couldn't figure out how to get it to use a response file for command line arguments (and not just object files.) |
|
Squashed into a few commits (didn't really find a way how to break it up better, without leaving a lot of noise in the history) and merged as 2114bf2...bd3f16e. Thank you for this work! 👍 Once I manage to finish the leftovers (array/cube annotation flags for images), I might have some followup tasks for this, and the Basis detection/proxying as well. |
I tried to break it up into commits specific to each importer, but the test dependency (as well as the single commit for registering both plugins for CI) kinda defeated that plan. Hope it wasn't too much work to squash these. Thanks for the (as always) smooth review process and speedy addition of required Magnum changes along the way |
|
Ha, did I not close this yet? Interesting. |
👋
Here's a first draft for the KTX2 image importer. It supports block-compressed and uncompressed 1D, 2D, 3D images.
Array layers are imported as separate images, cube faces and mips are imported as image levels (in the same order asDdsImporter: all mips for face 0, then face 1, etc.).Array layers and cubemap faces are imported as extra image dimensions (so 1D array images become
Image2D, (layered) cube maps becomeImage3Dwith n*6 z slices). The exception are 3D array images, those are imported as separateImage3Dfor each layer. No API supports those and there is noImage4D.I wanted to get some early feedback on a few design decisions, so there's still a long list of todos, roughly sorted by importance:
package/ci/*.{bat,sh}for the CIs to pick them upKtxImageConverterPixelFormatwhere possiblearbitrary Vulkan formats, wrapped as customout of scope of this PRPixelFormatDdsImporter) or at least documentedBasis compressionout of scope of this PRsupercompression (zstd etc.)out of scope of this PR