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

Update draco decoder #12178

Merged
merged 4 commits into from Sep 16, 2017
Merged

Update draco decoder #12178

merged 4 commits into from Sep 16, 2017

Conversation

fanzhanggoogle
Copy link
Contributor

Rework of PR. Addressed comments and update Draco loader/decoder to the latest 1.1.0 release.

Now picking decoder according support for wasm is done in DRACOLoader.js. This will make it much easier to use the loader.

@donmccurdy
@mrdoob
@FrankGalligan
@ondys

@hccampos
Copy link

Since this is dynamically loading JS or WebAssembly files, it will become even more painful to use whenever there is a bundling step involved (i.e. when using webpack). It was already non-trivial before due to the direct usage of the THREE global...but at least that you could accomplish via some loader tricks. Now it will required either removing all the JS loading code or replacing it with ES2015 imports (https://webpack.js.org/api/module-methods/#import-).

It would be great if a strategy would be put in place to better support bundlers like webpack or rollup since any non-trivial app these days requires bundling (and probably also transpiling, etc).

<script src="js/loaders/draco/DRACOLoader.js"></script>
<script src="js/loaders/draco/geometry_helper.js"></script>
Copy link
Owner

Choose a reason for hiding this comment

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

Why not include this inside DRACOLoader.js?

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 found out this was not used in this example. So removed the file and the line.

@donmccurdy
Copy link
Collaborator

It would be great if a strategy would be put in place to better support bundlers like webpack or rollup since any non-trivial app these days requires bundling (and probably also transpiling, etc).

I agree, but this isn't currently possible within the examples/js/* directory. This change was added in response to @mrdoob's feedback on a previous PR: "How about adding all this logic inside DRACOLoader so it works transparently for the user?", and IMO it is an improvement from that version.

For the short-term, since this is already in examples/js/*, how about saying DRACOLoader should only attempt to request decoder files if a path is provided in the constructor? If not, it would be up to the user to provide an appropriate decoder through a loader.setDecoder(decoder) call or something.

Longer term... I'm curious how loading WebAssembly with JS fallback in webpack or rollup works. I'd be interested to see a proof of concept on this. 🙂

(dracoDecoderType !== undefined) ? dracoDecoderType : {};
this.drawMode = THREE.TrianglesDrawMode;
this.dracoSrcPath = (dracoPath !== undefined) ? dracoPath : './';
THREE.DRACOLoader.loadDracoDecoder(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about adding an option to set the decoder explicitly:

var loader = new THREE.DRACOLoader(null, decoderType, manager);
loader.setDecoder(decoder);
loader.load(function () {
  // ...
});

And then either (a) making the constructor decoderPath optional, or (b) only providing it through a similar setDecoderPath(path) method.

This would (I think) provide an opt-out for users with a bundler, who don't necessarily control their folder structure.

Thoughts @mrdoob?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the latest commit, I added the condition that if DracoDecoderModule is defined then skip loading the decoder file. I think it could somehow solve this issue. But still looking at how to make it work with wasm.

@fanzhanggoogle
Copy link
Contributor Author

Hi @hccampos , thanks for pointing out this problem. We don't have much experience with JS bundler so far, so good to learn! We made a change to DRACOLoader.js that the dynamic loading will not occur if DracoDecoderModule is defined.
So you could include draco_decoder.js in the html and set the decoderType to 'js'. This should avoid the dynamic loading. It's kinda a temporary solution for use cases like webpack since it doesn't work with wasm. We'll look for better solution for both JS and WAMS decoder.

@mrdoob mrdoob merged commit e752669 into mrdoob:dev Sep 16, 2017
@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2017

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.

None yet

4 participants