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

DRACOLoader: Add parse() method #17593

Closed
donmccurdy opened this issue Sep 27, 2019 · 6 comments
Closed

DRACOLoader: Add parse() method #17593

donmccurdy opened this issue Sep 27, 2019 · 6 comments

Comments

@donmccurdy
Copy link
Collaborator

The deprecated decodeDracoFile method should be removed and replaced with a parse method similar to other loaders. Currently decodeDracoFile does not propagate errors, and should do so. So probably a signature like:

dracoLoader.parse( arrayBuffer, /* options, maybe? */, onLoad, onError );
@mrdoob
Copy link
Owner

mrdoob commented Sep 27, 2019

/* options, maybe? */

What options does it need?

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Sep 27, 2019

Currently decodeDracoFile takes attributeIDs and attributeTypes, which are only needed if you create a .drc file in a very particular way (which most Draco users don't). There were some other options (quantization, triangle fans) that I've removed in the latest refactoring. I omitted them to simplify things, and because I don't have any examples of someone using them, but I wouldn't be opposed to re-implementing them if the need arises, and if so those would require two additional options.

For users with Draco-compressed .gltf or .glb files (which I think is the large majority), none of this is necessary.

@mrdoob
Copy link
Owner

mrdoob commented Sep 28, 2019

I think I would follow ImageBitmapLoader pattern.

var loader = new THREE.DracoLoader();
loader.setOptions( {} );
loader.parse( 'file.drc', onParse, onError );

@donmccurdy
Copy link
Collaborator Author

Disadvantage here is that attributeIDs and attributeTypes are likely to be different for each geometry decompressed. If each parse() call requires a new loader, then we'd need those loaders to share a pool of workers and balance tasks across them.

We could make sure the options are copied and stored for the task duration when parse is called, to support this pattern...

loader
  .setOptions( { ... } )
  .parse( buffer1, onLoad, onError )
  .setOptions( { ... } )
  .parse( buffer2, onLoad, onError );

... but I don't know if that's better or not. 🤔

@Gamesbxonline

This comment has been minimized.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 11, 2023

Currently decodeDracoFile does not propagate errors, and should do so.

After #25390 and #27306, parse() is available and properly reports errors now. Since decodeDracoFile() is still available (with error handling now as well), do we still need the options parameter /setOptions() method?

If there is no urgent demand, I vote to close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants