BufferGeometryUtils.mergeBufferGeometries() ignores existing groups

[WestLangley]

We can decide if we want to honor existing groups when merging.

If not, then this issue can be closed.

If we do chose to honor groups, it would be nice if the groups were optimized -- especially if the geometries share materials.

• [ x ] r115

[WestLangley]

Currently, setting useGroups to true does something different; it does not honor existing groups, it creates new ones.

Feedback requested.

/ping @takahirox
/ping @donmccurdy

[donmccurdy]

It's fine with me if you'd like to change how mergeBufferGeometries handles existing groups, yes.

That said, mergeBufferGeometries was originally written for a specific narrow purpose — creating multi-material meshes from ungrouped geometry in GLTFLoader, as a BufferGeometry-compatible replacement for Geometry.merge(). It's no longer used by GLTFLoader, and is now being used for more general things, but still retains the original narrow API. We could create a much more flexible API.

I've written a few suggestions on this in #18918 (comment), but you could also imagine a version that handles groups:

var geometry = new BufferGeometryBuilder()
  .add(geometry, matrix1)
  .add(geometry, matrix2)
  .add(geometry2, matrix1)
  .sortGroups()
  .toBufferGeometry();

To be honest I have mixed feelings about the multi-material groups API. I think users assume it reduces draw calls when it does not. And it makes real optimizations, like batching, much harder to implement. But this is probably a less focused answer than you were hoping for. 😇

[WestLangley]

@donmccurdy Thank you for your reply!

I think users assume it reduces draw calls when it does not.

If you are properly merging geometries having groups, and you are honoring the groups, it can reduce draw calls. Properly merging N BoxBufferGeometry, for example, will result in 6 draw calls.

I do not care if we support this or not. But in light of what @donmccurdy said, I would remove the useGroups flag from the existing implementation -- unless there is a compelling reason not to.

[WestLangley]

@donmccurdy FYI - closing this. Thank you for your feedback!

[donmccurdy]

Following up on #19164 (comment) in #22376.