add Gmsh mesh file parser #233

j8xixo12 · 2023-07-22T06:04:08Z

In this commit added mesh file io for modmesh, it lets modmesh has capabilty to read third-party mesh generator generated file and convert the content to a data structure that allow modmesh can use it to do some calculation.

Currently mesh io only support Gmsh format.

j8xixo12 · 2023-07-22T06:07:38Z

modmesh/app/sample_mesh.py

@@ -83,6 +83,25 @@ def help_other(set_command=False):
        view.mgr.pycon.command = cmd.strip()


+def help_mesh_viewer(path, set_command=False):


I also added a sample command in sample_mesh app for, once mesh viewer app is completed, then this sample command can be removed.

In this commit added mesh file io for modmesh, it lets modmesh has capabilty to read third-party mesh generator generated file and convert the content to a data structure that allow modmesh can use it to do some calculation. Currently mesh io only support Gmsh format.

yungyuc · 2023-07-22T22:38:29Z

This PR contains almost 1000 lines of diff and will not be easy to review. Would it be possible to break it into smaller pieces? For review efficiency a batch of review should be around 300 lines of diff.

yungyuc

Thanks for the great work. Let's polish the code.

yungyuc · 2023-07-22T22:40:52Z

modmesh/core.py

@@ -64,6 +64,7 @@
    'METAL_BUILT',
    'metal_running',
    'HAS_VIEW',
+    'Gmsh',


This is a class and should be put before constants, ordered in the same convention as other classes or functions.

yungyuc · 2023-07-22T22:44:49Z

cpp/modmesh/io/CMakeLists.txt

@@ -0,0 +1,23 @@
+


io the name conflict with Python io module. It is unnecessary ambiguity. Please rename this directory as inout.

I renamed the module name to inout in 7432534

yungyuc · 2023-07-22T22:46:20Z

cpp/modmesh/io/gmsh.hpp

