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

moved long function definitions out of Gmsh class declaration and header file #429

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+158 −152
Open

moved long function definitions out of Gmsh class declaration and header file #429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 153 additions & 0 deletions cpp/modmesh/inout/gmsh.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,159 @@ void Gmsh::build_interior(const std::shared_ptr<StaticMesh> & blk)
blk->build_boundary();
blk->build_ghost();
}

// Check the finite state machine state transition is valid or not to check msh file format is correct
bool Gmsh::is_valid_transition(const std::string s)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please help change the argument to be a const reference? It's a simple change but needs to be applied in both the declaration (in header file) and the definition (here).

To be specific, it is to change

bool Gmsh::is_valid_transition(const std::string s)

with

bool Gmsh::is_valid_transition(const std::string & s)

That is, change the argument of the function from const std::string s to const std::string & s.

The change will speed up the code by saving unnecessary string copying.

{
if (last_fmt_state == FormatState::BEGIN)
{
return s == "$MeshFormat";
}
else if (last_fmt_state == FormatState::META_END || last_fmt_state == FormatState::PYHSICAL_NAME_END)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks odd to me that you use "or" here. Why not split them into two else if?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is my bad, I have checked the gmsh manual section 10.3.1.
According to the manual, after MeshFormat, which is the META_END state, it can either be followed by $PhysicalNames or $Nodes. That is why the function returns an "or" expression. However the problem is that after physical name section, it only can be followed by Node. Therefore this code should be changed to

...
else if (last_fmt_state == FormatState::META_END)
{
    return s == "$PhysicalNames" || s == "$Nodes";
}
else if (last_fmt_state == FormatState::PYHSICAL_NAME_END)
{
    return s == "$Nodes";
}
...

Thanks @tigercosmos to point out this problem.
I think we can use another PR to fix this problem, it should add some test case to make sure this logic is correct!

I will file an issue to track this.

{
return s == "$PhysicalNames" || s == "$Nodes";
}
else if (last_fmt_state == FormatState::NODE_END)
{
return s == "$Elements";
}

return false;
}

void Gmsh::load_meta(void)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Gmsh::load_meta()?

