Open luisfonsivevo opened 1 year ago
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.
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.
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
}
}
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?).
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.
Description
I'm using
onUploadCallback
to remove the array because it saves hundreds of MB of RAM in our application.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 ofSkinnedMesh
andInstancedMesh
, the bounding sphere is now stored on theObject3D
itself, which can cause the bounding sphere to recompute afteronUploadCallback
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:
Here's my best guest as to how this can be fixed in three.js source:
Object3D.copy()
(or in each applicable subclass), make sure to copy theboundingSphere
thatSkinned/InstancedMesh
creates if it exists. If we look inBufferGeometry.copy
, it also clones the bounding sphere.WebGLRenderer.projectObject()
, during depth sorting, useobject.boundingSphere
instead ofgeometry.boundingSphere
for the case ofSkinned/InstancedMesh
. If we look inFrustum.intersectsObject()
, it's doing the same distinction that I thinkprojectObject()
should also be doing. (It's a little awkward though thatboundingSphere === null
vs.boundingSphere === undefined
is howFrustum
distinguishes between a normalMesh
vs.Skinned/InstancedMesh
)Mesh.computeBoundingSphere/Box()
and override the pair inSkinned/InstancedMesh
. Then we could removeBufferGeometry.computeBoundingBlah()
entirely and never have to detect whether to useobject.boundingSphere
vs.geometry.boundingSphere
.Reproduction steps
BufferAttribute.prototype.onUploadCallback
to set the array to null b. Clone the stormtrooper mesh (clonetrooper??)Code
/
Live example
/
Screenshots
No response
Version
r152
Device
Desktop
Browser
Chrome
OS
Linux