+
+namespace modmesh
+{
+namespace IO


namespace should not use uppercase, and it should not have the unnecessary ambiguity to the Python module named io.

In addition, IO is a pretty short name and may conflict with symbols in foreign code base.

Please rename it to inout.

I renamed the namespace to inout in 7432534 too.

yungyuc · 2023-07-22T22:47:38Z

cpp/modmesh/io/gmsh.hpp

+{
+namespace IO
+{
+namespace Gmsh


Please do not use uppercase in namespace. Beside, I don't think it is necessary to have 3 levels of namespace. Gmsh is just one of the format. Please remove the namespace.

I have removed namespace Gmsh in 7432534

yungyuc · 2023-07-22T22:48:15Z

cpp/modmesh/io/gmsh.hpp

+{
+namespace Gmsh
+{
+struct ElementDef


What about naming it like GmshElementDef?

I renamed ElementDef to GmshElementDef in 7432534

yungyuc · 2023-07-22T22:49:25Z

cpp/modmesh/io/gmsh.hpp

+inline ElementDef ElementDef::by_id(uint16_t id)
+{
+#define VEC(...) __VA_ARGS__
+#define MM_DECL_SWITCH_ELEMENT_TYPE(ID, NDIM, NNDS, MMTPN, MMCL) \


The use of macro makes the code easy to read. Thanks.

yungyuc · 2023-07-22T22:49:57Z

cpp/modmesh/io/gmsh.hpp

+    explicit Gmsh(const std::string & file_path)
+    {
+        bool meta_enter = false, node_enter = false, element_enter = false;
+        // clang-format off


You need to turn clang-format back on!!

yungyuc · 2023-07-22T22:51:01Z

cpp/modmesh/io/gmsh.hpp

+    };
+
+    // Check the finite state machine state transition is valid or not to check msh file format is correct
+    bool _isValidTransition(const std::string s)


Do not prepend _ to any C++ symbols. You already made them private and mixing the C++ access with Python private convention is a bad idea.

I have removed _ from these private member function.

yungyuc · 2023-07-22T22:54:30Z

cpp/modmesh/io/gmsh.hpp

+    using real_type = typename number_base::real_type;
+
+public:
+    explicit Gmsh(const std::string & file_path)


Could you make it also take a string as data, in addition to a string as path? Both are useful.

I change it to use string as data and python can take care of file stream to bytes, even if user is using Gmsh in cpp, cpp also can take care of file stream to string.

yungyuc · 2023-07-22T22:55:06Z

gtests/test_nopython_io.cpp

@@ -0,0 +1,350 @@
+#include <modmesh/io/io.hpp>


Too long to review. I trust you have made comprehensive testing. If not, please speak up.

I have tested each cell type conversion between gmsh and modmesh. Additionally, I also tested if a user inputs a cell type that is not listed in GmshElementDef. The expected behavior is that GmshElement::by_id should return an empty GmshElementDef object.

In this commit, I made the following additions and modifications: 1. Renamed cpp/modmesh/io to cpp/modmesh/inout to prevent unnecessary ambiguity. 2. Renamed the namespace IO to inout and removed the Gmsh namespace. 3. Renamed the struct ElementDef to GmshElementDef. 4. Moved non-template functions from gmsh.hpp to gmsh.cpp. 5. The Gmsh constructor now takes a string input as data, and the python wrapper also takes py::bytes as input, therefore file IO is now taken care of by python.

yungyuc

It's quite close to completion. Only some minor formatting issues remain.

yungyuc · 2023-07-29T03:38:49Z

cpp/modmesh/inout/gmsh.hpp

+    };
+
+    // Check the finite state machine state transition is valid or not to check msh file format is correct
+    bool isValidTransition(const std::string s)


Ah, I forgot to remind that C++ function names should follow snake_case. You used snake_case in other functions. This looks like an overlook. Could you change it and review all in your PR?

I fixed this in f7de14f
I changed isValidTransition and toblock to snake case in Gmsh class

yungyuc

Yet a minor mismatch. Should be a simple fix 🙂

yungyuc · 2023-07-30T02:27:15Z

cpp/modmesh/inout/pymod/wrap_Gmsh.cpp

+                    [](const py::bytes & data)
+                    { return std::make_shared<inout::Gmsh>(data); }),
+                py::arg("data"))
+            .def("toblock", &wrapped_type::to_block);


I guess it's still an overlook here. It makes sense to align the function names between Python and C++.

yungyuc · 2023-07-30T05:47:09Z

Thanks a lot!

j8xixo12 commented Jul 22, 2023

View reviewed changes

j8xixo12 force-pushed the gmsh_support branch 2 times, most recently from 82b1843 to a4610de Compare July 22, 2023 06:57

j8xixo12 force-pushed the gmsh_support branch from a4610de to 626ec62 Compare July 22, 2023 07:35

yungyuc requested changes Jul 22, 2023

View reviewed changes

j8xixo12 force-pushed the gmsh_support branch from 7432534 to 946f92c Compare July 26, 2023 16:52

modified gmsh parser and test case for good cross-platform capability

8e36ed1

yungyuc requested changes Jul 29, 2023

View reviewed changes

change gmsh parser function naming style from camel case to snake case

f7de14f

yungyuc requested changes Jul 30, 2023

View reviewed changes

align the Gmsh python api names with c++

becebae

yungyuc approved these changes Jul 30, 2023

View reviewed changes

yungyuc merged commit ecbffcc into solvcon:master Jul 30, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Gmsh mesh file parser #233

add Gmsh mesh file parser #233

j8xixo12 commented Jul 22, 2023

j8xixo12 Jul 22, 2023

yungyuc commented Jul 22, 2023

yungyuc left a comment

yungyuc Jul 22, 2023

yungyuc Jul 22, 2023

j8xixo12 Jul 26, 2023

yungyuc Jul 29, 2023

yungyuc Jul 22, 2023

j8xixo12 Jul 26, 2023 •

edited

Loading

yungyuc Jul 22, 2023

j8xixo12 Jul 26, 2023

yungyuc Jul 22, 2023

j8xixo12 Jul 26, 2023

yungyuc Jul 22, 2023

yungyuc Jul 22, 2023

yungyuc Jul 22, 2023

j8xixo12 Jul 26, 2023

yungyuc Jul 22, 2023 •

edited

Loading

j8xixo12 Jul 26, 2023

yungyuc Jul 22, 2023

j8xixo12 Jul 26, 2023

yungyuc left a comment

yungyuc Jul 29, 2023

j8xixo12 Jul 29, 2023

yungyuc left a comment

yungyuc Jul 30, 2023

yungyuc commented Jul 30, 2023

		@@ -83,6 +83,25 @@ def help_other(set_command=False):
		view.mgr.pycon.command = cmd.strip()


		def help_mesh_viewer(path, set_command=False):

add Gmsh mesh file parser #233

add Gmsh mesh file parser #233

Conversation

j8xixo12 commented Jul 22, 2023

Choose a reason for hiding this comment

yungyuc commented Jul 22, 2023

yungyuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j8xixo12 Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc commented Jul 30, 2023

j8xixo12 Jul 26, 2023 •

edited

Loading

yungyuc Jul 22, 2023 •

edited

Loading