{
std::string line;

while (std::getline(stream, line) && line.find('$') == std::string::npos)
{
auto tokens = tokenize(line, ' ');
msh_ver = std::stod(tokens[0]);

// The parse only support ver 2.2 msh file.
if (msh_ver != 2.2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will choose to use constexpr for the number.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just use a macro ?

Copy link
Member

Choose a reason for hiding this comment

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

Macro does not allow compilers to check type. When macros and constexpr both work, choose constexpr.

Copy link
Member

@yungyuc yungyuc Oct 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For version number, integer should be used. Floating could be inexact and should be avoided.

{
throw std::invalid_argument(Formatter() << "modmesh does not support msh file ver " << msh_ver << ".");
}

msh_file_type = std::stoi(tokens[1]);
msh_data_size = std::stoi(tokens[2]);
}

if (!line.compare("$EndMeshFormat"))
{
last_fmt_state = FormatState::META_END;
}
}

// TODO: PhysicalNames section parsing logic not complete yet, but without PhysicalNames section
// modmesh mesh viewer still working, therefore we can finish this later.
void Gmsh::load_physical(void)
{
std::string line;

while (std::getline(stream, line) && line.find('$') == std::string::npos) {}

if (!line.compare("$EndPhysicalNames"))
{
last_fmt_state = FormatState::PYHSICAL_NAME_END;
}
}

void Gmsh::load_nodes(void)
{
std::string line;
std::getline(stream, line);
auto nnode = std::stoul(line);

m_nds.remake(small_vector<size_t>{nnode, 3}, 0);

while (std::getline(stream, line) && line.find('$') == std::string::npos)
{
auto tokens = tokenize(line, ' ');
// gmsh node index is 1-based index
m_nds(std::stoul(tokens[0]) - 1, 0) = std::stod(tokens[1]);
m_nds(std::stoul(tokens[0]) - 1, 1) = std::stod(tokens[2]);
m_nds(std::stoul(tokens[0]) - 1, 2) = std::stod(tokens[3]);
}

if (!line.compare("$EndNodes"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you should maintain a table to keep these token strings.

{
last_fmt_state = FormatState::NODE_END;
}
}

void Gmsh::load_elements(void)
{
std::string line;
std::getline(stream, line);
auto nelement = std::stoul(line);
std::vector<uint_type> usnds;

m_cltpn.remake(small_vector<size_t>{nelement}, 0);
m_elgrp.remake(small_vector<size_t>{nelement}, 0);
m_elgeo.remake(small_vector<size_t>{nelement}, 0);
m_eldim.remake(small_vector<size_t>{nelement}, 0);

uint_type idx = 0;

while (std::getline(stream, line) && line.find('$') == std::string::npos && idx < nelement)
{
auto tokens = tokenize(line, ' ');

// parse element type
auto tpn = std::stoul(tokens[1]);
auto eldef = GmshElementDef::by_id(tpn);
// parse element tag
std::vector<uint_type> tag;
for (size_t i = 0; i < std::stoul(tokens[2]); ++i)
{
tag.push_back(std::stoul(tokens[3 + i]));
}

// parse node number list
std::vector<uint_type> nds;
for (size_t i = 3 + std::stoul(tokens[2]); i < tokens.size(); ++i)
{
nds.push_back(std::stoul(tokens[i]));
}

m_cltpn[idx] = eldef.mmtpn();
m_elgrp[idx] = tag[0];
m_elgeo[idx] = tag[1];
m_eldim[idx] = eldef.ndim();

small_vector<uint_type> nds_temp(nds.size() + 1, 0);
auto mmcl = eldef.mmcl();

for (size_t i = 0; i < mmcl.size(); ++i)
{
nds_temp[mmcl[i] + 1] = nds[i] - 1;
}
usnds.insert(usnds.end(), nds_temp.begin() + 1, nds_temp.end());
nds_temp[0] = mmcl.size();
m_elems.insert(std::pair{idx, nds_temp});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think emplace will be better?

idx++;
}

if (!line.compare("$EndElements"))
{
last_fmt_state = FormatState::ELEMENT_END;
}

// sorting used node and remove duplicate node id
std::sort(usnds.begin(), usnds.end());
auto remove = std::unique(usnds.begin(), usnds.end());
usnds.erase(remove, usnds.end());

// put used node id to m_ndmap
m_ndmap.remake(small_vector<size_t>{usnds.size()}, -1);
for (size_t i = 0; i < usnds.size(); ++i)
{
m_ndmap(usnds[i]) = i;
}
}

} /* end namespace inout */
} /* end namespace modmesh */

Expand Down
157 changes: 5 additions & 152 deletions cpp/modmesh/inout/gmsh.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,158 +78,11 @@ class Gmsh
ELEMENT_END
};

// Check the finite state machine state transition is valid or not to check msh file format is correct
bool is_valid_transition(const std::string s)
{
if (last_fmt_state == FormatState::BEGIN)
{
return s == "$MeshFormat";
}
else if (last_fmt_state == FormatState::META_END || last_fmt_state == FormatState::PYHSICAL_NAME_END)
{
return s == "$PhysicalNames" || s == "$Nodes";
}
else if (last_fmt_state == FormatState::NODE_END)
{
return s == "$Elements";
}

return false;
}

void load_meta(void)
{
std::string line;

while (std::getline(stream, line) && line.find('$') == std::string::npos)
{
auto tokens = tokenize(line, ' ');
msh_ver = std::stod(tokens[0]);

// The parse only support ver 2.2 msh file.
if (msh_ver != 2.2)
{
throw std::invalid_argument(Formatter() << "modmesh does not support msh file ver " << msh_ver << ".");
}

msh_file_type = std::stoi(tokens[1]);
msh_data_size = std::stoi(tokens[2]);
}

if (!line.compare("$EndMeshFormat"))
{
last_fmt_state = FormatState::META_END;
}
}

