Updated the osm-simplify tool to work on the coastline shapefile provided by the openstreetmap database.
ClosedPublic

Authored by dkolozsvari on Jun 25 2016, 7:46 PM.

Details

Summary
Test Plan

Still has some issues, like some polygons are rendered as linestrings inside Marble.

Diff Detail

Repository
R34 Marble
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dkolozsvari updated this revision to Diff 4732.Jun 25 2016, 7:46 PM
dkolozsvari retitled this revision from to Updated the osm-simplify tool to work on the coastline shapefile provided by the openstreetmap database..
dkolozsvari updated this object.
dkolozsvari edited the test plan for this revision. (Show Details)
dkolozsvari added reviewers: nienhueser, rahn.
dkolozsvari set the repository for this revision to R34 Marble.
dkolozsvari added a project: Marble.
dkolozsvari added a subscriber: Marble.
nienhueser added inline comments.Jun 25 2016, 9:26 PM
tools/osm-simplify/BaseClipper.cpp
86

should use a const reference for the parameter

106

Sign of divisor is discarded here, is that ok?

tools/osm-simplify/BaseClipper.h
2

please add copyright headers with the usual license bla and your name to each file

tools/osm-simplify/CMakeLists.txt
1

let's try 2.8.12 instead

tools/osm-simplify/LineStringProcessor.cpp
15

the code assumes that m_objects contains only placemarks in various places. It's better to reflect that in m_objects and change it from QList<GeoDataObject*> to QList<GeoDataPlacemark*>. If that violates the universal use of filters, introduce a convenience method for it.

I'd refactor it to have a private member QList<GeoDataPlacemark*> in LineStringProcessor that gets populated with document->placemarkList() in the constructor. I'd also store the document pointer in a private member variable, in my opinion protected member variables are a no-go.

nienhueser added inline comments.Jun 26 2016, 9:03 AM
tools/osm-simplify/main.cpp
150

Please update the usage instructions above to reflect the additional .shp support, e.g. using

parser.addPositionalArgument("input", "The input .osm or .shp file.");

nienhueser added inline comments.Jun 26 2016, 9:16 AM
tools/osm-simplify/ShpRunner.cpp
210 ↗(On Diff #4732)

This section seems to be the only difference to Marble's ShpRunner. I'd suggest to use that directly (like OsmRunner is included) and then postprocess the file (the resulting GeoDataDocument) and add the additional tag there. This avoids the copy duplication.

If you do not use any plugin internal API, you can also avoid compiling the runner at all. Just use Marble's parsing API:

MarbleModel model;
ParsingRunnerManager manager(model.pluginManager());
GeoDataDocument* document = manager.openFile(inputFileName);
dkolozsvari marked 6 inline comments as done.Jun 26 2016, 6:28 PM

I corrected most of the issues and little changes pointed out by Dennis.

dkolozsvari updated this revision to Diff 4752.Jun 26 2016, 6:44 PM

Update the diff.

nienhueser added inline comments.Jul 2 2016, 8:56 AM
tools/osm-simplify/BaseClipper.h
9

IIRC BaseClipper shares a lot of code with ClipPainter. In such cases please add the original authors as well (just copy all Copyright... lines from the file and put them on top this one)

nienhueser added inline comments.Jul 2 2016, 10:22 AM
tools/osm-simplify/ShpCoastlineProcessor.cpp
85

Manual polygon closing should not be needed anymore with http://commits.kde.org/marble/39fb2c303bc182d1d513c453c6233e5d1a341635

nienhueser accepted this revision.Jul 4 2016, 6:11 PM
nienhueser edited edge metadata.

I'll push this on your behalf and fix the two comments I had so Akshat can commit patches which depend on this one.

This revision is now accepted and ready to land.Jul 4 2016, 6:11 PM
This revision was automatically updated to reflect the committed changes.