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

GLTFLoader: Implement support for EXT_meshopt_compression #20508

Merged
merged 9 commits into from
Oct 27, 2020
Merged

GLTFLoader: Implement support for EXT_meshopt_compression #20508

merged 9 commits into from
Oct 27, 2020

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Oct 14, 2020

EXT_meshopt_compression is a recent extension that supports compressing
glTF data - geometry, animation, instance transforms - in a generic fashion.

Specification:
https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Vendor/EXT_meshopt_compression

It is possible to produce files that use the extension using gltfpack
(https://github.com/zeux/meshoptimizer/tree/master/gltf). To decompress
the files, loader needs to decode the bufferView data - the extension
specification details the compressed format, but meshoptimizer library
provides a portable decoder that can be readily consumed from JS; the
decoder requires WebAssembly and will automatically dispatch to Wasm SIMD
when available, or use MVP otherwise.

meshopt_decoder.js is a drop-in copy of
https://github.com/zeux/meshoptimizer/blob/master/js/meshopt_decoder.js;
since it implements the specified format, it can be updated at any point
without loss of backwards compatibility.

The PR for EXT_meshopt_compression in glTF repository
(KhronosGroup/glTF#1830) contains file sizes for
a lot of example models - the benefit of using this extension is that it
supports compressing every possible type of glTF binary data except for
images, and can decode at breakneck speeds (~1 GB/s when using Wasm SIMD
on a recent desktop Intel CPU) while only requiring a very small decoder
binary (meshopt_decoder.js is at 21 KB raw, 6 KB deflated, while
including two builds of the decoder, scalar and SIMD).

As an example, I've tested this PR on BrainStem model from
glTF-Sample-Models repository that contains animation data. The original
file size is 3120 KB, Draco-compressed version is 1942 KB, and
meshopt-compressed version is 362 KB, produced by gltfpack with -cc
option. The large size delta is due to animation data being compressed
with the extension; the loader extension decompresses bufferViews upon
request and the rest of the loader pipeline works without further
changes.

To integrate the extension after this change the user simply needs to
include the module, and pass the exported MeshoptDecoder global to the
loader:

import './js/libs/meshopt_decoder.js';
...
loader.setMeshoptDecoder( MeshoptDecoder );

EXT_meshopt_compression is a recent extension that supports compressing
glTF data - geometry, animation, instance transforms - in a generic
fashion.

Specification:
https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Vendor/EXT_meshopt_compression

It is possible to produce files that use the extension using gltfpack
(https://github.com/zeux/meshoptimizer/tree/master/gltf). To decompress
the files, loader needs to decode the bufferView data - the extension
specification details the compressed format, but meshoptimizer library
provides a portable decoder that can be readily consumed from JS; the
decoder requires WebAssembly and will automatically dispatch to Wasm SIMD
when available, or use MVP otherwise.

meshopt_decoder.js is a drop-in copy of
https://github.com/zeux/meshoptimizer/blob/master/js/meshopt_decoder.js;
since it implements the specified format, it can be updated at any point
without loss of backwards compatibility.

The PR for EXT_meshopt_compression in glTF repository
(KhronosGroup/glTF#1830) contains file sizes for
a lot of example models - the benefit of using this extension is that it
supports compressing every possible type of glTF binary data except for
images, and can decode at breakneck speeds (~1 GB/s when using Wasm SIMD
on a recent desktop Intel CPU) while only requiring a very small decoder
binary (meshopt_decoder.js is at 21 KB raw, 6 KB deflated, while
including *two* builds of the decoder, scalar and SIMD).

As an example, I've tested this PR on BrainStem model from
glTF-Sample-Models repository that contains animation data. The original
file size is 3120 KB, Draco-compressed version is 1942 KB, and
meshopt-compressed version is 362 KB, produced by gltfpack with `-cc`
option. The large size delta is due to animation data being compressed
with the extension; the loader extension decompresses bufferViews upon
request and the rest of the loader pipeline works without further
changes.

To integrate the extension after this change the user simply needs to
include the module, and pass the exported MeshoptDecoder global to the
loader:

	import './js/libs/meshopt_decoder.js';

	...

	loader.setMeshoptDecoder( MeshoptDecoder );
@zeux
Copy link
Contributor Author

zeux commented Oct 14, 2020

Note: based on the requests for my previous extension PRs I've omitted the example code/data; if you'd like to test this yourself and don't want to use gltfpack, I'm attaching a .zip with a .glb file (that should go to models/gltf) and an .html file (that's a quick edit of the existing animation example) for reference.

If you do want to use gltfpack to play around with this, note that this requires a recent gltfpack build, you can grab one from the artifacts here https://github.com/zeux/meshoptimizer/actions/runs/302546580 - gltfpack 0.15 will be released later this month on NPM that adheres to the final extension spec.

image

meshopt-test.zip

@mrdoob mrdoob added this to the r122 milestone Oct 14, 2020
… decoding

This makes sure that files with EXT_meshopt_compression used as a
non-required extension don't fail to load.
Long term we might want to have a way to indicate failure from a plugin,
but for now this seems reasonable.
@zeux
Copy link
Contributor Author

zeux commented Oct 15, 2020

The lint error is

Error: src/cameras/CubeCamera.d.ts(16,2): error TS2416: Property 'clear' in type 'CubeCamera' is not assignable to the same property in base type 'Object3D'.

Which doesn't look related to this change, so presumably a different change was merged that introduced this.

@donmccurdy
Copy link
Collaborator

Thanks @zeux! Excited to have this option available by default.

import './js/libs/meshopt_decoder.js';

@gkjohnson does this import method (side effects only) raise any red flags for issues you know of? I guess I'm wondering, if we're distributing the file with three.js so that it'd be imported from three/examples/js/lib/*, and we later want to put "sideEffects": false in our package.json, is it going to break?

For comparison, we made ZSTDDecoder a module (#19932) and didn't create a global/UMD version.

Note: based on the requests for my previous extension PRs I've omitted the example code/data;

At some point I think EXT_meshopt_compression should be included in one of the "extensions" examples: https://threejs.org/examples/?q=gltf#webgl_loader_gltf_extensions. Perhaps just adding a single additional version of one of the models already there, for QA testing purposes — it doesn't have to be included with this PR, though.

Which doesn't look related to this change, so presumably a different change was merged that introduced this.

Agreed, the lint error can be ignored.

@gkjohnson
Copy link
Collaborator

@donmccurdy

does this import method raise any red flags for issues you know of ... later want to put "sideEffects": false in our package.json, is it going to break?

I haven't tested all of this but here's what I imagine might happen: If a user uses this with a build process that supports commonjs like Webpack I believe they would have to use a named import syntax because the build process will provide a module context and fall into this condition. In something like rollup, however, which doesn't support commonjs without a plugin it's a bit more complicated. Named imports would have to be used only if the commonjs plugin is used but the global variable approach must be used if it is not. In the second case if rollup is set to respect the sideeffects: false flag then I would expect the import './js/libs/meshopt_decoder.js'; to be culled from the build because it doesn't import anything by name which would cause things to break.

I guess bottom line is is that depending on your build process you will have to use import './js/libs/meshopt_decoder.js'; or import MeshoptDecoder from './js/libs/meshopt_decoder.js'; and understanding the conditions in which you have to use one or the other will probably require a deeper understanding how your build setup works, sideeffects, and UMD module behavior which is confusing enough on it's own. Further if we add an example to this project we will have to use the import './js/libs/meshopt_decoder.js'; notation in the browser which may be confusing to those trying to apply the example to an app that uses a build process.

My preference would be to provide a named import file similar to ZSTDDecoder so a consistent import method can be used across browsers and build environments.

For comparison, we made ZSTDDecoder a module (#19932) and didn't create a global/UMD version.

With the conclusions reached in #20455 what should the plan be for supporting these libraries with the classic script tag? One solution could be to keep the canonical version as a module and use a rollup build to generate a UMD version of them. Of course we could just maintain two copies, as well.


And a small tangent but related to our discussion over on the forum here it looks like both the ZSTDDecoder and MeshoptDecoder wasm bundles are already base64 encoded which means they can be imported directly. And with WebPack 5 now automatically resolving publicPath in modern browsers these might be good cases to now consider using the dynamic import() statement for loading these files.

@zeux
Copy link
Contributor Author

zeux commented Oct 15, 2020

FWIW the reason why the import is state-modifying is that the .js file in question has a UMD-style declaration, so you can load it in a node environment using require, in a browser environment pre-ES imports using any of the existing import libraries, in a browser environment without any import libraries by just using <script> tag, or using an ES-style import. Unfortunately you can not export a symbol conditionally which forced me to resort to the solution that's present here.

The alternative here could be to change the UMD-style export at the end of meshopt_decoder.js to a ES6 export statement, which would force the user to use modules.

@gkjohnson
Copy link
Collaborator

FWIW the reason why the import is state-modifying is that the .js file in question has a UMD-style declaration,

Oh definitely it's a limitation of UMD sorry I didn't mean to imply otherwise -- just trying to look at cases where it might break or be confusing considering all the environments three.js aims to support. Aside from the .wasm files I don't think there are any files even in libs and can't be used via named import so switching to an export tag would be more consistent in this case.

Up until recently the plan was actually to remove all the global script files in the /js directory (see #20455 for more context) but it seems that now at least the plan is to make the JSM files the canonical versions of the examples files.

@zeux
Copy link
Contributor Author

zeux commented Oct 15, 2020

I can move the decoder js to jsm/libs/meshopt_decoder.module.js and change UMD to an export; this would require ES6 imports to use. The original plan for modules, I think, was to drop the plain JS files, but the issue linked suggests that it may not be the plan anymore, so I'm slightly concerned about this - is that something to worry about?

Alternatively I can add a module version to jsm/libs and keep the original one?

@gkjohnson
Copy link
Collaborator

The original plan for modules, I think, was to drop the plain JS files, but the issue linked suggests that it may not be the plan anymore, so I'm slightly concerned about this

I'm not exactly sure what the plan is -- it was left a bit ambiguous beyond the plan being to transition to build the /jsm directories to /js rather than the reverse which happens now. Of course I don't have the final say on how all this should work but I feel that at least a /jsm variant should be provided. It would be consistent with the way ZSTDDecoder works, as well.

This allows to use a ES6 style import that doesn't modify globals, e.g.

import { MeshoptDecoder } from './jsm/libs/meshopt_decoder.module.js';
@zeux
Copy link
Contributor Author

zeux commented Oct 15, 2020

Updated the PR with a module version of the decoder that uses ES6 export, so you can import like this:

import { MeshoptDecoder } from './jsm/libs/meshopt_decoder.module.js';

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

I'm not exactly sure what the plan is -- it was left a bit ambiguous beyond the plan being to transition to build the /jsm directories to /js rather than the reverse which happens now.

I'm not planning to add any new files to /js, but to add new features to /jsm instead. So KTX2Loader and ZSTDDecoder would remain available as ES modules, only. If we do set up automation that generates /jsm -> /js then those files could perhaps be included in that, TBD.

examples/js/loaders/GLTFLoader.js Outdated Show resolved Hide resolved
examples/js/loaders/GLTFLoader.js Outdated Show resolved Hide resolved
Instead of changing the extension parsing loop, we now use the fallback
only if extension is not present in the required list.
@zeux
Copy link
Contributor Author

zeux commented Oct 27, 2020

@mrdoob @donmccurdy lmk if I need to do anything else here

@donmccurdy
Copy link
Collaborator

No concerns here, functionally looks correct to me. 👍

@mrdoob if you'd prefer to stick with the earlier decision of not adding new libraries in examples/js, I think we could also release this with just the examples/jsm dependency for now and (optionally) generate the examples/js version later on.

@mrdoob
Copy link
Owner

mrdoob commented Oct 27, 2020

@mrdoob if you'd prefer to stick with the earlier decision of not adding new libraries in examples/js, I think we could also release this with just the examples/jsm dependency for now and (optionally) generate the examples/js version later on.

Nah, I think adding new libraries in examples/js in this case is good.

@mrdoob
Copy link
Owner

mrdoob commented Oct 27, 2020

Ops. Seems like the EXT_texture_webp PR created conflicts... @zeux could you resolve them?

@mrdoob mrdoob merged commit 9bb2226 into mrdoob:dev Oct 27, 2020
@mrdoob
Copy link
Owner

mrdoob commented Oct 27, 2020

Thanks!

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.

4 participants