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.

Sign up for GitHub

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

Jump to bottom

Add Tensor OBB #5389

Merged
merged 33 commits into from
Nov 30, 2022
Merged

Add Tensor OBB #5389

merged 33 commits into from
Nov 30, 2022

Conversation

yuecideng
Copy link
Collaborator

@yuecideng yuecideng commented Aug 1, 2022 •
edited
Loading

Summary:

  1. Add OrientedBoundingBox in tensor geometry.
  2. Change properties access method for pybind:
# Original.
min_bound = aabb.get_min_bound()
rotation = obb.get_rotation()
# Currently, return read only property.
min_bound = aabb.min_bound
rotation = obb.rotation
  1. Use box center as default arguments for Scale and Rotate.
  2. Add orthogonality check for rotation setting of obb.
  3. Fix some docs error related to aabb.

This change is Reviewable

@update-docs
Copy link

update-docs bot commented Aug 1, 2022

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

@yuecideng yuecideng marked this pull request as ready for review August 7, 2022 13:43
@yuecideng
Copy link
Collaborator Author

Enable type casting to part of both aabb and obb functions (eg. set_xxx, translate, ...) and add warning message for notification. @yxlao @reyanshsolis Is this modification looks good to you?

reyanshsolis
reyanshsolis requested changes Sep 27, 2022
View reviewed changes
cpp/open3d/t/geometry/BoundingVolume.cpp Outdated
@@ -30,6 +30,19 @@
#include "open3d/core/TensorFunction.h"
#include "open3d/t/geometry/kernel/PointCloud.h"

namespace {

void PrintWarningMessageFromTensor(const std::string &name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not recommend having utility functions for warnings/errors. The code is more readable with in-place warning messages. For error it is strongly recommended to have it in place since it prints the line-no., file info. for the error origin.

cpp/open3d/t/geometry/BoundingVolume.cpp Outdated
}

AxisAlignedBoundingBox &AxisAlignedBoundingBox::Translate(
const core::Tensor &translation, bool relative) {
core::AssertTensorDevice(translation, GetDevice());
core::AssertTensorShape(translation, {3});
core::AssertTensorDtype(translation, GetDtype());
if (translation.GetDtype() != GetDtype()) {
PrintWarningMessageFromTensor("translation", translation.GetDtype(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

we may restrict the dtype of translation, transformation, rotation, and center to Float32 and Float64, and perform auto-casting without warning.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Skip warning, silently convert.

cpp/open3d/t/geometry/BoundingVolume.cpp Outdated
core::AssertTensorDevice(center_d, GetDevice());
core::AssertTensorShape(center_d, {3});
if (center_d.GetDtype() != GetDtype()) {
PrintWarningMessageFromTensor("center", center_d.GetDtype(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Skip warning, silently convert.

cpp/open3d/t/geometry/BoundingVolume.cpp Outdated
extent_.GetItem({core::TensorKey::Index(2)}) * 0.5;

core::Tensor points = core::Tensor::Zeros({8, 3}, GetDtype(), GetDevice());
points.SetItem(core::TensorKey::Index(0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

To investigate a cleaner initialization possibility.

Copy link
Collaborator Author

@yuecideng yuecideng Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@reyanshsolis How about the following way:

const t::geometry::AxisAlignedBoundingBox aabb(GetExtent() * -0.5, GetExtent() * 0.5);
return aabb.GetBoxPoints().Matmul(GetRotation()).Add(GetCenter());

cpp/open3d/t/geometry/BoundingVolume.cpp Outdated
const core::Tensor &translation, bool relative) {
core::AssertTensorDevice(translation, GetDevice());
core::AssertTensorShape(translation, {3});
if (translation.GetDtype() != GetDtype()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Skip warning, silently convert.

cpp/open3d/t/geometry/BoundingVolume.cpp Outdated
const utility::optional<core::Tensor> &center) {
core::AssertTensorDevice(rotation, GetDevice());
core::AssertTensorShape(rotation, {3, 3});
if (rotation.GetDtype() != GetDtype()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Skip warning, silently convert.

cpp/open3d/t/geometry/BoundingVolume.cpp Outdated
const core::Tensor &transformation) {
core::AssertTensorDevice(transformation, GetDevice());
core::AssertTensorShape(transformation, {4, 4});
if (transformation.GetDtype() != GetDtype()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Skip warning, silently convert.

@yuecideng
Copy link
Collaborator Author

Remove the warning message when casting input tensor.

This was referenced Nov 14, 2022
Merged
Closed
@yxlao
Copy link
Collaborator

yxlao commented Nov 29, 2022

I have the same issue building the benchmark executable as the CI reports when CUDA is on. The master branch builds fine. I am looking into it.

@yxlao
Copy link
Collaborator

yxlao commented Nov 29, 2022

I have the same issue building the benchmark executable as the CI reports when CUDA is on. The master branch builds fine. I am looking into it.

Ok, it's due to missing ;.

@yxlao
Copy link
Collaborator

yxlao commented Nov 29, 2022

LGTM after CI.

@yuecideng yuecideng merged commit ba48315 into master Nov 30, 2022
@yuecideng yuecideng deleted the yueci/tensor-obb branch November 30, 2022 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yxlao yxlao yxlao approved these changes

@reyanshsolis reyanshsolis reyanshsolis approved these changes

@theNded theNded Awaiting requested review from theNded

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuecideng @yxlao @reyanshsolis