Three.js: Fix Box3 expandByObject issue

[pepitoxxx]

Describe the bug

The latest optimize for .expandByObject() #17940 (#17940) in r111 has caused an issue for the BoxHelper update() function. The boundingbox property will be set by expandByObject() at first and would not be updated afterwards.
If mesh attributes are updated dynamically, the BoxHelper will not reflect this change without explicitly call mesh.geometry.boundingBox= null or mesh.geometry.computeBoundingBox() before BoxHelper.update()

To Reproduce

Steps to reproduce the behavior:

1. create BufferGeometry with position BufferAttribute
2. create Mesh from this BufferGeometry
3. create BoxHelper from this Mesh
4. update position BufferAttribute during render loop

Live example

Here a jsfiddle to illustrate the problem:
https://jsfiddle.net/2b1sLun5/29/
A check box allowing to solve the problem is proposed but is not, in my opinion, an acceptable answer.

Expected behavior

BoxHelper should be updated according to the mesh geometry without explicitly compute or reset mesh geometry bounding box

Platform:

• Device: [Desktop]
• OS: [Windows]
• Browser: [Chrome, Firefox]
• Three.js version: [r121]

[wal0x]

I had the same issue after updating thrrejs with a mesh with changing geometry... The only solution I had is to nullify the bounding box as you said, which is a bit of a hack IMHO, giving also the fact that it was working before.

[Mugen87]

The only solution I had is to nullify the bounding box as you said,

This is not the correct workflow. If you create a geometry from scratch, bounding volumes are null and will be automatically computed by the engine. However if you change the geometry, it's your responsibility to recompute the bounding volumes, too.

17940 just made this topic more clear. So the proper fix is to call computeBoundingBox() and computeBoundingSphere() after changing the vertices of the geometry. If you miss the latter one, view frustum culling might not work, too.

https://jsfiddle.net/1kn5h3cg/3/

BTW: This code is not valid:

posAttribute.array = new Float32Array(newPos);

The array attribute is considered to be read-only. You should always change the values of the existing array.

[pepitoxxx]

Ok, I hear your arguments but don't you think the documentation should be updated? In particular
https://threejs.org/docs/#manual/en/introduction/How-to-update-things which does not mention re computing the bounding box at all

[Mugen87]

This particular doc page says:

If you change the position data values after the initial render, you may need to call .computeBoundingSphere() in order to recalculate the geometry's bounding sphere.

[Mugen87]

Maybe the following makes it more generic:

If you change the position data values after the initial render, you may need to recompute bounding volumes so other features of the engine like view frustum culling or helpers properly work.

[pepitoxxx]

Ok, so nothing concerning computeBoundingBox()...
Ps: sorry , you replied before my comment

[Mugen87]

Check out the PR, I've considered computeBoundingBox() in the code example.

[pepitoxxx]

Thanks for your reactivity. I think this topic can be closed.

[Mugen87]

That happens automatically when the PR gets merged 👍 .