Raster data to mesh #2984

rinkk · 2020-06-05T10:05:52Z

Adds a new node or cell array to a mesh based on specified raster.

Feature description was added to the changelog
Tests covering your feature were added?

Tests/MeshLib/TestRasterDataToMesh.cpp

endJunction · 2020-06-05T10:26:01Z

MeshLib/MeshEditing/RasterDataToMesh.h

+
+bool projectToElements(MeshLib::Mesh& mesh, GeoLib::Raster const& raster,
+                       double const default, std::string const& array_name);
+};


Suggested change

};

} // end namespace RasterDataToMesh

endJunction · 2020-06-05T10:28:52Z

Applications/DataExplorer/DataView/RasterDataToMeshDialog.cpp

+        OGSError::box("Please specify path to raster file.");
+        return;
+    }
+    else if (this->arrayNameEdit->text().isEmpty())


No else after a return needed. Will be a warning in clang-tidy.

Suggested change

else if (this->arrayNameEdit->text().isEmpty())

if (this->arrayNameEdit->text().isEmpty())

This is correct the way it is. Otherwise the user drowns in error pop-ups.

After OGSError::box is displayed, there is a return. Therefor the other if-conditions will not be checked and the user is presented with single error box.

Exactly. This is the way it is supposed to work.

endJunction · 2020-06-05T10:31:50Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+namespace RasterDataToMesh
+{
+
+bool checkMesh2D(MeshLib::Mesh const& mesh)


I'd prefer all functions not exposed in the interface to have internal linkage. Either static or anonymous namespace.
Makes life of a linker easier.

endJunction · 2020-06-05T10:32:21Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+    return true;
+}
+std::string getValidName(std::vector<std::string> const& vec_names,
+                     std::string const& array_name)


clang-format. Also few other places.

endJunction · 2020-06-05T10:35:50Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+    bool name_exists = std::find(vec_names.cbegin(), vec_names.cend(),
+                                 array_name) != vec_names.end();
+    while (name_exists)
+    {
+        count++;
+        new_name = array_name + std::to_string(count);
+        name_exists = std::find(vec_names.cbegin(), vec_names.cend(),
+                                array_name) != vec_names.end();
+    }


I think this would be equivalent:

Suggested change

bool name_exists = std::find(vec_names.cbegin(), vec_names.cend(),

array_name) != vec_names.end();

while (name_exists)

{

count++;

new_name = array_name + std::to_string(count);

name_exists = std::find(vec_names.cbegin(), vec_names.cend(),

array_name) != vec_names.end();

}

while (std::find(vec_names.cbegin(), vec_names.cend(), array_name)

!= vec_names.end())

{

count++;

new_name = array_name + std::to_string(count);

}

endJunction · 2020-06-05T10:37:33Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+    return new_name;
+}
+
+double evaluatePixel(double value, double no_data, double replacement)


Suggested change

double evaluatePixel(double value, double no_data, double replacement)

double evaluatePixel(double const value, double const no_data, double const replacement)

endJunction · 2020-06-05T10:41:30Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+    for (auto node : nodes)
+    {
+        vec->push_back(
+            evaluatePixel(raster.getValueAtPoint(*node), no_data, default));
+    }


Suggested change

for (auto node : nodes)

{

vec->push_back(

evaluatePixel(raster.getValueAtPoint(*node), no_data, default));

}

transform(cbegin(nodes), cend(nodes), back_inserter(*vec), [&](auto const node)

{

return evaluatePixel(raster.getValueAtPoint(*node), no_data, default));

}

Adding

std::transform(nodes.cbegin(), nodes.cend(), std::back_inserter(vec), [&](auto const node){ evaluatePixel(raster.getValueAtPoint(*node), no_data, default); });

results in

\MSVC\14.25.28610\include\xutility(5302,1): error C2825: "_Container": Muss eine Klasse oder ein Namespace sein, wenn gefolgt von "::". /ogs/MeshLib/MeshEditing/RasterDataToMesh.cpp(67): message : Siehe Verweis auf die gerade kompilierte Klasse Vorlage-Instanziierung "std::back_insert_iterator<MeshLib::PropertyVector<double> *>". \MSVC\14.25.28610\include\xutility(5302,64): error C2510: "_Container": Der linke Teil von "::" muss eine Klasse/Struktur/Union sein \MSVC\14.25.28610\include\xutility(5302,1): error C2182: "_Val": Unzulässige Verwendung des Typs "void" \MSVC\14.25.28610\include\xutility(5307,1): error C2825: "_Container": Muss eine Klasse oder ein Namespace sein, wenn gefolgt von "::". \MSVC\14.25.28610\include\xutility(5307,58): error C2510: "_Container": Der linke Teil von "::" muss eine Klasse/Struktur/Union sein \MSVC\14.25.28610\include\xutility(5307,1): error C2182: "_Val": Unzulässige Verwendung des Typs "void"

Since vec is a pointer to a PropertyVector, std::back_inserter(*vec) should work.

Thanks Tom, I fixed above in the suggestion too.

Thanks, Tom!

✔️

