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

(WIP) KTX2Loader: Replace manual transcoder path with dynamic imports #24946

Closed

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Nov 13, 2022

Related

Description

This is a "proof-of-concept", removing the requirement for users to configure DRACOLoader and KTX2Loader with static decoder paths. Instead, we'd use dynamic imports. This approach does not work within three/examples/js, and so this would be blocked on the move to three/addons / three/examples/jsm.

To my understanding, modern browsers and bundlers now support dynamic imports of ES Modules pretty well. That solves part of the problem. But we also need to import the .wasm binaries, and it appears browsers and bundlers diverge on how to do that:

  • Browsers require us to load WASM with fetch() (MDN)
  • Bundlers expect us to load WASM with import() (parcel, webpack, vite)

Perhaps that's not a problem, as long as we can detect which case we're in? I'm not sure how to do that yet.

The goal would be — From a CDN, relative paths should be resolved automatically. For developers copying files around manually on disk, they'll need to specify a path. For developers who are using a bundler, the presence of (even conditional) dynamic imports should cause the bundler to put the JS and WASM in the correct place within the final build. Or that's the hope ... the next step would be testing this against major bundlers, if we want to go down this path.

/cc @gkjohnson

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented Nov 13, 2022

This is a fragile area of Vite ATM vitejs/vite#10837 => vitejs/vite#10839, causing broken behavior in production builds but not dev mode. I haven't tested the WASM case, but the second issue indicates that local or absolute paths (as currently implemented) should work.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Nov 14, 2022

To my understanding, modern browsers and bundlers now support dynamic imports of ES Modules pretty well.

I think this needs to be validated. My understanding is that bundlers like webpack require a statically declared line so it can be analyzed and referenced file chunked at compile time. Using dynamic paths as is done in this PR seems like it would break this functionality. Ie we'd need the import url to be specified in the import statement itself:

import( new URL( '../../js/libs/basis/basis_transcoder.js', import.meta.url ) ), ...

I'll also say at some point that it would be nice to define the workers as an external file instead of constructing string blobs in the file. I think it would make things more understandable and maintainable but there may be some other limitations when it comes to CDNs that would need to be figured out...

I'm excited to see this happening!

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Nov 14, 2022

My understanding is that bundlers like webpack require a statically declared line so it can be analyzed and referenced file chunked at compile time.

Hm, that's possible. I was hoping the static URL alone might be enough. So something like this would be sufficient?

if ( config.jsPath ) {

  await import( config.jsPath );

} else {

  await import( new URL( 'basis_transcoder.js', import.meta.url ) );

}

In any case we'll have to test it, yeah.


...it would be nice to define the workers as an external file...

If we need fully static imports, then I think putting the workers in an external file would be a requirement, rather than using our current stringified worker bodies. That sounds nice for a few reasons (and should be fine with CDNs?) but I'm not sure if the bundler support is there yet.

Module Expressions would also do the job, and feel simpler to me, but are still a proposal at this stage.

@gkjohnson
Copy link
Collaborator

So something like this would be sufficient?

I'd think that should work, yeah.

That sounds nice for a few reasons (and should be fine with CDNs?) but I'm not sure if the bundler support is there yet.

I know at least webpack and parcel support this style of imports for webworkers (webpack/webpack#6472 (comment), parcel-bundler/parcel#5430) though I'm not sure of status on some other bundlers.

For CDNs - you'd think there wouldn't be issues. But browser's don't seem to let you instantiate web workers from remote origins for some reason... See this fiddle (https://jsfiddle.net/2eqkhx4z/) which throws this error when instantiating the threes-mesh-bvh web worker:

Uncaught DOMException: Failed to construct 'Worker': Script at 'https://unpkg.com/three-mesh-bvh@0.5.16/src/workers/generateAsync.worker.js' cannot be accessed from origin 'https://fiddle.jshell.net'.
    at new GenerateMeshBVHWorker (https://unpkg.com/three-mesh-bvh@0.5.16/src/workers/GenerateMeshBVHWorker.js:9:17)
    at https://fiddle.jshell.net/_display/?editor_console=true:123:16

It seems that worker URLs must adhere to the "same-origin" policy and that a wildcard is not good enough? Some references:

I'm not sure if there's a clean workaround at the moment but it's really hard to not feel like the web ecosystem has conspired to make WebWorkers impossible to cleanly use. The security feature here seems a bit nonsensical, too, since I can load JS from the origin and evaluate it in the main context. And presumably through module imports at remote urls in the worker, as well.

@arpu
Copy link

arpu commented Feb 24, 2023

hi have done some testing to include basis wasm in our js build, same as meshopt do it
using https://gist.github.com/arpu/2506379fb87b847af423d3d841ef798b as the basis wasm binary source

using https://github.com/zeux/meshoptimizer/blob/master/tools/wasmpack.py

@donmccurdy
Copy link
Collaborator Author

Blocked by limited bundler support for WASM, and Web Worker API's poor usability for libraries. It's pretty frustrating that these APIs are not designed for practical use in libraries, as opposed to applications. Closing PR for now.

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

Successfully merging this pull request may close these issues.

None yet

4 participants