// TODO: PhysicalNames section parsing logic not complete yet, but without PhysicalNames section
// modmesh mesh viewer still working, therefore we can finish this later.
void load_physical(void)
{
std::string line;

while (std::getline(stream, line) && line.find('$') == std::string::npos) {}

if (!line.compare("$EndPhysicalNames"))
{
last_fmt_state = FormatState::PYHSICAL_NAME_END;
}
}

void load_nodes(void)
{
std::string line;
std::getline(stream, line);
auto nnode = std::stoul(line);

m_nds.remake(small_vector<size_t>{nnode, 3}, 0);

while (std::getline(stream, line) && line.find('$') == std::string::npos)
{
auto tokens = tokenize(line, ' ');
// gmsh node index is 1-based index
m_nds(std::stoul(tokens[0]) - 1, 0) = std::stod(tokens[1]);
m_nds(std::stoul(tokens[0]) - 1, 1) = std::stod(tokens[2]);
m_nds(std::stoul(tokens[0]) - 1, 2) = std::stod(tokens[3]);
}

if (!line.compare("$EndNodes"))
{
last_fmt_state = FormatState::NODE_END;
}
}

void load_elements(void)
{
std::string line;
std::getline(stream, line);
auto nelement = std::stoul(line);
std::vector<uint_type> usnds;

m_cltpn.remake(small_vector<size_t>{nelement}, 0);
m_elgrp.remake(small_vector<size_t>{nelement}, 0);
m_elgeo.remake(small_vector<size_t>{nelement}, 0);
m_eldim.remake(small_vector<size_t>{nelement}, 0);

uint_type idx = 0;

while (std::getline(stream, line) && line.find('$') == std::string::npos && idx < nelement)
{
auto tokens = tokenize(line, ' ');

// parse element type
auto tpn = std::stoul(tokens[1]);
auto eldef = GmshElementDef::by_id(tpn);
// parse element tag
std::vector<uint_type> tag;
for (size_t i = 0; i < std::stoul(tokens[2]); ++i)
{
tag.push_back(std::stoul(tokens[3 + i]));
}

// parse node number list
std::vector<uint_type> nds;
for (size_t i = 3 + std::stoul(tokens[2]); i < tokens.size(); ++i)
{
nds.push_back(std::stoul(tokens[i]));
}

m_cltpn[idx] = eldef.mmtpn();
m_elgrp[idx] = tag[0];
m_elgeo[idx] = tag[1];
m_eldim[idx] = eldef.ndim();

small_vector<uint_type> nds_temp(nds.size() + 1, 0);
auto mmcl = eldef.mmcl();

for (size_t i = 0; i < mmcl.size(); ++i)
{
nds_temp[mmcl[i] + 1] = nds[i] - 1;
}
usnds.insert(usnds.end(), nds_temp.begin() + 1, nds_temp.end());
nds_temp[0] = mmcl.size();
m_elems.insert(std::pair{idx, nds_temp});
idx++;
}

if (!line.compare("$EndElements"))
{
last_fmt_state = FormatState::ELEMENT_END;
}

// sorting used node and remove duplicate node id
std::sort(usnds.begin(), usnds.end());
auto remove = std::unique(usnds.begin(), usnds.end());
usnds.erase(remove, usnds.end());

// put used node id to m_ndmap
m_ndmap.remake(small_vector<size_t>{usnds.size()}, -1);
for (size_t i = 0; i < usnds.size(); ++i)
{
m_ndmap(usnds[i]) = i;
}
}

bool is_valid_transition(const std::string s);
void load_meta(void);
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to have void in the argument.

void load_physical(void);
void load_nodes(void);
void load_elements(void);
void build_interior(const std::shared_ptr<StaticMesh> & blk);

std::stringstream stream;
Expand Down
Loading