Commit 9dd870a6 authored by Volker Krause's avatar Volker Krause Committed by Torsten Rahn

Avoid losing the OSM relation member type information

There seems to be the assumption in a number of places here that a single
64bit number is enough to identify any OSM element. This however isn't
entirely correct, as OSM nodes, ways and relations use separate identifier
spaces, so we either need to infer the type from context or explicitly
carry it along next to the number. This wasn't done for non-geometry
relation members however, resulting in all member types ending up as "way"
when serialized back to o5m, which quite thoroughly corrupts those
relations.

This change does the bare minimum to fix this specific issue, API not
relevant for this is adjusted to keep the old broken behavior.

With relation member types preserved we however hit a subsequent problem
in the o5m encoder, which needs to track differential id encoding state
separately per type.
parent d9093ac1
......@@ -726,7 +726,7 @@ void Placemark::updateRelations(const Marble::GeoDataPlacemark &placemark)
for (auto iter = relation->osmData().relationReferencesBegin(), end = relation->osmData().relationReferencesEnd();
iter != end; ++iter) {
if (iter.value() == QStringLiteral("stop") || iter.value() == QStringLiteral("platform")) {
placemarkIds << iter.key();
placemarkIds << iter.key().id;
}
}
}
......
......@@ -86,11 +86,11 @@ GeoDataFeature *GeoDataRelation::clone() const
return new GeoDataRelation(*this);
}
void GeoDataRelation::addMember(const GeoDataFeature *feature, qint64 id, const QString &role)
void GeoDataRelation::addMember(const GeoDataFeature *feature, qint64 id, OsmType type, const QString &role)
{
Q_D(GeoDataRelation);
d->m_features << feature;
d->m_osmData.addRelation(id, role);
d->m_osmData.addRelation(id, type, role);
d->m_memberIds << id;
}
......
......@@ -12,6 +12,7 @@
#include "GeoDataCoordinates.h"
#include "GeoDataPlacemark.h"
#include "osm/OsmPlacemarkData.h"
#include "geodata_export.h"
......@@ -55,7 +56,7 @@ public:
const char* nodeType() const override;
GeoDataFeature * clone() const override;
void addMember(const GeoDataFeature* feature, qint64 id, const QString &role);
void addMember(const GeoDataFeature* feature, qint64 id, OsmType type, const QString &role);
QSet<const GeoDataFeature*> members() const;
OsmPlacemarkData &osmData();
......
......@@ -19,6 +19,11 @@
namespace Marble
{
inline uint qHash(Marble::OsmIdentifier ident, uint seed)
{
return ::qHash(ident.id, seed) ^ ::qHash((int)ident.type, seed);
}
OsmPlacemarkData::OsmPlacemarkData():
m_id( 0 )
{
......@@ -260,27 +265,35 @@ QHash< int, OsmPlacemarkData >::const_iterator OsmPlacemarkData::memberReference
return m_memberReferences.constEnd();
}
void OsmPlacemarkData::addRelation( qint64 id, const QString &role )
void OsmPlacemarkData::addRelation( qint64 id, OsmType type, const QString &role )
{
m_relationReferences.insert( id, role );
m_relationReferences.insert( { id, type }, role );
}
void OsmPlacemarkData::removeRelation( qint64 id )
{
m_relationReferences.remove( id );
/// ### this is wrong and just done this way for backward behavior compatible
/// ### this method should probably take type as an additional argument
m_relationReferences.remove( { id, OsmType::Node } );
m_relationReferences.remove( { id, OsmType::Way } );
m_relationReferences.remove( { id, OsmType::Relation } );
}
bool OsmPlacemarkData::containsRelation( qint64 id ) const
{
return m_relationReferences.contains( id );
/// ### this is wrong and just done this way for backward behavior compatible
/// ### this method should probably take type as an additional argument
return m_relationReferences.contains( { id, OsmType::Node } )
|| m_relationReferences.contains( { id, OsmType::Way } )
|| m_relationReferences.contains( { id, OsmType::Relation } );
}
QHash< qint64, QString >::const_iterator OsmPlacemarkData::relationReferencesBegin() const
QHash< OsmIdentifier, QString >::const_iterator OsmPlacemarkData::relationReferencesBegin() const
{
return m_relationReferences.begin();
}
QHash< qint64, QString >::const_iterator OsmPlacemarkData::relationReferencesEnd() const
QHash< OsmIdentifier, QString >::const_iterator OsmPlacemarkData::relationReferencesEnd() const
{
return m_relationReferences.constEnd();
}
......
......@@ -26,6 +26,27 @@ class QXmlStreamAttributes;
namespace Marble
{
/** Type of OSM element. */
enum class OsmType {
Node,
Way,
Relation
};
/** Identifier for an OSM element.
* @note OSM uses distinct id spaces for all its three basic element types, so just the numeric id
* on its own doesn't identify an element without knowing its type.
*/
struct OsmIdentifier {
inline OsmIdentifier() = default;
inline OsmIdentifier(qint64 _id, OsmType _type) : id(_id), type(_type) {}
qint64 id = 0;
OsmType type = OsmType::Way;
inline bool operator==(OsmIdentifier other) const { return id == other.id && type == other.type; }
};
/**
* This class is used to encapsulate the osm data fields kept within a placemark's extendedData.
* It stores OSM server generated data: id, version, changeset, uid, visible, user, timestamp;
......@@ -174,12 +195,12 @@ public:
* @brief addRelation calling this makes the osm placemark a member of the relation
* with @p id as id, while having the role @p role
*/
void addRelation( qint64 id, const QString &role );
void addRelation( qint64 id, OsmType type, const QString &role );
void removeRelation( qint64 id );
bool containsRelation( qint64 id ) const;
QHash< qint64, QString >::const_iterator relationReferencesBegin() const;
QHash< qint64, QString >::const_iterator relationReferencesEnd() const;
QHash< OsmIdentifier, QString >::const_iterator relationReferencesBegin() const;
QHash< OsmIdentifier, QString >::const_iterator relationReferencesEnd() const;
/**
* @brief isNull returns false if the osmData is loaded from a source
......@@ -223,7 +244,7 @@ private:
* Eg. an entry ( "123", "stop" ) means that the parent placemark is a member of
* the relation with id "123", while having the "stop" role
*/
QHash<qint64, QString> m_relationReferences;
QHash<OsmIdentifier, QString> m_relationReferences;
};
......
......@@ -136,7 +136,7 @@ void OsmRelationManagerWidget::handleItemChange( QTreeWidgetItem *item, int colu
QString role = item->text( Column::Role );
qint64 id = item->data( Column::Name, Qt::UserRole ).toLongLong();
d->m_placemark->osmData().addRelation( id, role );
d->m_placemark->osmData().addRelation( id, OsmType::Way, role );
update();
}
......
......@@ -46,17 +46,17 @@ void OsmRelationManagerWidgetPrivate::populateRelationsList()
if ( m_placemark->hasOsmData() ) {
const OsmPlacemarkData &osmData = m_placemark->osmData();
QHash< qint64, QString >::const_iterator it = osmData.relationReferencesBegin();
QHash< qint64, QString >::const_iterator end = osmData.relationReferencesEnd();
auto it = osmData.relationReferencesBegin();
const auto end = osmData.relationReferencesEnd();
for ( ; it != end; ++it ) {
if ( !m_allRelations->contains( it.key() ) ) {
mDebug()<< QString( "Relation %1 is not loaded in the Annotate Plugin" ).arg( it.key() );
if ( !m_allRelations->contains( it.key().id ) ) {
mDebug()<< QString( "Relation %1 is not loaded in the Annotate Plugin" ).arg( it.key().id );
continue;
}
const OsmPlacemarkData &relationData = m_allRelations->value( it.key() );
const OsmPlacemarkData &relationData = m_allRelations->value( it.key().id );
QTreeWidgetItem *newItem = new QTreeWidgetItem();
QString name = relationData.tagValue(QStringLiteral("name"));
......
......@@ -162,6 +162,17 @@ void OsmRelation::createMultipolygon(GeoDataDocument *document, OsmWays &ways, c
}
}
static OsmType stringToType(const QString &str)
{
if (str == "relation") {
return OsmType::Relation;
}
if (str == "node") {
return OsmType::Node;
}
return OsmType::Way;
}
void OsmRelation::createRelation(GeoDataDocument *document, const QHash<qint64, GeoDataPlacemark*>& placemarks) const
{
if (m_osmData.containsTag(QStringLiteral("type"), QStringLiteral("multipolygon"))) {
......@@ -180,7 +191,7 @@ void OsmRelation::createRelation(GeoDataDocument *document, const QHash<qint64,
for (auto const &member: m_members) {
auto const iter = placemarks.find(member.reference);
if (iter != placemarks.constEnd()) {
relation->addMember(*iter, member.reference, member.role);
relation->addMember(*iter, member.reference, stringToType(member.type), member.role);
}
}
......
......@@ -159,7 +159,7 @@ void O5mWriter::writeRelations(const OsmConverter::Relations &relations, QDataSt
stream << qint8(0xff); // reset delta encoding counters
StringTable stringTable;
qint64 lastId = 0;
qint64 lastReferenceId = 0;
qint64 lastReferenceId[3] = { 0, 0, 0 };
QByteArray bufferData;
QBuffer buffer(&bufferData);
......@@ -217,32 +217,32 @@ void O5mWriter::writeTrailer(QDataStream &stream) const
stream << qint8(0xfe); // o5m file end indicator
}
void O5mWriter::writeMultipolygonMembers(const GeoDataPolygon &polygon, qint64 &lastId, const OsmPlacemarkData &osmData, StringTable &stringTable, QDataStream &stream) const
void O5mWriter::writeMultipolygonMembers(const GeoDataPolygon &polygon, qint64 (&lastId)[3], const OsmPlacemarkData &osmData, StringTable &stringTable, QDataStream &stream) const
{
qint64 id = osmData.memberReference(-1).id();
qint64 idDiff = id - lastId;
qint64 idDiff = id - lastId[(int)OsmType::Way];
writeSigned(idDiff, stream);
lastId = id;
lastId[(int)OsmType::Way] = id;
writeStringPair(StringPair("1outer", QString()), stringTable, stream); // type=way, role=outer
for (int index = 0; index < polygon.innerBoundaries().size(); ++index) {
id = osmData.memberReference( index ).id();
qint64 idDiff = id - lastId;
qint64 idDiff = id - lastId[(int)OsmType::Way];
writeSigned(idDiff, stream);
writeStringPair(StringPair("1inner", QString()), stringTable, stream); // type=way, role=inner
lastId = id;
lastId[(int)OsmType::Way] = id;
}
}
void O5mWriter::writeRelationMembers(const GeoDataRelation *relation, qint64 &lastId, const OsmPlacemarkData &osmData, O5mWriter::StringTable &stringTable, QDataStream &stream) const
void O5mWriter::writeRelationMembers(const GeoDataRelation *relation, qint64 (&lastId)[3], const OsmPlacemarkData &osmData, O5mWriter::StringTable &stringTable, QDataStream &stream) const
{
Q_UNUSED(relation);
for (auto iter = osmData.relationReferencesBegin(), end = osmData.relationReferencesEnd(); iter != end; ++iter) {
qint64 id = iter.key();
qint64 idDiff = id - lastId;
qint64 id = iter.key().id;
qint64 idDiff = id - lastId[(int)iter.key().type];
writeSigned(idDiff, stream);
auto const key = QString("1%1").arg(iter.value());
writeStringPair(StringPair(key, QString()), stringTable, stream); // type=way, role=...
lastId = id;
const QString key = QLatin1Char('0' + (int)iter.key().type) + iter.value();
writeStringPair(StringPair(key, QString()), stringTable, stream);
lastId[(int)iter.key().type] = id;
}
}
......
......@@ -38,8 +38,8 @@ private:
void writeRelations(const OsmConverter::Relations &relations, QDataStream& stream) const;
void writeTrailer(QDataStream& stream) const;
void writeMultipolygonMembers(const GeoDataPolygon &polygon, qint64 &lastId, const OsmPlacemarkData &osmData, StringTable &stringTable, QDataStream &stream) const;
void writeRelationMembers(const GeoDataRelation *relation, qint64 &lastId, const OsmPlacemarkData &osmData, StringTable &stringTable, QDataStream &stream) const;
void writeMultipolygonMembers(const GeoDataPolygon &polygon, qint64 (&lastId)[3], const OsmPlacemarkData &osmData, StringTable &stringTable, QDataStream &stream) const;
void writeRelationMembers(const GeoDataRelation *relation, qint64 (&lastId)[3], const OsmPlacemarkData &osmData, StringTable &stringTable, QDataStream &stream) const;
void writeReferences(const GeoDataLineString &lineString, qint64 &lastId, const OsmPlacemarkData &osmData, QDataStream &stream) const;
void writeVersion(const OsmPlacemarkData &osmData, QDataStream &stream) const;
void writeTags(const OsmPlacemarkData &osmData, StringTable &stringTable, QDataStream &stream) const;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment