Differential D2074 Diff 4937 tools/osm-simplify/NodeReducer.cpp

Changeset View

Standalone View

View Options

tools/osm-simplify/NodeReducer.cpp

  • This file was added.
1​//
2​// This file is part of the Marble Virtual Globe.
3​//
4​// This program is free software licensed under the GNU LGPL. You can
5​// find a copy of this license in LICENSE.txt in the top directory of
6​// the source code.
7​//
8​// Copyright 2016 Akshat Tandon <akshat.tandon@research.iiit.ac.in>
9​//
10​
11​#include "BaseClipper.h"
12​#include "GeoDataPlacemark.h"
13​#include "GeoDataTypes.h"
14​#include "GeoDataLineString.h"
15​#include "GeoDataCoordinates.h"
16​#include "MarbleMath.h"
17​#include "NodeReducer.h"
18​
19​#include <QDebug>
20​#include <QVector>
21​
22​NodeReducer::NodeReducer(GeoDataDocument* document, int zoomLevel) :
23​ PlacemarkFilter(document)
24​{
25​ m_resolution = resolutionForLevel(zoomLevel);
26​ m_count = 0;
27​}
28​
29​void NodeReducer::process(){
30​ foreach (GeoDataObject* object, m_objects) {
31​ GeoDataPlacemark* placemark = static_cast<GeoDataPlacemark*>(object);
32​ if(placemark->geometry()->nodeType() == GeoDataTypes::GeoDataLineStringType)
nienhueserUnsubmitted
Done

Imho having uninitialized variables at the start of the function is old-school and rather dangerous. Better just move them to where they are first used and initialize them there directly.

nienhueser: Imho having uninitialized variables at the start of the function is old-school and rather…
33​ reduce(static_cast<GeoDataLineString*>(placemark->geometry()) );
34​ else if(placemark->geometry()->nodeType() == GeoDataTypes::GeoDataLinearRingType)
35​ reduce(static_cast<GeoDataLinearRing*>(placemark->geometry()) );
36​ else if(placemark->geometry()->nodeType() == GeoDataTypes::GeoDataPolygonType){
37​ GeoDataPolygon *polygon = static_cast<GeoDataPolygon*>(placemark->geometry());
38​ GeoDataLinearRing &outerBoundary = polygon->outerBoundary();
39​ reduce(&outerBoundary);
40​ QVector<GeoDataLinearRing>& innerBoundaries = polygon->innerBoundaries();
41​ for(int i = 0; i < innerBoundaries.size(); i++){
nienhueserUnsubmitted
Not Done

m_objects should either just store GeoDataPlacemark or you need a sanity check on nodeType() here before doing the static cast.

nienhueser: m_objects should either just store GeoDataPlacemark or you need a sanity check on nodeType()…
tandonAuthorUnsubmitted
Not Done

I have made NodeReducer a sub-class of PlacemarkFilter which guarantees that m_object only contains GeoDataPlacemarks. Do I still need a check on nodeType() ?

tandon: I have made NodeReducer a sub-class of PlacemarkFilter which guarantees that m_object only…
42​ reduce(&innerBoundaries[i]);
43​ }
44​ }
rahnUnsubmitted
Not Done

There are two exceptions that we'd need to make here when it comes to erasing:

  1. The most important exception is that Starting and End-point need to be preserved!

Your current implementation doesn't necessarily keep the end-point which needs to be changed.
Can you fix this? :)

  1. In theory also street junctions should be excluded from erasing to ensure proper rendering:

Street Junctions require a shared point between both involved streets. So these points should be preserved. However this has two problems involved: on one hand a performance problem: the continous comparisons (even when working with comparisons against start/end points in the current latLonBox) would cause a significant performance overhead. and on the other hand the current implementation inside GeoDataLineString would override any such precautions.
So this second issue isn't easy to fix - and it probably doesn't occur frequently - so let's ignore this second problem for now (but keep it on the radar in case it becomes a real issue).

rahn: There are two exceptions that we'd need to make here when it comes to erasing: 1. The most…
45​ }
rahnUnsubmitted
Done

I guess right now resolutionForLevel() gets evaluated for each iteration. Instead we should probably store it in a const variable.

rahn: I guess right now resolutionForLevel() gets evaluated for each iteration. Instead we should…
46​ qDebug()<<"Total nodes reduced: "<<m_count<<endl;
nienhueserUnsubmitted
Done