endJunction · 2020-06-05T10:42:06Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+    auto vec = props.createNewPropertyVector<double>(
+        name, MeshLib::MeshItemType::Cell, 1);
+    double const no_data = raster.getHeader().no_data;
+    for (auto elem : elems)


transform here too.

endJunction

minor corrections. ⏩

endJunction · 2020-06-05T10:47:06Z

For namespace error in vtk take Wenqing's commit: 'git cherry-pick c596c46'

TomFischer · 2020-06-09T04:12:56Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+        ERR("This functionality is currently only available for 2D meshes.");
+        return false;
+    }
+    return true;


Why not testing mesh.getDimension() == 2? The function name lets one expect that only with 2d grids true is returned. At the moment for 1d meshes the function returns also true.

Yes, this works for 1D meshes, too. So >=2 is correct.

When the check returns true for 1d and 2d meshes the name of the function should be changed.

TomFischer · 2020-06-09T04:14:43Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+}
+
+bool projectToNodes(MeshLib::Mesh& mesh, GeoLib::Raster const& raster,
+                    double const default, std::string const& array_name)


Since default is a keyword it should not be used as a variable name. At least renaming default -> def fixed compile errors for me.

Didn't cause any issues with MSVC. Anyway, I've changed the name. ✔️

... though default_value or whatever default there is default_status default_future_multiplier would be better and would save explanation of the function arguments in doxygen comments.

bump.
Rename or documentation would improve the code.
From the function signature it is not clear, what that def thingy is.

TomFischer · 2020-06-09T04:16:37Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+}
+
+bool projectToElements(MeshLib::Mesh& mesh, GeoLib::Raster const& raster,
+                       double const default, std::string const& array_name)


Same as above: default doesn't seem to be a good variable name.

TomFischer · 2020-06-09T07:08:11Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+                    double const default, std::string const& array_name)
+{
+    if (!checkMesh2D(mesh))
+        return false;


This gives a new warning: statement should be inside braces [google-readability-braces-around-statements]

Suggested change

return false;

{

return false;

}

TomFischer · 2020-06-09T07:09:13Z

MeshLib/MeshEditing/RasterDataToMesh.cpp

+static double evaluatePixel(double value, double no_data, double replacement)
+{
+    if (std::abs(value - no_data) < std::numeric_limits<double>::epsilon())
+        return replacement;


This gives a new warning: statement should be inside braces [google-readability-braces-around-statements]

Suggested change

return replacement;

{

return replacement;

}

TomFischer

When tests are green: ⏩

ogsbot · 2020-06-19T13:25:10Z

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

rinkk force-pushed the RasterDataToMesh branch from 7245b7c to 5016cfe Compare June 5, 2020 10:21

endJunction reviewed Jun 5, 2020

View reviewed changes

Tests/MeshLib/TestRasterDataToMesh.cpp Show resolved Hide resolved

endJunction reviewed Jun 5, 2020

View reviewed changes

endJunction approved these changes Jun 5, 2020

View reviewed changes

TomFischer reviewed Jun 9, 2020

View reviewed changes

rinkk added 6 commits June 9, 2020 11:16

[meshlib] creating mesh arrays based on raster data

41ef522

[gui] dialog to use RasterDataToMesh from GUI

656c1d0

[gui] making RasterDataToMeshDialog accessible from GUI

7e449da

[meshlib] adding test for RasterDataToMesh

19e229d

fixed doxygen tags and linkage

1a2e8d0

[meshlib] replaced/optimised loops for RasterDataToMesh

04980c7

rinkk force-pushed the RasterDataToMesh branch from 50cdfd2 to 04980c7 Compare June 9, 2020 09:19

wenqing and others added 2 commits June 9, 2020 11:41

[MeshLib] Resolved the namespace conflict with VTK

228a481

[meshlib] renaming functions and vars in RasterDataToMesh

19accca

TomFischer approved these changes Jun 9, 2020

View reviewed changes

TomFischer merged commit 75e89e9 into ufz:master Jun 10, 2020

rinkk deleted the RasterDataToMesh branch July 1, 2020 09:55

	else if (this->arrayNameEdit->text().isEmpty())
	if (this->arrayNameEdit->text().isEmpty())

	double evaluatePixel(double value, double no_data, double replacement)
	double evaluatePixel(double const value, double const no_data, double const replacement)

Raster data to mesh #2984

Raster data to mesh #2984

Conversation

rinkk commented Jun 5, 2020 • edited by bilke 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

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 Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFischer Jun 9, 2020 • 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

endJunction left a comment

Choose a reason for hiding this comment

endJunction commented Jun 5, 2020

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 Jun 9, 2020 • 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 Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

TomFischer Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

TomFischer left a comment

Choose a reason for hiding this comment

ogsbot commented Jun 19, 2020

rinkk commented Jun 5, 2020 •

edited by bilke

Loading

endJunction Jun 5, 2020 •

edited

Loading

TomFischer Jun 9, 2020 •

edited

Loading

endJunction Jun 9, 2020 •

edited

Loading

TomFischer Jun 9, 2020 •

edited

Loading

TomFischer Jun 9, 2020 •

edited

Loading