Encode astc support #952
Encode astc support #952
Conversation
There was a problem hiding this comment.
Thanks @wasimabbas-arm. Can you now get metrics for ASTC compressed files?
You will need to add tests of the new functionality to the tools CTS and possibly need to update some existing tests.
wasimabbas-arm
force-pushed
the
astc_encode_support
branch
from
8f4b5a6
to
eb9ce30
Compare
| (kFormat, "KTX format enum that specifies the KTX file output format." | ||
| " The enum names are matching the VkFormats without the VK_FORMAT_ prefix." | ||
| " The VK_FORMAT_ prefix is ignored if present." | ||
| "\nIt can't be used with --codec and is only valid for ASTC encoding." |
There was a problem hiding this comment.
Delete "is only valid for ASTC encoding" as the start of the next sentence basically repeats this.
There was a problem hiding this comment.
I didn't think so. This says --codec and --format are mutualy exclusive options. The following says:
"\nThe format must be an ASTC format. When specified the ASTC encoder specific"
" options becomes valid."
This to me just says the format must be ASTC format, it doesn't talk about mutual exclusion with --codec
There was a problem hiding this comment.
Yes. I am not suggesting you remove "It can't be used with --codec", only the latter half of the sentence, which is what is repeated in the next sentence.
There was a problem hiding this comment.
I see. Happy to remove what you suggest.
Just adding my 2ps. I still think saying that the --format is only valid for ASTC encoding is still useful. Because here we are talking about the "option" itself while later we are talking about the "value" of the option. I am sure one can deduce this from "format needs to be only ASTC format means its not relevant for anything else bit" but thinking about someone who doesn't know this could trip on that.
There was a problem hiding this comment.
When I read it, it feels duplicative. The whole piece is talking about the --format option.
Change "The format must be" to "The value must be". I think then it is fine.
| } else { | ||
| ret = ktxTexture2_CompressBasisEx(texture, &options); | ||
| if (ret != KTX_SUCCESS) | ||
| fatal(rc::IO_FAILURE, "Failed to encode KTX2 file with codec \"{}\". KTX Error: {}", ktxErrorString(ret)); |
There was a problem hiding this comment.
The codec type argument is missing.
| options.mode = KTX_PACK_ASTC_ENCODER_MODE_LDR; // TODO: Fix me for HDR textures | ||
| ret = ktxTexture2_CompressAstcEx(texture, &options); | ||
| if (ret != KTX_SUCCESS) | ||
| fatal(rc::IO_FAILURE, "Failed to encode KTX2 file with codec \"{}\". KTX Error: {}", ktxErrorString(ret)); |
There was a problem hiding this comment.
Since this is for ASTC the message should say '... encode KTX2 file to "{}"'. Also the codec type (in this case format type) argument is missing.
|
I've merged the CTS changes. They only update some messages. Please add tests that use encode to encode KTX2 files to ASTC. |
|
@MarkCallow thanks. I will make the rest of the changes. Can I just ask. I know you have mentioned
But every time I have come back to this PR I wasn't sure how many things needs doing (apart from fixing those tests of course). And thats put me off a bit, and ended up procrastinating. Is there a complete list of things that I need to do for this? I am hoping to go through all at once to reduce lots of back and forth. At the moment I have
|
That only modifies 2 golden files to account for the changes in the text of some messages. We need to test the functionality of encoding to ASTC with I'll address the rest of the questions in your comment tomorrow. It's late here now. |
Updated version of #810.
Since #810 was adding a separate tool. Its easier to start another cleaner PR.
This PR has:
Commit to add --format option and make it exclusive with --codec
Add ASTC support to encode