None of merge() calls involving BufferGeometry are working

[makc]

What's the point of having these methods if they do nothing? https://jsfiddle.net/f2Lommf5/2523/ I thought you were deprecating THREE.Geometry

[donmccurdy]

It does something, but not what people expect / not what Geometry.prototype.merge() does. To get a meaningful result when merging BufferGeometry, you must give an offset; the contents of the first geometry will be replaced with the other's, starting at that offset. In r91 there will be a warning when calling bufferGeometry.merge() without any offset.

To losslessly merge BufferGeometries you must pre-allocate size, or (in r91) use —

BufferGeometryUtils.mergeBufferGeometries( [ geom1, geom2, geom3, ... ] );

See: #13241

[looeee]

I'm still not sure what the purpose of BufferGeometry.merge is though. What's a typical use case?

Also, now we have two different methods involving BufferGeometry that have the same name but do different things. This seems unnecessarily confusing.

[donmccurdy]

I think a.merge( b ) exists for backwards-compatibility with THREE.Geometry, even though it does something subtly different. A couple use cases I can think of for lossless merging:

1. A-Frame allows users to merge primitive geometry generated by their markup at runtime, for performance improvements. We removed this feature after hitting the inconsistencies with BufferGeometry.
2. Could imagine the Editor or another authoring tool wanting to merge meshes with shared materials to optimize a model before writing to OBJExporter or GLTFExporter.
3. GLTFLoader could use this method internally to combine primitives into a single multi-material mesh during loadout.

I guess the question is, (a) do those use cases justify having the full implementation in three.js core, and (b) if not, should we keep the somewhat inconsistent API we have, or just deprecate a.merge( b ) entirely in favor of something in examples/js/*?

[makc]

a.merge( b ) exists for backwards-compatibility with THREE.Geometry, even though it does something subtly different.

well there is no any:

merge: function ( geometry, offset ) {

they do not take matrix, but offset. that's not subtly different, that's entirely different. with that, the assumption that I operated on is invalid, and so is this issue :) thanks for BufferGeometryUtils.merge pointer!

[looeee]

@donmccurdy perhaps I'm misunderstanding, but at least your 2nd and 3rd examples would require the BufferGeometryUtils version, right? That one has obvious use cases and we should definitely keep it, in core or examples.
I'm wondering about the less obvious and destructive version which is currently part of the core. From my perspective, we should probably switch these two around, or even remove BufferGeometry.merge entirely unless we can identify some common use cases.

[donmccurdy]

Yeah, I expect the use cases for both are the same. The BufferGeometryUtils version is a bit more code and an even-less-backwards-compatible API, so I assumed it should be in examples/js instead of replacing .merge() as some previous PRs tried to do. But for all of my use cases, merging N geometries in a batch is preferable to merging pair by pair.

[WestLangley]

I'm still not sure what the purpose of BufferGeometry.merge is though. What's a typical use case?

BufferGeometry.merge() is for merging non-indexed buffer-geometries into a single BufferGeometry.

One way to use it:

1. Instantiate a new BufferGeometry.
2. Add the desired attributes, and allocate the attribute buffers with sufficient size to hold the geometries you want to merge.
3. Call bufferGeometry.merge( bg, offset ) for each non-indexed buffer-geometry, incrementing offset as you go by attributes.position.count.
4. Set bufferGeometry.drawRange if the allocated attribute buffers are over-sized.

[looeee]

@WestLangley yes, I'm aware that you can do that. However it's overly complex and not obvious.

If the only use case for BufferGeometry.merge is to go through all the steps you just mentioned, shouldn't the method do it automatically? Which is presumably what BufferGeometryUtils.merge does.

[WestLangley]

However it's overly complex and not obvious.

I do not think it is complex at all, it is just low-level.

I had previously considered proposing BufferGeometry.merge() be renamed to BufferGeometry.copyAt(), because that is what is does.

Thing is, where copy() copies all attributes from the source into new destination attributes, merge() populates the existing destination attributes with data from the source.

Maybe merge() can be renamed to populateAt() -- or mergeAtIndex().

[looeee]

Ok, I guess complexity is a personal opinion - I'm trying to think about it from the point of view of a beginner.

However the main issue here is that we have three geometry merging functions, and this one does something different to the other two. I agree that it should be renamed. I would go for mergeAtIndex.