Account for existing groups in BufferGeometryUtils.mergeBufferGeometries

[zach-capalbo]

Related issue: N/A

Description

Currently, when merging BufferGeometries that already have groups, BufferGeometryUtils.mergeBufferGeometries will totally ignore the existing groups, even if useGroups is set to true. This can be quite annoying, especially if you string together multiple calls to mergeBufferGeometries, since each time you will lose the groups created by the previous call (and of course any pre-existing groups.)

This PR changes the behavior of mergeBufferGeometries to preserve existing groups, but offset the material index for each geometry. If none of the geometries have groups, then the behavior is unchanged.

Example

Say you have geometry A and B, something like:

A groups:

{ offset: 0, count: 5, materialIndex: 0 }
{ offset: 5, count: 5, materialIndex: 1 }

B groups:

{ offset: 0, count: 3, materialIndex: 0 }
{ offset: 3, count: 3, materialIndex: 0 }
{ offset: 6, count: 3, materialIndex: 1 }

The current behavior results in the following groups for the merged geometry:

{ offset: 0, count: 10, materialIndex: 0}
{ offset: 10, count 9, materialIndex: 1}

With this PR, the groups for the merged geometry would instead be:

{ offset: 0, count: 5, materialIndex: 0 }
{ offset: 5, count: 5, materialIndex: 1 }
{ offset: 10, count: 3, materialIndex: 2 }
{ offset: 13, count: 3, materialIndex: 2 }
{ offset: 13, count: 3, materialIndex: 3 }

[WestLangley]

Related: #19164

[zach-capalbo]

It looks like this has already been discussed fairly extensively (Thanks @WestLangley for the pointer) and the approach was rejected in favor of a future BatchedMesh feature (#22376).

I'm happy to withdraw this PR if that's still the direction to go. However, I'll leave it open for just a bit longer in case anyone wants to reconsider. It has been over two years now since #19164 was originally opened, and there's still no BatchedMesh replacement. Even if this doesn't reduce draw calls, it can still be useful for reducing the number of times needed for traversal or for matrix updates when there are many meshes that could be merged.

[WestLangley]

@donmccurdy Do you have an opinion or preference on this?

[donmccurdy]

I'm still expecting we can add BatchedMesh (or similar) before too long, and as I've said before I'm not terribly fond of the geometry "groups" API. But this PR is much more concise than I expected the necessary changes might be, so I don't see any problem with including the additions in mergeBufferGeometries. Thanks @zach-capalbo! 🙏

[WestLangley]

Clarification: @donmccurdy @gkjohnson do you approve this PR?

[gkjohnson]

I'm not sure if this is correct. It looks like this code assumes that all groups are sequential and situated next to each other but that is not guaranteed. I believe group.start needs to be accounted for.

[zach-capalbo]

Yep, good catch. I've adjusted this now to account for group.start

[gkjohnson]

It seems that "nextOffset" is set to the end of the final group, correct, and not the end of the full geometry? So the geometry will effectively be trimmed only on the end?

It might be a corner case but I don't think that's what I would expect. If geometry unreferenced by groups is going to be culled it should be done consistently (ie at the beginning and in the middle of the group ranges, as well). I think I'd expect all the geometry (vertices, indices, etc) to be merged the same way always with the groups being the only thing that's handled differently.

[zach-capalbo]

Agreed. That would be unexpected.

I've pushed an additional change which addresses this issue. At this point, I've considered these cases; am I missing any?

• Contiguous groups
• Incrementing materialIndex
• Non-incrementing materialIndex
• Overlapping group start/ends
• Gaps between group start/ends
• Groups that don't span the entire position attribute
• Groups that don't start at 0

Here are cases that I'm intentionally not dealing with. I expect these are "invalid" groups:

• group.start < 0
• group.count < 0
• group.materialIndex < 0
• Groups where group.start + group.count >= count (Group reaches past the end of the geometry's vertices)

[gkjohnson]

I'm still thinking through this code - but shouldn't the "material index" parameter here be driven by nextGroupIndex instead of i?

[zach-capalbo]

Yes.. I've changed it to groupIndex (Set to nextGroupIndex at the start of the outer for loop)

[WestLangley]

As currently-written, the program does this:

geometry1.addGroup( 0, Infinity, 0 );
geometry2.addGroup( 0, Infinity, 1 );

geometry = BufferGeometryUtils.mergeBufferGeometries( [ geometry1, geometry2 ], true );

output:
groups: Array(2)
0: {start: 0, count: Infinity, materialIndex: 0}
1: {start: 6, count: Infinity, materialIndex: 2} // <===

The behavior of Infinity is one issue that has to be resolved, but my primary concern is:

I do not think the material indices should change when geometries are merged.

And to be frank, I am not sure it is even useful to merge geometries having different materials, since it is a separate draw call per group, anyway. Maybe someone can provide a compelling use case...

/ping @donmccurdy @Mugen87

[zach-capalbo]

The behavior of Infinity is one issue that has to be resolved

I didn't realize that was valid. I've updated this PR in a way that I believe addresses that.

And to be frank, I am not sure it is even useful to merge geometries having different materials

I'll wait for this to be decided before making further changes here.

[Mugen87]

The behavior of Infinity is one issue that has to be resolved

Actually, your code snippet is not valid (see #23740). The docs state:

Every vertex and index must belong to exactly one group — groups must not share vertices or indices, and must not leave vertices or indices unused.

Since mergeBufferGeometries() supports groups, the issue mentioned in the first post should be fixed.

[WestLangley]

Actually, your code snippet is not valid

Yes it is. It is the output from the merge that is not valid.

Infinity is supported for both draw range and groups count.

//

As I said, the primary issue is:

I do not think the material indices should change when geometries are merged.

Changing a material index to a different value is not helpful. The user should make sure the group material indices are as-desired prior to merging.

[zach-capalbo]

Changing a material index to a different value is not helpful. The user should make sure the group material indices are as-desired prior to merging.

What should the behavior be when merging geometry that has groups with geometry that doesn't? Current useGroups behavior sets a different material index for different geometries. It seems confusing to me to keep that behavior and not update the material index of merged groups. E.g., this seems odd:

geometry1.addGroup( 0, 6, 0 );
geometry2.groups.length = 0;
geometry3.addGroup( 0, 6, 0 );

geometry = BufferGeometryUtils.mergeBufferGeometries( [ geometry1, geometry2 ], true );

output:
groups: Array(3)
0: {start: 0, count: 6, materialIndex: 0}
1: {start: 6, count: 6, materialIndex: 1}
2: {start: 12, count: 6, materialIndex: 0}

It seems like it would be more consistent to either: 1) Offset the material index of output group 2 to be 2, like I've attempted in this PR, or 2) change the behavior of useGroups to not increment the material index, so that the materialIndex of output group 1 becomes 0.

[Mugen87]

@WestLangley Sorry, but the code snippet is not valid since your groups overlap.

[WestLangley]

@WestLangley Sorry, but the code snippet is not valid since your groups overlap.

@Mugen87 No they do not. They are on different geometries.

It is the responsibility of the merge to set proper group start and count.

[Mugen87]

They are on different geometries.

Sorry, I have missed that. It's indeed geometry1 and geometry2.