Fix BoxHelper Bounding box update issue

[zhihuidev]

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.

This fix will reset the geometry boundingbox value to null, so the Box3 expandByObject() function will work properly.

[Mugen87]

I don't think this change is correct. Can you please explain this issue in more detail?

[zhihuidev]

Hi , recently we update to v111 and having an issue with these code:

var sourceBoxHelper = new THREE.BoxHelper(sourceObject)
var sourceBox = new THREE.Box3();

sourceBoxHelper.update()
sourceBox.setFromObject(sourceBoxHelper)

Here the sourceBox will not get the correct updated geometry information (which works fine before v111).

I found the problem is that the latest optimize for .expandByObject() #17940 which will always set the boundingbox value when the BoxHelper first constructed.

Then afterwards when BoxHelper update() get called again, the boundingbox will not be set to the updated value (the boundingbox will not be calculated when update() get called, I think this is not a correct behavior).

[mrdoob]

What code is using this.geometry.boundingBox?

BoxHelper uses this.geometry.boundingSphere:

three.js/src/helpers/BoxHelper.js

Line 87 in e0e541b

this.geometry.computeBoundingSphere();

[zhihuidev]

Before r111, the geometry.boundingBox in BoxBoxHelper is always set to null, then when we call this method:

sourceBox.setFromObject(sourceBoxHelper)

the sourceBox will get a correct geometry boundingbox, since geometry.boundingBox in BoxBoxHelper is null and it will call the computeBoundingBox() function.

after r111, the geometry.boundingBox in BoxBoxHelper is not null, and will not be updated properly. In the same situation, the sourceBox will not get the correct geometry (the computeBoundingBox() function will not be called).

[mrdoob]

Why do you use a helper? Why don't you do this directly?

sourceBox.setFromObject(sourceObject)

[zhihuidev]

Well, this code snippet is from the Bounding volume collision detection with THREE.js demo on mozilla doc site.

https://developer.mozilla.org/en-US/docs/Games/Techniques/3D_collision_detection/Bounding_volume_collision_detection_with_THREE.js

If we change the Mesh position, rotation, scale, etc. we need to call the update() method so the BoxHelper instance matches its linked Mesh. We also need to call setFromObject again in order to make the Box3 follow the Mesh.

knot.position.set(-3, 2, 1);
knot.rotation.x = -Math.PI / 4;
// update the bounding box so it stills wraps the knot
knotBoxHelper.update();
box3.setFromObject(knotBoxHelper);

I am sure not only me is using BoxHelper this way and I think we should fix it.

[Mugen87]

In this case, Mozilla docs need an update.

BoxHelper is the recommended way to handle 3D collisions with bounding volumes in Three.js. You will miss sphere tests, but the tradeoffs are well worth it.

I don't think this statement is correct. Helpers are not intended for collision detection.

[huantingchen]

Same situation after upgrading from r110 to r111. Obviously, BoxHelper.update() and BoxHelper.setFromObject() are not working properly and not updating since then. But I'm not sure if that has something to do with the bounding box mentioned above or #17940.

[Mugen87]

Can you please demonstrate with a fiddle what's exactly is going wrong? I've prepared a template with BoxHelper that can be used as a starting point: https://jsfiddle.net/vhjptb86/

As you can see in the demo, it seems the helper updates itself correctly.

[huantingchen]

Sorry it was just out of viewport. I realised my sourceObject.boundingBox was manipulated earlier by
calling sourceObject.localToWorld( sourceObject.geometry.boundingBox.max ). The new Box3.expandByObject() then applied the local to world conversion again. Cloning the boundingBox.max before passing into localToWorld() will fix the issue. Thanks.

[Mugen87]

@mrdoob This PR can be closed. IMHO, the suggested change is incorrect.