Expansion of GoCAD interface #2586

rinkk · 2019-08-01T11:55:17Z

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.

Applications/FileIO/GocadIO/GocadAsciiReader.h

endJunction · 2019-08-01T13:17:57Z

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

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


else after a return is not needed...

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

endJunction · 2019-08-01T13:25:53Z

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

@@ -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 is copy of the following if for TSURF. Please avoid code duplications.

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); }

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.

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.

endJunction · 2019-08-01T13:27:32Z

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

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


Please constrain variables to minimum possible scope.

Same goes for data array.

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

Applications/FileIO/GocadIO/GocadAsciiReader.h

Applications/Utils/FileConverter/GocadTSurfaceReader.cpp

endJunction · 2019-08-01T13:34:41Z

clang-format

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

TomFischer · 2019-08-02T07:25:40Z

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

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


GocadTSurfaceReader -> GocadAsciiReader?

Applications/FileIO/GocadIO/GocadAsciiReader.h

endJunction · 2019-08-02T15:24:03Z

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

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


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

Also no else after return is needed.

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.

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

endJunction · 2019-08-02T15:46:03Z

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

+GocadAsciiReader::GocadAsciiReader()
+: _export_type(GocadDataType::ALL) {}
+
+GocadAsciiReader::GocadAsciiReader(GocadDataType const t)


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)

endJunction · 2019-08-02T16:03:39Z

Applications/FileIO/GocadIO/GocadAsciiReader.cpp

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


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());

TomFischer · 2019-08-08T09:01:40Z

The file GocadEnums.h is missing.

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

endJunction · 2019-08-11T10:31:56Z

Applications/FileIO/GocadIO/GocadEnums.h

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


Suggested change

std::string DataType2Str(DataType const t);

std::string dataType2String(DataType const t);

endJunction · 2019-08-11T10:34:57Z

Applications/FileIO/GocadIO/GocadEnums.cpp

+    {
+        return "model";
+    }
+    return "[all data]";


all data vs. all types above. Maybe wrong.

endJunction

@TomFischer when OK, merge.

ogsbot · 2020-06-19T13:03:22Z

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

endJunction reviewed Aug 1, 2019

View reviewed changes

Applications/FileIO/GocadIO/GocadAsciiReader.h Outdated Show resolved Hide resolved

endJunction reviewed Aug 1, 2019

View reviewed changes

Applications/FileIO/GocadIO/GocadAsciiReader.cpp Outdated Show resolved Hide resolved

endJunction reviewed Aug 1, 2019

View reviewed changes

Applications/FileIO/GocadIO/GocadAsciiReader.cpp Outdated Show resolved Hide resolved

endJunction reviewed Aug 1, 2019

View reviewed changes

Applications/FileIO/GocadIO/GocadAsciiReader.h Outdated Show resolved Hide resolved

endJunction reviewed Aug 1, 2019

View reviewed changes

Applications/FileIO/GocadIO/GocadAsciiReader.h Outdated Show resolved Hide resolved

endJunction reviewed Aug 1, 2019

View reviewed changes

Applications/Utils/FileConverter/GocadTSurfaceReader.cpp Show resolved Hide resolved

TomFischer reviewed Aug 2, 2019

View reviewed changes

Applications/FileIO/GocadIO/GocadAsciiReader.cpp Outdated Show resolved Hide resolved

TomFischer reviewed Aug 2, 2019

View reviewed changes

Applications/FileIO/GocadIO/GocadAsciiReader.h Outdated Show resolved Hide resolved

endJunction reviewed Aug 2, 2019

View reviewed changes

Applications/FileIO/GocadIO/GocadAsciiReader.cpp Show resolved Hide resolved

endJunction reviewed Aug 2, 2019

View reviewed changes

rinkk force-pushed the GocadExpansion branch from 36c2af3 to f28823b Compare August 7, 2019 16:06

endJunction reviewed Aug 11, 2019

View reviewed changes

endJunction approved these changes Aug 11, 2019

View reviewed changes

rinkk added 8 commits August 12, 2019 06:18

added support for GoCAD PLine data, checking for VSet and Model3d

189fcd0

added flag that allows to read lines or surfaces only from a file

e984d7e

added check to handle identically named meshes and add a unique id

7f894f4

fixing documentation and output

63d7fd8

renaming enum classes

8bf1634

using ogs-mesh-counter instead of boost uuid

e1d5298

streamlining code

271b11c

adjusting variable scope and default parameters

10eb959

rinkk and others added 7 commits August 12, 2019 06:18

minimising external interface

5d69d7d

added missing enum-definitions

31a12c5

[A/GocadIO] Include header for std::vector.

291a6ab

[A/GocadIO] Remove unnecessary namespace.

ec2996b

added type flag in conversion utility

8ccb231

[A/FileIO/Gocad] Rename functions according to style guide.

1d2acb9

[A/FileIO/Gocad] Clang fmt. Enums & AsciiReader.

701de2c

TomFischer force-pushed the GocadExpansion branch from a50fcd9 to 701de2c Compare August 12, 2019 04:28

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

rinkk deleted the GocadExpansion branch August 12, 2019 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expansion of GoCAD interface #2586

Expansion of GoCAD interface #2586

rinkk commented Aug 1, 2019

endJunction Aug 1, 2019

rinkk Aug 2, 2019

endJunction Aug 1, 2019

rinkk Aug 2, 2019

endJunction Aug 2, 2019

rinkk Aug 6, 2019

rinkk Aug 6, 2019

endJunction Aug 1, 2019

rinkk Aug 2, 2019

endJunction Aug 2, 2019

rinkk Aug 6, 2019

endJunction commented Aug 1, 2019

TomFischer Aug 2, 2019 •

edited

Loading

rinkk Aug 2, 2019

endJunction Aug 2, 2019

rinkk Aug 6, 2019

endJunction Aug 2, 2019 •

edited

Loading

rinkk Aug 6, 2019

endJunction Aug 2, 2019

rinkk Aug 6, 2019

TomFischer commented Aug 8, 2019

endJunction Aug 11, 2019

endJunction Aug 11, 2019

endJunction left a comment

ogsbot commented Jun 19, 2020

	GocadAsciiReader::GocadAsciiReader(GocadDataType const t)
	GocadAsciiReader::GocadAsciiReader(GocadDataType const t = GocadDataType::ALL)

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

	std::string DataType2Str(DataType const t);
	std::string dataType2String(DataType const t);

Expansion of GoCAD interface #2586

Expansion of GoCAD interface #2586

Conversation

rinkk commented Aug 1, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endJunction commented Aug 1, 2019

TomFischer Aug 2, 2019 • 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

endJunction Aug 2, 2019 • 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

TomFischer commented Aug 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endJunction left a comment

Choose a reason for hiding this comment

ogsbot commented Jun 19, 2020

TomFischer Aug 2, 2019 •

edited

Loading

endJunction Aug 2, 2019 •

edited

Loading