Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bounding box of object containing concave geometry #19030

Closed
xawill opened this issue Apr 3, 2020 · 3 comments
Closed

Bounding box of object containing concave geometry #19030

xawill opened this issue Apr 3, 2020 · 3 comments

Comments

@xawill
Copy link
Contributor

xawill commented Apr 3, 2020

Description of the problem

I have been trying to compute the bounding box of an object containing a concave geometry (with new Box3().setFromObject()) without success : the resulting bounding box is too big.

After a lot of digging in the three.js source code to understand how it works in details, I now fully understand why this happens : the bbox of the concave geometry is computed from local vertices, then the 8 corners of this bbox are transformed in world space (with corner.applyMatrix4(object.matrixWorld)) and finally the resulting bbox is the bbox from those transformed 8 corners in world space (with Box3.setFromPoints(corners)).

The issue is that with concave geometries, some corners of the local bbox are outside the geometry. It means that when we apply the world transform (typically containing a rotation), those points of the local bbox will have to be part of the final transformed bbox. This looks wrong to me, right ?

In the image below, we see in a 2D projection the corners of the local geometry bbox (in red), the final world bbox (in red) and the actual points and bbox I would personally expect from the computation (in orange, slightly shifted for you to see both bbox where they overlap in this particular case).

bbox

Of course now I know how to compute a correct bbox for this particular case :
new Box3().setFromPoints(geometry.vertices.map(v => v.clone().applyMatrix4(object.matrixWorld)))

But I find it quite cumbersome (since I cannot use bbox.expandByObject(object) without having to override the function) and it is very confusing (at least for new users) that computing a bounding box in the traditional way (the one we find it everywhere on three.js help forums) doesn't actually work as expected for concave geometries.

I know there is already some discussion about Geometry.boundingBox (#18334, for instance), but I am not sure this is the same issue. Is it a bug, a 'feature' (i.e. intended behavior) ? If this is the intended behavior, is there a better workaround than the one I wrote here ?

Thank you for considering.

Three.js version

r115

Browser

All of them

OS

All of them

Hardware Requirements (graphics card, VR Device, ...)

None in particular.

@WestLangley
Copy link
Collaborator

WestLangley commented Apr 3, 2020

Yes, the bounding box computation was changed recently to be more efficient. The result is a box that is now conservative (too big), rather than exact.

In other words, the computed bounding box is not necessarily the minimal one.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 3, 2020

Related PR: #17940

@xawill Notice that this topic was already discussed in detail and a reasonable decision was made based on the pro and cons.

@xawill
Copy link
Contributor Author

xawill commented Apr 3, 2020

Okey, thank you, I missed this discussion... For what it is worth, I still disagree with the outcome of the discussion, even though I clearly understand the pros and cons. If you have no plan to implement another function which computes an accurate bbox, I guess you can close this issue and it will be every user's role to implement it on his side.

In any case, I think the documentation for Box3.setFromObject should be clarified. After all my hours spent digging inside the source code, I now understand the mention 'The function may result in a larger box than strictly necessary'. But I think the reason why this is the case (and maybe a suggested workaround) should be added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants