SkinnedMesh+InstancedMesh: Removing the buffer array in onUploadCallback throws errors while rendering

[GoJermangoGo]

Description

I'm using onUploadCallback to remove the array because it saves hundreds of MB of RAM in our application.

//delete the buffer after uploading to GPU to free RAM
BufferAttribute.prototype.onUploadCallback = InterleavedBuffer.prototype.onUploadCallback = function()
{
	this.array = null;
};

With r150, this was a safe operation that didn't have any problems. Starting with r151 though, there have been a few changes to the WebGLRenderer's render pipeline regarding the bounding sphere's usage: #25913, #25591. In the case of SkinnedMesh and InstancedMesh, the bounding sphere is now stored on the Object3D itself, which can cause the bounding sphere to recompute after onUploadCallback runs. I'm seeing errors like this:

I'm able to use this workaround during model initialization to prevent the renderer from doing the computation too late:

var o3dCopy = THREE.Object3D.prototype.copy;
THREE.Object3D.prototype.copy = function(source, recursive)
{
	this.boundingSphere = source.boundingSphere?.clone(); //applicable to SkinnedMesh and InstancedMesh
	return o3dCopy.apply(this, arguments);
};

avatar.traverse(function(node)
{
	if((node.isSkinnedMesh || node.isInstancedMesh) && !node.geometry.boundingSphere)
	{
		//both node.boundingSphere and node.geometry.boundingSphere are used by WebGLRenderer, but node.geometry.boundingSphere isn't computed until after onUploadCallback runs
		//guessing that referencing both is a mistake and only node.boundingSphere should be used?
		node.geometry.computeBoundingSphere();
	}
});

Here's my best guest as to how this can be fixed in three.js source:

1. In Object3D.copy() (or in each applicable subclass), make sure to copy the boundingSphere that Skinned/InstancedMesh creates if it exists. If we look in BufferGeometry.copy, it also clones the bounding sphere.
2. In WebGLRenderer.projectObject(), during depth sorting, use object.boundingSphere instead of geometry.boundingSphere for the case of Skinned/InstancedMesh. If we look in Frustum.intersectsObject(), it's doing the same distinction that I think projectObject() should also be doing. (It's a little awkward though that boundingSphere === null vs. boundingSphere === undefined is how Frustum distinguishes between a normal Mesh vs. Skinned/InstancedMesh)
3. To clean up the API, it would be nice to have just Mesh.computeBoundingSphere/Box() and override the pair in Skinned/InstancedMesh. Then we could remove BufferGeometry.computeBoundingBlah() entirely and never have to detect whether to use object.boundingSphere vs. geometry.boundingSphere.

Reproduction steps

1. Try out this modified version of the collada loader skinning demo. The changes I made were:
   a. Change BufferAttribute.prototype.onUploadCallback to set the array to null
   b. Clone the stormtrooper mesh (clonetrooper??)
2. You will see the same error as the above console screenshot
3. Uncomment the workaround inside the loader callback. Error goes away

Code

/

Live example

/

Screenshots

No response

Version

r152

Device

Desktop

Browser

Chrome

OS

Linux

[Mugen87]

I think your workflow is problematic and it just became visible in recent versions of three.js. Let me explain why:

Using the onUploadCallback() for saving memory is fine but it also leads to a negative side effect since bounding volumes can't be computed anymore. The new bounding volume computation methods in SkinnedMesh and InstancedMesh level are more strict than the ones from BufferGeometry since they expect proper buffer data.

If you decide to delete buffer data right after the upload, you have to define bounding volumes by yourself. This will avoid the call of computeBoundingSphere() by the engine and the error goes away. Providing proper bounding volumes is highly recommended otherwise you end up with no view frustum culling an poor ray casting performance (and potentially other draw backs).

In any event, some suggestions of your proposed fixes make sense. E.g. the bounding volumes should be honored by copy() methods and the depth sorting should be optimized. Let me file PRs for 1 and 2.

[Mugen87]

Regarding point 3. We had to introduce bounding volumes on certain object3D classes since the bounding volumes on geometry level can't access all information which are required to compute correct results.

However, I'm not sure how I feel with completely removing the bounding volumes on geometry level though. But I agree some API clean up to avoid the pattern in Frustum would be nice.

[GoJermangoGo]

What if we were to make a change like this? This way we could keep BufferGeometry.bundingVolume() and still avoid repeating the pattern in Frustum. It would be a rather big change to the Object3D class hierarchy, though.

class Renderable extends Object3D {
	
	constructor() {
		
		this.isRenderable = true;
		
		this.boundingSphere = null;
		this.boundingBox = null;
		
	}
	
	computeBoundingVolume() {
	
		this.geometry.computeBoundingVolume();
		
	}
	
	get boundingVolume() {
		
		return this.geometry.boundingVolume;
		
	}
}

class Mesh/Sprite/Lines/Points extends Renderable {

	// no change except "extends Renderable"
	
}

class Skinned/InstancedMesh extends Mesh {
	
	// custom implementation for computing bounding volumes
	
	copy() {
		
		super.copy();
		// also clone the bounding volumes
		
	}
}

[LeviPesin]

I think it would make more sense not to extend Object3D with a Renderable class but rather just update Object3D (because all objects are assumed to be renderable, no?).

[GoJermangoGo]

I'm defining "renderable" as an object that has a BufferGeometry that can be passed to glDrawElements() or glDrawArrays(). Classes like Camera, Light, Group, etc. wouldn't have a bounding volume of their own. There might be a better name for the proposed new class.