Add Tensor AxisAlignedBoundingBox #5267

yuecideng · 2022-06-30T15:04:13Z

This PR implements the tensor based AxisAlignedBoundingBox with the summary as follow:

Use three tensors (3, ) to represent min_bound, max_bound and color, which are only supported float32.
There is no Transform and Rotate functions implemented (or may be implemented with exception like legacy)?
Add valid check for min and max bound as Cropping a cropped PCD returns empty #5087 mentioned.
The implemented methods follow the legacy version and tensor geometry specific functions.
pybind and unit test are also implemented.
The method GetPointIndicesWithinBoundingBox is implemented using kernel.
Add GetAxisAlignedBoundingBox in tensor PointCloud, TriangleMesh and LineSet.

In addition to these, I think it should be added to support Draw for visualization.

This change is

update-docs · 2022-06-30T15:04:17Z

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

reyanshsolis · 2022-07-12T07:35:19Z

We might not need kernels for the ops here. Please try to use Tensor ops only.
We can have a discussion anytime you are facing an issue in the implementation. Thanks.

reyanshsolis

Reviewable status: 0 of 22 files reviewed, all discussions resolved (waiting on @benjaminum, @errissa, @ssheorey, and @yxlao)

cpp/open3d/t/geometry/BoundingVolume.h line 73 at r3 (raw file):

    /// bound tensor, which must be on the same device and have the same data
    /// type.
    /// \param min_bound Lower bounds of the bounding box for all axes. Tensor

update: Added tensor dtype info in docstring for functions in this file.

cpp/open3d/t/geometry/BoundingVolume.h line 224 at r3 (raw file):

protected:
    core::Device device_ = core::Device("CPU:0");
    core::Dtype dtype_ = core::Float32;

Since we are restricting the dtype behavior to: All attributes (max/min bound, color) must have same dtype (f32 or f64), I have added dtype_ member and GetDtype func.

cpp/open3d/t/geometry/BoundingVolume.cpp line 206 at r2 (raw file):

std::string AxisAlignedBoundingBox::ToString() const {
    const core::Dtype dtype = min_bound_.GetDtype();

update: moved this globally [dtype_ as member function].

cpp/open3d/t/geometry/BoundingVolume.cpp line 262 at r2 (raw file):

}

bool AxisAlignedBoundingBox::CheckValid(bool exception) const {

see the above comment related to CheckValid

cpp/open3d/t/geometry/BoundingVolume.cpp line 62 at r3 (raw file):

    // Check if the bounding box is valid.
    if (Volume() < 0) {

update: We can check Volume directly, the CheckValid function was not required. When the error is thrown, the traceback is more clear coming from the function directly, so prefer having error statements in the main function over helper/util functions.

cpp/open3d/t/geometry/BoundingVolume.cpp line 204 at r3 (raw file):

}

core::Tensor AxisAlignedBoundingBox::GetBoxPoints() const {

previously:

core::Tensor AxisAlignedBoundingBox::GetBoxPoints() const {
    const core::Tensor extent = GetExtent().To(core::Float32);
    core::Tensor points =
            core::Tensor::Zeros({8, 3}, core::Float32, GetDevice());
    const float *extent_data = extent.GetDataPtr<float>();

    points[0] = min_bound_;
    points[1] = min_bound_ +
                core::Tensor::Init<float>({extent_data[0], 0, 0}, GetDevice());
    points[2] = min_bound_ +
                core::Tensor::Init<float>({0, extent_data[1], 0}, GetDevice());
    points[3] = min_bound_ +
                core::Tensor::Init<float>({0, 0, extent_data[2]}, GetDevice());
    points[4] = max_bound_;
    points[5] = max_bound_ +
                core::Tensor::Init<float>({-extent_data[0], 0, 0}, GetDevice());
    points[6] = max_bound_ +
                core::Tensor::Init<float>({0, -extent_data[1], 0}, GetDevice());
    points[7] = max_bound_ +
                core::Tensor::Init<float>({0, 0, -extent_data[2]}, GetDevice());
    return points;
}

update:

core::Tensor AxisAlignedBoundingBox::GetBoxPoints() const {
    const core::Tensor extent_3x3 =
            core::Tensor::Eye(3, GetDtype(), GetDevice())
                    .Mul(GetExtent().Reshape({3, 1}));

    return core::Concatenate({min_bound_.Reshape({1, 3}),
                              min_bound_.Reshape({1, 3}) + extent_3x3,
                              max_bound_.Reshape({1, 3}),
                              max_bound_.Reshape({1, 3}) - extent_3x3});
}

Avoid working with pointers, if tensor ops can be used. The previous implementation will also not work when the Tensor is on GPU, as you can't work with a pointer to GPU memory on CPU directly.

reyanshsolis

Reviewable status: 0 of 22 files reviewed, 5 unresolved discussions (waiting on @benjaminum, @errissa, @ssheorey, @yuecideng, and @yxlao)

cpp/open3d/t/geometry/BoundingVolume.h line 173 at r4 (raw file):

    double GetXPercentage(double x) const;
    double GetYPercentage(double y) const;

leave one line space between function declaration.

cpp/pybind/t/geometry/boundingvolume.cpp line 63 at r4 (raw file):

        - AxisAlignedBoundingBox::GetColor()
        - AxisAlignedBoundingBox::SetColor(const core::Tensor &color)
    - Value tensor must have shape {3}.

update the constructor description from the updated cpp docstrings. Mention the supported dtypes and range for color values.

cpp/pybind/t/geometry/boundingvolume.cpp line 174 at r4 (raw file):

            {{"translation",
              "Transformation [Tensor of shape (4,4)].  Should be on "
              "the same "

It should be Translation, not Transformation and the translation tensor must be {3,} not {4, 4}.

cpp/pybind/t/geometry/boundingvolume.cpp line 180 at r4 (raw file):

            m, "AxisAlignedBoundingBox", "scale",
            {{"scale", "The scale parameter."},
             {"center", "Center used for the scaling operation."}});

mention the supported dtype and shape for center and other parameters. (in most of the cases the dtype must be same as bounding box dtype.

cpp/tests/t/geometry/BoundingVolume.cpp line 73 at r4 (raw file):

    EXPECT_EQ(aabb.GetDevice(), core::Device("CPU:0"));

    // ToString

Change comment ToString to Print Information.. (add the .).

reyanshsolis · 2022-07-26T14:01:18Z

LGTM after minor changes.

yuecideng

Reviewable status: 0 of 22 files reviewed, 5 unresolved discussions (waiting on @benjaminum, @errissa, @reyanshsolis, @ssheorey, and @yxlao)

cpp/open3d/t/geometry/BoundingVolume.h line 173 at r4 (raw file):