Erase will run in O(n), see http://doc.qt.io/qt-5/containers.html#algorithmic-complexity and this will slow down things considerably for large input. I wonder if we can avoid that. Does not need to be part of this patch, but lets keep it in mind for a next task.

nienhueser: Erase will run in O(n), see http://doc.qt.io/qt-5/containers.html#algorithmic-complexity and…
rahnUnsubmitted
Done

Can't we just rebuild and reassign the linestring? I guess that would be faster.

rahn: Can't we just rebuild and reassign the linestring? I guess that would be faster.
nienhueserUnsubmitted
Done

Is that check useful? To me it seems to make no difference except that it slows down execution (an equality check on equal linestrings has to compare every coordinate in each of them).

nienhueser: Is that check useful? To me it seems to make no difference except that it slows down execution…
tandonAuthorUnsubmitted
Not Done

The NodeReducer::reduce() method was returning the input pointer in case of line-strings having less than 2 points. When placemark->setGeometry(..) is called, it deletes the previous geometry which would have been harmful if both the previous geometry and the geometry which we are going to set are same. Hence I had included the check.
However now I have modified reduce() to always return pointer of a new object thus making the check useless.

tandon: The NodeReducer::reduce() method was returning the input pointer in case of line-strings having…
47​}
48​
49​void NodeReducer::reduce(GeoDataLineString* lineString){
50​ qint64 prevSize = lineString->size();
51​ if(prevSize < 2)
52​ return;
53​ QVector<GeoDataCoordinates>::iterator itCoords = lineString->begin();
nienhueserUnsubmitted
Done

see above

nienhueser: see above
54​ GeoDataCoordinates currentCoords = *itCoords;
55​ ++itCoords;
tandonAuthorUnsubmitted
Done

I have copied the resolutionForLevel() from GeoDataLineString class since it was private; should I make the GeoDataLineString method public and static and remove the one below ?

tandon: I have copied the resolutionForLevel() from GeoDataLineString class since it was private…
rahnUnsubmitted
Done

I don't think that this should be part of the public API. Right now it's just part of the GeoDataLineString for reasons of convenience. After all the level<->resolution mapping is not something that would be a natural property of a linestring. I think it should go elsewhere (possibly into the viewport/tile/projection classes.
BTW: There is also an "antagonist" levelForResolution as part of the AbstractProjectionPrivate API.

For now I think we can live with a copy until we have a clearer idea where to put this.

rahn: I don't think that this should be part of the public API. Right now it's just part of the…
56​ while(itCoords != (lineString->end() - 1)){
57​ if(distanceSphere( currentCoords, *itCoords ) < m_resolution){
58​ lineString->erase(itCoords);
59​ }else{
60​ currentCoords = *itCoords;
61​ ++itCoords;
62​ }
63​ }
64​ qint64 reducedSize = lineString->size();
65​ m_count += (prevSize - reducedSize);
66​ //qDebug()<<"Nodes reduced "<<(prevSize - reducedSize)<<endl;
67​}
68​
69​qreal NodeReducer::resolutionForLevel(int level) {
70​ switch (level) {
71​ case 0:
72​ return 0.0655360;
73​ break;
74​ case 1:
75​ return 0.0327680;
76​ break;
77​ case 2:
78​ return 0.0163840;
79​ break;
80​ case 3:
81​ return 0.0081920;
82​ break;
83​ case 4:
84​ return 0.0040960;
85​ break;
86​ case 5:
87​ return 0.0020480;
88​ break;
89​ case 6:
90​ return 0.0010240;
91​ break;
92​ case 7:
93​ return 0.0005120;
94​ break;
95​ case 8:
96​ return 0.0002560;
97​ break;
98​ case 9:
99​ return 0.0001280;
100​ break;
101​ case 10:
102​ return 0.0000640;
103​ break;
104​ case 11:
105​ return 0.0000320;
106​ break;
107​ case 12:
108​ return 0.0000160;
109​ break;
110​ case 13:
111​ return 0.0000080;
112​ break;
113​ case 14:
114​ return 0.0000040;
115​ break;
116​ case 15:
117​ return 0.0000020;
118​ break;
119​ case 16:
120​ return 0.0000010;
121​ break;
122​ default:
123​ case 17:
124​ return 0.0000005;
125​ break;
126​ }
127​}
128​