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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Basis plugins to 1.15 #112

Closed
wants to merge 75 commits into from
Closed

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Oct 19, 2021

This PR upgrades BasisImporter and BasisImageConverter to work with basis_universal 1.15 (more specifically, tag v1_15_update2 because that contains a few important changes).

Not entirely finished with everything, but the majority is there, so feel free to give it a first review 馃檱

New features

Along with the upgraded dependency come a few new features:

  • import of all image types supported by Basis, including 2D arrays/cubemaps/cubemap arrays/3D/video as ImageData3D
  • KTX2 import and export, including UASTC and Zstd supercompression
    • import happens transparently, with the minor caveat that 3D images are not supported. basisu itself can only generated 2D arrays for KTX2, anyway.
    • in BasisImageConverter this is simply a new config option (defaults to off)
  • support for sRGB formats:
    • in BasisImporter this is a flag read from the file header, affecting the output format
    • in BasisImageConverter it's detected from the image format and sets the correct parameters for the encoder. Can be overridden by config options, if desired
  • support for user-supplied mipmaps in BasisImageConverter
    • mipmap generation by basis is still possible, but is disabled if the user supplies their own mipmaps

Todo

Or rather: questions to @mosra

  • allow export of 3D images in BasisImageConverter? Is this a priority? nope, but probably revisit once image flags are in, for cube and 2D array texture export
  • the importer code got rather messy, should I write a small abstraction over the two transcoders to make the code legible again? eh, that'll just sweep the code into another corner
  • should we flip z in 3D images? Files have a y-flip flag so we could use that to deduce y-flip = z-flip, but I'm not sure how foolproof that is since the basisu tool doesn't do any z-flipping at all, regardless of y_flip being set nope, "solved" now that we import 3D images as 2D array layers
  • BC7RGB seems deprecated since UAST, should this go away? Investigate what this means for the output data they both mean the same thing, removing the RGB variant
  • print a useful error about PVRTC1 requiring power-of-two, instead of just "transcoding failed"? However, then we won't have an easy way to test the transcoding failing 馃ぁ PVRTC is pretty ancient, not worth the extra effort
  • more validation on .basis files. The KTX2 transcoder does all that, but the basis transcoder is missing a few checks we need, e.g. all array layers having the same size.
  • update the config options, there are a few new ones , some of them seem obsolete and we should add some comments (just take whatever basisu itself says). Also: the docs say the config options follow the basisu tool, and the .conf file says they follow the C++ API. The latter is true, but we should decide on one. I think basisu options would be more intuitive but require some extra wiring + tests...
  • test BasisImageConverter sRGB handling

Related issue: #110. Can be closed if this PR and the upcoming PR for KtxImporter Basis-forwarding are merged.

If zstd can't be found by FindBasisUniversal.cmake, compile with Zstandard (de)compression support disabled. At runtime, detect this and print a useful error (instead of "transcoding failed").
This sets the correct basis parameters depending on the image format. Can still be overridden by config values, if present.

TODO tests
Array layers, cube faces and z slices are imported in an extra image dimension, ie. Image3D. Image type is exposed via texture().

TODO tests, docs, some extra validation on .basis files
basisu doesn't do it for us, only in .basis files
Removed from the basisu C++ API, can be replaced by swizzle=rrrg
basisu doesn't flip level 1 or higher
All mipmaps in basis are 2D, but they'd need to halve in Z for correct 3D mipmaps. Print a warning and import them as 2D array textures with mipmaps instead.
@mosra mosra added this to TODO in Asset management via automation Oct 20, 2021
@mosra mosra added this to the 2021.0a milestone Oct 20, 2021
@pezcode
Copy link
Contributor Author

pezcode commented Oct 20, 2021

I realized that importing video files as a single Image3D is probably not what you'd want most of the time, assuming the average video is rather rather large. I'll change that back to report multiple images, hopefully basisu saves the KTX2 animation metadata so we can detect videos in those, too.

@pezcode
Copy link
Contributor Author

pezcode commented Oct 25, 2021

I have a drastic suggestion: remove big-endian detection, hoping nobody ever needs it 馃 On CMake 3.20 we can use CMAKE_CXX_BYTE_ORDER without having to enable C and causing all these headaches.

@mosra
Copy link
Owner

mosra commented Oct 25, 2021

Oh wait, so if you enable C it causes the C files to be compiled as C on static builds even though told to be compiled as CXX, and if you don't enable C the C files get properly compiled as CXX?

Or is that C enablement independent of the problem with C files being compiled as C on static builds?

# Conflicts:
#	src/MagnumPlugins/BasisImporter/BasisImporter.cpp
#	src/MagnumPlugins/BasisImporter/Test/BasisImporterTest.cpp
@pezcode
Copy link
Contributor Author

pezcode commented Oct 25, 2021

Oh wait, so if you enable C it causes the C files to be compiled as C on static builds even though told to be compiled as CXX, and if you don't enable C the C files get properly compiled as CXX?

Or is that C enablement independent of the problem with C files being compiled as C on static builds?

Yeah, without enabling C it would just ignore those files altogether, causing unresolved symbol errors. When I forced CXX without enabling C, I don't remember getting these C-specific errors. I don't know what weird interactions are going wrong but I'm thinking it might not be worth the effort.

@pezcode
Copy link
Contributor Author

pezcode commented Oct 25, 2021

I went ahead and disabled the endianness detection on CMake < 3.9 for now. Maybe worth revisiting for later, but I can't remote diagnose this without force-pushing and eating all your CI minutes 馃檴

There's one weird error about pthread_create missing, I assume this is related to the help text for BasisImageConverter. But why is the importer linking to pthread... oooo, it's zstd... the bad news is that even though basisu doesn't use anything from zstd that would use threads (as far as I can tell only the compression API needs pthread), the bundled zstd.c has a hard #define ZSTD_MULTITHREAD so we can't disable it with a compile definition. Sigh.

@mosra
Copy link
Owner

mosra commented Oct 26, 2021

Okay, I'm taking over with finishing the PR, this whole thing seems (again!) way more cursed than anticipated.

and test the converted image, not the original(??) image
G is taken from the alpha channel, matching the recommended basisu -swizzle rrrg for two channels. We don't control this, it's done by the transcoder.
鈥ir own

This fixes the use case of feeding an RG image + swizzle rrrg (as recommended by the basis docs). This would lead to the output becoming rrrr since BasisImageConverter already performed that swizzle. Also document the behaviour.
@mosra
Copy link
Owner

mosra commented Oct 31, 2021

Merged as 809fc18...670133c, with some of my commits mixed in (external zstd dependency, fix to that C language enablement craziness, ...).

The commits are regrouped and squashed into small enough logical chunks so it looks quite different from the patch set in this PR, but the total bulk of changes is the same ;) Thank you for the excellent work again!

@mosra mosra closed this Oct 31, 2021
Asset management automation moved this from TODO to Done Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants