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

Speed up removeUnusedVertices #161

Closed
lasalvavida opened this issue Aug 11, 2016 · 1 comment
Closed

Speed up removeUnusedVertices #161

lasalvavida opened this issue Aug 11, 2016 · 1 comment

Comments

@lasalvavida
Copy link
Contributor

removeUnusedVertices and by proxy mergeDuplicateVertices are a major bottleneck for speed in the main pipeline right now.

My impression is that removeUnusedVertices is so slow because mergeDuplicateVertices ends up creating lots of holes in the buffer that need to be closed. In order to close those holes, the entire buffer has to be moved over which can be very time consuming for large models.

This could be faster by dividing the buffer up into chunks, closing the gaps in the smaller chunks, then concatenating the used parts of the chunks. It may also be faster to iteratively read through and copy chunks into a new buffer, though probably less memory efficient.

@mramato
Copy link
Contributor

mramato commented May 2, 2017

I ran into this today (specifically mergeDuplicateVertices being slow). I have a ~500mb obj model that I'm running through obj2gltf and then gltf-pipeline and if I comment out the call to mergeDuplicateVertices in Pipeline.js, it converts in ~150 seconds. If I run with mergeDuplicateVertices, it never completes (I killed it after 30 minutes). @lilleyse suggested we disable mergeDuplicateVertices by default until this issue can be resolved. I'm going to open a PR to put it behind a flag.

mramato added a commit that referenced this issue May 2, 2017
This works around #161 by adding a `mergeVertices` and disabling it by
default.  Once `mergeDuplicateVertices` is refectored to use less memory
and be orders of magnitude faster, we can take out the option and just
always do it.
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

3 participants