GLTFLoader: Invalid interleaved buffer setup when multiple attribute accessors share the same buffer

[zeux]

When input GLTF file contains accessors with stride that doesn't match the expected stride based on the accessor type, GLTFLoader creates the interleaved attribute as follows:

var ibCacheKey = 'InterleavedBuffer:' + accessorDef.bufferView + ':' + accessorDef.componentType;
var ib = parser.cache.get( ibCacheKey );

if ( ! ib ) {
    // Use the full buffer if it's interleaved.
    array = new TypedArray( bufferView );

    // Integer parameters to IB/IBA are in array elements, not bytes.
    ib = new THREE.InterleavedBuffer( array, byteStride / elementBytes );

    parser.cache.add( ibCacheKey, ib );
}

bufferAttribute = new THREE.InterleavedBufferAttribute( ib, itemSize, byteOffset / elementBytes, normalized );

As a result, the attribute objects that are created have .count property reflect the full size of the buffer view. If multiple meshes share the same buffer view, then what will happen is that all of them will have the same .count (that is equal to the total vertex count across all meshes), but access to .getX() etc. will apply the byte offset for each mesh.

This is a problem. For example, in this code in computeBoundingSphere:

for ( var i = 0, il = position.count; i < il; i ++ ) {
    vector.fromBufferAttribute( position, i );
    maxRadiusSq = Math.max( maxRadiusSq, center.distanceToSquared( vector ) );
}

For each mesh we'll iterate over the number of elements in the buffer view, but during access in fromBufferAttribute an offset will be applied, which - for meshes with offset != 0 - will result in vector being undefined and maxRadiusSq equal to NaN.

I'm considering options for how to fix this.

One option is to make caching logic more involved by incorporating accessor count into the cache, and only storing a slice of the buffer view that matches the accessor count; something along these lines:

var ibSlice = Math.floor( byteOffset / byteStride );
var ibCacheKey = 'InterleavedBuffer:' + accessorDef.bufferView + ':' + accessorDef.componentType + ':' + ibSlice + ':' + accessorDef.count;
var ib = parser.cache.get( ibCacheKey );

if ( ! ib ) {
    array = new TypedArray( bufferView, ibSlice * byteStride, accessorDef.count * byteStride / elementBytes );

    // Integer parameters to IB/IBA are in array elements, not bytes.
    ib = new THREE.InterleavedBuffer( array, byteStride / elementBytes );

    parser.cache.add( ibCacheKey, ib );
}

bufferAttribute = new THREE.InterleavedBufferAttribute( ib, itemSize, (byteOffset - ibSlice * byteStride) / elementBytes, normalized );

Another option is to change InterleavedBufferAttribute.count to a field that's computed in the constructor from an optional extra count argument like this:

function InterleavedBufferAttribute( interleavedBuffer, itemSize, offset, normalized, count ) {
    this.data = interleavedBuffer;
    this.itemSize = itemSize;
    this.offset = offset;
    this.count = count || interleavedBuffer.count;
    this.normalized = normalized === true;
}

Thoughts?

[zeux]

A downside for approach 1 is that the bufferview will be effectively split into multiple smaller views which results in more GL objects being allocated - it's probably okay in the web world, but isn't optimal.

A downside for approach 2 is that it requires changing core code. Although TBH the limitations of BufferAttributes are weird in general, it's odd that the 3D API (WebGL) is perfectly capable of natively representing the GLTF data source, but due to the abstraction around it GLTF loader has to jump through hoops such as caching the view/component type.

