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 ability to compress textures with Basis Universal #16142

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

alexchuber
Copy link
Contributor

@alexchuber alexchuber commented Feb 4, 2025

This PR exposes the method EncodeTextureToBasisAsync which will encode a non-HDR, non-cube texture data to a KTX v2 image with Basis Universal supercompression.

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 4, 2025

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 4, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 4, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 4, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 4, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 4, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 5, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 6, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 6, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 6, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 6, 2025

* Initialize resources for the Basis Universal encoder.
* @returns a promise that resolves when the Basis Universal encoder resources are initialized
*/
export async function InitializeBasisEncoderAsync(): Promise<void> {
Copy link
Contributor Author

@alexchuber alexchuber Feb 7, 2025

Choose a reason for hiding this comment

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

Nitty, but any opinions about whether the encoder should be a class or not?

On the one hand,

  • It is stateful (workerpool management)
  • Module level functions clutter the BABYLON namespace and (I assume) require unique names

On the other,

  • Since this is a singleton, it makes sense to leave things at the module-level
  • Opportunity to move this all into basis.ts, our transcoder module that is also module-level. Only if you guys think so?

Copy link
Contributor

Choose a reason for hiding this comment

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

@RaananW will probably vote for no class :) (And I agree!)

@alexchuber alexchuber marked this pull request as ready for review February 7, 2025 18:11
@alexchuber alexchuber requested review from bghgary and RaananW February 7, 2025 18:32
* @returns a promise that resolves when the worker is initialized
* @internal
*/
export function initializeWebWorker(worker: Worker, wasmBinary: ArrayBuffer, moduleUrl?: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: I'd like to eventually move this to workerUtils and reuse, if possible, across other places... but perhaps sometime after this PR. I don't think this makes it to the public API right now, anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

yup let's keep this one under control :D

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 10, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 10, 2025

* @returns a promise resolving with the basis-encoded image data
* @experimental This API is subject to change in the future.
*/
export async function EncodeTextureToBasisAsync(babylonTexture: BaseTexture, options?: Pick<BasisEncoderParameters, "basisFormat">): Promise<Uint8Array> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered making the input just an ArrayBuffer[View] of 32-bit RGBA pixel data, but decided against it for now for ease of use (and so I do not have to check so many cases :))

We can always create another method later, should a user want to be able to pass raw image data.

Does this make sense?

*/
export async function EncodeTextureToBasisAsync(babylonTexture: BaseTexture, options?: Pick<BasisEncoderParameters, "basisFormat">): Promise<Uint8Array> {
// Wait for texture to load so we can get its size
await WhenTextureReadyAsync(babylonTexture);
Copy link
Contributor Author

@alexchuber alexchuber Feb 10, 2025

Choose a reason for hiding this comment

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

If a texture never loads (maybe it is delay-loaded), then this function will hang indefinitely. Is this OK?

On a higher-level note, I keep going back and forth over whether we should wait for the texture to load, or if the user should.
I initially thought that we should handle it internally, which is the case right now. But then I considered the delay loading case above, and now I'm not so sure.

(For context, the texture needs to be loaded for using readPixels and getting the texture's size.)

Copy link
Contributor

Choose a reason for hiding this comment

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

that's fine by me, the error will still be on the console for the user to debug. That being said, you can detect that the texture is delay loaded and force load it

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 10, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 10, 2025

@alexchuber alexchuber marked this pull request as draft February 11, 2025 20:12
@alexchuber
Copy link
Contributor Author

alexchuber commented Feb 11, 2025

Converting back to draft until we decide on a long-term plan to standardize how we use compression codecs.

Especially because we have a separate KTX2 decoder package, we think this would also be best as a separate package. Same goes for other codecs like Draco's.

Once that's sorted, I'll stage what's here accordingly and re-open.

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.

3 participants