[feat] allow GLBufferAttribute as an attribute type

[mikkom]

Why

The current typings don't allow setting GLBufferAttribute as an attribute in BufferGeometry. This functionality has been in Three.js for quite some time (originally introduced in mrdoob/three.js#13196 back in 2020).

Three.js master continues to support this feature in BufferGeometry (see e.g. https://github.com/mrdoob/three.js/blob/master/src/core/BufferGeometry.js#L371), documents the attribute type in https://threejs.org/docs/#api/en/core/GLBufferAttribute and has a usage example in https://github.com/mrdoob/three.js/blob/master/examples/webgl_buffergeometry_glbufferattribute.html#L115-L117.

What

This change adds GLBufferAttribute as a valid type for setAttribute and getAttribute methods in BufferGeometry. The type for GLBufferAttribute had already been defined and exported but it was not used anywhere else in the typings.

Note that the change can break consumers of the typings as GLBufferAttribute does not implement the same interface as BufferAttribute. The way to fix these issues is to use the boolean flags (isBufferAttribute, isInterleavedBufferAttribute, or isGLBufferAttribute) to discriminate between the different options (or alternatively by resorting to type assertions). Despite the potential breakage, I believe these typings are more correct.

Checklist

• Checked the target branch (current goes master, next goes dev)
• Added myself to contributors table
• Ready to be merged

[0b5vr]

I don't think these false are explicitly set.

[mikkom]

True that. Should I rather set these as undefined instead of false? If we omit these fields completely then we no longer can use them to discriminate between the different attribute types.

[0b5vr]

Ok that's the motivation, I got it.
You want to use position.isGLBufferAttribute here right?

three-ts-types/types/three/test/controls/controls-pointercontrols.ts

Line 129 in 9ce3aa4

if (!position.isGLBufferAttribute) {

But having false here only for this is lying since it isn't actually false.

You can solve this by either of these 3 solutions:

1. use !(position instanceof GLBufferAttribute) instead for the condition
2. use 'setXYZ' in position instead for the condition
3. type assert the position, let position = floorGeometry.attributes.position as BufferAttribute;

I prefer the (3), since it's obvious that the position is BufferAttribute here since it's made by new THREE.PlaneGeometry().

[mikkom]

To me it seems that it would be most natural to use the publicly documented boolean fields (e.g. https://threejs.org/docs/#api/en/core/BufferAttribute.isBufferAttribute) for checking this. So my suggestion would be to still change the false values to undefined and then the types would precisely match the returned values. If we omit the non-true fields from TS types then it's not possible to refer to the boolean field unless you already have the corresponding attribute type. And then there's no longer any point in checking a trivially true field :)

The three solutions you listed above definitely do work and nothing prevents the user from choosing any of them. And I agree that in many cases a type assertion can be the most straightforward option. I just think that it would be better to support the documented fields in a meaningful way in TS, as well. WDYT?

[0b5vr]

where is it set?

[mikkom]

Good point. For whatever reason the name property isn't initialized as an empty string in GLBufferAttribute even though both the other attribute types do that. I'll remove the comment and the property (even though this adds risk of further breakage if people are relying on using the name property on attributes...).

[mikkom]

Another option would be to set the field as

    /**
     * @default undefined
     */
    name?: string;

[0b5vr]

I agree that it's weird GLBufferAttribute doesn't have name on the Three.js side.
But, if it's not appeared in the Three.js source yet, it should not be added here I think.

(even though this adds risk of further breakage if people are relying on using the name property on attributes...)

That's actually a good point.
I feel like we have to ask Three.js side about GLBufferAttribute.name first.

Another option would be to set the field as

    /**
     * @default undefined
     */
    name?: string;

I choose this if Three.js side says GLBufferAttribute.name should exist.

@mikkom Would you ask about this somewhere? I would choose Discourse.

[0b5vr]

You did it on Three.js side, wooo! Thank you! Now it's valid to say the .name (in the next revision, r149).

[mikkom]

Heh yeah, finally got to pushing this forward :) I think I should now retarget this PR against dev instead of master, right?

[0b5vr]

Check 2 comments I left. Otherwise it looks good to me.