mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
100.86k stars 35.22k forks source link

Unexpected result from calling Box3.setFromObject() on transformed mesh #8432

Closed foorjdev closed 8 years ago

foorjdev commented 8 years ago
Description of the problem

It looks like .setFromObject() is returning the union of the transformed bounding boxes of the object's children instead of the union of the bounding boxes of the object's transformed children.

See this fiddle.

Expected result: origin-centered sphere with z-axis rotation of pi/4 should have the same bounding box as when there is no z-axis rotation.

Observed result: the bounding box of the rotated sphere mesh is larger than expected, instead corresponding to the bounds of the sphere's original bounding box rotated by pi/4.

Three.js version
WestLangley commented 8 years ago

Yep.

Cleaner fiddle: https://jsfiddle.net/j0q7gt3c/1/

Do you have a suggestion for fixing this?

mrdoob commented 8 years ago

@WestLangley Seems like it's something I "broke" a couple releases ago... https://github.com/mrdoob/three.js/commit/278fdbdffd96384b9aaed8f2060ab2fbe97aed95#commitcomment-16790029

foorjdev commented 8 years ago

It looks like the change in 278fdbdffd96384b9aaed8f2060ab2fbe97aed95 was made for performance reasons, which I can definitely understand given the time it takes to find the bounding box for large geometries (try maintaining 60fps rendering the bounding box of a 500k triangle model as it rotates... it's a nice challenge).

Perhaps the variant of .setFromObject() added in 278fdbdffd96384b9aaed8f2060ab2fbe97aed95 would be useful as a more performant, less accurate method called .setFromObjectBoxes(), for example. Another way would be to add an optional boolean argument to .setFromObject() that would bypass the computation of bounding boxes.

In any case, thank you both for taking a look at this.

brianchirls commented 8 years ago

Now that this is released, yes there is a very large performance hit. @foorjdev's suggestion of a .setFromObjectBoxes() method would be helpful.

mrdoob commented 8 years ago

How large? Do you have numbers?

brianchirls commented 8 years ago

No, I don't have numbers because I immediately copy/pasted the old code into mine. I was using it for ray picking in VR. I have a custom octree (not the one included in your examples), and I recalculate the world-space position of bounding boxes of moving objects on every frame.

As soon as I upgraded to r76, it completely killed my frame rate to the point that it wasn't even worth measuring. It's a very simple scene, with only I think a single moving object. But it's a fairly complex BufferGeometry, so a full recalculation of that bounding box is a no-go.

mrdoob commented 8 years ago

You're recomputing the BufferGeometry per frame? What's your use case?

brianchirls commented 8 years ago

That's the thing. I'm not recomputing the BufferGeometry. But 278fdbdffd96384b9aaed8f2060ab2fbe97aed95 is crawling through the whole geometry anyway.

mrdoob commented 8 years ago

I understand. Do you mind explaining in more details what your use case is?

brianchirls commented 8 years ago

My use case is that I'm storing all my objects in a custom octree to optimize CPU ray picking. For every object in the octree, I store two axis-aligned bounding boxes (AABB):

The geometry AABB takes a long time to calculate, so I don't ever update it once it's computed the first time. The object AABB is derived quickly by transforming the geometry bounding box by the matrixWorld of the object. Some objects in the scene are marked as "static" (e.g. floors, walls, etc.) and are fixed in the octree. Other objects can move on any/every frame, so their placement in the octree needs to be recomputed regularly. That requires recomputing the object AABB (not the one on geometry) based on the updated matrixWorld.

The object's bounding box serves two purposes: 1) it tells me where in the octree to place the object, and 2) serves as a quick way to determine that I don't need to bother checking the ray against every single triangle in the geometry. A bounding box is by definition an approximation of the shape, so it doesn't break anything if one implementation is a little "more approximate" than another. The AABB from the proposed setFromObjectBoxes is slightly bigger than the one from the current setFromObject, so there will be a small number of increased false positives that require ray casting the whole geometry. But that's much less computationally expensive than traversing every single triangle of every moving object on every frame.

brianchirls commented 8 years ago

Further, I would suggest that setFromObject is a good name for the way it was, i.e. computing from bounding boxes, and that setFromGeometry might be a good name for the current technique of recomputing the tight bounding box based on every triangle in every geometry of every descendent object.

foorjdev commented 8 years ago

The name was somewhat confusing to me too at first, since it's not necessarily clear that it may be such an expensive operation. But after considering it for some time, I think it's actually fairly descriptive: it sets the box to match the bounds of the object, which is, at times, an unavoidably expensive operation. That said, I think setFromGeometry is probably even more descriptive.

I wouldn't necessarily agree that an AABB is an approximation of any kind. You can use it as such, but it's still an exact function of the input vertices and coordinate basis vectors. It has properties that are assumed to be true by many algorithms that use it (e.g. collision detection), and given its well-established meaning, I would expect a fair amount of confusion to arise if, as in the fiddle from my initial ticket, rotating a sphere changes its AABB, for example.

What's more, imagine taking a sphere geometry, adding it to a mesh, rotating the mesh, adding it to an object, rotating the object, adding that to another object, rotating, and so on and so forth. You will end up with an arbitrarily large bounding box around your sphere. In other words, the error introduced by the approximation of using child bounding boxes instead of child geometries can propagate very quickly and lead to drastically inaccurate results.