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 KHR_draco_mesh_compression #13194

Merged
merged 15 commits into from
Feb 16, 2018

Conversation

donmccurdy
Copy link
Collaborator

Supports use of Draco compression on geometry in a glTF asset. Specification. Demo.

I'm planning to include a dedicated example (loaders / gltf / draco) but haven't got that ready yet. This PR just includes the Draco extension as an option on several of the existing examples. Up for discussion whether we want to merge with with these, remove the examples and add them in a future PR, or wait for a nicer dedicated example.

Thanks @fanzhanggoogle for support on this PR and the rest of the Draco team. 🙂

/cc @takahirox

@mrdoob mrdoob added this to the r91 milestone Jan 30, 2018
@donmccurdy
Copy link
Collaborator Author

PR for the Draco extension has been merged.

Here's a demo with drag-and-drop support for any glTF+Draco model: https://gltf-viewer-experimental.donmccurdy.com/.

@mrdoob
Copy link
Owner

mrdoob commented Feb 15, 2018

Should this be merged then?

@donmccurdy
Copy link
Collaborator Author

One more thing I want to do here is check all of the latest glTF+Draco sample models.. otherwise if there are no code comments, I think it is OK to merge.

/cc @zellski in case you've come across any issues so far.

@zellski
Copy link

zellski commented Feb 15, 2018

@donmccurdy I did minimal testing, but what I did do worked fine!

@donmccurdy
Copy link
Collaborator Author

Ok, fixed one last thing to ensure that Draco meshes referenced multiple times are only decoded once. Sample models are all loading fine. I think this is ready. 👍

@mrdoob mrdoob merged commit b23b2d7 into mrdoob:dev Feb 16, 2018
@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2018

Thanks!

@takahirox
Copy link
Collaborator

takahirox commented Feb 16, 2018

Sorry for the late reply after merging. (Finally I've completed tons of paperwork and I'm back to dev!)

  1. Duplicated draco decoders

Seems like draco decoders are duplicated in examples/js/loaders/draco/ and examples/js/loaders/draco/gltf/. If I'm right, we can remove the ones in examples/js/loaders/draco/.

  1. What do you think of moving draco decoders to examples/js/libs(/draco)?

I'm considering if it'd be good to move draco decoders to examples/js/libs or examples/js/libs/draco because Three.js doesn't manage the draco decoders but copies from https://github.com/google/draco/tree/master/javascript. Users can easily understand that master files of decoders are in another repo if putting them under libs.

Or adding readme explaining that master files are in another repo in examples/js/loaders/draco/ would be another option.

  1. Need more explanation about Draco loader/decoder in Doc

In the doc, I feel we need some more explanation about DRACOLoader

  • users need to add <script src="DRACOLoader.js"></script> in their code to use draco loader
  • what files should be under the decoder path

Adding a link to https://github.com/google/draco/blob/master/javascript/example/README.md to doc would be good.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Feb 16, 2018

  1. Duplicated draco decoders

The Draco decoders for use with the glTF extension are locked to a particular version of the Draco bitstream, until/unless a future version of the extension bumps them. The ones in examples/js/loaders/draco/ might be compatible now, but will not remain so.

  1. What do you think of moving draco decoders to examples/js/libs(/draco)?

That sounds like a good idea, yes.

  1. Need more explanation about Draco loader/decoder in Doc

That would be good to document, sure. I am planning to add a loaders / gltf /draco standalone example, but haven't got a model that quite fits the bill yet.

@donmccurdy
Copy link
Collaborator Author

And, welcome back! 😃

@takahirox
Copy link
Collaborator

  1. Duplicated draco decoders
    The Draco decoders for use with the glTF extension are locked to a particular version of the Draco bitstream, until/unless a future version of the extension bumps them. The ones in examples/js/loaders/draco/ might be compatible now, but will not remain so.

Ah, that's the reason why webgl_loader_draco example uses the decoders in examples/js/loaders/draco/ while webgl_loader_gltf_extensions examples uses the ones in examples/js/loaders/draco/gltf/.

Hm, I think we need readme in examples/js/loaders/draco/ explaining it for now at least. Two decoders in the repo without any explanation are confusing to users/devs.

@donmccurdy
Copy link
Collaborator Author

Updated via #13351

@donmccurdy donmccurdy deleted the feat-gltf-draco-extension-v4 branch July 17, 2018 18:02
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.

5 participants