Add BufferGeometry index optimization and memory functions

[gkjohnson]

I added optimizeTriangleIndices and getMemoryUse functions to the BufferGeometry object.

Behavior

BufferGeometry.mergeVertices(tolerance = 1e-4)

Updates (or creates) the vertex attributes and index arrays with optimized, deduped versions representing the same mesh. The tolerance argument determines the decimal precision used when comparing and deduping vertex attributes.

BufferGeometryUtils.estimateMemoryUse(geometry)

Returns the memory allocated for the geometry (vertex attribute and index arrays) as bytes. Useful for seeing how much memory a model is currently taking up.

BufferGeometry.interleaveAttributes(attrArray)

Converts the provided normal attributes into a set of interleaved attributes that share a buffer.

Other

• Add copy and clone functions to InterleavedAttribute

Uses

• Allow memory-constrained applications to release memory (on GPU and CPU) by processing geometry into a more compressed form
• Optimize geometry from loaders and file formats that don't allow triangle indices (Collada Loader does not set BufferGeometry.index #13708)
• Optimize geometry before saving it with an exporter to reduce the file format size
• Estimate how much memory a piece of geometry is using (for debugging and optimization)

Example

https://gkjohnson.github.io/threejs-sandbox/mergeVertices/

Thanks!
Garrett

[takahirox]

1. Does this optimization work fine even with .groups?

2. Just my preference tho, BufferGeometryUtils would be a better place to add these methods?

[gkjohnson]

1. Does this optimization work fine even with .groups?

It does not! Thanks for catching that. I'll rework it so each group is optimized independently. I suppose one of the complexities is that groups can have overlapping draw indices, right? If that's the case I'll probably just print a warning and not optimize.

2. Just my preference tho, BufferGeometryUtils would be a better place to add these methods?

I didn't realize that was there! I think getMemoryUse definitely belongs there, but the triangle optimization pairs nicely with the toNonIndexed function, which is in BufferGeometry.

Thanks @takahirox!

[takahirox]

I suppose one of the complexities is that groups can have overlapping draw indices, right

I think so.

If that's the case I'll probably just print a warning and not optimize.

Perhaps it sounds reasonable. Good start.

[WestLangley]

To summarize, you provide a method that will convert non-indexed "triangle soup" to indexed BufferGeometry that shares vertices when possible, reducing the memory footprint.

This is akin to Geometry.mergeVertices(), and requires that the user specify a precision value, which is used for comparing floats.

Does this PR work for interleaved BufferAttrbutes?

[gkjohnson]

To summarize, you provide a method that will convert non-indexed "triangle soup" to indexed BufferGeometry that shares vertices when possible, reducing the memory footprint.

That's right, but in this case it's using every attribute in the hash to make sure they're all the same before merging the vertices.

This is akin to Geometry.mergeVertices(), and requires that the user specify a precision value, which is used for comparing floats.

I hadn't seen that function, but yeah! A precision value isn't required here -- it's optional and defaults to 3. Maybe it makes more sense for the function to be called mergeVertices so it's consistent with Geometry?

Would it make sense to expose precision as an option for Geometry.mergeVertices, as well?

Does this PR work for interleaved BufferAttrbutes?

It doesn't. I hadn't encountered those before -- I can take a look at what it would take to support that. Is it okay if the process of merging vertices de-interleaves the attributes?

Thanks!

[WestLangley]

That's right, but in this case it's using every attribute in the hash to make sure they're all the same before merging the vertices.

Of course. That is required for BufferGeometry. I think the attributes should be exactly the same to justify merging them.

A precision value isn't required here -- it's optional and defaults to 3

No, a precision value is required. It has a default value of 3.

Maybe it makes more sense for the function to be called mergeVertices() so it's consistent with Geometry?

That was my thinking, too.

Is it okay if the process of merging vertices de-interleaves the attributes?

Probably not.

Would it make sense to expose precision as an option for Geometry.mergeVertices, as well?

It would make sense. There has been no demand for that, though.

[gkjohnson]

No, a precision value is required. It has a default value of 3.

Ha, fair enough.

Is it okay if the process of merging vertices de-interleaves the attributes?

Probably not.

I'll think about that a little more, then.

It also just occurred to me that a BufferAttribute can be shared across multiple pieces of geometry in the scene, so modifying them in place probably isn't okay (as I am here), is that right? It seems like the function should be creating and setting new BufferAttributes, right?

