Update InterleavedBufferAttribute API to allow shared WebGL buffer across different parameters #21656

takahirox · 2021-04-15T06:22:58Z

Related issue: #17089

Description

In #17089 a problem was raised that the current InterleavedBuffer and InterleavedBufferAttribute doesn't allow shared WebGL buffer across different parameters like type.

We discussed it in that thread and it would be worth to enable shared WebGL buffer. But we need to change interleaved buffer APIs. So I opened this draft PR to get feedbacks from other devs if it's ok to change the API for the improvement.

New API suggestion:

Shared InterleavedBuffer which is just a buffer containing the contents
Customizable gl.vertexAttribPointer() parameters in InterleavedBufferAttribute.

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 and it would be consistent with
// BufferAttribute and InterleavedBufferAttribute relationship
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() * 255;
    colorArray[i * 16 + 13] = Math.random() * 255;
    colorArray[i * 16 + 14] = Math.random() * 255;
    colorArray[i * 16 + 15] = Math.random() * 255;
}
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
  false, // normalized
  16, // stride in bytes
  12, // offset in bytes
  count
);

Pros:

With the shared WebGL buffer, loaders or libs can optimize WebGL memory consumption. Especially the standard 3D model format glTF specification allows a single WebGL buffer (=glTF.buffer) shared by all entities. So if we optimize GLTFLoader users would get benefits without changing their code. (GLTFLoader optimization is not in this PR. I would like to suggest to do that in another PR not to make this PR bigger.)

Cons:

Interleaved buffer API change. But we may warn and automatically handle the legacy API in a few next cycles. And the change affects GLTFLoader, IFCLoader, and some other example modules but not so many (see the diff of this PR). Users can directly use interleaved buffer API like webgl_buffergeometry_points_interleaved example but perhaps it isn't main stream use case so I hope this change doesn't make a big bad impact.

Changes:

In addition to API change, we need to manage reference counter to shared InterleavedBuffer because the corresponding WebGL buffer needs to be released when all the attributes referring to it are disposed.
InterleavedBuffer will be just a buffer and won't have type. So we may need to drop some methods relying on the type.

Note:

Because of the API change the serialization/deserialization and documents needs to be updated but I don't change them yet. I want to update them after I get a consensus that it's ok to change the API.
In this PR, stride and offset in the new InterleavedBufferAttribute are in bytes for flexibility. And I assume stride bytes is multiple of multiple of type.BYTES_PER_ELEMENT for the efficient array accessor methods. If stride isn't type.BYTES_PER_ELEMENT aligned, the accessor methods may need to create data view for each call. Is this limitation good?

Advanced topics:

In Improving BufferAttribute (maybe) #17089 @donmccurdy suggested that we may combine BufferAttribute and InterleavedAttribute while keeping BufferAttribute API for simpler API. I think it would be possible and sounds good to me. I would like to know your folks opinions about it. Even if we do it, I would like to suggest to make a separated PR because the changes will be in our center core codes, like renderer, so I want to carefully and incrementally proceed.

…Attribute

donmccurdy · 2021-04-15T06:44:05Z

src/core/InterleavedBufferAttribute.js

