ARM-software / astc-encoder

The Arm ASTC Encoder, a compressor for the Adaptive Scalable Texture Compression data format.
https://developer.arm.com/graphics
Apache License 2.0
1.06k stars 237 forks source link

Improve array argument #281

Closed wasimabbas-arm closed 2 years ago

wasimabbas-arm commented 3 years ago

The following -array <size> argument:

       -array <size>
           Loads an array of <size> 2D image slices to use as a 3D image.
           The input filename given is used is decorated with the postfix
           "_<slice>" to find the file to load. For example, an input named
           "input.png" would load as input_0.png, input_1.png, etc.

Is used to specify a set of textures that will be used to create a 3D ASTC texture. There are two things that can be improved about this argument:

  1. Array and texture array has a specific meaning withing Graphics APIs. Assuming this doesn't mean a texture array. This makes the name of this argument confusing. Perahps the argument itself should be called something -3D <Width x Height x Depth> or -3D <depth>
  2. The way input is accepted with this argument forces texture file names on users. It should access a list of names for a 3D texture.
solidpixel commented 3 years ago

Don't disagree with a rename, although it's an "API break" for anyone using the existing command line tool, so we might want to sit on it if it's not urgent.

Don't disagree with (2) either, but fixing it is going to be a pain without rewriting a significant chunk of the command line parsing as it means the mandatory arguments are no longer a fixed length. I really don't want to change the command line syntax "just" for 3D textures, as it will impact a lot of developers using the 2D functionality for a niche use case that is rarely used in practice.

What might convince me is rolling that up with a more generic scheme that can support all forms of structured input:

... plus the KTX/KTX2 output format support for all of that, and the test framework to validate it.

wasimabbas-arm commented 3 years ago

For backward compatibility one can leave existing argument as it is and add -3D <depth> as well that of course comes with its own issues.

it means the mandatory arguments are no longer a fixed length

Some CLIs takes an @list which could be a text file with all the image paths in it. It helps with fix command line but doesn't help with mismatch between <depth> and number of images in the list. Either way I think this should be the responsibility of the CLI to verify if the arguments provided are valid or not. You don't know if all the _<slice> images exist or not either.

Generic scheme for multi image input sounds interesting, you would still need some other argument to specifiy what you want with those.

solidpixel commented 3 years ago

Generic scheme for multi image input sounds interesting, you would still need some other argument to specifiy what you want with those.

Yes, this would be a change, but at least then it's one change in a future-proof direction.

... You don't know if all the _ images exist or not either.

Validating correct images for most of the composites can be fiddly (format and resolution constraints). I wouldn't expect the CLI handling to cope with validating all of that.

solidpixel commented 2 years ago

The -array argument has been renamed to -zdim for 4.0, which is at least a descriptive name. For the other use cases the current recommended option is to use the Khronos toktx utility, so there is no plan to add more container format to astcenc at this time.

Closing, as planned work is complete.