[WestLangley]

Geometry will be deprecated at some point -- or at least will no longer be renderable -- so converting Geometry to BufferGeometry ("triangle soup") will no longer be so much of an issue.

Where I think this PR may be useful is in merging (sharing) vertices. I do not think reducing the footprint is that big of a deal.

There will be limits to the use cases for with your approach will work. You will have to determine that yourself. :)

In the end, it remains to be seen if this PR is "useful enough".

[gkjohnson]

Geometry will be deprecated at some point -- or at least will no longer be renderable -- so converting Geometry to BufferGeometry ("triangle soup") will no longer be so much of an issue.

That's good to know! I was kind of wondering if that was going to happen. Is there a roadmap or anything documenting some of the future plans for the library? I don't see anything in the wiki or docs.

Where I think this PR may be useful is in merging (sharing) vertices. I do not think reducing the footprint is that big of a deal.
...
In the end, it remains to be seen if this PR is "useful enough".

I recognize that my use cases aren't necessarily common ones. I do feel that minimizing the memory footprint before exporting a model to a file is more generally useful, especially when the intent is to load that back to the web.

I'd included the function in BufferGeometry because it seemed like a natural fit along with "toNonIndexed". If the consensus is that this belongs somewhere else then I'll gladly move it! I'm just here to contribute work I'm doing otherwise to a broader community who can hopefully benefit. On that topic, is there any documentation on vision or what does or doesn't belong in the core library? Or is it more community driven?

And finally, here are some of the changes and updates I've made:

Changes

• Merging geometry actually does work with overlapping and non overlapping groups
• Function renamed to mergeVertices
• Function creates new BufferAttributes so as to not mess with other geometry that may also be using them.
• Merging vertices now works with InterleavedBufferAttributes, but de-interleaves them in the process. I'll need a bit more time to think about a clean way to handle that if it's critical that it not de-interleave.
• Added interleaveAttributes to BufferGeometryUtils to make an interleaved attribute from an array of attributes
• Added getMemoryUse to BufferGeometryUtils
• Added copy, clone, and modified the clone for BufferGeometry because it was failing to clone when interleaved buffers were used
• Updated demo to show the changes

[Usnul]

I have a coupld of late API change requests.

precision is not an easy to use concept as it has no intuitive default. I recommend changing this to error or tolerance instead, when error === 0 it will basically do nothing, when error === 0.001 it will work the same way as precision === 3 (if i understand the current logic correctly). So when i use the API i can supply 0 to have nothing done - this is a handy convention. Also, precision parameter affords more control over deduping behaviour (wider range of values, not just 0-X) and it allows working with very large models, where one might want to set precision to a value greater than 1.

I recommend changing name getMemoryUse. First of all, I suggest using word compute instead of get as it conveys more than retrieval. Second, I recommend BuffersByteSize instead of MemoryUse. I.e. computeBuffersByteSize - it's a bit less intuitive, but more explicit in terms of conveying expectations on returned value. Alternatives: BuffersBytesUsed or Attributes instead of Buffers, however, I feel that Buffers is more clear and generic at the same time.

[gkjohnson]

@Usnul Sorry I missed these comments.

I can see the argument for using error rather than precision. Let me look into the right way to implement that approach. I'll also consider the name change for the memory function.

I was also looking back at the memory use function and realized that it probably won't work with interleaved attributes, so I'll fix that, as well.

Thanks!

[gkjohnson]

I've update the PR, again! If this all looks good I can add documentation for the function to the BufferGeometry docs page!

Changes

• Rename BufferGeometryUtils.getMemoryUse to BufferGeometryUtils.estimateBytesUsed
• Fix estimateBytesUsed function to work with interleaved buffers
• Update the mergeVertices function to take a tolerance variable rather than precision

[donmccurdy]

This PR was helpful to me for reducing the size of files when converting an old Geometry example (sittingBox.js) to glTF using GLTFExporter. By default the conversion from Geometry to BufferGeometry drops indices and increases file size 5x, and the mergeVertices function here brought that back down. One suggestion, though, would be to preserve morphAttributes throughout the merge:

https://gist.github.com/donmccurdy/bf17af7acfcdd5ebc57da1200e7b1e46

[donmccurdy]

Suggestions:

• Move .mergeVertices to BufferGeometryUtils
  • I can't think of a compelling reason it needs to be in the BufferGeometry class, so inclined to leave it out.
