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

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

benjaminum added 2 commits Aug 12, 2019
Added new geometry type TetraMesh.
Copy link
Contributor

left a comment

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

left a comment

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.

benjaminum added 5 commits Aug 19, 2019
-qhull for tetrahedralization
-primal contouring for surface extraction
to ComputeDelaunayTetrahedralization
@yxlao
yxlao approved these changes Aug 20, 2019
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

LGTM

@yxlao yxlao dismissed griegler’s stale review Aug 20, 2019

@griegler already approved

@yxlao yxlao merged commit 794cf05 into intel-isl:master Aug 20, 2019
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 13 discussions left (benjaminum, griegler)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.