Optimize Box3.expandByObject #17940
Optimize Box3.expandByObject #17940
Conversation
Instead of recomputing the bounding box every time, we now use the cached bounding box from the object, and transform it by the world transform. Note that this results in slightly less accurate bounding box compared to the previous implementation. This seems like a fair tradeoff - it's possible to get the precise box by using Box3.setFromBufferAttribute if desired. In addition to making the box more precise, this honors morph target data and supports arbitrary future improvements to object bounding box computation automatically.
Instead of creating a new GC'ed object for each object we traverse, pre-create the _box similarly to other globals. This is slightly awkward since this needs to be done after we define the ctor.
@Mugen87 Can you please explain why you feel that is "acceptable"? |
|
Why not? |
|
@Mugen87 Why do you feel an "inaccurate" bounding box is acceptable? |
and apart from that
|
If those issues can be established, it remains to discuss if we need two methods -- one fast, and one minimal. I hope we don't have to go down that route. |
I just don't like the somewhat provocative nature of your posts. I'm not responding to them anymore... |
|
@WestLangley The new method is generating a non-minimal (it's not guaranteed that on every face of the resulting box lies at least one vertex of the scene) but conservative (it's guaranteed that all vertices of the scene are contained in the resulting box) result. The previous method was minimal (assuming axis-aligned result) and mostly conservative (in absence of morph targets) but slow. The error induced is non-hierarchical and is very small if each object's geometry is resembling a box. In some cases the error can be large, although it's limited to something like half-diagonal of each source geometry's bounding box. In general it seems fair to assume that users of this function are fine with non-minimal result. After all, an axis-aligned bounding box is often an imprecise approximation to begin with (e.g. rotating a box diagonally and computing the axis aligned bounding box of the result generates much larger volume). If precision is required, it seems more appropriate to introduce separate methods for this that produce more complex volumes, e.g. k-DOP. |
|
@zeux Thank you for your reply. Because I feel it is important for @mrdoob, @donmccurdy, and others to understand, I'll summarize:
Your answer to that is "yes". The bounding box that is returned is now not necessarily the minimal bounding box, it may be larger.
Your answer to that is "yes". It is a valid bounding box -- just not necessarily the smallest one. // @zeux wrote:
Actually, I don't think that is true. Maybe you meant "if each object's geometry is resembling a sphere"? Or cube?
That is true.
I think in fairness to @mrdoob, it would be nice to provide a simple fiddle or illustration so he can see an example of a worse case and the trade-off involved. //
This really is not an "optimization" because you are returning a different result. But I understand the performance benefits, and I agree that it seems reasonable. Where do you think the docs and inline docs should be clarified? |
|
@Mugen87 Apologies. My earlier comment was overly harsh. I have removed it. |
|
Because an AABB is an inherently inexact bounding method generally chosen for its performance advantage over per-face operations, expanding an AABB using that same performance/precision tradeoff seems — at first reaction — like a consistent choice. Agreed that updating the docs would be worthwhile. |
I'm not sure what you mean. I meant exactly what I wrote. When object's geometry is resembling an (axis aligned) box, approximating it with a box and then rotating that is close to exact.
If you're asking for an illustration of a worse case, it happens when the bounding box is a poor approximation for the original object, but after rotating the original object the bounding box becomes a tighter fit.
I'm not sure why this is not an optimization? It makes performance substantially better. All optimizations carry caveats, so does this one - it's clearly highlighted in the PR. |
|
I'm happy to update documentation to something but I'm not sure to what. Here's what expandByObject says right now:
This remains valid - it never talks about the individual vertices in the first place. I could add something to the effect of "The function may result in a larger box than strictly necessary"? edit pushed this change. |
The comment is worded to not interfere too much with possible future changes to the function's implementation.
|
@zeux said
I do not think it is fair to @mrdoob to minimize the trade off here. Just make it clear what the trade-off is. This is the use case I think of:
It is misleading to claim it is an optimization because it produces a different result. I am OK with it as long as everyone understands that with this change, the three.js AABB may no longer be the minimal one, and in fact, may be significantly larger than that minimal one. |
|
Honestly I don't want to deal with this anymore. If the PR shouldn't be merged feel free to close this. |
|
Thanks for trying to clarify the ramifications of this change guys 🙏 /me reads this thoroughly |
|
I thought this was a "hot" method, but turns out we don't use this method that much. The only place where this method is used is in And, as far as I can see... @zeux I guess you're using the method directly in your app? |
|
@mrdoob Yes that’s correct - the use is similar to that of editor viewport I would imagine, which is to provide a reasonable baseline framing for the scene. As far as I know there’s no other way in Three to get aggregate bounding information for the scene. |
|
Okay. Sounds good to me then! |
|
Thanks! |
|
@zeux what do you think if we turned your code into |
|
This seems fine to me although note that the old implementation didn't handle morph targets at all. |
|
One alternative is to make a new method, |
|
A proper bounding box is computed, it just may no longer be the minimal one. How about this API? Box3.setFromObject( object, minimal = false );The docs can explain that if |
|
I've just ran in to this issue while migrating from r67 (lmao) to r117. I'm glad I found this thread and that I'm not going crazy though! Is this something that someone is looking at right now, or should I take a stab at a PR myself? (kinda new to this.) |
Not likely.
Sure! |
Instead of recomputing the bounding box every time, we now use the
cached bounding box from the object, and transform it by the world
transform.
Note that this results in slightly less accurate bounding box compared
to the previous implementation. This seems like a fair tradeoff -
it's possible to get the precise box by using Box3.setFromBufferAttribute
if desired.
In addition to making the box calculation faster, this honors morph target
data and supports arbitrary future improvements to object bounding box
computation automatically.
Related to #17937.