Improving BufferAttribute (maybe)

[zeux]

While working with GLTF loader a fair bit in the past few months I've hit a few issues that seem to be impossible to fix perfectly short of changing parts of the core library. This issue presents the issues and solicits feedback.

Ideally, GLTF loader should be able to create a WebGL buffer for each bufferView that is present in the file. The glTF spec is structured to make this possible through various limitations on alignment and colocated data in a single view. Given a good glTF processor like gltfpack you can then achieve a minimum number of WebGL buffers used for the entire scene - for example, one buffer for everything (this requires interleaving vertex data), or maybe one buffer per each unique stride (this is commonly how glTF files exported from Sketchfab seem to look). This is independent of the complexity of the scene.

Doing so carries a few benefits - it minimizes the number of buffer objects used to represent the scene, which improves memory consumption, minimizes the cost of switching between geometry buffers during rendering, and in theory unlocks some future accelerated rendering scenarios using multi draw (unfortunately, WEBGL_multi_draw specifically isn't optimal for this, but maybe this can be improved in the future). Additionally, the loading logic can be more straightforward and can become faster for files with lots of meshes.

There are two important features that are required to make this work (see #16802 for what triggered all of this, and GLTFLoader.js for the current somewhat sad workarounds):

• We need to be able to reference the same WebGL buffer from multiple buffer attribute objects, at different byte offsets. This is currently impossible with BufferAttribute. This seems possible with InterleavedBufferAttribute but it doesn't truly work because all IBAs that refer to the same IB share the same .count property which breaks various assumptions in various parts of three.js.

• We need to be able to reference the same WebGL buffer from multiple buffer attribute objects with different component types. This is current impossible with BA or IBA - in both cases the backing array on the JS side is a typed array, not an array buffer.

It seems to me that InterleavedBuffer is not pulling its weight. It doesn't solve either of the two problems above - effectively it only works if you want to interleave data of the same component type within a buffer for a single mesh, but doesn't allow mixing component types or packing multiple meshes into a single buffer. WebGL is perfectly capable of all of these, but Three.JS lacks a good interface to this.

Additionally, the fact that there are two different constructs with two different interfaces to represent, conceptually, the exact same thing - an array of typed components used for vertex processing - seems problematic. This results in some number of type checks for which BufferAttribute implementation it is, and not all code can handle both.

Now, if we agree that these problems should be solved, there's still an option of continuing to evolve InterleavedBufferAttribute to support these use cases - it probably involves extending InterleavedBuffer to optionally contain an ArrayBuffer instead of a TypedArray, adding a .count property to InterleavedBufferAttribute that can be set independently, and fixing code that directly works with InterleavedBuffer. If this was done, you'd be able to create InterleavedBuffer objects exclusively in glTF loader.

However, it seems like there's another way to fix this problem - instead of fixing InterleavedBuffer, we can improve BufferAttribute and deprecate InterleavedBuffer instead (it could still potentially exist as a thin wrapper over BufferAttribute to maintain backwards compatibility).

Here's how this could work.

Right now BufferAttribute is created from a typed array. We would add a new Buffer object that can be created from an ArrayBuffer, and a way to create BufferAttribute from a Buffer object with a byte offset and a component type (which would create a typed array for a slice of the buffer object). Existing constructor from a typed array would create a private Buffer object, so existing code would work as is.

BufferAttribute would be extended with a stride; to keep everything working nicely we will require that the stride is divisible by the component size so that it can be expressed in terms of the type of the typed array - this isn't a significant limitation in practice because the largest usable type is 32-bit float, and for various legacy reasons offsets need to be aligned by 4 bytes both in glTF spec and in general use.

Code that directly accesses BufferAttribute.array and isn't aware of the stride will need to be taught about the stride; code that works with BufferAttribute through getX/etc. accessors will continue to work.

After this is done, glTF loader can forget that interleaved buffers exist and create Buffer from each bufferView and BufferAttribute from each geometry accessor. (in glTF specifically there's one more source of inefficiency atm which is the morph target buffer duplication, which I think should be solved at the core level as well but that's a separate discussion) We can then separately deprecate InterleavedBuffer and either just remove it, or reroute IBA & IB methods to work through BA & B - they might just extend these classes and add the relevant functions if necessary.

As a result, three.js can gain a Buffer object that is roughly equal in power to WebGL buffers, glTF loader can stop creating thousands of tiny buffers for scenes with a large number of meshes, and (eventually) the whole "there are two different types of buffer attributes" issue can disappear.

Thoughts?

[donmccurdy]

It seems to me that InterleavedBuffer is not pulling its weight.

Of the various threejs loaders, GLTFLoader is currently the only one creating InterleavedBuffers and InterleavedBufferAttributes. I think this makes a strong case that (1) the current interleaving API may not yet offer enough benefits, and (2) we can afford to make changes that are not backward-compatible there, or to remove IBA in favor of a more flexible BufferAttribute implementation.

Ideally, I'd suggest that we evolve the IBA class in the direction you're suggesting: so that IBA can be used as a drop-in replacement for BufferAttribute initialized with a typed array, or can be initialized with an improved Buffer object. This would be backward-compatible with BufferAttribute, but not necessarily backward-compatible with today's IBA.

I say that mainly because it sounds like a safer and more incremental upgrade path, even if the eventual goal is to have only one implementation. If the largest changes are all within the renderer (which is very possible; I'm not as familiar with that code) it may not really be as "incremental" as I'm hoping. I would defer to others on how manageable this sounds.

Aside: There is some complexity here for the .dispose() API. A BufferGeometry sharing a backing buffer with another geometry cannot simply be disposed. But that can be solved when we come to it, I think.

[donmccurdy]

^I'd be interested in maintainers' preferences on this. Above, I'd suggested evolving InterleavedBufferAttribute rather than BufferAttribute, mostly because it is easier, but extending the BufferAttribute constructor...

var attribute = new THREE.BufferAttribute( new THREE.Buffer( arrayBuffer ), itemSize, normalized, stride );

... does not need to be a breaking change either. We have a lot of BufferAttribute types today ...

• THREE.Float64BufferAttribute
• THREE.Float32BufferAttribute
• THREE.Float16BufferAttribute
• THREE.Uint32BufferAttribute
• THREE.Int32BufferAttribute
• THREE.Uint16BufferAttribute
• THREE.Int16BufferAttribute
• THREE.Uint8ClampedBufferAttribute
• THREE.Uint8BufferAttribute
• THREE.Int8BufferAttribute
• THREE.BufferAttribute
• THREE.InterleavedBufferAttribute
• THREE.InstancedBufferAttribute

... and so the option of merging InterleavedBufferAttribute into BufferAttribute does sound kind of nice.

I'm running into a problem that is described generally in @zeux's comments above, but I'll call it out here more specifically. Given a mesh with multiple vertex attributes, we might want to both (1) use lighter component types than float32 and (2) interleave vertex data. You wouldn't necessarily choose the same component types for every attribute — vertex colors work well with uint8, vertex positions might use int16 — but the current InterleavedBuffer API does not allow mixing and matching.

[takahirox]

I think WebGL buffer which can be shared across different types, counts, or other properties attributes is worth for Three.js. Especially it's nice if users get benefit without changing their code by letting glTF loader support it. (Of course, glTF assets needs to be optimized, too.)

I made a prototype to figure out the change and to determine how we move it forward, like replacing the IBA, extending BA, or so on.

dev...takahirox:InterleavedBufferAttribute2

In the prototype, I added two new classes so far, Buffer and InterleavedBufferAttribute2 (just a temporal name!). This doesn't mean I suggest to add new classes for shared WebGL buffer but it's just a prototype.

Buffer corresponds to WebGL buffer and can be referenced by multiple InterleavedBufferAttribute2. And the constructor just takes ArrayBuffer.

InterleavedBufferAttribute2 constructor takes a reference to Buffer, count, and parameters used for gl.vertexAttribPointer(), like itemSize, type(Float32Array, Uint16Array, ...), normalized, stride in bytes, and offset in bytes.

The changes needed in the renderer are

1. Use attribute's parameters for gl.vertexAttribPointer() if attribute is InterleavedBufferAttribute2.
2. Count the number of references to Buffer from InterleavedAttributeBuffer2 and delete the WebGL buffer if it becomes zero because Buffer can be shared by multiple InterleavedBufferAttribute2. (The existing InterleavedBuffer can be shared by multiple InterleavedAttribteBuffer in Three.js API , too, so this reference counter has been needed regardless of introducing the shared WebGL buffer feature.)

And one concern about InterleavedBufferAttribute2 API. It has stride in bytes. If the stride is not be multiple of bytes_per_element (eg. 4 for Float32Array and 2 for Uint16Array) the get/set API to access values may need to create data view for each call. It may be inefficient so I assume(limit) the stride is multiple of bytes_per_element so far.

Sorry for the long comment and I still need to brush it up, but I would say the shared WebGL buffer feature can likely be introduced without so big change.

[takahirox]

I'm thinking of the upgrade path. And first I would like to know the reason why the current InterleavedBuffer and InterleavedBufferAttributes don't allow to share a WebGL buffer across different type or other parameters? Any historical reasons? If we realize it's a bad design due to lack of consideration, it might be ok to change their APIs for improvement even if it hurts for the existing IB/IBA users. (Of course it would be the best if we come up with an idea not breaking the compatibility tho.)

[donmccurdy]

GLTFLoader is the only loader using InterleavedBuffer today — I think we should feel free to change its API. I would rather not add any new *BufferAttribute classes without replacing one or more of the old ones, at this point, to avoid confusing users.

[donmccurdy]

And first I would like to know the reason why the current InterleavedBuffer and InterleavedBufferAttributes don't allow to share a WebGL buffer across different type or other parameters? Any historical reasons?

At the time these were written, there probably weren't any loaders parsing in already-interleaved vertex data, so the interleaving was presumably happening on the fly in the browser, and writing into float32 was not a big deal. But in 2020 it is very easy to make a glTF file with interleaved vertex data using smaller (int8, int16, ...) types, and I think it does not make sense for InterleavedBuffer to use a typed array as its storage now.

[takahirox]

I made a prototype for changing the existing interleaved buffers API.

dev...takahirox:NewInterleavedBufferAttribute

New API

InterleavedBuffer(buffer: ArrayBuffer);
InterleavedBufferAttribute(
  buffer: InterleavedBuuffer,
  itemSize: number,
  type: TypedArray constructor,
  normalized: boolean,
  stride: number, // in bytes
  offset: number, // in bytes
  count: number
);
// Currently InstancedInterleavedBuffer but 
// InstancedInterleavedBufferAttribute may be more natural
// in the new API
InstancedInterleavedBufferAttribute(
  buffer: InterleavedBuuffer,
  itemSize: number,
  type: TypedArray constructor,
  normalized: boolean,
  stride: number, // in bytes
  offset: number, // in bytes
  count: number,
  meshPerAttribute: number
);

Example

const arrayBuffer = new ArrayBuffer(bufferSize);
const positionArray = new Float32Array(arrayBuffer);
const colorArray = new Uint8Array(arrayBuffer);
// Position (x, y, z) in float32, Color (r, g, b, a) in uint8
for (let i = 0; i < count; i++) {
    positionArray[i * 4] = Math.random();
    positionArray[i * 4 + 1] = Math.random();
    positionArray[i * 4 + 2] = Math.random();
    colorArray[i * 16 + 12] + Math.random();
    colorArray[i * 16 + 13] + Math.random();
    colorArray[i * 16 + 14] + Math.random();
    colorArray[i * 16 + 15] + Math.random();
}
const interleavedBuffer = new InterleavedBuffer(arrayBuffer);
const positioniAttribute = new InterleavedBufferAttribute(
  interleavedBuuffer,
  3, // itemSize
  Float32Array, // type
  false, // normalized
  16, // stride in bytes
  0, // offset in bytes
  count
);
const colorAttribute = new InterleavedBufferAttribute(
  interleavedBuuffer,
  4, // itemSize
  Uint8Array, // type
  true, // normalized
  16, // stride in bytes
  12, // offset in bytes
  count
);

Any thoughts?

GLTFLoader is the only loader using InterleavedBuffer today.

Nit: IFCLoader recently added also uses it.

I think we should feel free to change its API.

Users can directly use InterleavedBuffer and InterleavedBufferAttribute like webgl_buffergeometry_points_interleaved example. Such codes will be broken with this change. But it would be rare use case? I hope changing API is acceptable for the improvement.

[donmccurdy]

Thanks @takahirox, I agree with the direction of this. I wonder if we could go even further:

1. Do we need InstancedInterleavedBufferAttribute? Could InterleavedBufferAttribute "just work" with an optional final parameter?
2. Would it be crazy to try to eliminate some existing classes, e.g. to support an API like this in BufferAttribute itself, or to cut the [TypedArray]BufferAttribute classes? I'm not sure what the history is on all that, but I'm foreseeing a lot of user questions about how to convert a BufferAttribute to an interleaved and/or instanced version, or how to use them with InstancedMesh, and it's a lot of combinations to explain.

Such codes will be broken with this change.

These seem like cases we could catch and warn about, or even handle automatically for some period of time, perhaps? I do agree that changing the existing interleaved attribute API is justified.

[takahirox]

1. Do we need InstancedInterleavedBufferAttribute? Could InterleavedBufferAttribute "just work" with an optional final parameter?

I think it just works. No strong opinion about adding the final constructor parameter to InterleavedBufferAttribute or adding InstancedInterleavedBufferAttribute class. In the prototype I added InstancedInterleavedBufferAttribute so far for the consistency with BufferAttribute and InstancedBufferAttribute relationship.

2. Would it be crazy to try to eliminate some existing classes, e.g. to support an API like this in BufferAttribute itself, or to cut the [TypedArray]BufferAttribute classes? I'm not sure what the history is on all that, but I'm foreseeing a lot of user questions about how to convert a BufferAttribute to an interleaved and/or instanced version, or how to use them with InstancedMesh, and it's a lot of combinations to explain.

Personally combining BufferAttribute and the new InterleavedBufferAttribute may be ok to me. But It may be good to keep BufferAttribute API because it's one of core API and can affect a lot of user code and libs. I would like to hear other devs opinions and it is a big picture so may be better to have a discussion about it in a separated issue (after we merge the new InterleavedBufferAttribute API)?

These seem like cases we could catch and warn about, or even handle automatically for some period of time, perhaps?

Yes, probably.

[takahirox]

I'm thinking of making a draft PR to get feedbacks from more many devs.

[donmccurdy]

But It may be good to keep BufferAttribute API because it's one of core API and can affect a lot of user code and libs.

Yes, if (1) we can maintain backwards-compatibility with the current BufferAttribute API, and (2) the code is not much more complex as a result, then I think this is worth considering. Otherwise I'm completely fine with just modifying the InterleavedBufferAttribute class and leaving BufferAttribute alone.

The current BufferAttribute constructor is:

BufferAttribute( array : TypedArray, itemSize : Integer, normalized : Boolean )

Arguably this could be a backwards-compatible API for a BufferAttribute class supporting interleaved and/or instanced use cases:

BufferAttribute(
  array: TypedArray | InterleavedBuffer,
  itemSize: number,
  normalized: boolean = false,
  type: ? = undefined,
  stride: number = undefined,
  offset: number = undefined,
  count: number = undefined,
  meshPerAttribute: number = undefined
)

---

EDIT: Could also group things into options, depending on preferences...

BufferAttribute(
  array: TypedArray | InterleavedBuffer,
  itemSize: number,
  normalized: boolean = false,
  options?: { 
    interleaved?: { stride: number, offset: number, count: number, type: ? }, 
    instanced?: { meshPerAttribute: number }
  }
)

[takahirox]

Thanks for the investigation! Yeah, it looks we can maintain backward compatibility.

I haven't looked into the core code for extending BufferAttribute and combining InterleavedBufferAttribute yet and the changes will be in center cores like renderer so I would like to carefully and incrementally proceed. I would like to suggest to break the changes up, first updating InterleavedBufferAttribute API and then combining BufferAttribute and InterleavedBufferAttribute. Or does it sound like inefficient?

[donmccurdy]

Sounds good to me! We can wait to decide on whether to combine BufferAttribute and InterleavedBufferAttribute at the end.

[takahirox]

Opened the draft PR #21656 /cc @zeux @donmccurdy