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

Added intersection/distance tests to AxisAlignedBoundingBox #3497

Closed
wants to merge 8 commits into from

Conversation

mattj23
Copy link
Contributor

@mattj23 mattj23 commented May 25, 2021

This PR adds a handful of new intersection/distance tests for the AxisAlignedBoundingBox, as well as a Join method.

  • The AxisAlignedBoundingBox::Join(...) method performs the same action as the operator+= operator, except that it does not have the additional logic tied to the IsEmpty() property. This is important because not all applications regard an axis aligned bounding box to be semantically empty simply because it has a single dimension with zero extent. A triangle that is parallel with any one of the Cartesian planes, for example, will generate an AxisAlignedBoundingBox that is "empty" and thus will be completely lost if the += operator is used on it. Adding the Join method leaves += exactly as it was before, but allows client code to use an equivalent method that functions correctly for flat AABBs.

  • IntersectionTests::PlaneAABB checks for an intersection between an AABB and an Eigen::Hyperplane<double, 3>, which is a building block for plane collision and intersection tests.

  • IntersectionTests::ClosestPointAABB finds the closest point inside an AABB to a test point. If the test point is inside the AABB an identical point is returned. This is a building block for accelerated distance checking.

  • IntersectionTests::FarthestPointAABB finds the farthest point inside an AABB from a test point. This is a building block for accelerated distance checking, as when combined with ClosestPointAABB can be used to prune the contents of AABBs from distance checks.

I believe I've added unit tests for everything. These features are part of the foundation for a BVH feature branch I'm preparing, but can be useful on their own as well.


This change is Reviewable

mattj23 added 6 commits May 12, 2021 23:04
* Added 38 unit tests to capture the existing behavior of the
  AxisAlignedBoundingBox
* added a Join() method which combines AABBs without the empty
  volume logic
* Added closest and farthest point tests for AABBs
* Added an intersection test for AABBs and planes
* Unit tests added for new features
@update-docs
Copy link

update-docs bot commented May 25, 2021

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@benjaminum
Copy link
Contributor

Hi @mattj23 , thanks for this PR.
Do you have already some code for the BVH?

@mattj23
Copy link
Contributor Author

mattj23 commented Jul 6, 2021

@benjaminum I do have some code ready, though I did hit a wall because I needed some of these features in order to have something functional.

I'm also happy to delete this PR and create a new one that includes the BVH.

@benjaminum
Copy link
Contributor

Smaller PRs are fine. I just trying to understand the context better.
I am wondering what the feature set of the BVH will be.

@mattj23
Copy link
Contributor Author

mattj23 commented Jul 6, 2021

@benjaminum I can give you the quick overview. My working branch as I last touched it is here.

Basically the core goal that I start with was to create a BVH for accelerated intersection and distance operations on TriangleMesh, as you and I discussed way back in 2019 in issue 1321. As I went through that process there ended up being a lot of pieces that made sense to add.

  1. The generalized operations of a BVH are agnostic to the specific geometry they're handling, and it seemed wasteful to write one specifically for triangles. I tried a few different approaches as to how to generalize, and in the end the simplest felt the most natural, which ended up being a template class cpp/open3d/geometry/Bvh.h which has a generalized framework for top-down construction, intersection testing, and closest/farthest distance testing. It's not meant to be used directly, but when implementing a BVH for some type of geometry you just need to provide a few functions to the template to do things like measure distances, extract the AABB, test for an intersection with another geometry type, etc.
  2. Then, using this template class, I created one specifically for working with TriangleMesh, cpp/open3d/geometry/TriangleMeshBvh.h. This provides a concrete template instantiation of the BVH for a triangle mesh which provides things like line/ray/segment intersections, closest/farthest distance checking, inside/outside tests, and I intend to add other features like plane intersections.
  3. Finally, to make the TriangleMesh implementation more efficient, I built it around an acceleration class called TriangleBounds cpp/open3d/geometry/TriangleBounds.h which I originally wrote to speed up ray tests but also contains some other features for distance and point tests.

I'm happy to discuss this further here or on the Discord. I know I went off and slowly started implementing a bunch of things by myself, but I tried to keep an eye on what people were asking for in issues and on Discord to make sure I was building something useful. I'm also very willing to adjust the features and the approach if it makes sense to implement them differently from what I've done.

@benjaminum
Copy link
Contributor

@mattj23 Thanks for the reminder, I now remember the discussion.

I had a quick look at your bvh branch and like it a lot!
I think adding a general BVH class is a good idea and your implementations look already very comprehensive.

