Three.js: BufferGeometry: .setIndex method does not take null as input

[martinRenou]

The following TypeScript code does not compile:

const geometry = new THREE.BufferGeometry();

geometry.setIndex(null); // Argument of type `null` is not assignable to parameter of type `BufferAttribute | number[]`.

I guess supporting this code would mean allowing to dynamically turn an indexed geometry into a non-indexed geometry without going through the toNonIndexed helper method _e.g._ without creating another BufferGeometry instance.

I am wondering if that's a case that could easily be supported and if I should open a PR for that. If not, feel free to close this issue.

[martinRenou]

Also, the documentation seems wrong concerning the .index attribute and the .getIndex method, it does not say that index can be null.
EDIT: I can come up with a PR for this.

[Mugen87]

Feel free to improve the TS declaration file and the docs 😊

[martinRenou]

Sure!

Do you know if the following JavaScript code simply works out-of-the-box? Is it safe to use?

const geometry = new THREE.BufferGeometry();

geometry.setIndex(null);

So should I change its TypeScript definition to:

setIndex( index: BufferAttribute | number[] | null ): void;

?

[makc]

@martinRenou but if you just nuke the index, vertices become unordered mess, there might not even be enough of them to form valid mesh

[martinRenou]

Yes, I simply expect my users to know what they are doing. So if they change the index, they must change the vertices.

[martinRenou]

Just to give more information about what I'm doing, I'm exposing this API to my users:

  set index (index: Uint32Array | null) {
    this._index = index;

    const indexBuffer = index == null ? null : new THREE.BufferAttribute(index, 1);

    // I need a corner case for indexBuffer == null here if setIndex does not support null
    this.geometry.setIndex(indexBuffer);
  }

  get index () {
    return this._index;
  }

  _index: Uint32Array | null

[makc]

if they change all the attributes any way, how is it different from creating new geometry?

[martinRenou]

Not different, it is true. I won't pretend I am doing things right here. But if it is not that different, maybe it's not a big deal?

I'm really just proposing more homogeneity in the ThreeJS API by allowing to pass null to the setIndex method the same way getIndex can return null. I understand it is not a common use case though. If that is an issue for you I can simply just open a PR for fixing the documentation and the .getIndex definition and forget about this setIndex thing.

[Mugen87]

I'm really just proposing more homogeneity in the ThreeJS API by allowing to pass null to the setIndex method the same way getIndex can return null.

This makes definitely sense.

[donmccurdy]

Agreed, it should be possible to undo a setIndex call.

For consistency, note that BufferGeometry has setAttribute / getAttribute / deleteAttribute, and setIndex / getIndex, but there is no deleteIndex. I could definitely be persuaded that we don't need delete methods if the setters accept null...

[Mugen87]

Um, I did not think of deleteIndex() so far. But I really like it^^.

[Mugen87]

@martinRenou Do you want to introduce BufferGeometry.deleteIndex()?

[donmccurdy]

With apologies in advance for bikeshedding, the name deleteFoo feels unusual in JavaScript, to me... What do you think of removeFoo instead? Or setFoo( null )?

EDIT: For reference remove\w+\( has ~380 matches in the codebase, delete\w+\( has ~40. Methods like object.remove(...), el.removeEventListener(...), etc.

[WestLangley]

Agreed, it should be possible to undo a setIndex call.

What would be a compelling use case? I am inclined to agree with @makc.

[donmccurdy]

For me it's mostly an API consistency and ergonomics issue... if the following code is valid:

var geometry = new BufferGeometry();
geometry.setAttribute( 'position', positionAttribute );

// index is null.
geometry.setIndex( index );
// index is not null.

... then we are allowing users to incrementally construct BufferGeometry instances with "invalid" intermediate states, and as long as the state is valid when rendering, that's fine. For the reverse — removing an index — not to work feels like a surprising "gotcha" in the API. Patterns exist for immutable objects (Builder patterns, or requiring immutable properties as constructor arguments) but irreversible setters are unusual.

I can contrive some use cases...

• declarative frameworks like react-three-fiber or A-Frame might want to clear properties in one method and set the new values in another
• user might use multiple indexes as more flexible draw ranges on a "triangle soup" mesh, or remove that index to draw the entire mesh

... but IMO the priority is "what is least likely to surprise the developer" here.

[martinRenou]

For me it's mostly an API consistency and ergonomics issue

:+1:

Actually I came to open this issue because of this:
I am exposing an index getter and setter to users, hiding some complexity to the user. The thing is TypeScript does not seem to allow the setter to take a different type than the getter returns, which makes total sense. This enforces my index setter to accept null type as input.

[martinRenou]

I just want to add a note on the fact the setIndex implementation might already accept null as input, it's only the TypeScript definition that lacks this input type.

[xuhaowei9527]

thanks

[Mugen87]

I just want to add a note on the fact the setIndex implementation might already accept null as input, it's only the TypeScript definition that lacks this input type.

Then feel free to enhance the respective TS declaration file if this solves your original issue 👍

[Mugen87]

the name deleteFoo feels unusual in JavaScript, to me... What do you think of removeFoo instead? Or setFoo( null )?

setAttribute() and deleteAttribute() were introduced since they map nicely to the interface of Map. Example:

// map

const myMap = new Map();

myMap.set( 'key', 'value' );
myMap.get( 'key' );  
myMap.delete( 'key' );

// buffer attribute

const geometry = new BufferGeometry();

geometry.setAttribute( 'position', attribute );
geometry.getAttribute( 'position' );
geometry.deleteAttribute( 'position' );

So I think deleteIndex() would be okay against this backdrop.