Added GLBufferAttribute

[raub]

If present, would delegate VBO creation to the user-supplied function. See more info on this in #11883. The default behavior is untouched.

[pailhead]

More hooks like this!!

[raub]

Now a new DynamicBufferAttribute class exists. Example: webgl_buffergeometry_points_dynamicbufferattribute.html.

new DynamicBufferAttribute(...) parameters:

• gl - renderer.context,
• buffer - an actual VBO,
• type - e.g. gl.FLOAT,
• itemSize - vector size, e.g. 3 for xyz position,
• count - vertex count.

[mrdoob]

Do you mind renaming it to GLBufferAttribute? BufferAttribute has a dynamic property (which sets gl.DYNAMIC_DRAW) and I could see people getting confused.

[raub]

I see your point, but all of them (buffer types) are of GL kind... GLBufferAttribute is not a perfect name either. Could it be something else? Like

• ManualBufferAttribute
• ExternalBufferAttribute
• SharedBufferAttribute
• (other suggestions welcome)

I think prefix GL tells really nothing about what is special in this new attribute class, so it should be the last resort name of choise.

Edit: I found out that the name DynamicBufferAttribute has already been taken by a deprecated class. I personally like the idea of the ExternalBufferAttribute name, because this is all about the VBO's created elsewhere. Would you agree?

[raub]

@mrdoob, since no answer from you, I've decided to take some action and finally rename the class to ExternalBufferAttribute. Is this fine? What else is to be done before the merge could hapen?

[mrdoob]

I still prefer GLBufferAttribute. We're planning to also add GLLine for people that want to use raw gl.LINE instead of the new line with linewidth support. This naming also hints the developer that no other renderers will be able to handle these classes.

Sorry I couldn't elaborate in time for r92, moving to r93.

[raub]

Ok, will do soon

[raub]

Done, GLBufferAttribute. Seems to work fine with the latest dev branch.

[WestLangley]

Nice demo. :-)

Can you please: add your new example to examples/files.js, update the docs with your new class, and check your formatting. https://zz85.github.io/mrdoobapproves/

[Mugen87]

Is elementSize also intended to be private?

[raub]

It is intended to be public. The idea is as follows: you know the type of your data, you pass it, the elementSize becomes available, carrying the size-in-bytes value for you.

It is not necessary. We could let the app do all the calculations and leave here only what's necessary for three.js to handle the attribute.

[raub]

Ah wait, I found it here: src\renderers\webgl\WebGLAttributes.js

			buffers.set( attribute, {
				buffer: attribute.buffer,
				type: attribute.type,
				bytesPerElement: attribute.elementSize,
				version: attribute.version
			} );

so basically it is there for three.js to get the byte size, because there is no Array instance to do so (bytesPerElement: array.BYTES_PER_ELEMENT,).

[Mugen87]

Um, have you considered to let the user set this value? I mean GLBufferAttribute is obviously a low-level API not necessarily intended for beginners. Devs who use it should also know the correct bytes per element.

I would suggest to put as less logic as possible into the ctor and let instead the user defined such information. In this way, you probably don't even need a WebGL rendering context in GLBufferAttribute which I find a bit strange anyway.

[raub]

The logic behind this is that passing both type and elementSize to the constructor is redundant, because the type always implies the only possible elementSize. So it kind of creates a room for an error to occur when you ask the user to pass both. So I decided to calculate it on the ctor side.

But in general it would be fine either way. If the ctor takes elementSize, we get rid of all the logic and the contraversial this.sizes member. Also no need for gl then. Do you approve of the latter scenario?

[Mugen87]

I would say yes. Keeping GLBufferAttribute simple and let the app handle this bit sound like the better design to me. Especially since we can assume the user knows the relationship between type and elementSize if this class is going to be used.

The final decision is up to @mrdoob though 😉

[raub]

Any updates on that matter? @Mugen87 @mrdoob

[Mugen87]

Let's wait for @mrdoob's feedback.

[raub]

sizes: [ [ number, number ] ]; I think should be sizes: { [number]: number }; something. Unsure if this is a fine piece of TS syntax though. How can we validate it?

[Mugen87]

If you are unsure about this, use any for the moment^^.

[pvaananen]

Sorry for bothering you but I'm really looking forward to this addition. So the current status is that the PR is waiting for @mrdoob's feedback?