For adding new functionality, one important background information is that we will slowly transition to the classes in cpp/open3d/t/.... The classes use our Tensor class to make it easier to develop for CPU and GPU devices.
Since the transition will take some time it is probably better to first stick to the current TriangleMesh, integrate everything and then later add support for the Tensor-based TriangleMesh.

Another thing I want to mention is that we recently added a wrapper class for embree called RaycastingScene #3637 . We were thinking about calling it just EmbreeScene first. It has some disadvantages compared to your approach, e.g., there is always a copy of the mesh and the user does not have fine-grained control over the data structures, which prevents applying special knowledge or customizing the implementation.
Therefore, I think having the structures you sketched in points 1.,2.,3. in addition would be a good contribution. What do you think?

@mattj23
Copy link
Contributor Author

mattj23 commented Jul 7, 2021

@benjaminum, I would love to finish up the BVH constructs I've made, get them into Open3D, and then spend some time learning how to (1) work with the new tensor geometries so that I can start translating code to them, and (2) finally learn how to write python bindings so that some of my contributions are accessible to python users.

Regarding the RaycastingScene, I think it's at least partially orthogonal to my TriangleMeshBvh, in that if you're trying to do a ray tracing rendering workload you should definitely use Embree if you can, but my implementation is actually more geared towards general collision, intersection, and measurement work. The tasks I was building my original code for back in 2019 were actually engineering simulations of LOS and uncertainty in optical measurements, and a lot of the work I want to use it for is taking cross sections, measurements, and doing alignments on meshes (much the way you can align PointCloud but can't on TriangleMesh, as was mentioned in #2062).

What should my next steps be? Should I delete this PR and work on putting forward one that has everything together? It will be a somewhat large PR but it will have the benefit of having everything in context. Would it be less effort for the reviewers to do it that way than as two separate PRs?

I'm not far from having the BVH ready to submit, mostly I just need the AxisAlignedBoundingBox.Join(...) method implemented in this PR because += doesn't do what I was expecting and the BVH relies on it.

@benjaminum benjaminum self-requested a review July 7, 2021 19:21
@benjaminum
Copy link
Contributor

What should my next steps be? Should I delete this PR and work on putting forward one that has everything together? It will be a somewhat large PR but it will have the benefit of having everything in context. Would it be less effort for the reviewers to do it that way than as two separate PRs?

This PR is fine. Smaller PRs are easier to review and the context is now clear from the discussion. The problem was that no one was assigned to it.

@mattj23
Copy link
Contributor Author

mattj23 commented Jul 7, 2021

Understood. I appreciate you reviewing it; I know that when I submitted it originally everyone was busy scrambling to get 0.13 out, so I didn't mind it getting overlooked.

Copy link
Contributor

@benjaminum benjaminum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mattj23)


cpp/open3d/geometry/BoundingVolume.h, line 204 at r2 (raw file):

    /// \brief Adds another AxisAlignedBoundingBox in place, such that the
    /// current object becomes the union of both boxes. However, if this
    /// AxisAlignedBoundingBox is empty (has a volume of 0), this object will

The motivation here is to ignore uninitialized AABBs, right?


cpp/open3d/geometry/BoundingVolume.h, line 217 at r2 (raw file):

    /// \param other an AxisAlignedBoundingBox to join with this one
    /// \return a reference to this AxisAlignedBoundingBox
    AxisAlignedBoundingBox& Join(const AxisAlignedBoundingBox& other);

Would it make sense to add another parameter ignore_empty_aabbs=False and handle both cases in Join? += would then become just a shortcut for Join(x,true).


cpp/tests/geometry/IntersectionTest.cpp, line 37 at r2 (raw file):

using pnt_t = std::tuple<Vector3d, Vector3d>;
using pln_t = std::tuple<Vector3d, Vector3d, bool>;
#define ROOT2 1.0 / sqrt(2.0)

just nitpicking, SQRT1_2, SQRT1_3 seems more intuitive,

@mattj23
Copy link
Contributor Author

mattj23 commented Jul 9, 2021

@benjaminum, I haven't forgotten about this, I just hadn't updated against master for a few months and how I'm having problems getting Open3D to build on my machine. As soon as it's working I'll implement the changes you recommended.

@benjaminum
Copy link
Contributor

@benjaminum, I haven't forgotten about this, I just hadn't updated against master for a few months and how I'm having problems getting Open3D to build on my machine. As soon as it's working I'll implement the changes you recommended.

No problem, it is not urgent.

@benjaminum
Copy link
Contributor

Closing because of long time of no activity.
@mattj23 please let me know if there is still interest in this PR.

@benjaminum benjaminum closed this Jan 20, 2023
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 this pull request may close these issues.

2 participants