From 88c6fb9aeb5c320f0c723c0aad8399c50e34dbd3 Mon Sep 17 00:00:00 2001 From: Maxim Pimenov Date: Wed, 11 Jul 2018 13:57:34 +0300 Subject: [PATCH] [base] Osm::Id fixes. The masks in the higher bits of our osm id encoding used to be disjoint. After a refactoring, the encoding was switched to packing 4 values into 2 bits and thus the masks started to intersect. The checks were left untouched, though, resulting in Relation passing tests for being a Node and a Way. This commit fixes the bug and introduces an enum to store the osm id type, hoping to make things more clear. --- base/base_tests/osm_id_test.cpp | 24 +++---- base/osm_id.cpp | 64 +++++++++---------- base/osm_id.hpp | 23 ++++--- feature_list/feature_list.cpp | 2 +- generator/cities_boundaries_builder.cpp | 2 +- generator/coastlines_generator.cpp | 6 +- generator/extract_addr/extract_addr.cpp | 2 +- generator/feature_builder.cpp | 15 +++-- generator/locality_sorter.cpp | 7 +- generator/routing_helpers.cpp | 2 +- generator/ugc_db.cpp | 2 +- generator/ugc_translator.cpp | 2 +- indexer/indexer_tests/locality_index_test.cpp | 4 +- routing/cross_mwm_connector_serialization.hpp | 4 +- transit/transit_graph_data.cpp | 6 +- 15 files changed, 84 insertions(+), 81 deletions(-) diff --git a/base/base_tests/osm_id_test.cpp b/base/base_tests/osm_id_test.cpp index d3cd7e7285..e5ecaea065 100644 --- a/base/base_tests/osm_id_test.cpp +++ b/base/base_tests/osm_id_test.cpp @@ -2,29 +2,23 @@ #include "base/osm_id.hpp" -using namespace osm; - +namespace osm +{ UNIT_TEST(OsmId) { Id const node = Id::Node(12345); - TEST_EQUAL(node.OsmId(), 12345ULL, ()); - TEST(node.IsNode(), ()); - TEST(!node.IsWay(), ()); - TEST(!node.IsRelation(), ()); + TEST_EQUAL(node.GetOsmId(), 12345ULL, ()); + TEST_EQUAL(node.GetType(), Id::Type::Node, ()); TEST_EQUAL(DebugPrint(node), "node 12345", ()); Id const way = Id::Way(93245123456332ULL); - TEST_EQUAL(way.OsmId(), 93245123456332ULL, ()); - TEST(!way.IsNode(), ()); - TEST(way.IsWay(), ()); - TEST(!way.IsRelation(), ()); + TEST_EQUAL(way.GetOsmId(), 93245123456332ULL, ()); + TEST_EQUAL(way.GetType(), Id::Type::Way, ()); TEST_EQUAL(DebugPrint(way), "way 93245123456332", ()); Id const relation = Id::Relation(5); - TEST_EQUAL(relation.OsmId(), 5ULL, ()); - // sic! - TEST(relation.IsNode(), ()); - TEST(relation.IsWay(), ()); - TEST(relation.IsRelation(), ()); + TEST_EQUAL(relation.GetOsmId(), 5ULL, ()); + TEST_EQUAL(relation.GetType(), Id::Type::Relation, ()); TEST_EQUAL(DebugPrint(relation), "relation 5", ()); } +} // namespace osm diff --git a/base/osm_id.cpp b/base/osm_id.cpp index 70c588911f..4fe42ec891 100644 --- a/base/osm_id.cpp +++ b/base/osm_id.cpp @@ -1,17 +1,17 @@ #include "base/osm_id.hpp" +#include "base/assert.hpp" + #include namespace { // Use 2 higher bits to encode type. -// -// todo The masks are not disjoint for some reason -// since the commit 60414dc86254aed22ac9e66fed49eba554260a2c. +// Note that the masks are not disjoint. uint64_t const kNode = 0x4000000000000000ULL; uint64_t const kWay = 0x8000000000000000ULL; uint64_t const kRelation = 0xC000000000000000ULL; -uint64_t const kReset = ~(kNode | kWay | kRelation); +uint64_t const kTypeMask = 0xC000000000000000ULL; } // namespace namespace osm @@ -22,60 +22,60 @@ Id::Id(uint64_t encodedId) : m_encodedId(encodedId) Id Id::Node(uint64_t id) { + ASSERT_EQUAL(id & kTypeMask, 0, ()); return Id(id | kNode); } Id Id::Way(uint64_t id) { + ASSERT_EQUAL(id & kTypeMask, 0, ()); return Id(id | kWay); } Id Id::Relation(uint64_t id) { + ASSERT_EQUAL(id & kTypeMask, 0, ()); return Id(id | kRelation); } -uint64_t Id::OsmId() const +uint64_t Id::GetOsmId() const { - return m_encodedId & kReset; + ASSERT_NOT_EQUAL(m_encodedId & kTypeMask, 0, ()); + return m_encodedId & ~kTypeMask; } -uint64_t Id::EncodedId() const +uint64_t Id::GetEncodedId() const { return m_encodedId; } -bool Id::IsNode() const +Id::Type Id::GetType() const { - return (m_encodedId & kNode) == kNode; + uint64_t const mask = m_encodedId & kTypeMask; + switch (mask) + { + case kNode: return Id::Type::Node; + case kWay: return Id::Type::Way; + case kRelation: return Id::Type::Relation; + } + CHECK_SWITCH(); } -bool Id::IsWay() const +std::string DebugPrint(Id::Type const & t) { - return (m_encodedId & kWay) == kWay; + switch (t) + { + case Id::Type::Node: return "node"; + case Id::Type::Way: return "way"; + case Id::Type::Relation: return "relation"; + } + CHECK_SWITCH(); } -bool Id::IsRelation() const +std::string DebugPrint(Id const & id) { - return (m_encodedId & kRelation) == kRelation; -} - -std::string DebugPrint(osm::Id const & id) -{ - std::string typeStr; - // Note that with current encoding all relations are also at the - // same time nodes and ways. Therefore, the relation check must go first. - if (id.IsRelation()) - typeStr = "relation"; - else if (id.IsNode()) - typeStr = "node"; - else if (id.IsWay()) - typeStr = "way"; - else - typeStr = "ERROR: Not initialized Osm ID"; - - std::ostringstream stream; - stream << typeStr << " " << id.OsmId(); - return stream.str(); + std::ostringstream oss; + oss << DebugPrint(id.GetType()) << " " << id.GetOsmId(); + return oss.str(); } } // namespace osm diff --git a/base/osm_id.hpp b/base/osm_id.hpp index c38f471ef0..515a28dd01 100644 --- a/base/osm_id.hpp +++ b/base/osm_id.hpp @@ -9,6 +9,13 @@ namespace osm class Id { public: + enum class Type + { + Node, + Way, + Relation + }; + static const uint64_t kInvalid = 0ULL; explicit Id(uint64_t encodedId = kInvalid); @@ -17,17 +24,14 @@ public: static Id Way(uint64_t osmId); static Id Relation(uint64_t osmId); - uint64_t OsmId() const; - uint64_t EncodedId() const; - - bool IsNode() const; - bool IsWay() const; - bool IsRelation() const; + uint64_t GetOsmId() const; + uint64_t GetEncodedId() const; + Type GetType() const; bool operator<(Id const & other) const { return m_encodedId < other.m_encodedId; } bool operator==(Id const & other) const { return m_encodedId == other.m_encodedId; } bool operator!=(Id const & other) const { return !(*this == other); } - bool operator==(uint64_t other) const { return OsmId() == other; } + bool operator==(uint64_t other) const { return GetOsmId() == other; } private: uint64_t m_encodedId; @@ -35,8 +39,9 @@ private: struct HashId : private std::hash { - size_t operator()(Id const & id) const { return std::hash::operator()(id.OsmId()); } + size_t operator()(Id const & id) const { return std::hash::operator()(id.GetOsmId()); } }; -std::string DebugPrint(osm::Id const & id); +std::string DebugPrint(Id::Type const & t); +std::string DebugPrint(Id const & id); } // namespace osm diff --git a/feature_list/feature_list.cpp b/feature_list/feature_list.cpp index edd1398c6e..c3ca02bea0 100644 --- a/feature_list/feature_list.cpp +++ b/feature_list/feature_list.cpp @@ -220,7 +220,7 @@ public: f.GetPreferredNames(name, secondary); if (name.empty()) name = operatr; - string const & osmId = strings::to_string(osmIt->second.EncodedId()); + string const & osmId = strings::to_string(osmIt->second.GetEncodedId()); string const & uid = BuildUniqueId(ll, name); string const & lat = strings::to_string_with_digits_after_comma(ll.lat, 6); string const & lon = strings::to_string_with_digits_after_comma(ll.lon, 6); diff --git a/generator/cities_boundaries_builder.cpp b/generator/cities_boundaries_builder.cpp index bca1011a4d..e6e70a5b59 100644 --- a/generator/cities_boundaries_builder.cpp +++ b/generator/cities_boundaries_builder.cpp @@ -162,7 +162,7 @@ bool SerializeBoundariesTable(std::string const & path, OsmIdToBoundariesTable & { WriteToSink(sink, static_cast(ids.size())); for (auto const & id : ids) - WriteToSink(sink, id.EncodedId()); + WriteToSink(sink, id.GetEncodedId()); } return true; diff --git a/generator/coastlines_generator.cpp b/generator/coastlines_generator.cpp index d479aae17c..447077b17d 100644 --- a/generator/coastlines_generator.cpp +++ b/generator/coastlines_generator.cpp @@ -114,9 +114,11 @@ namespace osm::Id const firstWay = fb.GetFirstOsmId(); osm::Id const lastWay = fb.GetLastOsmId(); if (firstWay == lastWay) - LOG(LINFO, ("Not merged coastline, way", firstWay.OsmId(), "(", fb.GetPointsCount(), "points)")); + LOG(LINFO, ("Not merged coastline, way", firstWay.GetOsmId(), "(", fb.GetPointsCount(), + "points)")); else - LOG(LINFO, ("Not merged coastline, ways", firstWay.OsmId(), "to", lastWay.OsmId(), "(", fb.GetPointsCount(), "points)")); + LOG(LINFO, ("Not merged coastline, ways", firstWay.GetOsmId(), "to", lastWay.GetOsmId(), + "(", fb.GetPointsCount(), "points)")); ++m_notMergedCoastsCount; m_totalNotMergedCoastsPoints += fb.GetPointsCount(); } diff --git a/generator/extract_addr/extract_addr.cpp b/generator/extract_addr/extract_addr.cpp index 83e5975dfd..1fb43757c5 100644 --- a/generator/extract_addr/extract_addr.cpp +++ b/generator/extract_addr/extract_addr.cpp @@ -56,7 +56,7 @@ void PrintFeature(FeatureBuilder1 const & fb, uint64_t) ToJSONObject(*geometry, "coordinates", coordinates); auto properties = my::NewJSONObject(); - ToJSONObject(*properties, "id", fb.GetMostGenericOsmId().EncodedId()); + ToJSONObject(*properties, "id", fb.GetMostGenericOsmId().GetEncodedId()); if (!name.empty() && !category.empty() && category != "building-address") { ToJSONObject(*properties, "name", name); diff --git a/generator/feature_builder.cpp b/generator/feature_builder.cpp index c7d7bf9e08..0839863c1b 100644 --- a/generator/feature_builder.cpp +++ b/generator/feature_builder.cpp @@ -414,7 +414,7 @@ void FeatureBuilder1::SerializeBorder(serial::GeometryCodingParams const & param data.clear(); PushBackByteSink sink(data); - WriteToSink(sink, GetMostGenericOsmId().EncodedId()); + WriteToSink(sink, GetMostGenericOsmId().GetEncodedId()); CHECK_GREATER(m_polygons.size(), 0, ()); @@ -498,12 +498,13 @@ osm::Id FeatureBuilder1::GetMostGenericOsmId() const auto result = m_osmIds.front(); for (auto const & id : m_osmIds) { - if (id.IsRelation()) + auto const t = id.GetType(); + if (t == osm::Id::Type::Relation) { result = id; break; } - else if (result.IsNode() && id.IsWay()) + else if (t == osm::Id::Type::Way && result.GetType() == osm::Id::Type::Node) { result = id; } @@ -514,8 +515,10 @@ osm::Id FeatureBuilder1::GetMostGenericOsmId() const bool FeatureBuilder1::HasOsmId(osm::Id const & id) const { for (auto const & cid : m_osmIds) + { if (cid == id) return true; + } return false; } @@ -581,8 +584,8 @@ bool FeatureBuilder1::IsDrawableInRange(int lowScale, int highScale) const uint64_t FeatureBuilder1::GetWayIDForRouting() const { - if (m_osmIds.size() == 1 && m_osmIds[0].IsWay() && IsLine() && IsRoad()) - return m_osmIds[0].OsmId(); + if (m_osmIds.size() == 1 && m_osmIds[0].GetType() == osm::Id::Type::Way && IsLine() && IsRoad()) + return m_osmIds[0].GetOsmId(); return 0; } @@ -622,7 +625,7 @@ void FeatureBuilder2::SerializeLocalityObject(serial::GeometryCodingParams const data.m_buffer.clear(); PushBackByteSink sink(data.m_buffer); - WriteToSink(sink, GetMostGenericOsmId().EncodedId()); + WriteToSink(sink, GetMostGenericOsmId().GetEncodedId()); auto const type = m_params.GetGeomType(); WriteToSink(sink, static_cast(type)); diff --git a/generator/locality_sorter.cpp b/generator/locality_sorter.cpp index 7bc547e8b2..2f2eecb6d4 100644 --- a/generator/locality_sorter.cpp +++ b/generator/locality_sorter.cpp @@ -135,9 +135,8 @@ public: holder.SetInner(); auto const id = fb2.GetMostGenericOsmId(); CHECK(holder.TryToMakeStrip(hullPoints), - ("Error while building tringles for object with OSM Id:", id.OsmId(), - "Type:", id.IsRelation() ? "Relation" : "Way", "points:", points, - "hull:", hull.Points())); + ("Error while building tringles for object with OSM Id:", id.GetOsmId(), + "Type:", id.GetType(), "points:", points, "hull:", hull.Points())); } } } @@ -248,7 +247,7 @@ bool GenerateGeoObjectsData(string const & featuresDir, string const & nodesFile auto const needSerialize = [&nodeIds](FeatureBuilder1 & fb) { auto & fb2 = static_cast(fb); return fb2.IsLocalityObject() || - (!fb.GetOsmIds().empty() && nodeIds.count(fb.GetMostGenericOsmId().EncodedId()) != 0); + (!fb.GetOsmIds().empty() && nodeIds.count(fb.GetMostGenericOsmId().GetEncodedId()) != 0); }; DataHeader header; diff --git a/generator/routing_helpers.cpp b/generator/routing_helpers.cpp index 13479e28d9..03e2c8e871 100644 --- a/generator/routing_helpers.cpp +++ b/generator/routing_helpers.cpp @@ -17,7 +17,7 @@ bool ForEachRoadFromFile(string const & filename, ToDo && toDo) { return generator::ForEachOsmId2FeatureId(filename, [&](osm::Id const & osmId, uint32_t const featureId) { - if (osmId.IsWay()) + if (osmId.GetType() == osm::Id::Type::Way) toDo(featureId, osmId); }); } diff --git a/generator/ugc_db.cpp b/generator/ugc_db.cpp index 0b998cf825..52265f282d 100644 --- a/generator/ugc_db.cpp +++ b/generator/ugc_db.cpp @@ -59,7 +59,7 @@ bool UGCDB::Get(osm::Id const & id, std::vector & blob) results.values << "["; std::ostringstream cmd; - cmd << "SELECT value FROM ratings WHERE key=" << id.EncodedId() << ";"; + cmd << "SELECT value FROM ratings WHERE key=" << id.GetEncodedId() << ";"; char * zErrMsg = nullptr; auto rc = sqlite3_exec(m_db, cmd.str().c_str(), callback, &results, &zErrMsg); diff --git a/generator/ugc_translator.cpp b/generator/ugc_translator.cpp index a6ff0c7ce0..9e1e58687e 100644 --- a/generator/ugc_translator.cpp +++ b/generator/ugc_translator.cpp @@ -36,7 +36,7 @@ bool UGCTranslator::TranslateUGC(osm::Id const & id, ugc::UGC & ugc) if (size > 1) { - LOG(LWARNING, ("Osm id duplication in UGC database", id.EncodedId())); + LOG(LWARNING, ("Osm id duplication in UGC database", id.GetEncodedId())); return false; } diff --git a/indexer/indexer_tests/locality_index_test.cpp b/indexer/indexer_tests/locality_index_test.cpp index 01eb2a42a4..ce396fbedc 100644 --- a/indexer/indexer_tests/locality_index_test.cpp +++ b/indexer/indexer_tests/locality_index_test.cpp @@ -53,7 +53,7 @@ template Ids GetIds(LocalityIndex const & index, m2::RectD const & rect) { Ids ids; - index.ForEachInRect([&ids](osm::Id const & id) { ids.insert(id.EncodedId()); }, rect); + index.ForEachInRect([&ids](osm::Id const & id) { ids.insert(id.GetEncodedId()); }, rect); return ids; }; @@ -62,7 +62,7 @@ RankedIds GetRankedIds(LocalityIndex const & index, m2::PointD const & center, m2::PointD const & border, uint32_t topSize) { RankedIds ids; - index.ForClosestToPoint([&ids](osm::Id const & id) { ids.push_back(id.EncodedId()); }, center, + index.ForClosestToPoint([&ids](osm::Id const & id) { ids.push_back(id.GetEncodedId()); }, center, MercatorBounds::DistanceOnEarth(center, border), topSize); return ids; }; diff --git a/routing/cross_mwm_connector_serialization.hpp b/routing/cross_mwm_connector_serialization.hpp index 4615699394..15a25c50c3 100644 --- a/routing/cross_mwm_connector_serialization.hpp +++ b/routing/cross_mwm_connector_serialization.hpp @@ -77,7 +77,7 @@ public: void WriteCrossMwmId(osm::Id const & id, uint8_t bits, BitWriter & w) const { CHECK_LESS_OR_EQUAL(bits, connector::kOsmIdBits, ()); - w.WriteAtMost64Bits(id.EncodedId(), bits); + w.WriteAtMost64Bits(id.GetEncodedId(), bits); } template @@ -439,7 +439,7 @@ private: for (Transition const & transition : transitions) osmId = std::max(osmId, transition.GetCrossMwmId()); - return bits::NumUsedBits(osmId.OsmId()); + return bits::NumUsedBits(osmId.GetOsmId()); } static uint32_t CalcBitsPerCrossMwmId( diff --git a/transit/transit_graph_data.cpp b/transit/transit_graph_data.cpp index 2e83e6f460..79788bcbf1 100644 --- a/transit/transit_graph_data.cpp +++ b/transit/transit_graph_data.cpp @@ -172,18 +172,18 @@ void DeserializerFromJson::operator()(FeatureIdentifiers & id, char const * name auto const it = m_osmIdToFeatureIds.find(osmId); if (it != m_osmIdToFeatureIds.cend()) { - CHECK(!it->second.empty(), ("Osm id:", osmId, "(encoded", osmId.EncodedId(), + CHECK(!it->second.empty(), ("Osm id:", osmId, "(encoded", osmId.GetEncodedId(), ") from transit graph does not correspond to any feature.")); if (it->second.size() != 1) { // Note. |osmId| corresponds to several feature ids. It may happen in case of stops, // if a stop is present as a relation. It's a rare case. - LOG(LWARNING, ("Osm id:", osmId, "( encoded", osmId.EncodedId(), ") corresponds to", + LOG(LWARNING, ("Osm id:", osmId, "( encoded", osmId.GetEncodedId(), ") corresponds to", it->second.size(), "feature ids.")); } id.SetFeatureId(it->second[0]); } - id.SetOsmId(osmId.EncodedId()); + id.SetOsmId(osmId.GetEncodedId()); } void DeserializerFromJson::operator()(EdgeFlags & edgeFlags, char const * name)