KTX-Software icon indicating copy to clipboard operation
KTX-Software copied to clipboard

Port JS binding updates from Phasmatic

Open aqnuep opened this issue 1 year ago • 1 comments

aqnuep avatar Mar 22 '24 07:03 aqnuep

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?

aqnuep avatar May 02 '24 15:05 aqnuep

@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 avatar May 31 '24 08:05 MarkCallow

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

Screenshot 2024-05-31 at 18 40 29

MarkCallow avatar May 31 '24 09:05 MarkCallow

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.

ViNeek avatar Jun 04 '24 15:06 ViNeek

@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.

MarkCallow avatar Jun 05 '24 04:06 MarkCallow