Regarding that TypeScript question on Apr 27, may I suggest declaring an array of number pairs with sizes:Array<[number, number]> or sizes:[number, number][].

[mrdoob]

Ah, too close to release now... Lets see if we can merge this at the beginning on the next dev cycle.

[Mugen87]

@mrdoob Before merging, can you please review https://github.com/mrdoob/three.js/pull/13196/files#r416037718? I'm not sure about the current public interface of GLBufferAttribute.

[raub]

1. Removed "gl" constructor param.
2. Removed "normalized" constructor param.
3. Added "elementSize" constructor param.
4. Removed "sizes" property.
5. Updated the TS class declaration.
6. Updated the docs.
7. Updated the example to fit new interface.
8. Updated the example logic for e2e compliance.
9. Debugged and fixed an issue when buffers don't change under VAO mode. (pls review!)
10. Generated a screenshot and passed e2e.

[Mugen87]

Thanks @raub! This looks much better to me now!

[mrdoob]

I think you can delete webgl_buffergeometry_points_glbufferattribute.png.

[mrdoob]

I'll merge and clean up.

[mrdoob]

Thanks!

[Mugen87]

@raub Can you please explain why you have added this line? When removed, the respective demo does not seem to work correctly anymore.

It's actually not correct to always force a call of setupVertexAttributes() on each frame if e.g. only the buffer contents change.

[raub]

I don't exactly understand what this part of the system is supposed to do. But my guess is:

• if there is no VAO, bind appropriate buffers
• if there is a VAO and the attributes were not changed, just switch to that VAO
• if attributes changed, update the VAO, i.e. switch to it and bind the updated buffers

That all sounds good in theory. But when I noticed my demo was broken, I started debugging and ended up here. So basically I set needsUpdate to my attribute and expect the engine to do the job of updating whatever was changed (the buffer). That was not the case here.

For some reason, cachedAttribute always contained an already-updated version of my attribute. So the VAO never got updated (hence the demo was broken). I double-checked this fact by adding some custom properties directly into the buffer, like buffer.name=111.

So I decided to opt-out from this optimization with my attribute type. I expect that line 179 effectively makes VAO unused for GLBufferAttribute. That is fine with me. Although if you understand what is going wrong and how to make it work with this optimization, that would also be great.

[Mugen87]

The problem is that the current implementation is bad for performance since a lot of unnecessary state changes are done.

We definitely have to revisit this issue otherwise I vote to revert GLBufferAttribute before the next release.

[raub]

The current implementation is vastly superior than having no implementation. And if I'm right, for webgl1 people the further discussion makes no sense, since they always get exactly this level of performance. So there is a considerable group of people who either get this feature or not, regardless of VAO optimizations.

Now to optimizations. I would gladly fixed the VAO issue, given it was at all possible for you (or anybody) to explan why the cached attribute is always the updated attribute itself. And if it is the correct behavior, how it can be possibly fixed.

If you want, I might go on and report this behavior as a bug in a separate issue. Because now it is my understanding, that when I set needsUpdate on my attribute, the cache should contain the old version. If I have just updated the attribute, and the "cached" version does already contain what I have updated, it is a bug in WebGLBindingStates unless proven otherwise.

[Mugen87]

This has nothing to do with WebGL 1 or 2. VAO is available for most WebGL 1 users. Besides, even without VAO the buffer attribute bindings should only be done if necessary. Not if just buffer data changes.

I think the problem in your demo is this line:

posAttr.buffer = ( posAttr.buffer === pos ) ? pos2 : pos;

You should exchange the entire attribute and not just the buffer. Or you keep the buffer the same and only change the contents. But just exchanging buffers is not something the engine wants to promote (it's not supported anyway and support will NOT be added).

[Mugen87]

This pattern works without the hack in WebGLBindingStates:

var posAttr1 = new THREE.GLBufferAttribute( pos, gl.FLOAT, 3, 4, particles );
var posAttr2 = new THREE.GLBufferAttribute( pos2, gl.FLOAT, 3, 4, particles );
geometry.setAttribute( 'position', posAttr1 );

setInterval( function () {

    var attr = geometry.getAttribute( 'position' );

    geometry.setAttribute( 'position', ( attr === posAttr1 ) ? posAttr2 : posAttr1 );
 
}, 2000 );