-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
zhengyu/create_box #5190
zhengyu/create_box #5190
Conversation
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 7 files at r1, all commit messages.
Reviewable status: 4 of 7 files reviewed, 13 unresolved discussions (waiting on @yxlao and @ZhengyuDiao)
cpp/open3d/t/geometry/TriangleMesh.h
line 371 at r1 (raw file):
bool HasTriangleColors() const { return HasTriangleAttr("colors"); } /// Factory function to create a box mesh (TriangleMeshFactory.cpp)
Rewrite as:
Create a box triangle mesh. One vertex of the box will be placed at the origin and the box aligns with the positive x, y, and z axes.
cpp/open3d/t/geometry/TriangleMeshFactory.cpp
line 35 at r1 (raw file):
TriangleMesh TriangleMesh::CreateBox( float width /* = 1.0*/,
it is not necessary to add /* xxx */
here, consider removing them.
cpp/open3d/t/geometry/TriangleMeshFactory.cpp
line 41 at r1 (raw file):
core::Dtype int_dtype /* = core::Int64*/, const core::Device &device) {
remove extra space here.
cpp/open3d/t/geometry/TriangleMeshFactory.cpp
line 53 at r1 (raw file):
} utility::LogInfo("width: {}, height: {}, depth: {}", width, height, depth);
remove unnecessary prints
cpp/open3d/t/geometry/TriangleMeshFactory.cpp
line 66 at r1 (raw file):
// Vertices. core::Tensor vertex_positions({8, 3}, float_dtype);
You can just do:
core::Tensor vertex_positions;
This will create the tensor without allocation. Since we will be allocating again with core::Tensor::Init
.
cpp/open3d/t/geometry/TriangleMeshFactory.cpp
line 67 at r1 (raw file):
// Vertices. core::Tensor vertex_positions({8, 3}, float_dtype); if (float_dtype == core::Float32){
Since these creation functions are small, we can afford to make copies: always create with Float64
and then convert to Float32
if necessary. You can even fuse the sanity check here. This will simplify the code.
core::Tensor vertex_positions = core::Tensor::Init<float>{xxx};
if (float_dtype == core::Float32) {
vertex_positions = vertex_positions.To(core::Float32);
} else if (float_dtype != core::Float64) {
Print error.
}
cpp/open3d/t/geometry/TriangleMeshFactory.cpp
line 80 at r1 (raw file):
} // Triangles.
Same comment as above.
cpp/pybind/t/geometry/trianglemesh.cpp
line 229 at r1 (raw file):
// Triangle Mesh's creation APIs. triangle_mesh.def_static("create_box", &TriangleMesh::CreateBox, "Factory function to create a box. The left bottom "
update the docstring to be the same as c++
cpp/tests/t/geometry/TriangleMesh.cpp
line 458 at r1 (raw file):
TEST_P(TriangleMeshPermuteDevices, CreateBox){ // to do
Add two tests:
- Create with default parameters, and check the value of the triangle mesh.
- Create with custom parameters, and check the value of the triangle mesh.
python/test/t/geometry/test_trianglemesh.py
line 41 at r1 (raw file):
apply style: http://www.open3d.org/docs/latest/contribute/styleguide.html
python/test/t/geometry/test_trianglemesh.py
line 43 at r1 (raw file):
def test_create_box():
remove empty space
python/test/t/geometry/test_trianglemesh.py
line 46 at r1 (raw file):
# Test legacy # box = o3d.geometry.TriangleMesh.create_box() # box.compute_vertex_normals()
remove commented code
python/test/t/geometry/test_trianglemesh.py
line 50 at r1 (raw file):
# To implement
Add two tests:
- Create with default parameters, and check the value of the triangle mesh.
- Create with custom parameters, and check the value of the triangle mesh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @reyanshsolis and @ZhengyuDiao)
cpp/pybind/t/geometry/trianglemesh.cpp
line 233 at r3 (raw file):
"width"_a = 1.0, "height"_a = 1.0, "depth"_a = 1.0, "vertex_dtype"_a = core::Float64,
float_dtype
, int_dtype
use float32 and int64 as default
cpp/tests/t/geometry/TriangleMesh.cpp
line 459 at r3 (raw file):
core::Device device = GetParam(); // test with default parameters
nit: // Test xxxx .
- end with
.
- capitalilze first character
cpp/tests/t/geometry/TriangleMesh.cpp
line 492 at r3 (raw file):
EXPECT_TRUE(box_default.GetTriangleIndices().AllClose(triangle_indices)); // test with custom parameters
same as above
cpp/tests/t/geometry/TriangleMesh.cpp
line 494 at r3 (raw file):
// test with custom parameters t::geometry::TriangleMesh box_custom = t::geometry::TriangleMesh::CreateBox(2, 3, 4);
Same as python, specify float dtype, int dtype, device.
python/test/t/geometry/test_trianglemesh.py
line 27 at r3 (raw file):
# ---------------------------------------------------------------------------- from turtle import width
remove turtle
python/test/t/geometry/test_trianglemesh.py
line 42 at r3 (raw file):
def test_create_box():
Add @pytest.mark.parametrize("device", list_devices())
to iterate multiple devices. See python\test\t\geometry\test_pointcloud.py
.
python/test/t/geometry/test_trianglemesh.py
line 44 at r3 (raw file):
def test_create_box(): # test with default parameters box_default = o3d.t.geometry.TriangleMesh.create_box()
o3d.t.geometry.TriangleMesh.create_box(device=device)
Code quote:
o3d.t.geometry.TriangleMesh.create_box()
python/test/t/geometry/test_trianglemesh.py
line 61 at r3 (raw file):
[1, 5, 7], [1, 7, 3], [2, 3, 7], [2, 7, 6], [0, 4, 1], [1, 4, 5]]), dtype=o3c.Dtype.Int64,
Remove .Dtype
python/test/t/geometry/test_trianglemesh.py
line 68 at r3 (raw file):
# test with custom parameters box_custom = o3d.t.geometry.TriangleMesh.create_box(2, 4, 3)
o3d.t.geometry.TriangleMesh.create_box(2, 3, 4, o3c.float64, o3c.int32, devcie)
Code quote:
o3d.t.geometry.TriangleMesh.create_box(2, 4, 3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm after ci
Wait for #5248 to pass before merging. |
create_box api in tensor-based module
This change is