Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BufferGeometry.merge or BufferGeometryUtils.mergeBufferGeometries: Add support for transformation matrix #18918

Closed
1 of 2 tasks
finnbear opened this issue Mar 18, 2020 · 13 comments

Comments

@finnbear
Copy link
Contributor

finnbear commented Mar 18, 2020

Summary of feature request

.addScaledVector but instead of adding a scaled vector, merging a transformed BufferGeometry

Description of feature request

For optimization purposes, I want to render a single buffer geometry instead of rendering each object in my scene separately

const geometry = THREE.BufferGeometryUtils.mergeBufferGeometries(this.children.map(child =>
    child.geometry.clone().applyMatrix4(child.matrix) // The issue
));
this.add(new THREE.Mesh(geometry, this.children[0].material));
for (const child of this.children) {
     child.visible = false;
}

(simplified code, not tested but should convey my point)

Unfortunately, the existing buffer geometry merge methods don't support transforming each merged vertex by a custom matrix. That means I have to clone each child's geometry and apply the matrix to the entire clone at once.

This is a waste of memory, and could be avoided with

  • BufferGeometry.merge(bufferGeometry, optionalMatrix)
  • BufferGeometryUtils.mergeBufferGeometries(bufferGeometries, optionalMatrices)

The 2nd option is slightly more awkward but is preferred because BufferGeometryUtils is lossless (doesn't require manual index argument)

If a matrix is passed in, each vertex being merged in should have it applied.

Note: I know I could get around this by pre-applyMatrix4-ing all my objects' geometries, but I don't want to do that...I want them to share a single, applyMatrix4'd buffer geometry instance.

Note 2: For my use case, each child's BufferGeometry is the same. A solution specific to my case is :

  • BufferGeometryUtils.stamp(bufferGeometry, matrices)

Which would take a single geometry and stamp it at multiple locations/rotations/scales defined by each matrix.

Three.js version
  • Dev
  • r114
@gkjohnson
Copy link
Collaborator

BufferGeometry.merge(bufferGeometry, optionalMatrix)

Can you explain how this function signature would work?

BufferGeometryUtils is lossless (doesn't require manual index argument)

What do you mean by this?

Note 2: For my use case, each child's BufferGeometry is the same. A solution specific to my case is :
...
Which would take a single geometry and stamp it at multiple locations/rotations/scales defined by each matrix.

It seems like InstancedMesh is much better suited for your use case, then, and will use significantly less memory than merging a bunch of geometries.

@donmccurdy
Copy link
Collaborator

What do you mean by this?

bufferGeometry1.merge(bufferGeometry2) overwrites data rather than doing a union. This is rarely what users want.

That means I have to clone each child's geometry and apply the matrix to the entire clone at once. ... This is a waste of memory ...

I'm not sure how we'd implement it in BufferGeometryUtils any other way, without duplicating a fair bit of code from:

  • BufferGeometry.prototype.applyMatrix4
  • BufferAttribute.prototype.applyMatrix4
  • BufferAttribute.prototype.applyNormalMatrix
  • BufferAttribute.prototype.transformDirection

I agree with @gkjohnson that InstancedMesh may be a better fit for this use case.

@finnbear
Copy link
Contributor Author

finnbear commented Mar 18, 2020

@gkjohnson thanks for the reply

BufferGeometry.merge(bufferGeometry, optionalMatrix)

for (vertex of vertices)
   if (optionalMatrix === undefined) {
       do things normally;
   } else {
        apply optionalMatrix to vertex;
        do things normally;
    }
}

To avoid code duplication, a buffer THREE.Vector3 could be allocated once, set to each vertex, applied with the matrix, and then read.

@donmccurdy you're right, that's what I meant by the first thing

As for using an instanced mesh, I think that would be very wasteful for my usecase. Each child object is simply a plane (4 vertices). While I haven't tested it, and while merged geometry and instanced geometry would be the same amount of drawcalls, I think 100000 merged planes will render faster than 100000 instanced planes. (whereas 100000 instanced monkey heads would render faster than 100000 merged monkey heads)

@finnbear
Copy link
Contributor Author

I guess it would be interesting to quantify at what point instancing becomes more efficient than merging. A plane? A box? An icosphere? A monkey head?

@finnbear
Copy link
Contributor Author

I guess I'll try using the InstancedMesh approach, so closing this issue for now. I'll open up another issue if something goes seriously wrong with that approach, and maybe I'll submit a PR for this.

@donmccurdy
Copy link
Collaborator

For something as simple as quads, you may also just want to write the data into a large BufferGeometry directly... the work of transforming geometry without constructing any of the usual three.js primitives (BufferGeometry, BufferAttribute) is not something I'd want to maintain inside of BufferGeometryUtils.mergeBufferGeometries, which is meant to do only one thing. But perhaps a separate utility function could be written. Let us know how that goes, or if you want to consider a PR!

@finnbear
Copy link
Contributor Author

I was almost finished implementing Instanced Mesh usage and the performance was promising (relative to each quad being a separate mesh).

Unfortunately, I am running into errors like this:

Failed to execute 'uniform3fv' on 'WebGL2RenderingContext': No function was found that matched the signature provided. at TypeError: Failed to execute 'uniform3fv' on 'WebGL2RenderingContext': No function was found that matched the signature provided.
    at WebGL2RenderingContext.uniform3fv (<anonymous>)
    at lk.Gj [as setValue] (https://mazean.com/js/lib/three.js/three.min.js:52:470)
    at Function.Eb.upload (https://mazean.com/js/lib/three.js/three.min.js:674:476)
    at x (https://mazean.com/js/lib/three.js/three.min.js:193:436)
    at Renderer_Renderer.renderBufferDirect (https://mazean.com/js/lib/three.js/three.min.js:209:207)
    at k (https://mazean.com/js/lib/three.js/three.min.js:176:309)
    at m (https://mazean.com/js/lib/three.js/three.min.js:176:6)

@gkjohnson
Copy link
Collaborator

You'll have to post a small snippet of your code that sets the mesh or material uniforms -- it sounds like a uniform isn't being set to a valid value. The forum might be a better place to ask.

@finnbear
Copy link
Contributor Author

You'll have to post a small snippet of your code that sets the mesh or material uniforms -- it sounds like a uniform isn't being set to a valid value. The forum might be a better place to ask.

Turned out it was simply a case of material.color.set(0xffffff) instead of material.color = 0xffffff
That always gets me 😆

@finnbear
Copy link
Contributor Author

finnbear commented Mar 19, 2020

Alright, the results are in. I implemented two functions, one that converts my scene to 5 instanced meshes and one that converts it to 5 merged buffer geometries. Instanced meshes were marginally faster (around 18ms per frame instead of 19ms). Mixing the two techniques, using merging for quads and instancing for more complex objects (cubes), seemed like a good idea but the performance was worse for some reason (21ms per frame). However, one clear difference with the two approaches is that with merged buffer geometries, there was a 50-100ms spike in frame latency when it is recompiled (in part due to having to clone each child buffer geometry, the original problem behind this feature request). That doesn't seem to exist with instanced meshes, even when the entire instanceMatrix is resized and rewritten. I'll probably use instanced meshes.

Side note: Using BufferGeometryUtils.mergeVertices actually made the merged scene faster, at 17ms per frame. However, the rebuilding latency increases to 200ms and the memory usage goes from 22mb to 85mb for some reason. Not worth it.

@gkjohnson @donmccurdy pinging you because you might find this interesting, although you did recommend instanced meshes from the beginning. Thanks :)

The code, in case you have any suggestions mergeType(type) { const lowercaseType = type.toLowerCase(); const mergedKey = `_${lowercaseType}sMerged`; const existingCacheKey = `existing${type}s`;
    if (this[mergedKey]) {
        this[mergedKey].geometry.dispose();
        this.remove(this[mergedKey]);
    }

    const geometry = this._stock.geometriesLoader[lowercaseType];

    const mergedGeometry = THREE.BufferGeometryUtils.mergeBufferGeometries(this[existingCacheKey].map(existingObject => {
        existingObject.merged = true;
        return geometry.clone().applyMatrix4(existingObject.matrixWorld);
    }));

    const optimizedGeometry = THREE.BufferGeometryUtils.mergeVertices(mergedGeometry, 0.001);

    this[mergedKey] = new THREE.Mesh(optimizedGeometry, this._stock.materials[lowercaseType]);

    if (!this[mergedKey].parent) {
        this.add(this[mergedKey]);
    }
}

instanceType(type) {
    const lowercaseType = type.toLowerCase();
    const mergedKey = `_${lowercaseType}sMerged`;
    const existingCacheKey = `existing${type}s`;
    let cacheKey = `${lowercaseType}s`;
    if (!this[cacheKey]) {
        cacheKey = existingCacheKey;
    }
    if (!this[mergedKey] || this[mergedKey].maxCount < this[existingCacheKey].length) {
        if (this[mergedKey]) {
            this.remove(this[mergedKey]);
        }
        this[mergedKey] = new THREE.InstancedMesh(this._stock.geometriesLoader[lowercaseType], this._stock.materials[lowercaseType], Math.round(this[cacheKey].length * 1.05 + 6));
        this[mergedKey].maxCount = this[mergedKey].count;
        this[mergedKey].instanceMatrix.setUsage(THREE.DynamicDrawUsage);
    }

    this[mergedKey].count = this[existingCacheKey].length;

    for (let i = 0; i < this[existingCacheKey].length; i++) {
        this[mergedKey].setMatrixAt(i, this[existingCacheKey][i].matrixWorld);
        this[existingCacheKey][i].merged = true;
    }

    this[mergedKey].instanceMatrix.needsUpdate = true;

    if (!this[mergedKey].parent) {
        this.add(this[mergedKey]);
    }
}

@donmccurdy
Copy link
Collaborator

@finnbear both your results and your code look reasonable to me.

I expect that a carefully designed function could write arbitrary objects, with arbitrary transforms, into a large geometry buffer efficiently enough to make the 50-100ms latency go away. For an imaginary BufferGeometryBuilder example:

var merged = new BufferGeometry();
merged.setAttribute( 'position', new Float32Array( 1e6 ), 3 );
merged.setAttribute( 'normal', new Float32Array( 1e6 ), 3 );
merged.setAttribute( 'uv', new Float32Array( 1e6 ), 2 );
merged.setIndex( new Uint8Array( 1e6 ), 3 );

var builder = new THREE.BufferGeometryBuilder( merged );
var offset1 = builder.append( geometry, matrix1 );
var offset2 = builder.append( geometry, matrix2 );
var offset3 = builder.append( geometry2, matrix1 );

// later
builder.update( offset2, geometry, matrix3 );

However, this sort of API seems better as a new thing, rather than a change to mergeBufferGeometries.

@titansoftime
Copy link
Contributor

@finnbear both your results and your code look reasonable to me.

I expect that a carefully designed function could write arbitrary objects, with arbitrary transforms, into a large geometry buffer efficiently enough to make the 50-100ms latency go away. For an imaginary BufferGeometryBuilder example:

var merged = new BufferGeometry();
merged.setAttribute( 'position', new Float32Array( 1e6 ), 3 );
merged.setAttribute( 'normal', new Float32Array( 1e6 ), 3 );
merged.setAttribute( 'uv', new Float32Array( 1e6 ), 2 );
merged.setIndex( new Uint8Array( 1e6 ), 3 );

var builder = new THREE.BufferGeometryBuilder( merged );
var offset1 = builder.append( geometry, matrix1 );
var offset2 = builder.append( geometry, matrix2 );
var offset3 = builder.append( geometry2, matrix1 );

// later
builder.update( offset2, geometry, matrix3 );

However, this sort of API seems better as a new thing, rather than a change to mergeBufferGeometries.

Now that THREE.Geometry is being removed this suggestion/idea is even more important.

@donmccurdy
Copy link
Collaborator

Following up on this idea in #22376.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants