Generalize instanced attributes

[OndrejSpanel]

Description

Currently there are two instanced attributes which get a special treatment from the engine: instanceColor and instanceMatrix:

        var geometryAttribute = geometryAttributes(name)
        if (geometryAttribute == undefined) {
          if (name == "instanceMatrix" && `object`.instanceMatrix) geometryAttribute = `object`.instanceMatrix
          if (name == "instanceColor" && `object`.instanceColor) geometryAttribute = `object`.instanceColor
        }

I think it would be possible to make this more flexible by allowing any custom attributes present in InstancedMesh to get the same treatment.

Solution

The code above should be changed to:

        var geometryAttribute = geometryAttributes(name)
        if (geometryAttribute == undefined) {
          if (name == "instanceMatrix" && `object`.instanceMatrix) geometryAttribute = `object`.instanceMatrix
          if (name == "instanceColor" && `object`.instanceColor) geometryAttribute = `object`.instanceColor
          if (`object`.attributes.has(name)) geometryAttribute = `object`.attributes.get(name)
        }

The member attributes would be added into InstancedMesh.

Alternatives

At the moment one can either abuse instanceColor if not needing it for coloring, or provide custom attributes in the geometry, which means for different instancing chains the geometry has to be cloned.

Additional context

For simplicity I suggest keeping current instanceMatrix and instanceColor as they are, but perhaps they could also be moved to the attributes and the code handling them would become simple:

        var geometryAttribute = geometryAttributes(name)
        if (geometryAttribute == undefined) {
          if (`object`.attributes.has(name)) geometryAttribute = `object`.attributes.get(name)
        }

This needs some more though, so that backward compatibility is maintained.

[OndrejSpanel]

What is the sentiment regarding this suggestion? If it has chance to be accepted, I will gladly prepare a PR.

[donmccurdy]

I'm not sure what the special treatment is that you're trying to generalize, could you say more about the problem this change is intended to solve? Can InstancedBufferAttibute + attributes not solve that problem?

[OndrejSpanel]

The InstancedBufferAttibute cannot be added to the InstacedMesh, only to the Geometry. The solution I suggest is just a small extension - allowing any instanced attributes to be added to the InstacedMesh, the same way matrix and color can be now.

The reason is the same as with colors - you want to reuse your geometry for multiple instancing groups.

[donmccurdy]

When a BufferGeometry is reused by two InstancedMesh objects with different instance transforms or colors ... do we upload that geometry to the GPU only once? Or have separate vertex buffers on the GPU for each?

I think instanceColor and instanceMatrix exist on InstancedMesh as API conveniences, I wasn't aware that there was any special case in the GPU upload.

[OndrejSpanel]

When you copy or clone a BufferGeometry, new cloned attributes are created. Perhaps it might be possible to somehow convince different geometries to share attributes, but this is not how it normally works.

The vertex buffer setup is done in WebGLBindingStates.setup, using WebGLAttributes update and createBuffer.

[donmccurdy]

Right, I'm aware that cloning a BufferGeometry will duplicate its data on the GPU. I'm less aware of what guarantees we are trying to make, if any, about whether a single geometry reused with different instance attributes will or will not be duplicated. This has implications for future APIs, such as #17089, so I think we need to be clear about the intended outcome and not merge changes that we do not intend to support.

[OndrejSpanel]

While I understand the concern about guarantees and promises, I think this should not prevent making the API more orthogonal. I suggest extending the privileged handling which is currently done for placement and color to any custom attributes user may have. What is so special about color it can get this handling, and other parameters cannot?