KTX2Loader: Add setTranscoderUrls() for custom transcoder URLs#31446
KTX2Loader: Add setTranscoderUrls() for custom transcoder URLs#31446saqsun wants to merge 3 commits into
Conversation
|
@donmccurdy Looking good to you? |
|
Thanks @saqsun! I agree that being able to specify one or more file paths, not folder paths, would be better for bundlers. It might be ideal if only a single https://web.dev/articles/bundling-non-js-resources#universal_pattern_for_browsers_and_bundlers ... but I suspect that is more complex and require us customizing the transcoder build, which we probably don't want to do. So assuming we want to go with the approach in this PR instead, all that I would change would be the naming. Elsewhere (as in LoadingManager) we use all-uppercase URL in methods like .setURLModifier and .resolveURL, and the method is still accepting paths and not full URLs, so I'm not sure "URL" is a good way to differentiate... perhaps one of these? // (a)
.setTranscoderPath('path/to/script.js', 'path/to/binary.wasm')
// (b)
.setTranscoderPath({js: 'path/to/script.js', wasm: 'path/to/binary.wasm'})Note that THREE.DRACOLoader might benefit from the same changes, but that doesn't need to be included in this PR. |
|
@donmccurdy Thanks for the feedback! 🙌 I've updated the API to use the object-based signature as suggested: .setTranscoderPath({ js: 'path/to/script.js', wasm: 'path/to/binary.wasm' });I agree this version is clearer, more extensible, and aligns well with modern API conventions, similar to PixiJS v8’s Also, I totally see what you meant about the ideal setup where only a single Note: This is a breaking change, previous usages will need to be updated. |
|
Sorry for the delayed response. Thinking about this further, I'd like to spend a "breaking change" on this API only if the result would be that users with modern tools would not be required to configure the path at all (using the web.dev pattern), perhaps with the option of setting the path kept for users of older tools. But I don't think this PR needs to go that far. It would be a simpler first step to support the two-path API you're requesting, with a fallback if only a single path is given, to avoid a breaking change. I think my preference then would be: .setTranscoderPath(scriptOrDirectoryPath: string, wasmPath?: string)@Mugen87 @gkjohnson just want to confirm with you that this sounds OK? |
|
@donmccurdy this sounds good to me. I prefer the more explicit paths for the sake of bundling - and I mention it every once and awhile but I also want to raise the idea of switching the internals of the DRACO and KTX2 loaders to use a The blocking issue previously was that the import.metal syntax may not be supported in Webpack 4 but at this point Webpack 5 was released 5 years ago and there is a plugin for supporting import.meta in Webpack 4 so I think it's time (if not past) to keep the project and import ergonomics moving forward and not be held back by old and unsupported bundlers. If we switch to this model then the |
|
Related: @gkjohnson I think I agree that we could ship Also OK with merging this PR after the changes in #31446 (comment), as a quicker fix. |
Why would would a change be needed? |
|
Oh, yes that might be enough then. There's one note in the Vite docs about SSR (and presumably Node.js more broadly) but we're depending on Web Workers in both KTX2 and Draco loaders already so I don't think that's a concern. |
|
Thanks everyone for the detailed discussion, just to confirm that I understand the next steps correctly:
// Old behavior (folder)
loader.setTranscoderPath('/path/to/folder/');
// New behavior (explicit files)
loader.setTranscoderPath('/path/to/script.js', '/path/to/binary.wasm');This provides flexibility for bundlers and single-file setups without changing existing usage.
new URL('./basis_transcoder.js', import.meta.url)so that modern bundlers (Vite, Webpack 5, Rollup, etc.) can handle these dependencies automatically, and setTranscoderPath() would remain only for legacy overrides. If so, I can adjust the PR to the two-path version. @donmccurdy please let me know. |
|
I'm wondering if #33603 address the intent behind this PR? It will no longer necessary to call "setTranscoderPath" when instantiating |
What
Adds a new method
setTranscoderUrls()toKTX2Loader:This method allows to provide explicit URLs for the transcoder JS and WASM files, instead of relying on fixed filenames and directory paths.
Why
The existing
setTranscoderPath()method assumes thatbasis_transcoder.jsand.wasmare located in the same directory and have fixed names. This is not always suitable for:setTranscoderUrls()provides more control and flexibility while remaining fully backward-compatible.Implementation Notes
setTranscoderUrls()is used, it overridessetTranscoderPath().