• Consider leaving InterleavedBufferAttribute clone and copy methods for another PR
  • Are they needed in this one? I don't see where, if so. We've discussed these methods before (InterleavedBufferAttribute has no clone method #15395, GLTFLoader: Support morph targets w/ interleaved attributes #12800 (comment)) and it was thorny, so separating them would simplify this review.
• (Optional) Add a parameter to mergeVertices so it works with points and lines
  • The fact that this requires an extra parameter seems like an argument in favor of creating MeshGeometry / PointGeometry / ... classes.
• (Optional) Basic unit tests

[gkjohnson]

• Move .mergeVertices to BufferGeometryUtils

Done. The function now returns a new BufferGeometry instance, as well.

• Consider leaving InterleavedBufferAttribute clone and copy methods for another PR

Are you referring to these lines?

if ( oldAttribute.isInterleavedBufferAttribute ) {

    attribute = new THREE.BufferAttribute( buffer, oldAttribute.itemSize, oldAttribute.itemSize );

} else {

    attribute = geometry.getAttribute( name ).clone();
    attribute.setArray( buffer );

}

You mean I should get rid of the special case for the interleaved attributes? I can add a warning log, instead.

The other two I'll leave for other PRs, I think. I'll take a look at adding unit tests but the point geometry etc seems more complicated.

You can see the new function working in the two examples in my original repo:

https://gkjohnson.github.io/threejs-sandbox/mergeVertices/
https://gkjohnson.github.io/threejs-sandbox/mergeVertices/animated.html

[yomboprime]

This could serve as a unit test: Could you modify the webgl_physics_volume example? There are only two minor changes:

1 - Edit the processGeometry function to this:

function processGeometry( bufGeometry ) {
	// Obtain a indexed BufferGeometry with merged vertices
	var indexedBufferGeom = THREE.BufferGeometryUtils.mergeVertices( bufGeometry, 0.0001 );
	// Create index arrays mapping the indexed vertices to bufGeometry vertices
	mapIndices( bufGeometry, indexedBufferGeom );
}

2 - Delete the function createIndexedBufferGeometryFromGeometry

Oh, and you will have to include the BufferGeometryUtils

[donmccurdy]

You mean I should get rid of the special case for the interleaved attributes? I can add a warning log, instead.

You've added .copy() and .clone() methods to the InterleavedBufferAttribute class, but then in mergeVertices you've specifically avoided calling clone() on the interleaved attributes. So it doesn't seem like there's any need to modify InterleavedBufferAttribute.js here?

[gkjohnson]

You've added .copy() and .clone() methods to the InterleavedBufferAttribute class, but then in mergeVertices you've specifically avoided calling clone() on the interleaved attributes. So it doesn't seem like there's any need to modify InterleavedBufferAttribute.js here?

Ha, I completely forgot I did that (it's been awhile) -- I've removed it, now.

This could serve as a unit test: Could you modify the webgl_physics_volume example? There are only two minor changes:

That's not a bad idea. I'll look into doing that in a bit.

Thanks!

[gkjohnson]

@donmccurdy @yomboprime

I've gotten the webgl_physics_volume example to work with mergeVertices. One caveat is that I needed to create a new geometry with only the position and index attributes so it merged correctly for this use case. Maybe in a future PR an includeAttributes or excludeAttributes option can be added to the function so only a certain set of attribute data will be used in the hash.

function processGeometry( bufGeometry ) {

    // Ony consider the position values when merging the vertices
    var posOnlyBufGeometry = new THREE.BufferGeometry();
    posOnlyBufGeometry.addAttribute( 'position', bufGeometry.getAttribute( 'position' ) );
    posOnlyBufGeometry.setIndex( bufGeometry.getIndex() );

    // Merge the vertices so the triangle soup is converted to indexed triangles
    var indexedBufferGeom = THREE.BufferGeometryUtils.mergeVertices( posOnlyBufGeometry );

    // Create index arrays mapping the indexed vertices to bufGeometry vertices
    mapIndices( bufGeometry, indexedBufferGeom );

}

[donmccurdy]

nit: i think this got converted from tabs to spaces

[mrdoob]

Yep. Line 199 too.

[gkjohnson]

Thanks I'll get these when I get home today

[gkjohnson]

Done! Let me know if there's anything else!

[mrdoob]

Thanks!

[WestLangley]

Can you update the docs please?