KTX compare tool - phase 1 #815
Conversation
aqnuep
force-pushed
the
ktx-compare-dev
branch
4 times, most recently
from
b35e6a2 to
ea23f5d
Compare
Sorry about that. I've been waiting for @lexaknyazev's review. Since he already agreed to the behavior change to an error and the changes are fairly minor I'll merge it now. |
No worries, just wanted to be up-front to expect that unrelated change showing up in this PR. |
aqnuep
force-pushed
the
ktx-compare-dev
branch
from
ea23f5d to
77c715c
Compare
aqnuep
force-pushed
the
ktx-compare-dev
branch
2 times, most recently
from
0bccdd1 to
0fede0e
Compare
|
This PR is now fully tested with the tests in PR KhronosGroup/KTX-Software-CTS#13 and fixed accordingly. It should be ready for review, I just left the PR as draft because there will be a need to update the CTS commit reference. @MarkCallow, let me know if you have any questions or comments. |
Many small things including: * Update release notes. * Update pyktx README.md with note about macOS build warning. * Fix many small issues with pyktx deployment. * Fix pyktx version number to include normalized tweak. * Add tweak to version number in Android package names. * Fix typo in example in documentation. * Fix Emscripten build to meet new requirement to enable GetProcAddress via compile option. * Fix KhronosGroup#851: image corruption on loading. * Prepare for publishing pyktx to PyPI.
|
To fix the Travis build you will need to rebase to current main (4.3.0-beta1) which has the fix for the Emscripten build issue. |
There was a problem hiding this comment.
Looks good. I haven't done a really detailed review of the new code just gave myself an overview. I did find a couple of very minor things. I'm marking this "Request changes" because of the need to rebase to get the fix for the Emscripten change that has broken the Travis build.
| @@ -245,7 +245,7 @@ const char* dfdToStringColorModel(khr_df_model_e value) { | |||
| return NULL; | |||
| } | |||
|
|
|||
| const char* dfdToStringSampleDatatypeQualifiers(uint32_t bit_index, bool bit_value) { | |||
| const char* dfdToStringSampleDatatypeQualifiersBit(uint32_t bit_index, bool bit_value) { | |||
| if (!bit_value) | |||
There was a problem hiding this comment.
Why the rename? Wouldn't dfdToStringSampleDatatypeQualifierBits be more accurate than ...QualifiersBit?
There was a problem hiding this comment.
All other similar bit conversion functions have the Bit suffix. For some reason this one did not have it, so I fixed a consistency issue.
| @b raw - Encoded image data is compared verbatim, as it appears in the file. <br /> | ||
| @b image - Effective image data is compared per texel block. <br /> | ||
| @b ignore - Ignore image contents. <br /> | ||
| The default mode is @b raw, meaning that the encoded image data must match exactly. |
There was a problem hiding this comment.
Should explain "effective image data" in an additional sentence following "exactly."
There was a problem hiding this comment.
This part will go through some major changes in phase 2, so I'll leave it as is for now, because a lot of things will get clarified in the meantime.
Refine size calculation.
by creating a .nojelkyll file when deploying docs. Fixes KhronosGroup#825.
This fixes KhronosGroup#827, the conflicting permissions in the directories that lead to impossibility to install the package.
Draft implementation for the compare tool with no image comparison yet. Basic test set covering the following: * Help * Command line errors * Use with invalid files without the "allow-invalid-input" flag * Use with invalid files with the "allow-invalid-input" flag * Unicode filename support
aqnuep
force-pushed
the
ktx-compare-dev
branch
from
4c9ad53 to
2f0484c
Compare
|
FYI: since it seems this and the corresponding CTS PR KhronosGroup/KTX-Software-CTS#13 is unlikely to get merged before we proceed with the image diff part of the implementation, we've started working on that here, so likely we'll have one pair of large PRs in the end. |
Draft implementation for the compare tool with no image comparison yet.
Basic test set covering the following:
Note: This PR contains the changes in PR #809 too, because the CTS is based on the CTS main branch which already got merged, yet the code wasn't, so there was no other way to make tests pass otherwise.