I think this also means that with the current code, GLTF loader is duplicating buffers in memory for interleaved storage scenarios? E.g. if I use a skinned mesh, it would typically use Float32 for vertex/normal/UV data, Uint8 for weight data and Uint16 for joint indices - at least that's what I commonly see as far as data formats go. Currently GLTFLoader.js will create three InterleavedBuffers for this because there are three component types involved, and WebGL renderer (I think?) will create three separate GL buffers, one per InterleavedBuffer. Neither solution I'm proposing fixes that (neither makes it worse either, but it's definitely an unsatisfying status quo).

[donmccurdy]

I think this also means that with the current code, GLTF loader is duplicating buffers in memory for interleaved storage scenarios?

I'd have to check, but that's probably true. 😕

I'm fine with (1), certainly that's a quicker fix.

But at this point, there are no loaders except GLTFLoader using the InterleavedBuffer[Attribute] feature. So — at least from my perspective — a core API change to support this correctly might not be too difficult, and could make the feature available to far more developers than use it now.

...if I use a skinned mesh, it would typically use Float32 for vertex/normal/UV data, Uint8 for weight data and Uint16 for joint indices - at least that's what I commonly see as far as data formats go.

Is that compatible with the glTF spec? I'm not sure what the performance/compatibility reasons here were, but from Data Alignment —

For performance and compatibility reasons, each element of a vertex attribute must be aligned to 4-byte boundaries inside bufferView (i.e., accessor.byteOffset and bufferView.byteStride must be multiples of 4).

[zeux]

@donmccurdy weight/joints would commonly use vec4 so weight data would use uint8 vec4 with byteStride 4, and joint data would use uint16 vec4 with byteStride 8 - all of this is within spec.

[zeux]

Looking at the glTF-Sample-Models repository, no models there seem to hit the buffer duplication issue because they are all non-interleaved with the exception of a single test, Box-Interleaved, that only uses Float32 accessors (and this test also only has a single mesh so it doesn't hit the issue with incorrect count either).

Since the fix to the InterleavedBuffer is simpler anyhow, I will submit a PR with it - my actual usecase doesn't hit the buffer duplication issue either so that can be solved some other day.

Also, cloneBufferAttribute in GLTFLoader is likely incorrect as well since it accesses count and internal array object inside the InterleavedBufferAttribute...

[donmccurdy]

On the longer-term issue, I'm concerned by the fact that the InterleavedBuffer constructor accepts a typed array rather than an ArrayBuffer. That alone seems like a problem.

If you happen to have glTF model(s) that run into all the pain points here (multiple meshes, mixed component types) I'd be happy to look into the necessary changes to the core API. But that may take a while. 🙂

[zeux]

On the longer-term issue, I'm concerned by the fact that the InterleavedBuffer constructor accepts a typed array rather than an ArrayBuffer. That alone seems like a problem.

Yup - agreed. Ideally three.js should have a buffer object that is associated with an untyped buffer, and an attribute object that provides a view into that. This would make it possible to have glTF loader load data in a more direct fashion without hitting these odd issues (and without a need for caches etc.).

I think I will work around this issue for now on my end since the short-term solutions seem suboptimal in various ways - changing InterleavedBufferAttribute might be problematic if long term IB/IBA is going to change in a different way, and caching IB per interleaved buffer slice seems like a hack. glTF models in the wild seem to not hit this issue right now so it's probably better to make a fix to this that extends core in the right way.

[zeux]

I've hit a wall with my workaround - I actually need to fix this issue now, because working around this issue generates .gltf files that break bounding box generation in Babylon.JS (the details are... complicated... and are summarized in zeux/meshoptimizer#44 (comment)).

@donmccurdy Have you had a chance to think about what the changes in the core API should be to support this well? I can no longer ignore this problem but I'm willing to do the leg work to fix this properly.

[donmccurdy]

It sounds like there are several proposals here to consider:

1. Alter InterleavedBufferAttribute to support multiple meshes per InterleavedBuffer

I'd suggest we do this without modifying the constructor, to maintain backward compatibility and similarity with the BufferAttribute constructor, while we figure whether other API changes are needed (below). For example:

const iba = new InterleavedBufferAttribute( ib, itemSize, offset, normalized );
iba.setCount( count );

2. Alter InterleavedBuffer to support multiple meshes.

This seems like the harder change. Parts of the IB API (.stride, .count) don't really make sense with multiple meshes, and would probably need to be removed.

3. Alter InterleavedBuffer to support attributes of different types.

Take an ArrayBuffer instead of a typed array as a parameter in the constructor. This change alone would allow attributes of different types to share a single InterleavedBuffer, for use by a single mesh. With changes 1-2, the change could extend to multiple meshes.

---

Which of these do you need, @zeux? Or have I missed anything? Is there any value in doing (1) or (2) independently, or are they a package deal? I think (3) could be done separately, but I'm not clear that it solves your concern.

I'll be offline July 5-14th, but ideas from others on these suggestions would be welcome too. :)

[zeux]

I've thought about this more, and I believe there's four problems with buffer management in glTF loader.

Problem A Multiple accessors that have non-natural stride (byteStride != itemSize * elementBytes) but share the same buffer view will be marked as interleaved and will share the same InterleavedBuffer.count, which breaks logic in various places. This is a correctness issue.

Problem B Multiple accessors that share byteStride and are used to render the same geometry in an interleaved scenario, but have different component types, will redundantly allocate GPU memory (right now this can happen when meshes are using skinning). This is an efficiency issue.

Problem C Multiple accessors that share the same underlying buffer view will allocate multiple WebGL buffer objects for rendering - for example, this is the case when the stride is natural, so we don't follow the interleaved buffer path. Multiple objects result in more memory allocated and slower rendering due to the extra management overhead. This is an efficiency issue.

Problem D WebGL provides a powerful method to setup vertex attributes using buffer objects with stride/offset; JS provides a powerful method to provide multiple potentially overlapping typed views into a single ArrayBuffer object; however, Three.JS provides two abstractions (BufferAttribute and InterleavedBufferAttribute) that are both unable to use the full capabilities of relevant JS or WebGL features. This is a design issue.

I'm very motivated to solve problem A, because it's actively preventing my ability to synthesize .gltf files that can be efficient in other frameworks.
I'm less motivated to solve problems B and C in isolation from problem D, because problems B and C are created by problem D.

Because of this I'll submit a PR with my initial workaround that is going to convert instances of problem A ("things don't work") to instances of problem C ("each slice of the buffer gets its own interleaved buffer object"). After this we can try to figure out if we can solve the problems B-D in one fell swoop.

[zeux]

@donmccurdy Solutions from your comment apply to the problems from mine as follows:

• Solution 1 would solve problem A, would not solve problem B, would not solve problem C but wouldn't make it worse (since it only applies to interleaved data).
• I don't understand Solution 2 since it seems the same as solution 1 or solution 3 - I don't think it makes sense in isolation
• Solution 3 would solve problem B, but wouldn't solve problems A or C

Solution 1 could be extended to solve both A and C if glTF loader stops creating usual typed arrays and starts exclusively creating interleaved buffers using the typed array created over the buffer view as a backing object, but this makes problem B worse because it extends its scope from "component type mismatch when accessors with unnatural stride share buffer views" to "component type mismatch when accessors share buffer views".

The only way I see to solve problems A-C is to solve problem D. That is, we need to make sure that there's a buffer attribute object that can be created using a backing ArrayBuffer (potentially through a separate wrapper class if this is required to track changes) with a per-attribute component type, stride & offset.

I don't know if we can extend InterleavedBuffer to fix this or if it needs backwards-incompatible changes. The name isn't fully right either way, and it seems like at least some of the functionality won't work.

In theory we could try to extend BufferAttribute instead - it's currently missing a separation between item size and stride, and ability to share a WebGL object. I think it may be possible to fix it in a backwards-compatible way, for example:

• We make a new Buffer object, that can be created using ArrayBuffer
• BufferAttribute gets an optional stride property (stride is defined in terms of typed elements, so if element is Float32, byte stride has to be divisible by 4); all internal operations respect the stride
• Code that uses BufferAttribute.array property directly needs to be taught about stride
• BufferAttribute gets a new constructor from Buffer object and an offset; this creates a typed view into the ArrayBuffer.
• WebGL renderer gets updated to store WebGL buffer objects associated with the Buffer object of BufferAttribute, if present

As a result InterleavedBuffer is subsumed in functionality by BufferAttribute/Buffer and can be deprecated / removed / aliased to BufferAttribute / whatever. glTF loader can always create Buffer objects for BufferViews and BufferAttribute objects for accessors.

[zeux]

After this issue gets closed via the tactical fix I will open another issue and summarize the remaining efficiency issues so that we can discuss whether we want to fix them by reworking parts of core and if so, how.