From 7cf1c575bc859c44c3075e908a5eb40bdacf3f10 Mon Sep 17 00:00:00 2001 From: Saurabh Khanduja Date: Mon, 13 Nov 2023 00:00:20 +0100 Subject: [PATCH 1/8] Change invalid values of min_bound and max_bound as done in legacy, related pr #6444 --- cpp/open3d/t/geometry/BoundingVolume.cpp | 9 ++++++--- cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp | 9 +++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/cpp/open3d/t/geometry/BoundingVolume.cpp b/cpp/open3d/t/geometry/BoundingVolume.cpp index 0e529f27d1d..bf3e740779f 100644 --- a/cpp/open3d/t/geometry/BoundingVolume.cpp +++ b/cpp/open3d/t/geometry/BoundingVolume.cpp @@ -42,9 +42,12 @@ AxisAlignedBoundingBox::AxisAlignedBoundingBox(const core::Tensor &min_bound, // Check if the bounding box is valid. if (Volume() < 0) { - utility::LogError( - "Invalid axis-aligned bounding box. Please make sure all " - "the elements in max bound are larger than min bound."); + utility::LogWarning( + "max_bound {} of bounding box is smaller than min_bound {} in " + "one or more axes. Fix input values to remove this warning.", + max_bound_.ToString(false), min_bound_.ToString(false)); + max_bound_ = open3d::core::Maximum(min_bound, max_bound); + min_bound_ = open3d::core::Minimum(min_bound, max_bound); } } diff --git a/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp b/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp index 333895493ba..144b837dc89 100644 --- a/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp +++ b/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp @@ -60,10 +60,6 @@ TEST_P(AxisAlignedBoundingBoxPermuteDevices, Constructor) { core::Tensor min_bound = core::Tensor::Init({-1, -1, -1}, device); core::Tensor max_bound = core::Tensor::Init({1, 1, 1}, device); - // Attempt to construct with invalid min/max bound. - EXPECT_THROW(t::geometry::AxisAlignedBoundingBox(max_bound, min_bound), - std::runtime_error); - t::geometry::AxisAlignedBoundingBox aabb(min_bound, max_bound); // Public members. @@ -76,6 +72,11 @@ TEST_P(AxisAlignedBoundingBoxPermuteDevices, Constructor) { core::Tensor::Init({1, 1, 1}, device))); EXPECT_EQ(aabb.GetDevice(), device); + + // Attempt to construct with invalid min/max bound should create a valid + // bounding box with a warning. + t::geometry::AxisAlignedBoundingBox aabb_invalid(max_bound, min_bound); + EXPECT_TRUE(aabb_invalid.GetBoxPoints().AllClose(aabb.GetBoxPoints())); } TEST_P(AxisAlignedBoundingBoxPermuteDevicePairs, CopyDevice) { From 722f2906a47b66cffeca397ef15498fcc4284c85 Mon Sep 17 00:00:00 2001 From: Saurabh Khanduja Date: Mon, 13 Nov 2023 00:11:21 +0100 Subject: [PATCH 2/8] Correct points length check when creating AABB from points. --- cpp/open3d/geometry/BoundingVolume.cpp | 3 +++ cpp/open3d/t/geometry/BoundingVolume.cpp | 6 ++++-- cpp/open3d/t/geometry/BoundingVolume.h | 2 +- cpp/pybind/t/geometry/boundingvolume.cpp | 2 +- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/cpp/open3d/geometry/BoundingVolume.cpp b/cpp/open3d/geometry/BoundingVolume.cpp index ba2594b6ba3..36881eca93e 100644 --- a/cpp/open3d/geometry/BoundingVolume.cpp +++ b/cpp/open3d/geometry/BoundingVolume.cpp @@ -330,6 +330,9 @@ AxisAlignedBoundingBox AxisAlignedBoundingBox::CreateFromPoints( const std::vector& points) { AxisAlignedBoundingBox box; if (points.empty()) { + utility::LogWarning( + "The number of points is 0 when creating axis-aligned bounding " + "box."); box.min_bound_ = Eigen::Vector3d(0.0, 0.0, 0.0); box.max_bound_ = Eigen::Vector3d(0.0, 0.0, 0.0); } else { diff --git a/cpp/open3d/t/geometry/BoundingVolume.cpp b/cpp/open3d/t/geometry/BoundingVolume.cpp index bf3e740779f..9aec549d57a 100644 --- a/cpp/open3d/t/geometry/BoundingVolume.cpp +++ b/cpp/open3d/t/geometry/BoundingVolume.cpp @@ -231,8 +231,10 @@ AxisAlignedBoundingBox AxisAlignedBoundingBox::CreateFromPoints( const core::Tensor &points) { core::AssertTensorShape(points, {utility::nullopt, 3}); core::AssertTensorDtypes(points, {core::Float32, core::Float64}); - if (points.GetLength() <= 3) { - utility::LogWarning("The points number is less than 3."); + if (points.GetLength() <= 0) { + utility::LogWarning( + "The number of points is 0 when creating axis-aligned bounding " + "box."); return AxisAlignedBoundingBox(points.GetDevice()); } else { const core::Tensor min_bound = points.Min({0}); diff --git a/cpp/open3d/t/geometry/BoundingVolume.h b/cpp/open3d/t/geometry/BoundingVolume.h index 27fe7e4aaa6..2a23507b8e7 100644 --- a/cpp/open3d/t/geometry/BoundingVolume.h +++ b/cpp/open3d/t/geometry/BoundingVolume.h @@ -206,7 +206,7 @@ class AxisAlignedBoundingBox : public Geometry, public DrawableGeometry { /// Creates the axis-aligned box that encloses the set of points. /// \param points A list of points with data type of float32 or float64 (N x - /// 3 tensor, where N must be larger than 3). + /// 3 tensor). /// \return AxisAlignedBoundingBox with same data type and device as input /// points. static AxisAlignedBoundingBox CreateFromPoints(const core::Tensor &points); diff --git a/cpp/pybind/t/geometry/boundingvolume.cpp b/cpp/pybind/t/geometry/boundingvolume.cpp index 7505e02ae52..946fb9617df 100644 --- a/cpp/pybind/t/geometry/boundingvolume.cpp +++ b/cpp/pybind/t/geometry/boundingvolume.cpp @@ -197,7 +197,7 @@ The scaling center will be the box center if it is not specified.)", m, "AxisAlignedBoundingBox", "create_from_points", {{"points", "A list of points with data type of float32 or float64 (N x 3 " - "tensor, where N must be larger than 3)."}}); + "tensor)."}}); py::class_, std::shared_ptr, Geometry, DrawableGeometry> From cf31c9420d375cef996ab3378a9f0f92a598be16 Mon Sep 17 00:00:00 2001 From: Saurabh Khanduja Date: Mon, 13 Nov 2023 01:18:11 +0100 Subject: [PATCH 3/8] Print min and max bound of AABB when printing AABB --- cpp/open3d/t/geometry/BoundingVolume.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cpp/open3d/t/geometry/BoundingVolume.cpp b/cpp/open3d/t/geometry/BoundingVolume.cpp index 9aec549d57a..7f2034a4949 100644 --- a/cpp/open3d/t/geometry/BoundingVolume.cpp +++ b/cpp/open3d/t/geometry/BoundingVolume.cpp @@ -223,7 +223,9 @@ core::Tensor AxisAlignedBoundingBox::GetPointIndicesWithinBoundingBox( } std::string AxisAlignedBoundingBox::ToString() const { - return fmt::format("AxisAlignedBoundingBox[{}, {}]", GetDtype().ToString(), + return fmt::format("AxisAlignedBoundingBox[{} - {}, {}, {}]", + GetMinBound().ToString(false), + GetMaxBound().ToString(false), GetDtype().ToString(), GetDevice().ToString()); } From b301eccd50764fd52eed686a6c116fcb070f07d2 Mon Sep 17 00:00:00 2001 From: Saurabh Khanduja Date: Mon, 13 Nov 2023 01:24:12 +0100 Subject: [PATCH 4/8] Add brief docs for methods and variables, fix warning messages. --- cpp/open3d/geometry/BoundingVolume.cpp | 2 +- cpp/open3d/geometry/BoundingVolume.h | 9 ++++++- cpp/open3d/t/geometry/BoundingVolume.cpp | 10 +++---- cpp/open3d/t/geometry/BoundingVolume.h | 27 ++++++++++++++----- .../visualization/visualizer/Visualizer.h | 2 ++ 5 files changed, 36 insertions(+), 14 deletions(-) diff --git a/cpp/open3d/geometry/BoundingVolume.cpp b/cpp/open3d/geometry/BoundingVolume.cpp index 36881eca93e..89b0ca70e5d 100644 --- a/cpp/open3d/geometry/BoundingVolume.cpp +++ b/cpp/open3d/geometry/BoundingVolume.cpp @@ -303,7 +303,7 @@ AxisAlignedBoundingBox& AxisAlignedBoundingBox::Scale( AxisAlignedBoundingBox& AxisAlignedBoundingBox::Rotate( const Eigen::Matrix3d& rotation, const Eigen::Vector3d& center) { utility::LogError( - "A rotation of a AxisAlignedBoundingBox would not be axis aligned " + "A rotation of an AxisAlignedBoundingBox would not be axis-aligned " "anymore, convert it to an OrientedBoundingBox first"); return *this; } diff --git a/cpp/open3d/geometry/BoundingVolume.h b/cpp/open3d/geometry/BoundingVolume.h index 76cdca5977d..b4b310f3ca0 100644 --- a/cpp/open3d/geometry/BoundingVolume.h +++ b/cpp/open3d/geometry/BoundingVolume.h @@ -151,7 +151,8 @@ class OrientedBoundingBox : public Geometry3D { /// \class AxisAlignedBoundingBox /// -/// \brief A bounding box that is aligned along the coordinate axes. +/// \brief A bounding box that is aligned along the coordinate axes and defined +/// by the min_bound and max_bound. /// /// The AxisAlignedBoundingBox uses the coordinate axes for bounding box /// generation. This means that the bounding box is oriented along the @@ -227,14 +228,20 @@ class AxisAlignedBoundingBox : public Geometry3D { /// extents. double GetMaxExtent() const { return (max_bound_ - min_bound_).maxCoeff(); } + /// Calculates the percentage position of the given x-coordinate within + /// the x-axis range of this AxisAlignedBoundingBox. double GetXPercentage(double x) const { return (x - min_bound_(0)) / (max_bound_(0) - min_bound_(0)); } + /// Calculates the percentage position of the given y-coordinate within + /// the y-axis range of this AxisAlignedBoundingBox. double GetYPercentage(double y) const { return (y - min_bound_(1)) / (max_bound_(1) - min_bound_(1)); } + /// Calculates the percentage position of the given z-coordinate within + /// the z-axis range of this AxisAlignedBoundingBox. double GetZPercentage(double z) const { return (z - min_bound_(2)) / (max_bound_(2) - min_bound_(2)); } diff --git a/cpp/open3d/t/geometry/BoundingVolume.cpp b/cpp/open3d/t/geometry/BoundingVolume.cpp index 7f2034a4949..6a221502efe 100644 --- a/cpp/open3d/t/geometry/BoundingVolume.cpp +++ b/cpp/open3d/t/geometry/BoundingVolume.cpp @@ -83,7 +83,7 @@ void AxisAlignedBoundingBox::SetMinBound(const core::Tensor &min_bound) { if (Volume() < 0) { utility::LogWarning( "Invalid axis-aligned bounding box. Please make sure all " - "the elements in min bound are smaller than min bound."); + "the elements in min bound are smaller than max bound."); min_bound_ = tmp; } } @@ -113,8 +113,8 @@ void AxisAlignedBoundingBox::SetColor(const core::Tensor &color) { if (color.Max({0}).To(core::Float64).Item() > 1.0 || color.Min({0}).To(core::Float64).Item() < 0.0) { utility::LogError( - "The color must be in the range [0, 1], but for range [{}, " - "{}].", + "The color must be in the range [0, 1], but found in range " + "[{}, {}].", color.Min({0}).To(core::Float64).Item(), color.Max({0}).To(core::Float64).Item()); } @@ -392,8 +392,8 @@ void OrientedBoundingBox::SetColor(const core::Tensor &color) { if (color.Max({0}).To(core::Float64).Item() > 1.0 || color.Min({0}).To(core::Float64).Item() < 0.0) { utility::LogError( - "The color must be in the range [0, 1], but for range [{}, " - "{}].", + "The color must be in the range [0, 1], but found in range " + "[{}, {}].", color.Min({0}).To(core::Float64).Item(), color.Max({0}).To(core::Float64).Item()); } diff --git a/cpp/open3d/t/geometry/BoundingVolume.h b/cpp/open3d/t/geometry/BoundingVolume.h index 2a23507b8e7..b11cae01777 100644 --- a/cpp/open3d/t/geometry/BoundingVolume.h +++ b/cpp/open3d/t/geometry/BoundingVolume.h @@ -93,12 +93,12 @@ class AxisAlignedBoundingBox : public Geometry, public DrawableGeometry { /// \param min_bound Tensor with {3,} shape, and type float32 or float64. void SetMinBound(const core::Tensor &min_bound); - /// \brief Set the max boundof the box. + /// \brief Set the max bound of the box. /// If the data type of the given tensor differs from the data type of the /// box, an exception will be thrown. /// /// If the max bound makes the box invalid, it will not be set to the box. - /// \param min_bound Tensor with {3,} shape, and type float32 or float64. + /// \param max_bound Tensor with {3,} shape, and type float32 or float64. void SetMaxBound(const core::Tensor &max_bound); /// \brief Set the color of the box. @@ -156,16 +156,22 @@ class AxisAlignedBoundingBox : public Geometry, public DrawableGeometry { /// Returns the half extent of the bounding box. core::Tensor GetHalfExtent() const { return GetExtent() / 2; } - /// Returns the maximum extent, i.e. the maximum of X, Y and Z axis' + /// \brief Returns the maximum extent, i.e. the maximum of X, Y and Z axis' /// extents. double GetMaxExtent() const { return GetExtent().Max({0}).To(core::Float64).Item(); } + /// Calculates the percentage position of the given x-coordinate within + /// the x-axis range of this AxisAlignedBoundingBox. double GetXPercentage(double x) const; + /// Calculates the percentage position of the given y-coordinate within + /// the y-axis range of this AxisAlignedBoundingBox. double GetYPercentage(double y) const; + /// Calculates the percentage position of the given z-coordinate within + /// the z-axis range of this AxisAlignedBoundingBox. double GetZPercentage(double z) const; /// Returns the volume of the bounding box. @@ -173,8 +179,9 @@ class AxisAlignedBoundingBox : public Geometry, public DrawableGeometry { return GetExtent().Prod({0}).To(core::Float64).Item(); } - /// Returns the eight points that define the bounding box. The Return tensor - /// has shape {8, 3} and data type same as the box. + /// \brief Returns the eight points that define the bounding box. + /// + /// The Return tensor has shape {8, 3} and data type same as the box. core::Tensor GetBoxPoints() const; /// \brief Indices to points that are within the bounding box. @@ -212,10 +219,15 @@ class AxisAlignedBoundingBox : public Geometry, public DrawableGeometry { static AxisAlignedBoundingBox CreateFromPoints(const core::Tensor &points); protected: + /// The device to use for the bounding box. The default is CPU:0. core::Device device_ = core::Device("CPU:0"); + /// The data type of the bounding box. core::Dtype dtype_ = core::Float32; + /// The lower x, y, z bounds of the bounding box. core::Tensor min_bound_; + /// The upper x, y, z bounds of the bounding box. core::Tensor max_bound_; + /// The color of the bounding box in RGB. The default is white. core::Tensor color_; }; @@ -373,8 +385,9 @@ class OrientedBoundingBox : public Geometry, public DrawableGeometry { return GetExtent().Prod({0}).To(core::Float64).Item(); } - /// Returns the eight points that define the bounding box. The Return tensor - /// has shape {8, 3} and data type same as the box. + /// \brief Returns the eight points that define the bounding box. + /// + /// The Return tensor has shape {8, 3} and data type same as the box. /// /// \verbatim /// ------- x diff --git a/cpp/open3d/visualization/visualizer/Visualizer.h b/cpp/open3d/visualization/visualizer/Visualizer.h index 02126229bc3..247ee2c91fe 100644 --- a/cpp/open3d/visualization/visualizer/Visualizer.h +++ b/cpp/open3d/visualization/visualizer/Visualizer.h @@ -127,6 +127,7 @@ class Visualizer { /// Visualizer should be updated accordingly. /// /// \param geometry_ptr The Geometry object. + /// \param reset_bounding_box Reset viewpoint to view all geometries. virtual bool AddGeometry( std::shared_ptr geometry_ptr, bool reset_bounding_box = true); @@ -140,6 +141,7 @@ class Visualizer { /// added by AddGeometry /// /// \param geometry_ptr The Geometry object. + /// \param reset_bounding_box Reset viewpoint to view all geometries. virtual bool RemoveGeometry( std::shared_ptr geometry_ptr, bool reset_bounding_box = true); From 8e521e36994c4ec63a6eb6774e3da8dfe0bd90d8 Mon Sep 17 00:00:00 2001 From: Saurabh Khanduja Date: Mon, 13 Nov 2023 01:38:32 +0100 Subject: [PATCH 5/8] Remove obsolete tag --- docs/Doxyfile.in | 7 ------- 1 file changed, 7 deletions(-) diff --git a/docs/Doxyfile.in b/docs/Doxyfile.in index 9a514ce1535..0ed539ff60e 100644 --- a/docs/Doxyfile.in +++ b/docs/Doxyfile.in @@ -1146,13 +1146,6 @@ VERBATIM_HEADERS = YES ALPHABETICAL_INDEX = YES -# The COLS_IN_ALPHA_INDEX tag can be used to specify the number of columns in -# which the alphabetical index list will be split. -# Minimum value: 1, maximum value: 20, default value: 5. -# This tag requires that the tag ALPHABETICAL_INDEX is set to YES. - -COLS_IN_ALPHA_INDEX = 5 - # In case all classes in a project start with a common prefix, all classes will # be put under the same header in the alphabetical index. The IGNORE_PREFIX tag # can be used to specify a prefix (or a list of prefixes) that should be ignored From 4138291c000294cba3fee3ae85f459b0fe1a1cf6 Mon Sep 17 00:00:00 2001 From: Saurabh Khanduja Date: Mon, 13 Nov 2023 02:52:28 +0100 Subject: [PATCH 6/8] Update Changelog. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d3ba84ed84..df92535007d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ * Fix printing of tensor in gpu and add validation check for bounds of axis-aligned bounding box (PR #6444) * Python 3.11 support. bump pybind11 v2.6.2 -> v2.11.1 * Check for support of CUDA Memory Pools at runtime (#4679) +* Fix `toString`, `CreateFromPoints` methods and improve docs in `AxisAlignedBoundingBox`. 🐛📝 ## 0.13 From 2bd40c0414dba4f9249f87889f68d23eec8cef00 Mon Sep 17 00:00:00 2001 From: Sameer Sheorey <41028320+ssheorey@users.noreply.github.com> Date: Mon, 13 Nov 2023 21:55:25 -0800 Subject: [PATCH 7/8] Fix AABB ToString() test --- cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp b/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp index 144b837dc89..c9070297629 100644 --- a/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp +++ b/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp @@ -51,7 +51,7 @@ TEST_P(AxisAlignedBoundingBoxPermuteDevices, ConstructorNoArg) { EXPECT_EQ(aabb.GetDevice(), core::Device("CPU:0")); // Print Information. - EXPECT_EQ(aabb.ToString(), "AxisAlignedBoundingBox[Float32, CPU:0]"); + EXPECT_EQ(aabb.ToString(), "AxisAlignedBoundingBox[[0 0 0] - [0 0 0], Float32, CPU:0]"); } TEST_P(AxisAlignedBoundingBoxPermuteDevices, Constructor) { From 843ba13b4ec40069180fc6613cf7d735da2ee038 Mon Sep 17 00:00:00 2001 From: Sameer Sheorey <41028320+ssheorey@users.noreply.github.com> Date: Mon, 13 Nov 2023 22:00:54 -0800 Subject: [PATCH 8/8] style fix: AxisAlignedBoundingBox.cpp --- cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp b/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp index c9070297629..54405b414ff 100644 --- a/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp +++ b/cpp/tests/t/geometry/AxisAlignedBoundingBox.cpp @@ -32,6 +32,7 @@ INSTANTIATE_TEST_SUITE_P( AxisAlignedBoundingBoxPermuteDevicePairs::TestCases())); TEST_P(AxisAlignedBoundingBoxPermuteDevices, ConstructorNoArg) { + using ::testing::AnyOf; t::geometry::AxisAlignedBoundingBox aabb; // Inherited from Geometry3D. @@ -51,7 +52,11 @@ TEST_P(AxisAlignedBoundingBoxPermuteDevices, ConstructorNoArg) { EXPECT_EQ(aabb.GetDevice(), core::Device("CPU:0")); // Print Information. - EXPECT_EQ(aabb.ToString(), "AxisAlignedBoundingBox[[0 0 0] - [0 0 0], Float32, CPU:0]"); + EXPECT_THAT( + aabb.ToString(), // Compiler dependent output + AnyOf("AxisAlignedBoundingBox[[0 0 0] - [0 0 0], Float32, CPU:0]", + "AxisAlignedBoundingBox[[0.0 0.0 0.0] - [0.0 0.0 0.0], " + "Float32, CPU:0]")); } TEST_P(AxisAlignedBoundingBoxPermuteDevices, Constructor) {