Differential D2128 Diff 5313 tools/osm-simplify/WayChunk.cpp

Changeset View

Standalone View

View Options

tools/osm-simplify/WayChunk.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 <QList>
12​#include <QVector>
13​#include <QDebug>
14​
15​#include "WayChunk.h"
16​#include "GeoDataCoordinates.h"
17​#include "GeoDataFeature.h"
18​#include "GeoDataPlacemark.h"
19​#include "GeoDataLineString.h"
20​#include "OsmPlacemarkData.h"
21​
22​namespace Marble
23​{
24​
25​WayChunk::WayChunk(GeoDataPlacemark *placemark, qint64 first, qint64 last)
26​{
27​ m_wayList.append(placemark);
28​ m_first = first;
29​ m_last = last;
nienhueserUnsubmitted
Done

Some streets are not tagged with a name, but instead using ref. See e.g. https://www.openstreetmap.org/edit#map=17/48.92478/8.27570 which corresponds to the highway rendered in orange near Durmersheim.

nienhueser: Some streets are not tagged with a name, but instead using ref. See e.g. https://www.
30​ m_visualCategory = placemark->visualCategory();
31​}
32​
33​WayChunk::~WayChunk()
34​{
35​
36​}
37​
38​qint64 WayChunk::first() const
39​{
40​ return m_first;
41​}
42​
43​qint64 WayChunk::last() const
44​{
45​ return m_last;
46​}
47​
48​void WayChunk::append(GeoDataPlacemark *placemark, qint64 last)
49​{
50​ m_wayList.append(placemark);
51​ m_last = last;
52​
53​}
54​
55​void WayChunk::prepend(GeoDataPlacemark *placemark, qint64 first)
56​{
57​ m_wayList.prepend(placemark);
58​ m_first = first;
nienhueserUnsubmitted
Done

Would this implementation work as well?

m_wayList << chunk->m_wayList;
m_last = last;

Sparing the last parameter seems reasoanble also, i.e. just do

void WayChunk::append(WayChunk* chunk)
{
  m_wayList << chunk->m_wayList;
  m_last = chunk->last();
}
nienhueser: Would this implementation work as well? m_wayList << chunk->m_wayList; m_last = last…
59​
60​}
61​
62​void WayChunk::append(WayChunk *chunk)
63​{
64​ m_wayList << chunk->m_wayList;
65​ m_last = chunk->last();
66​}
nienhueserUnsubmitted
Done

Is this method used anywhere?

nienhueser: Is this method used anywhere?
tandonAuthorUnsubmitted
Not Done

I anticipated that I might need such a method in the future but eventually it was not required, hence I have removed it for now.

tandon: I anticipated that I might need such a method in the future but eventually it was not required…
67​
68​GeoDataPlacemark* WayChunk::merge()
69​{
70​ Q_ASSERT(!m_wayList.isEmpty());
71​
nienhueserUnsubmitted
Done

I think a

Q_ASSERT(!m_wayList.isEmpty());

is fine also because you pass a placemark in the ctor that ends up in m_wayList. id() below also assumes that m_wayList cannot be empty.

nienhueser: I think a Q_ASSERT(!m_wayList.isEmpty()); is fine also because you pass a placemark in the…
72​ GeoDataPlacemark *placemark = new GeoDataPlacemark(*(m_wayList.first()));
73​ GeoDataLineString *line = static_cast<GeoDataLineString*>(placemark->geometry());
74​ QList<GeoDataPlacemark*>::iterator itr = m_wayList.begin();
75​ QList<GeoDataPlacemark*>::iterator itrEnd = m_wayList.end();
76​ ++itr;
77​ for (; itr != itrEnd; ++itr) {
78​ GeoDataLineString *currentLine = static_cast<GeoDataLineString*>( (*itr)->geometry() );
nienhueserUnsubmitted
Done

Here you return a placemark from m_wayList, while below you'd return a newly created. Doesn't that result in an unsolvable (or annoying to solve) memory management problem which results in either memory leaks or double deletions?

nienhueser: Here you return a placemark from m_wayList, while below you'd return a newly created. Doesn't…
tandonAuthorUnsubmitted
Not Done

I have rectified this issue by always creating a new placemark object.
Also, I googled a bit and found that the correct way of returning a new object from a member function is to return via a smart pointer (ex QSharedPointer), however , as of now, this method is called from only one place and also since using a shared pointer gives a very little performance hit, I decided on using only a regular pointer.

tandon: I have rectified this issue by always creating a new placemark object. Also, I googled a bit…
79​ currentLine->remove(0);
nienhueserUnsubmitted
Done

nullptr

nienhueser: nullptr
80​ (*line) << *currentLine;
81​ }
82​ //qDebug()<<"Merging placemark";
83​ return placemark;
84​}
85​
86​void WayChunk::reverse()
87​{
88​ std::reverse(m_wayList.begin(), m_wayList.end());
89​ QList<GeoDataPlacemark*>::iterator itr = m_wayList.begin();
90​ for (; itr != m_wayList.end(); ++itr) {
91​ GeoDataPlacemark *placemark = *itr;
92​ GeoDataLineString *line = static_cast<GeoDataLineString*>(placemark->geometry());
93​ line->reverse();
94​ }
95​ qSwap(m_first, m_last);
96​}
97​
98​qint64 WayChunk::id() const
99​{
100​ return m_wayList.first()->osmData().id();
101​}
102​
nienhueserUnsubmitted
Done

Please move into the loop, same below. Same for a couple more places in the code.

nienhueser: Please move into the loop, same below. Same for a couple more places in the code.
103​void WayChunk::printIds() const
104​{
105​ QList<GeoDataPlacemark*>::const_iterator itr = m_wayList.begin();
106​ qDebug()<<"IDs of placemarks in chunk";
107​ for (; itr != m_wayList.end(); ++itr) {
108​ qDebug()<<"Id :- "<<(*itr)->osmData().id();
109​ }
nienhueserUnsubmitted
Done

Please replace with

qSwap(m_first, m_last);
nienhueser: Please replace with qSwap(m_first, m_last);
110​}
111​
nienhueserUnsubmitted
Done

This seems pretty inefficient. The standard approach for reversing strings works in place by swapping all elements from start/end towards the middle.

I'd go for adding a reverse() method to GeoDataLineString which internally calls std::reverse on d->m_vector like shown in http://stackoverflow.com/a/1340291/1802551 and also clears/adjusts the other private members.

nienhueser: This seems pretty inefficient. The standard approach for reversing strings works in place by…
112​int WayChunk::size() const
113​{
114​ return m_wayList.size();
115​}
116​
117​bool WayChunk::concatPossible(GeoDataPlacemark *placemark) const
118​{
119​ GeoDataFeature::GeoDataVisualCategory category = placemark->visualCategory();
120​ return (category == m_visualCategory);
121​}
122​
123​GeoDataFeature::GeoDataVisualCategory WayChunk::visualCategory() const
124​{
125​ return m_visualCategory;
126​}
127​
128​
129​}
nienhueserUnsubmitted
Done

Should return just int, no? QList cannot hold more.

nienhueser: Should return just int, no? QList cannot hold more.
nienhueserUnsubmitted
Done

should be const

nienhueser: should be const