gltfpack: Deindex meshes with abnormally large vertex accessors #885
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously we have supported sparse accessors for cgltf_accessor_unpack_floats but not for various read_ functions. This could lead to applications using read_ functions for specific cases and the code would work for most glTF files but fail on files with sparse data. This change supports sparse indices by using a binary search to find the index; the keys in sparse accessors are guaranteed to monotonically increase. This is still slower than a single linear pass that unpack_floats does, but will work correctly and have acceptable performance.
The last parameter here is the number of floats per element read. per element read. It happens to be 4, as Attr has 4 floats, but should be specified explicitly.
…e fly If the index buffer is much smaller than vertex accessors, and vertex accessors are shared, we end up re-unpacking the accessors repeatedly and producing huge meshes that are inefficient for the rest of the processing pipeline. This change detects these cases and switches to reading the individual elements according to the index buffer, producing an unindexed mesh; the rest of the processing will re-index it again.
Point clouds use empty indices, so indices.size() would be less than the vertex count. The actual code that would execute then is a no-op, as it would leave sparse empty, but if the mechanism of enabling sparse mode changes in the future this may regress, so add a condition to be explicit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the index buffer is much smaller than vertex accessors, and vertex accessors
are shared, we end up re-unpacking the accessors repeatedly and producing huge
meshes that are inefficient for the rest of the processing pipeline.
This change detects these cases and switches to reading the individual elements
according to the index buffer, producing an unindexed mesh; the rest of the
processing will re-index it again.
For some extreme cases this may result in significant improvements in parsing and
processing time; the mesh from #884 could not be converted before this change,
and with this change it takes just 0.3s to do so.
Before settling on the approach here, I've also tried trimming the index buffers to
their min..max range and extracting an associated subrange from the accessors;
this worked but only got the problematic model down to 30s of conversion time
because many indices spanned a large range. Doing an early reindexing in this case
got this down to ~3s, but it still felt too long and too complicated, whereas this
approach generically solves this issue.
This relies on support for sparse accessors in cgltf_accessor_read functions,
which also fixes interaction between sparse accessors and parsing files with GPU
instancing extension; the patch has been submitted separately as jkuhlmann/cgltf#273.
Fixes #884.