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

Add normalise option to command create #977

Merged
merged 20 commits into from
Mar 3, 2025

Conversation

wasimabbas-arm
Copy link
Contributor

Fixes #812 partially. At the moment only adds support to create.

NOTE: I can't seem to work out how to add support to encode because its not using Image from ./tools/imageio/image.hpp its using streams to load this.

I have also tried to add this normalize option to ./tools/ktx/encode_utils_common.h first. You can see this in the first commit 53f728f. I am not sure if thats the way to go to see this option in both encode and create but it felt like its not an encoder (basisu, ASTC) specific option so feels misplaced.

The other option is to add another "general_util.h" which stores other commonly used options amongst the tools because it doesn't fit in all of the following categories either.

ktx create [OPTION...] <input-file...> <output-file>
Encode ASTC options:
Encode BasisLZ options:
Encode UASTC options:
Encode common options:
Generate Mipmap options:

@MarkCallow
Copy link
Collaborator

I think keeping encode focused on encoding and not adding image manipulations is the right choice. I.e normaiize only in ktx create is fine. I don't think ktxsc supported normalize either. It would have the same issue as it too doesn't use Image.

I can see at some point adding a function to make an Image from the data pointer in a ktxTexture2 texture object as the starting point to being able to manipulate the images in a KTX file. That is future work.

@MarkCallow
Copy link
Collaborator

MarkCallow commented Jan 28, 2025

Update create docs but feels like this has gone out of sync with the help text

The help output is a simplified version of the man page. They do need to be kept broadly in sync.

Any time you change the help you have to update the golden file in the CTS otherwise the create.help test will fail, as is happening here. This is separate from keeping help and man page in sync.

Copy link
Collaborator

@MarkCallow MarkCallow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wasimabbas-arm.

@wasimabbas-arm
Copy link
Contributor Author

@MarkCallow please review the CTS changes, most of the work was done there. KhronosGroup/KTX-Software-CTS#37

@wasimabbas-arm
Copy link
Contributor Author

Error: Unable to create a container for the artifact pyktx-4.3.2-cp311-cp311-win_amd64.whl at https://pipelinesghubeus24.actions.githubusercontent.com/P1YfxNQds3vsiK5yfr414BxgGxrn3zpqF8zRT7xD5N2qBPqBZC/_apis/pipelines/workflows/13117932387/artifacts?api-version=6.0-preview

This doesn't look like a related build issue, is it?

@MarkCallow
Copy link
Collaborator

This doesn't look like a related build issue, is it?

No it doesn't. It looks like a change to some part of the GitHub helpers.

@MarkCallow
Copy link
Collaborator

This doesn't look like a related build issue, is it?

No it doesn't. It looks like a change to some part of the GitHub helpers.

I reran the failed jobs but the same thing happened. I need to investigate. It's a piece of github functionality that is failing and, very typically of software generally, the error message gives no clue why the action is "unable to create a container."

Copy link
Collaborator

@MarkCallow MarkCallow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the suggested changes to the error handling and message.

@MarkCallow
Copy link
Collaborator

Error: Unable to create a container for the artifact pyktx-4.3.2-cp311-cp311-win_amd64.whl

Please rebase to main to get the fix for this.

@MarkCallow
Copy link
Collaborator

I am about to land PR #983 which updates the ASTC encoder to 5.2.0. After it lands you will likely have to regenerate the golden files in your tests.

Please finish this PR. I have almost completed the other items I want for the next release of this software and don't want to be held up by this.

@wasimabbas-arm
Copy link
Contributor Author

wasimabbas-arm commented Feb 24, 2025

@MarkCallow I am not sure what's happened to this PR.

Current status is that the following tests are failing:

The following tests FAILED:
	160 - ktx2check-test.all (Failed)
	205 - ktxsc-test.zcmp-cubemap (Timeout)
	533 - ktxToolsTest.extract.png_2d (Failed)
	581 - ktxToolsTest.create.basic_png (Failed)

All except cubemap ones are giving some clitest.py:20: SyntaxWarning: invalid escape sequence '\$' The cubemap one is timeout(ing).

And I have merged this with master, locally I don't have any conflicts. But Why is it not running the CI build? Its confusing to me why the merge conflict (if there is one really) is blocking a CI build (So I can't see what is actually failing if anything related). Its as if the CI build is only checking for CLA requirements now.

@MarkCallow
Copy link
Collaborator

And I have merged this with master, locally I don't have any conflicts. But Why is it not running the CI build? Its confusing to me why the merge conflict (if there is one really) is blocking a CI build (So I can't see what is actually failing if anything related). Its as if the CI build is only checking for CLA requirements now.

I think the CI build is not running because GH cannot merge the PR with main to create the tree it uses for the builds. I don't understand this well so I might be completely wrong.

The first step is that you need to rebase the CTS tests, force push the result to the CTS branch you are using then update the CTS reference (git commit tests/cts) in KTX-Software. (That conflict means both the update to KTX-Software that you are merging and your branch have modified the CTS reference.)

I don't know what the conflicts are in command_create.cpp. It tells me they are too complex to resolve in the web editor so it won't open it.

I am sorry for the hassle. I needed to merge the TF updates.

@MarkCallow
Copy link
Collaborator

All except cubemap ones are giving some clitest.py:20: SyntaxWarning: invalid escape sequence '\$' The cubemap one is timeout(ing).

I see a lot of these in the MingW test run. They don't seem to affect the running of the tests. I haven't noticed them anywhere else.

@wasimabbas-arm
Copy link
Contributor Author

Phew, that was painful. Anyways looks like everything pass now.

@MarkCallow MarkCallow merged commit 2f69183 into KhronosGroup:main Mar 3, 2025
20 checks passed
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 this pull request may close these issues.

ktx encode and create lost --normalize option
3 participants