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 JS bindings for full libktx #874

Open
wants to merge 68 commits into
base: main
Choose a base branch
from
Open

Add JS bindings for full libktx #874

wants to merge 68 commits into from

Conversation

aqnuep
Copy link
Collaborator

@aqnuep aqnuep commented Mar 22, 2024

This PR has been repurposed from adding the Phasmatic bindings for partial write functionality into adding bindings for nearly the full libktx functionality and a test for all the new functionality.

The PR makes the following minor API changes to libktx:

  • Adds ktxTexture_GetLevelSize function.
  • Makes the parameters to ktxTexture[12]_Create const.
  • Adds ktxTexture2_{GetPrimaries_e, SetOETF and SetPrimaries} functions.

It adds the following functions to the JS binding while modernizing and greatly improving the binding code:

  • A constructor equivalent to ktxTexture_Create.
  • createCopy
  • findKeyValue
  • getDataSize
  • getOETF
  • getPrimaries
  • getImage for retrieving an image from the texture. Fixes ktxTexture should expose its data via JS bindings #288.
  • setImageFromMemory
  • compressAstc
  • compressBasis
  • deflateZstd
  • deflateZLIB
  • addKVPairString and addKVPairByte
  • deleteKVPair
  • setOETF
  • setPrimaries
  • writeToMemory

It makes available two bindings. First is a read-only binding, libktx_read.js which has the previously available functions plus findKeyValue, getDataSize, getOETF, getPrimaries and getImage. The second, using the original name libktx.js, has functions for creating new KTX texture objects and writing them to files.

The JS ktxTexture class has been renamed texture as typical usage is with an Emscripten module instance name of ktx and new ktx.ktxTexture() looks weird. Newly added classes do not have ktx prefixes.

Compared to the Phasmatic bindings it:

  • Removes compressBasisU which was bundling calls to ktxTexture2_CompressBasisEx, ktxTexture2_DeflateZstd, ktxTexture2_WriteToMemory and ktxHashList_addKVPair. Instead each function now has a binding and can be called by the JS client.
  • Removes createFromBuffer which was using parameters passed from Javascript to determine the VKFormat and size of the texture then creating it with ktxTexture2_Create and finally loading an image from a passed ArrayBufferView into the texture via ktxTexture_SetImageFromMemory. Instead a ktxCreateInfo can be initialized in the JS code which can call new ktxTexture to create the texture then call setImageFromMemory to put the image into the texture object.

@aqnuep aqnuep requested a review from MarkCallow March 22, 2024 07:48
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
interface/js_binding/ktx_wrapper.cpp Outdated Show resolved Hide resolved
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.

I think the changes in these comments are all that are needed so ktx_js is only built when the corresponding feature is on.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
interface/js_binding/ktx_wrapper.cpp Outdated Show resolved Hide resolved
@aqnuep
Copy link
Collaborator Author

aqnuep commented May 2, 2024

Will it help to bracket these includes and any other write-related code in ktx_wrapper with#if KTX_FEATURE_WRITE?

For some reason I cannot reply on this comment, so replying here: what do you mean by that?

* Make createInfo parameter of ktxTexture{,1,2}_{Create,construct}
  constant so other language bindings can make it constant.

* Expose ktxTexture_calcLevelSize and ktxTexture_GetLevelSize. This
  became useful when KTX_FACESLICE_WHOLE_LEVEL was added to
  ktxTexture*_SetImageFrom* and should have been added then. It is
  prompted now by the addition of a getImage function to the JS
  binding.
@MarkCallow MarkCallow changed the title Port JS binding updates from Phasmatic Add JS bindings for full libktx May 31, 2024
@MarkCallow
Copy link
Collaborator

MarkCallow commented May 31, 2024

@abasilak, @ViNeek please review this PR. Please pay particular attention to the following:

  1. Will there be any difficulties using these bindings for the glTF-Compressor?
  2. I have changed the name of the factory function for creating the ktx module from LIBKTX to createKtxModule (and createKtxReadModule). I am not planning to provide the old name as an alias because it is a simple change for users to make. I have provided an alias. What is your opinion?
  3. In the test sample I am now setting window.ktx = <module returned from createKtxModule> so now I have code like var texture = new ktx.ktxTexture which is feels daft. I'd like to remove the ktx prefix from all the times that currently have it. What do you think? Note that the all the c++ code for the binding is in a ktx namespace. The extra prefixes really are superfluous in my opinion. I would, if I can figure out how, provide the old names as aliases. I have removed the ktx prefixes from the only existing class that had it, ktxTexture, and from the new classes and have provided an alias from ktxTexture to texture.
  4. Point out any errors or places for improvement in the code.
  5. Comment on the naming style of the classes, enums and values in the JS API.

I would like the glTF-Compressor to be updated to use this binding. Is there any room in your existing Khronos contract for that work? Better reply to me privately on this question.

@MarkCallow
Copy link
Collaborator

Here is a screenshot of the test for the new features.

Screenshot 2024-05-31 at 18 40 29

@ViNeek
Copy link

ViNeek commented Jun 4, 2024

Hi @MarkCallow thank you for the great work.

Regarding you comments:

  1. I can' t see any difficulties in integrating these changes to the gltf Compressor.
  2. This looks nice.
  3. Removing the prefix seems like a good, clean choice.
  4. The code seems very well structured and documented.
  5. I really like the ktx.SYMBOL style.

Test page also looks very good.

Use libktx names with `ktx{,_}` prefix and `_e` suffix removed.
Users will no longer need to remember or lookup the different
name used in the binding.
@MarkCallow
Copy link
Collaborator

@ViNeek thank you for your review and comments. I have made one final (fingers crossed) change. I have changed the enumerator names to be the same as the libktx names minus the ktx{,_} prefixes and _e suffixes for consistency and to make it easier for people to recall what the JS name will be. Please take a look at 6966f6c.

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.

ktxTexture should expose its data via JS bindings
3 participants