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

Ktximporter: forward Basis Universal compressed images to BasisImporter #113

Closed
wants to merge 77 commits into from

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Oct 29, 2021

Follow-up to #112. Only the two most recent commits are relevant, since this is on top of #112 to make sure the tests pass on CI.

If KtxImporter finds a file compressed with Basis Universal (either through BasisLZ supercompression, or a Basis UASTC format in the DFD), it transparently proxies to an internal instance of BasisImporter. Flags and config options are propagated, I shamelessly copied that from AnyImageImporter 馃

Possible Todos

  • config option to turn this behaviour off, in case users want full control over which plugin is used
  • when openData() is called with DataFlag::Owned, there's currently no way to forward this to another importer, incurring a needless copy. A fix would depend on Zero-copy importer plugin APIs聽magnum#240

Related issue: #110.

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.
because transcoding all frames at once is probably a stupid idea. We also catch any attempt to seek ahead as an error, since basis doesn't allow seeking (all but the first image are P-frames).
鈥 Verbose is set

No test because I don't know how to reliably redirect stdout. There's still no way to redirect basis output to custom sinks.
# Conflicts:
#	src/MagnumPlugins/BasisImporter/BasisImporter.cpp
#	src/MagnumPlugins/BasisImporter/Test/BasisImporterTest.cpp
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.
Config options and flags are propagated to an internal instance of BasisImporter. All functions are then proxied to that internal instance until this instance is closed.
@mosra mosra added this to the 2021.0a milestone Oct 31, 2021
@mosra mosra added this to TODO in Asset management via automation Oct 31, 2021
@mosra mosra mentioned this pull request Oct 31, 2021
@mosra
Copy link
Owner

mosra commented Oct 31, 2021

Merged as 6fbb0b8...cefedba. Sorry for the long delays with everything, to avoid delaying even further with another round of reviews, I pushed this directly with a few things done differently:

  • The plugin is not passing through all options anymore as I imagine KtxImporter could eventually get its own options and this behavior may lead to a mess. On the other hand I don't expect BasisImporter to get that many new options over time, so there's now a [basis] section containing just the format option.
  • There's extra logic about the format delegation in that it tries to preserve the format globally set for BasisImporter, and prints a warning in case the two plugins have conflicting values, with KtxImporter having a priority.
  • I split out the "revert back to plain KTX" test into a dedicated test case, to not have the main instanced test do too much.
  • And tested the absence of a plugin in a dedicated case as well, since that's quite easy to simulate there.

Thank you for completing this huge chunk of work!

@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