DRACOLoader: Use relative file urls by default.#33564
Conversation
Why do you want to drop support for the non-WASM path?
Are there use cases where you want to specify a custom path and not rely "coupled" decoder. |
|
Not reviewed closely yet (I think we'll want to try in several bundlers, and perhaps Node.js...) but I support the direction. I'd also be in favor of dropping the larger/slower JS path. WASM has been available across browsers since 2017 now and is baseline. I do think we should keep some version of a |
Same reason we drop support for any generally unused or unneeded feature: it's less code in the project and simplifies some of the conditions here. Is there a reason we should keep it? As Don has mentioned, the WASM path should be supported everywhere.
There are still some bundlers that do not "natively" support this kind of asset hint so I agree we will still need this until this "just works" everywhere. Is there another reason, though?
Node JS I'm unsure about, I suppose, but as far as I know this usage of URL & import.meta.url is valid in all modern bundlers. Though Rollup and ESBuild won't necessarily see it as an asset that needs to be bundled and loaded but it shouldn't break (sorry somewhat misstated before). In these cases the existing "setDecoderPath" can be used if needed. |
For dependencies managed by npm, it's an important option that users can override the dependency resolution, e.g. with yarn resolutions: for investigation with debug builds, testing changes to the dependency, or substituting an equivalent dependency. We're not using npm dependencies of course, but I think it's preferable to offer the same flexibility. Particularly since this is a large and complex dependency. For https://github.com/donmccurdy/three-gltf-viewer, I override the path because hosting large static WASM binaries is expensive, and can be offloaded to a CDN or Object Storage.
No strong opinion about Node.js at this point! I agree the support "should" be there in bundlers, I just think we should test if this is the first time we're using this strategy. Most bundlers claim to support handling assets for Web Workers, similarly, but in practice I've found some tricky inconsistencies in their implementations in the past few years. But I'm hopeful WASM should be fine. |
It's likely the case that DRACOLoader already won't work in node without some kind of shim, anyway.
Webworkers are a different beast and I'm still incredibly disappointed by the inability to use them practically in packages, but that's a different rant. Dynamic imports have also historically had problems but perhaps that's improved recently. I've used claude to set up configs for the major bundlers and evaluate whether they can load a wasm file using the above "URL" syntax. The built files are run in node using a shim for "fetch". You can see the test set up here but the results are what I've mentioned above:
One final thing to maybe be aware of is that vite will automatically inline any asset urls that are under 4kb (meaning these files may be included even if they are never actually loaded) but that's configurable with assetsInlineLimit. A possible problem seems to be that Vite will bundle that file regardless of size if set to "library" mode but imo that's a vite problem. |
|
Any further thoughts on this? |
|
Maybe it's best to give it a try and see how this change works in production? |
The I've tried Vite's "library" mode a few times, and each time it hasn't really seemed ready for non-trivial use. I'm not too worried about that part. For applications Vite is great. I would recommend other tools for someone building a library. OK with me to go ahead! |
It occurs to me that this won't really be an issue unless this project is being built to a library. Any other libraries for which DracoLoader is a dependency should still be imported with a bare import. |
| const WASM_BIN_URL = new URL( '../libs/draco/draco_decoder.wasm', import.meta.url ).toString(); | ||
| const WASM_JS_URL = new URL( '../libs/draco/draco_wasm_wrapper.js', import.meta.url ).toString(); | ||
| const JS_URL = new URL( '../libs/draco/draco_decoder.js', import.meta.url ).toString(); | ||
|
|
There was a problem hiding this comment.
@donmccurdy - I'm realizing that there are two draco loader decoders in this project: libs/draco/* and libs/draco/gltf/*.
What's the difference between these two? Is the glTF version the same as the basic "draco" one but with some unnecessary code stripped out? Does it make sense to adjust all the examples to remove the "setDecoderPath"? It's a bit unfortunate that we have to have two decoders when the gltf path is likely the most commonly used one. I'm wondering what to do here.
There was a problem hiding this comment.
See:
If that's still correct, then the "draco" decoder is a superset of the "draco/gltf" decoder, and "draco/gltf" may lack support for decoding point clouds. The glTF KHR_draco_mesh_compression extension doesn't include point cloud compression.
There was a problem hiding this comment.
Got it - so it's purely a file size issue and the non-glTF version seems like the right default.
I'm wondering what we should do to enable users to specify using the "gltf" variant, then? Looking some issue history it looks like #24946 or #31446 could be viable. Generally I don't think it's great that these classes require and examples demonstrate the use of functions like setDecoderPath that don't work in any bundlers.
There was a problem hiding this comment.
This is still correct – the gltf variant is smaller and supports only the subset of Draco that is allowed in glTF, whereas the other one supports all of the Draco spec (typically .drc files).
There was a problem hiding this comment.
Any thoughts on how we should cleanly enable using the gltf-draco-decoders in a bundler-friendly way? Looking around a bit more I'm actually not sure if bundlers support the same kind of URL* syntax with bare modules. Eg:
loader.setDecoderFiles(
new URL( 'three/addons/libs/draco/decoder.js' ).toString(),
new URL( 'three/addons/libs/draco/decoder.wasm' ).toString(),
);So we're back to either copying the files using CDNs if users want to use the gltf files (which they should). A simple option could just be a flag that toggles the default URLs:
const dracoLoader = new DRACOLoader();
dracoLoader.useGLTFVariant = true;
// loader will now load the gltf versions rather than the full draco versionsOpen to any other thoughts, though.
Related issue: #33140 (comment), #31446
Fixed #26403.
Description
This PR allows
DRACOLoaderto be used without callingsetDecoderPathby usingnew URL( 'filename.js', import.meta.url ).toString()approach, which all modern bundlers support, to enable specifying a relative path to the /libs files. This will improve the ergonomics of using class, which is currently quite difficult with a number of bundlers or (more commonly) requires pointing to a CDN. The PR is structured to backwards compatibility and allow for users to continue specifying their own config paths if needed.Some open questions (though not blocking)
Once merged I can submit future PRs for