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.

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

Feature/Geometry type for tetrahedral meshes #1109

Merged
merged 7 commits into from
Aug 20, 2019

Conversation

benjaminum
Copy link
Contributor

@benjaminum benjaminum commented Aug 12, 2019

This PR implements a new geometry type TetraMesh.

  • Creation from PointClouds with Delaunay triangulation

  • Isosurface extraction as TriangleMesh

  • Visualization as lines

  • Unit tests


This change is Reviewable

Copy link
Contributor

@griegler griegler left a comment

Choose a reason for hiding this comment

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

Nice! I just have a few minor comments below.

Reviewed 24 of 25 files at r1.
Reviewable status: 24 of 25 files reviewed, 12 unresolved discussions (waiting on @benjaminum)


src/Open3D/Geometry/Qhull.h, line 44 at r1 (raw file):

            const std::vector<Eigen::Vector3d>& points);

    static std::shared_ptr<TetraMesh> ComputeDelaunayTriangulation3D(

Suffix 3D needed? Why not simply ComputeDelaunayTriangulation? Or should it not be ComputeDelaunayTetrahedralization?


src/Open3D/Geometry/Qhull.cpp, line 119 at r1 (raw file):

        delaunay_triangulation->tetras_.push_back(Vector4i(0, 1, 2, 3));
        return delaunay_triangulation;
    } else if (points.size() < 3) {

Is this case not already handled above?


src/Open3D/Geometry/TetraMesh.h, line 89 at r1 (raw file):

    /// Function to extract a triangle mesh of the specified iso-surface at
    /// \param level. \param values are values per-vertex.

Can you add some references to the specific method that is used?


src/Open3D/Geometry/TetraMesh.h, line 94 at r1 (raw file):

    /// Function that creates a tetrahedral mesh (TetraMeshFactory.cpp).
    /// from a point cloud.

Can you add some references to the specific method that is used, i.e., Delaunay from qhull?


src/Open3D/Geometry/TetraMesh.cpp, line 190 at r1 (raw file):

dubplicates

typo


src/Open3D/Geometry/TetraMesh.cpp, line 281 at r1 (raw file):

    }

    auto surface_intersection_test = [](double v0, double v1, double level) {

Functions/Lambdas should be CamelCase => SurfaceIntersectionTest


src/Open3D/Geometry/TetraMesh.cpp, line 285 at r1 (raw file):

    };

    auto compute_edge_vertex = [&values, level, this](Index idx1, Index idx2) {

style


src/Open3D/Geometry/TetraMesh.cpp, line 299 at r1 (raw file):

    };

    auto compute_triangle_normal = [](const Eigen::Vector3d &a,

style


src/Open3D/Geometry/TetraMesh.cpp, line 302 at r1 (raw file):

                                      const Eigen::Vector3d &b,
                                      const Eigen::Vector3d &c) {
        Eigen::Vector3d ab(b - a);

Why using a constructor and not simply Eigen::Vector3d ab = b - a?


src/Open3D/Geometry/TetraMesh.cpp, line 307 at r1 (raw file):

    };

    auto make_sorted_tuple2 = [](Index a, Index b) {

style


src/Open3D/Geometry/TetraMesh.cpp, line 314 at r1 (raw file):

    };

    auto has_common_vertex_index = [](Index2 a, Index2 b) {

style


src/Open3D/Geometry/TetraMesh.cpp, line 315 at r1 (raw file):

    auto has_common_vertex_index = [](Index2 a, Index2 b) {
        if (std::get<0>(b) == std::get<0>(a) ||

direct return instead of if/else

Copy link
Contributor Author

@benjaminum benjaminum left a comment

Choose a reason for hiding this comment

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

Reviewable status: 24 of 25 files reviewed, 12 unresolved discussions (waiting on @griegler)


src/Open3D/Geometry/Qhull.h, line 44 at r1 (raw file):

Previously, griegler (Gernot) wrote…

Suffix 3D needed? Why not simply ComputeDelaunayTriangulation? Or should it not be ComputeDelaunayTetrahedralization?

Fn name was inspired by the name in CGAL. I am fine with 3D suffix or Tetrahedralization.


src/Open3D/Geometry/Qhull.cpp, line 119 at r1 (raw file):

Previously, griegler (Gernot) wrote…

Is this case not already handled above?

Yes. Done.


src/Open3D/Geometry/TetraMesh.h, line 89 at r1 (raw file):

Previously, griegler (Gernot) wrote…

Can you add some references to the specific method that is used?

Done.


src/Open3D/Geometry/TetraMesh.h, line 94 at r1 (raw file):

Previously, griegler (Gernot) wrote…

Can you add some references to the specific method that is used, i.e., Delaunay from qhull?

Done.


src/Open3D/Geometry/TetraMesh.cpp, line 190 at r1 (raw file):

Previously, griegler (Gernot) wrote…
dubplicates

typo

Done.


src/Open3D/Geometry/TetraMesh.cpp, line 281 at r1 (raw file):

Previously, griegler (Gernot) wrote…

Functions/Lambdas should be CamelCase => SurfaceIntersectionTest

Done.


src/Open3D/Geometry/TetraMesh.cpp, line 285 at r1 (raw file):

Previously, griegler (Gernot) wrote…

style

Done.


src/Open3D/Geometry/TetraMesh.cpp, line 299 at r1 (raw file):

Previously, griegler (Gernot) wrote…

style

Done.


src/Open3D/Geometry/TetraMesh.cpp, line 302 at r1 (raw file):

Previously, griegler (Gernot) wrote…

Why using a constructor and not simply Eigen::Vector3d ab = b - a?

Done.


src/Open3D/Geometry/TetraMesh.cpp, line 307 at r1 (raw file):

Previously, griegler (Gernot) wrote…

style

Done.


src/Open3D/Geometry/TetraMesh.cpp, line 314 at r1 (raw file):

Previously, griegler (Gernot) wrote…

style

Done.


src/Open3D/Geometry/TetraMesh.cpp, line 315 at r1 (raw file):

Previously, griegler (Gernot) wrote…

direct return instead of if/else

Done.

-qhull for tetrahedralization
-primal contouring for surface extraction
to ComputeDelaunayTetrahedralization
Copy link
Collaborator

@yxlao yxlao left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks Benjamin.

Reviewed 11 of 25 files at r1, 5 of 5 files at r2.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @benjaminum and @griegler)


src/Open3D/Geometry/TetraMesh.cpp, line 181 at r2 (raw file):

    typedef decltype(tetras_)::value_type::Scalar Index;
    typedef std::tuple<Index, Index, Index, Index> Index4;
    std::unordered_map<Index4, size_t, utility::hash_tuple::hash<Index4>>

It is okay since we did the same for triangle mesh. We could also use eigen::Vector4i and utility::hash_eigen to avoid defying a new type.

@griegler
Copy link
Contributor

LGTM

@yxlao yxlao merged commit 794cf05 into isl-org:master Aug 20, 2019
@AI-ML-Enthusiast
Copy link

@griegler
Would you give some idea to calculate Delaunay Triangulation using open3d in python? Thanks in advanced

@griegler
Copy link
Contributor

@AI-ML-Enthusiast
Copy link

@griegler Thanks for your comment. but it gives precision error.H How can I use Qhull option for scaling?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants