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

Expansion of GoCAD interface #2586

Merged
merged 15 commits into from Aug 12, 2019

Conversation

@rinkk
Copy link
Member

commented Aug 1, 2019

Can now also parse GoCAD PLine data.

Also added a check if multiple data sets in the same file have the same name and if so append a UID, so files don't get overwritten on export.

parseLine(in, nodes, elems, node_id_map, mesh_prop);
return true;
}
else if (line == "END")

This comment has been minimized.

Copy link
@endJunction

endJunction Aug 1, 2019

Member

else after a return is not needed...

This comment has been minimized.

Copy link
@rinkk

rinkk Aug 2, 2019

Author Member

✔️

@@ -108,7 +160,21 @@ MeshLib::Mesh* GocadTSurfaceReader::readMesh(std::ifstream& in,
return nullptr;
}
}
else if (str[0] == "TFACE")
else if (type == GOCAD_DATA_TYPE::PLINE && str[0] == "ILINE")

This comment has been minimized.

Copy link
@endJunction

endJunction Aug 1, 2019

Member

This is copy of the following if for TSURF. Please avoid code duplications.

This comment has been minimized.

Copy link
@rinkk

rinkk Aug 2, 2019

Author Member

✔️

This comment has been minimized.

Copy link
@endJunction

endJunction Aug 2, 2019

Member

The duplication is indeed resolved, but now the mesh creation logic is mixed up with the parsing of the cases. It is difficult to add new cases there.
It's up to you, but I'd have written something like

auto parse_mesh = [&](auto node_element_parser) {
    std::vector<MeshLib::Node*> nodes;
    std::vector<MeshLib::Element*> elems;
    std::map<std::size_t, std::size_t> node_id_map;
    if (!node_element_parser(in, nodes, elems, node_id_map, mesh_prop))
    {
        ERR(...);
        return nullptr;
    }
    return std::make_unique<MeshLib::Mesh>(mesh_name, ...);
};

And in the conditions:

if (type == TFACE)
{
    return parse_mesh(parseSurface);
}
if (type == PLINE)
{
    return parse_mesh(parseLine);
}

This comment has been minimized.

Copy link
@rinkk

rinkk Aug 6, 2019

Author Member

Honestly, I have no idea what happens there. And I know that it's difficult to add cases now, that's why it was written before such that cases were independent of each other.

This comment has been minimized.

Copy link
@rinkk

rinkk Aug 6, 2019

Author Member

To be more specific, I'd very much prefer the code the way it was originally written. That'd be six lines of duplicate code, which isn't a big issue in my opinion if it means the code is easier to read.

std::map<std::size_t, std::size_t> const& node_id_map,
MeshLib::Properties& mesh_prop)
{
std::string keyword;

This comment has been minimized.

Copy link
@endJunction

endJunction Aug 1, 2019

Member

Please constrain variables to minimum possible scope.

This comment has been minimized.

Copy link
@rinkk

rinkk Aug 2, 2019

Author Member

✔️

This comment has been minimized.

Copy link
@endJunction

endJunction Aug 2, 2019

Member

Same goes for data array.

This comment has been minimized.

Copy link
@rinkk

rinkk Aug 6, 2019

Author Member

✔️

@endJunction

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

clang-format

WARN(
"GocadTSurfaceReader::parseNodes() - Unknown keyword found: %s",
line.c_str());
WARN("GocadTSurfaceReader::parseNodes() - Unknown keyword found: %s", line.c_str());

This comment has been minimized.

Copy link
@TomFischer

TomFischer Aug 2, 2019

Member

GocadTSurfaceReader -> GocadAsciiReader?

This comment has been minimized.

Copy link
@rinkk

rinkk Aug 2, 2019

Author Member

✔️

}
if (line.substr(0, 13) == "GOCAD Model3d")
else if (line.substr(0, 11) == "GOCAD PLine")

This comment has been minimized.

Copy link
@endJunction

endJunction Aug 2, 2019

Member

I'd move this string to enum conversion to the place where the enum is defined.

Also no else after return is needed.

This comment has been minimized.

Copy link
@rinkk

rinkk Aug 6, 2019

Author Member

Do you suggest that I should implement a enum -> string conversion for this one use?

The return does not follow "else" but "while" and is required in case the end of the file is reached.

GocadAsciiReader::GocadAsciiReader()
: _export_type(GocadDataType::ALL) {}

GocadAsciiReader::GocadAsciiReader(GocadDataType const t)

This comment has been minimized.

Copy link
@endJunction

endJunction Aug 2, 2019

Member
Suggested change
GocadAsciiReader::GocadAsciiReader(GocadDataType const t)
GocadAsciiReader::GocadAsciiReader(GocadDataType const t = GocadDataType::ALL)

... then the default ctor is not needed.
(Sorry, wrong place, goes into the header file)

This comment has been minimized.

Copy link
@rinkk

rinkk Aug 6, 2019

Author Member

✔️

if (!skipToEND(in))
{
std::string const t = (type == GocadDataType::VSET) ? "VSet" : "Model3D";
ERR("Parsing of type %s is not implemented. Skipping section.", t);

This comment has been minimized.

Copy link
@endJunction

endJunction Aug 2, 2019

Member
Suggested change
ERR("Parsing of type %s is not implemented. Skipping section.", t);
ERR("Parsing of type %s is not implemented. Skipping section.", t.c_str());

This comment has been minimized.

Copy link
@rinkk

rinkk Aug 6, 2019

Author Member

✔️

@rinkk rinkk force-pushed the rinkk:GocadExpansion branch from 36c2af3 to f28823b Aug 7, 2019

@TomFischer

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

The file GocadEnums.h is missing.

GocadAsciiReader.h:15:10: fatal error: 'Applications/FileIO/GocadIO/GocadEnums.h' file not found
};

/// Given a Gocad DataType this returns the appropriate string.
std::string DataType2Str(DataType const t);

This comment has been minimized.

Copy link
@endJunction

endJunction Aug 11, 2019

Member
Suggested change
std::string DataType2Str(DataType const t);
std::string dataType2String(DataType const t);
{
return "model";
}
return "[all data]";

This comment has been minimized.

Copy link
@endJunction

endJunction Aug 11, 2019

Member

all data vs. all types above. Maybe wrong.

@endJunction
Copy link
Member

left a comment

@TomFischer when OK, merge.

@TomFischer TomFischer force-pushed the rinkk:GocadExpansion branch from a50fcd9 to 701de2c Aug 12, 2019

@TomFischer TomFischer merged commit 1eb6d3c into ufz:master Aug 12, 2019

3 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
deploy/netlify Deploy preview ready!
Details
ufz.ogs #20190812.1 succeeded
Details

@rinkk rinkk deleted the rinkk:GocadExpansion branch Aug 12, 2019

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.