GLTFLoader: Fix incorrect accessor count caching #16989
Conversation
When two accessors with a different count but the same component type share a buffer view, there are two cases in GLTF loader: - If accessor stride is "natural" (equal to item size in bytes), a TypedArray slice is created with the count matching accessor count - If accessor stride is "unnatural", an InterleavedBuffer is created for the entire buffer view; accessor count is ignored as a result. Incorrect count is problematic because various algorithms such as bounding box calculation would try to process elements that may not exist in the buffer. Note that this situation arises for any unnatural stride, whether the rest of the stride is used by another interleaved accessor or not. A solution is to make sure that the InterleavedBufferAttribute.count (which is derived from InterleavedBuffer.array.length) is always correct, by creating a TypedArray from a "slice" of the buffer view. In an actual interleaving situation this maintains the effectiveness of the cache - multiple accessors sharing the same interleaved data that are used in the same mesh primitive will get the same slice (since the byte offsets will be almost the same, and floor(byteoffset / stride) will match). Thus this change should only fix existing cases where .count is broken and not make anything worse. Fixed #16802.
Specify versions of three.js and Babylon.js that fully support gltfpack output files. Note that three.js version is opportunistically assuming mrdoob/three.js#16989 will get merged in time for the release.
|
@donmccurdy Welcome back! :) Sorry to ping you but hoping this can get into r107 and want to make sure this didn't get lost in the notification noise. |
|
Thanks! My assumption would be that this change causes the cache to work correctly, but that multiple InterleavedBuffers sharing underlying data will still end up duplicated in GPU memory. As that is still a step forward, I'm fine with merging this. However, I don't have access to a glTF model that exercises this code, or an easy way to create one – does gltfpack output something we can test here? |
This is still the case when component types differ, yes - in the interleaved storage scenario we by necessity need to create a buffer per each component type because the existing three.js API can't represent the real data layout. As noted on the linked issue, this change resolves the correctness problem, but doesn't resolve the efficiency problem. gltfpack hits this problem even though it doesn't technically interleave the buffers, but it e.g. stores normals as BYTE3 normalized data, and has to use byteStride=4 to align it properly. BYTE3 normalized normals are currently not valid as per spec but the same issue can occur in existing interleaving scenarios - it's easy for me to upload an example of the former, but harder to make an example of the latter because I don't have code on my hands to do that. |
If the former is enough to test this change, and you could test that, I'm satisfied. The latter does seem like a situation we should add to glTF-Asset-Generator if it isn't covered yet, but that may not happen right away. |
|
@donmccurdy Yeah this fix does resolve issues with gltfpack-generated meshes. https://github.com/KhronosGroup/glTF-Asset-Generator/blob/master/Output/Positive/Buffer_Interleaved/README.md only has basic single-mesh cases, so it's not hitting that - the problem in this case would have been if a second mesh was added that had an accessor into the same buffer view with a different offset. Interleaved buffers in general tend to be rare in glTF world as far as I know which is probably why nobody has hit this before... |
|
FWIW here's an example of a .glb file packed by gltfpack; it can be opened by Babylon.JS without issues, but trying to open in three.JS doesn't render the mesh, and produces the following errors in the output:
The error happens because of incorrectly propagated .count, as described in the related issue. This change fixes that. and after applying it the .glb file loads correctly. You can see this here https://timvanscherpenzeel.github.io/three-gltf-viewer/ (this viewer fork pulls GLTFLoader.js from meshoptimizer repository which has the fix applied) |
|
@mrdoob let me know if I need to do anything else to help get this merged |
|
All good! |
|
Thanks! |
When two accessors with a different count but the same component type
share a buffer view, there are two cases in GLTF loader:
If accessor stride is "natural" (equal to item size in bytes), a
TypedArray slice is created with the count matching accessor count
If accessor stride is "unnatural", an InterleavedBuffer is created for
the entire buffer view; accessor count is ignored as a result.
Incorrect count is problematic because various algorithms such as
bounding box calculation would try to process elements that may not
exist in the buffer. Note that this situation arises for any unnatural
stride, whether the rest of the stride is used by another interleaved
accessor or not.
A solution is to make sure that the InterleavedBufferAttribute.count
(which is derived from InterleavedBuffer.array.length) is always
correct, by creating a TypedArray from a "slice" of the buffer view.
In an actual interleaving situation this maintains the effectiveness of
the cache - multiple accessors sharing the same interleaved data that
are used in the same mesh primitive will get the same slice (since the
byte offsets will be almost the same, and floor(byteoffset / stride)
will match). Thus this change should only fix existing cases where
.count is broken and not make anything worse.
Fixed #16802.