@@ -91,91 +82,109 @@ class InterleavedBufferAttribute {

 	setX( index, x ) {

-		this.data.array[ index * this.data.stride + this.offset ] = x;
+		// Note: Assuming stride is multiple of type.BYTES_PER_ELEMENT


Let's require (i.e. not attempt to support anything else) the requirements of glTF here, they are there for a reason:

https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#data-alignment

Oh, good to know that. I have overlooked that part in the spec. Yeah let's require unless other specs need.

donmccurdy · 2021-04-15T06:44:48Z

src/core/InterleavedBufferAttribute.js


 		this.name = '';

-		this.data = interleavedBuffer;
+		this.data = buffer;
+		this.array = new type( buffer.array.buffer, offset );


Note that in addition to the requirement that stride be a multiple of component size, this also requires that offset be a multiple of 4 I believe. May need to document these alignment requirements.

Sounds good to me unless other specs need.

Requirement of Float32Array constructor itself as well: https://stackoverflow.com/a/8638875/1314762

Thanks, I didn't know that.

donmccurdy · 2021-04-15T06:45:54Z

src/core/InterleavedBufferAttribute.js

@@ -5,27 +5,18 @@ const _vector = new /*@__PURE__*/ Vector3();

 class InterleavedBufferAttribute {

-	constructor( interleavedBuffer, itemSize, offset, normalized ) {
+	constructor( buffer, itemSize, type, normalized, stride, offset, count ) {


I think that I would swap the order of type and normalized, to match existing BufferAtribute constructors. In this case normalized would be a required parameter; seems fine to me.

The number of parameters will be many so I thought making the order be same as gl.vertexAttribPointer() parameters help users understand the order, but no strong opinion. Swapping them sounds ok to me.

donmccurdy · 2021-04-15T06:51:42Z

I'm still on the fence about the need for a InstancedInterleavedBufferAttribute, or InstancedInterleavedBuffer for that matter. It seems like a lot of API and conversion overhead just for a meshPerAttribute property. But also not sure how to avoid this...

takahirox · 2021-04-15T07:22:12Z

I'm still on the fence about the need for a InstancedInterleavedBufferAttribute, or InstancedInterleavedBuffer for that matter.

No strong opinion about it. InstancedInterleavedBufferAttribute is just for consistency with BufferAttribute - InstancedBufferAttribute relation. But adding optional meshPerAttribute parameter to InstancedBufferAttribute and judging whether attribute is "instanced" or not by checking meshPerAttribute is set is also ok to me.

takahirox · 2021-05-15T01:24:06Z

Any other comments or feedbacks to this change? If no objection, I will switch this PR to "ready for review" after I reflect the review comments.

christjt · 2021-11-26T09:28:28Z

@takahirox Are you still awaiting comments or feedback on this? Seems like a great change that I would really like to see it being merged.

takahirox · 2021-11-27T08:20:54Z

Yeah, I still want comments. If it looks good, I want to move it forward.

christjt · 2021-11-29T09:16:18Z

@Mugen87 Can you take a look? 🙏

Mugen87 · 2021-11-29T09:41:29Z

Sorry, but other issues and changes have a higher priority right now to me.

In context of buffer attributes and buffer geometry I also vote to focus on #22874 first.

frericp · 2022-04-14T11:47:18Z

We recently found that using interleaved buffers with attributes of different types leads to the creation of multiple buffers in WebGL, which duplicates data on the GPU and defeats the purpose of interleaving data in the first place. @donmccurdy was so kind to point me to this proposal, and I really like it. It would allow us to continue using interleaved attributes instead of migrating to non-interleaved attributes.

@takahirox Is there any chance to pick this topic again anytime soon?

takahirox · 2022-04-14T16:38:02Z

@frericp I'm still waiting for comments. Please push @mrdoob or @Mugen87 if you want to raise the priority of this change.

frericp · 2022-04-27T08:58:53Z

Sorry for the delay, I just came back from vacation.

@takahirox If I got it right, you're mostly waiting for feedback on the API changes, correct?

@Mugen87 Any idea about when this proposal could make it onto the roadmap? I checked the other issue that you referred to above (#22874) and it seems to be mostly figured out by now. I'm happy to assist with code reviews if that helps.

jhurliman · 2022-07-16T17:42:07Z

I’m looking forward to this PR merging. We have a use case where point cloud data is received in an interleaved format with mixed types. Currently, we iterate over the structure and create separate position and color attributes on the CPU before uploading to the GPU. Ideally we could upload the data as-is to the GPU and access it using interleaved attributes.

donmccurdy · 2022-08-15T23:53:09Z

@snagy — given your comments in #24492, do you have thoughts on the practical impact of this PR (and #17089) for avoiding additional GL calls?

snagy · 2022-08-16T03:55:27Z

@donmccurdy I'm a fan of this change and think it will get some perf wins, but I'm not sure the scale of those wins. In the app I've been profiling lately, the per-frame cost of this on the JS side is entirely hidden by VAOs - it just remains a single call to set the VAO after it's been setup. The additional complexity of what's actually referenced in those VAOs certainly has some overhead, but I'd have to grab this change (and update the GLTFLoader) to be able to quantify that.

xpl · 2024-04-10T01:15:49Z

I would love to have this PR merged, can it be made possible anytime soon?

takahirox added 2 commits April 11, 2021 02:13

New InterleavedBufferAttribute API

9dbaacd

Merge remote-tracking branch 'upstream/dev' into NewInterleavedBuffer…

249d362

…Attribute

takahirox mentioned this pull request Apr 15, 2021

Improving BufferAttribute (maybe) #17089

Open

donmccurdy reviewed Apr 15, 2021

View reviewed changes

donmccurdy mentioned this pull request Mar 22, 2022

BufferGeometryUtils: Add mergeGroups(). #23756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update InterleavedBufferAttribute API to allow shared WebGL buffer across different parameters #21656

Update InterleavedBufferAttribute API to allow shared WebGL buffer across different parameters #21656

takahirox commented Apr 15, 2021 •

edited

Loading

donmccurdy Apr 15, 2021

takahirox Apr 15, 2021 •

edited

Loading

donmccurdy Apr 15, 2021

takahirox Apr 15, 2021

donmccurdy Apr 15, 2021

takahirox Apr 15, 2021

donmccurdy Apr 15, 2021

takahirox Apr 15, 2021

donmccurdy commented Apr 15, 2021

takahirox commented Apr 15, 2021

takahirox commented May 15, 2021

christjt commented Nov 26, 2021

takahirox commented Nov 27, 2021

christjt commented Nov 29, 2021

Mugen87 commented Nov 29, 2021

frericp commented Apr 14, 2022

takahirox commented Apr 14, 2022

frericp commented Apr 27, 2022

jhurliman commented Jul 16, 2022

donmccurdy commented Aug 15, 2022

snagy commented Aug 16, 2022

xpl commented Apr 10, 2024

Update InterleavedBufferAttribute API to allow shared WebGL buffer across different parameters #21656

Are you sure you want to change the base?

Update InterleavedBufferAttribute API to allow shared WebGL buffer across different parameters #21656

Conversation

takahirox commented Apr 15, 2021 • edited Loading

donmccurdy Apr 15, 2021

Choose a reason for hiding this comment

takahirox Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

donmccurdy Apr 15, 2021

Choose a reason for hiding this comment

takahirox Apr 15, 2021

Choose a reason for hiding this comment

donmccurdy Apr 15, 2021

Choose a reason for hiding this comment

takahirox Apr 15, 2021

Choose a reason for hiding this comment

donmccurdy Apr 15, 2021

Choose a reason for hiding this comment

takahirox Apr 15, 2021

Choose a reason for hiding this comment

donmccurdy commented Apr 15, 2021

takahirox commented Apr 15, 2021

takahirox commented May 15, 2021

christjt commented Nov 26, 2021

takahirox commented Nov 27, 2021

christjt commented Nov 29, 2021

Mugen87 commented Nov 29, 2021

frericp commented Apr 14, 2022

takahirox commented Apr 14, 2022

frericp commented Apr 27, 2022

jhurliman commented Jul 16, 2022

donmccurdy commented Aug 15, 2022

snagy commented Aug 16, 2022

xpl commented Apr 10, 2024

takahirox commented Apr 15, 2021 •

edited

Loading

takahirox Apr 15, 2021 •

edited

Loading