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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java bindings: Invalid check for ASTC input swizzle parameter #969

Closed
javagl opened this issue Dec 31, 2024 · 3 comments · Fixed by #972
Closed

Java bindings: Invalid check for ASTC input swizzle parameter #969

javagl opened this issue Dec 31, 2024 · 3 comments · Fixed by #972

Comments

@javagl
Copy link
Contributor

javagl commented Dec 31, 2024

(Just some sort of "TODO" for me, to keep track of this: )

The KtxAstcParams#setInputSwizzle function currently does a basic validity check for the given argument. This includes a check that the input only contains the characters rgba01. This breaks for the case where this value is initialized with its default value, which is a char[] array that contains only 0 values (in contrast to '0' characters).

For example:

KtxAstcParams a = new KtxAstcParams(); // All default
KtxAstcParams b = new KtxAstcParams(); // All default
b.setInputSwizzle(a.getInputSwizzle()); // This should work!

This will currently throw an IllegalArgumentException.

I'll try to fix this soon, either as a mini-PR, or as part of a larger PR.

@javagl
Copy link
Contributor Author

javagl commented Dec 31, 2024

Completely unrelated to the issue above, but as another ~"small thing that I'll probably fix soon":

There currently is a constant KtxPackAstcBlockDimension.D6x4. I think that this should be called D6x5. I'll rename that, unless someone disagrees...

@MarkCallow
Copy link
Collaborator

Please fix these soon. We'll be making a release in early February, hopefully.

@javagl
Copy link
Contributor Author

javagl commented Jan 1, 2025

@MarkCallow OK, given that this is only a small fix, I created a PR at #972 .

(Otherwise, I would have added this to #968 , but it's hard to give an exact timeline for that. It might be that in that process, further "small fixes" might be necessary, but we can address them when they come up).

Regarding the inputSwizzle: I already did have a look at this, in the context of #876 . It can be a bit tricky to "follow the path of the swizzle" here: It is taken from the command line as input-swizzle, stored as inputSwizzle (which is supposedly always a char[4], with 0-elements by default), passed on as swizzle (which then is a string), makes its way through a swizzle_array (as a vector<astcenc_swz*>), and ends up as swz (a astcenc_swizzle). For the "basis" swizzle, the path is different. And at each point, different checks and conversions are performed, handling the case of a "non-default swizzle" in different ways...

tl;dr: The main source of confusion here was probably that

  1. the swizzle parameter is initialized to a char[4] array with all 0 elements by default
  2. this is converted into a string at some point, resulting in an empty string for the default, which means "no swizzle"

So, strictly speaking, the documentation at https://github.khronos.org/KTX-Software/libktx/structktxAstcParams.html#a0d67004efa49e08a7f39f71c93286b0a that says

char inputSwizzle[4]

A swizzle to provide as input to astcenc. It must match the regular expression /^[rgba01]{4}$/.

should/could include the hint that the default value is a char[4] (0,0,0,0), and this is also an accepted value (similarly for ktxBasisParams at https://github.khronos.org/KTX-Software/libktx/structktxBasisParams.html#a0d67004efa49e08a7f39f71c93286b0a )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants