isl-org / Open3D

Open3D: A Modern Library for 3D Data Processing
http://www.open3d.org
Other
11.56k stars 2.32k forks source link

No bounds are set when moving an AxisAlignedBoundingBox instance to a different device #6721

Open ChihaoZhang opened 8 months ago

ChihaoZhang commented 8 months ago

Checklist

Describe the issue

Hi, thanks for the excellent work you've done. Today, I found an unnecessary warning displayed when I try to move an instance of AxisAlignedBoundingBox from CPU to CUDA though min_bound is indeed smaller than max_bound, which causes no values set to the returned bbox.

After reviewing the source code (in cpp/open3d/t/geometry/BoundingVolume.cpp), I guess the reason is when calling To() method, it creates an instance on the specific device with min_bound as well as max_bound set to 0 first, then SetMinBound() and SetMaxBound() will check if max_bound_ and min_bound_ are valid before setting by comparing it with the initial value, i.e., 0 in this case.

Since the only requirement for a bounding box specified by min/max bound is min_bound is less than max_bound strictly and when an instance calls To(), it's already valid, I believe data checking in To() is redundant.

Steps to reproduce the bug

import numpy as np
import open3d as o3d

device = o3d.core.Device('CUDA:0')

min_bound = np.array([-1, 1, -1], dtype=np.float64)
max_bound = np.array([-0.5, 2, 0.5], dtype=np.float64)
bbox = o3d.t.geometry.AxisAlignedBoundingBox(min_bound, max_bound).to(device)
print(bbox)

Error message

[Open3D WARNING] Invalid axis-aligned bounding box. Please make sure all the elements in max bound are larger than min bound. [Open3D WARNING] Invalid axis-aligned bounding box. Please make sure all the elements in min bound are smaller than max bound. AxisAlignedBoundingBox[[0 0 0] - [0 0 0], Float32, CUDA:0]

Expected behavior

min_bound and max_bound should be set successfully (without any warnings) to bbox which is an instance of AxisAlignedBoundingBox stored in CUDA.

Open3D, Python and System information

- Operating system: Ubuntu 20.04
- Python version: Python 3.10
- Open3D version: 0.18.0
- System architecture: x86
- Is this a remote workstation?: no
- How did you install Open3D?: build from source
- Compiler version (if built from source): gcc

Additional information

This problem can be sidestepped by creating min_bound as well as max_bound in the desired device, then there is no need to call to() method. But I do hope it can be solved in the source code.

saurabheights commented 8 months ago

Current code to transfer uses:-

    AxisAlignedBoundingBox box(device);
    box.SetMaxBound(max_bound_.To(device, true));
    box.SetMinBound(min_bound_.To(device, true));

The problem is that default box has min bound and max bound set to 0. Setting max_bound fails since this is negative in your usecase.

Easy fix should be to change open3d code to

    AxisAlignedBoundingBox box(min_bound_.To(device, true), max_bound_.To(device, true));

Will add a PR soon with unit tests.

ChihaoZhang commented 8 months ago

@saurabheights Thanks for your reply and I agree with the fix code which should be a solution to my problem with the minimal change. However, for fixing the problems in AxisAlignedBoundingBox existed for now, there are additional points I should remind you:

  1. Like what I said above, since an instance is already a valid AxisAlignedBoundingBox when it calls To() method, there is no need to do data check once more.
  2. Note that the root of the bug is data check in SetMinBound() and SetMaxBound(), which uses the preset value (i.e., 0s) together with input to compute the volume mistakenly. Since they are public methods and also bound to python interface, any calls with negative input will cause the warning and hence no value set. The fix should avoid volume check when only one bound is set.

If the first point is my personal stance and can be ignored, the second point is worthy of attention. And my personal view on them is to combine the two into a single one considering a valid AxisAlignedBoundingBox does need both min_bound and max_bound set explicitly.