diff --git a/editor/changeset_wrapper.cpp b/editor/changeset_wrapper.cpp index 35ad6a1fa9..549a47f5d6 100644 --- a/editor/changeset_wrapper.cpp +++ b/editor/changeset_wrapper.cpp @@ -1,16 +1,16 @@ -#include "base/logging.hpp" - -#include "indexer/feature.hpp" - #include "editor/changeset_wrapper.hpp" #include "editor/osm_feature_matcher.hpp" -#include "std/algorithm.hpp" -#include "std/sstream.hpp" -#include "std/map.hpp" +#include "indexer/feature.hpp" #include "geometry/mercator.hpp" +#include "base/assert.hpp" +#include "base/logging.hpp" + +#include "std/algorithm.hpp" +#include "std/sstream.hpp" + #include "private.h" using editor::XMLFeature; @@ -60,13 +60,15 @@ XMLFeature ChangesetWrapper::GetMatchingNodeFeatureFromOSM(m2::PointD const & ce pugi::xml_node const bestNode = GetBestOsmNode(doc, ll); if (bestNode.empty()) + { MYTHROW(OsmObjectWasDeletedException, ("OSM does not have any nodes at the coordinates", ll, ", server has returned:", doc)); + } return XMLFeature(bestNode); } -XMLFeature ChangesetWrapper::GetMatchingAreaFeatureFromOSM(set const & geometry) +XMLFeature ChangesetWrapper::GetMatchingAreaFeatureFromOSM(vector const & geometry) { // TODO: Make two/four requests using points on inscribed rectagle. for (auto const & pt : geometry) @@ -81,16 +83,14 @@ XMLFeature ChangesetWrapper::GetMatchingAreaFeatureFromOSM(set const continue; XMLFeature const way(bestWay); - if (!way.IsArea()) - continue; + ASSERT(way.IsArea(), ("Best way must be area.")); // AlexZ: TODO: Check that this way is really match our feature. // If we had some way to check it, why not to use it in selecting our feature? return way; } - MYTHROW(OsmObjectWasDeletedException, - ("OSM does not have any matching way for feature")); + MYTHROW(OsmObjectWasDeletedException, ("OSM does not have any matching way for feature")); } void ChangesetWrapper::ModifyNode(XMLFeature node) diff --git a/editor/changeset_wrapper.hpp b/editor/changeset_wrapper.hpp index 6f62ede85e..cc3df5ecb6 100644 --- a/editor/changeset_wrapper.hpp +++ b/editor/changeset_wrapper.hpp @@ -1,11 +1,14 @@ #pragma once +#include "base/exception.hpp" + #include "editor/server_api.hpp" #include "editor/xml_feature.hpp" -#include "base/exception.hpp" +#include "geometry/point2d.hpp" #include "std/set.hpp" +#include "std/vector.hpp" class FeatureType; @@ -32,7 +35,7 @@ public: /// Throws many exceptions from above list, plus including XMLNode's parsing ones. /// OsmObjectWasDeletedException means that node was deleted from OSM server by someone else. editor::XMLFeature GetMatchingNodeFeatureFromOSM(m2::PointD const & center); - editor::XMLFeature GetMatchingAreaFeatureFromOSM(set const & geomerty); + editor::XMLFeature GetMatchingAreaFeatureFromOSM(vector const & geomerty); /// Throws exceptions from above list. void ModifyNode(editor::XMLFeature node); diff --git a/editor/editor_tests/osm_feature_matcher_test.cpp b/editor/editor_tests/osm_feature_matcher_test.cpp index fae7f32e6f..f6ac0c3e47 100644 --- a/editor/editor_tests/osm_feature_matcher_test.cpp +++ b/editor/editor_tests/osm_feature_matcher_test.cpp @@ -5,100 +5,98 @@ #include "3party/pugixml/src/pugixml.hpp" - static char const * const osmRawResponseWay = R"SEP( - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + )SEP"; static char const * const osmRawResponseNode = R"SEP( - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + )SEP"; - -UNIT_TEST(GetBestOsmWay_test) +UNIT_TEST(GetBestOsmWay_Test) { { pugi::xml_document osmResponse; TEST(osmResponse.load_buffer(osmRawResponseWay, ::strlen(osmRawResponseWay)), ()); - set geometry = { - MercatorBounds::FromLatLon(53.8977484, 27.557359), - MercatorBounds::FromLatLon(53.8978710, 27.557681), - MercatorBounds::FromLatLon(53.8978034, 27.557764), - MercatorBounds::FromLatLon(53.8977652, 27.557803), - MercatorBounds::FromLatLon(53.8977254, 27.557837), - MercatorBounds::FromLatLon(53.8976570, 27.557661), - MercatorBounds::FromLatLon(53.8976041, 27.557518), + vector const geometry = { + MercatorBounds::FromLatLon(53.8977484, 27.557359), + MercatorBounds::FromLatLon(53.8978710, 27.557681), + MercatorBounds::FromLatLon(53.8978034, 27.557764), + MercatorBounds::FromLatLon(53.8977652, 27.557803), + MercatorBounds::FromLatLon(53.8977254, 27.557837), + MercatorBounds::FromLatLon(53.8976570, 27.557661), + MercatorBounds::FromLatLon(53.8976041, 27.557518), }; auto const bestWay = osm::GetBestOsmWay(osmResponse, geometry); @@ -107,14 +105,14 @@ UNIT_TEST(GetBestOsmWay_test) { pugi::xml_document osmResponse; TEST(osmResponse.load_buffer(osmRawResponseWay, ::strlen(osmRawResponseWay)), ()); - set geometry = { - MercatorBounds::FromLatLon(53.8975484, 27.557359), // diff - MercatorBounds::FromLatLon(53.8978710, 27.557681), - MercatorBounds::FromLatLon(53.8975034, 27.557764), // diff - MercatorBounds::FromLatLon(53.8977652, 27.557803), - MercatorBounds::FromLatLon(53.8975254, 27.557837), // diff - MercatorBounds::FromLatLon(53.8976570, 27.557661), - MercatorBounds::FromLatLon(53.8976041, 27.557318), // diff + vector const geometry = { + MercatorBounds::FromLatLon(53.8975484, 27.557359), // diff + MercatorBounds::FromLatLon(53.8978710, 27.557681), + MercatorBounds::FromLatLon(53.8975034, 27.557764), // diff + MercatorBounds::FromLatLon(53.8977652, 27.557803), + MercatorBounds::FromLatLon(53.8975254, 27.557837), // diff + MercatorBounds::FromLatLon(53.8976570, 27.557661), + MercatorBounds::FromLatLon(53.8976041, 27.557318), // diff }; auto const bestWay = osm::GetBestOsmWay(osmResponse, geometry); @@ -122,15 +120,15 @@ UNIT_TEST(GetBestOsmWay_test) } } - -UNIT_TEST(GetBestOsmNode_test) +UNIT_TEST(GetBestOsmNode_Test) { { pugi::xml_document osmResponse; TEST(osmResponse.load_buffer(osmRawResponseNode, ::strlen(osmRawResponseNode)), ()); auto const bestNode = osm::GetBestOsmNode(osmResponse, ms::LatLon(53.8977398, 27.5579251)); - TEST_EQUAL(editor::XMLFeature(bestNode).GetName(), "Главное управление капитального строительства", ()); + TEST_EQUAL(editor::XMLFeature(bestNode).GetName(), + "Главное управление капитального строительства", ()); } { pugi::xml_document osmResponse; diff --git a/editor/osm_feature_matcher.cpp b/editor/osm_feature_matcher.cpp index 989f7da3b9..389f9794b2 100644 --- a/editor/osm_feature_matcher.cpp +++ b/editor/osm_feature_matcher.cpp @@ -1,16 +1,20 @@ -#include "base/logging.hpp" - #include "editor/osm_feature_matcher.hpp" -namespace osm +#include "base/logging.hpp" + +using editor::XMLFeature; + +namespace { -double constexpr kPointDiffEps = MercatorBounds::GetCellID2PointAbsEpsilon(); +constexpr double kPointDiffEps = MercatorBounds::GetCellID2PointAbsEpsilon(); bool LatLonEqual(ms::LatLon const & a, ms::LatLon const & b) { return a.EqualDxDy(b, kPointDiffEps); } +/// @returns value form (-Inf, 1]. Negative values are used as penalty, +/// positive as score. double ScoreLatLon(XMLFeature const & xmlFt, ms::LatLon const & latLon) { auto const a = MercatorBounds::FromLatLon(xmlFt.GetCenter()); @@ -18,8 +22,24 @@ double ScoreLatLon(XMLFeature const & xmlFt, ms::LatLon const & latLon) return 1.0 - (a.Length(b) / kPointDiffEps); } +template +void ForEachWaysNode(pugi::xml_document const & osmResponse, pugi::xml_node const & way, + TFunc && func) +{ + for (auto const xNodeRef : way.select_nodes("nd/@ref")) + { + string const nodeRef = xNodeRef.attribute().value(); + auto const node = osmResponse.select_node(("osm/node[@id='" + nodeRef + "']").data()).node(); + ASSERT(node, ("OSM response have ref", nodeRef, "but have no node with such id.", osmResponse)); + XMLFeature xmlFt(node); + func(xmlFt); + } +} + +/// @returns value form [-0.5, 0.5]. Negative values are used as penalty, +/// positive as score. double ScoreGeometry(pugi::xml_document const & osmResponse, pugi::xml_node const & way, - set geometry) + vector geometry) { int matched = 0; int total = 0; @@ -28,35 +48,27 @@ double ScoreGeometry(pugi::xml_document const & osmResponse, pugi::xml_node cons // Sort them by y and x in y groups. // Then pass points from the second set through constructed struct and register events // like square started, square ended, point encounted. - - for (auto const xNodeRef : way.select_nodes("nd/@ref")) - { - ++total; - string const nodeRef = xNodeRef.attribute().value(); - auto const node = osmResponse.select_node(("osm/node[@id='" + nodeRef + "']").data()).node(); - if (!node) - { - LOG(LDEBUG, ("OSM response have ref", nodeRef, - "but have no node with such id.", osmResponse)); - continue; // TODO: or break and return -1000? - } - - XMLFeature xmlFt(node); - for (auto pointIt = begin(geometry); pointIt != end(geometry); ++pointIt) - { - if (LatLonEqual(xmlFt.GetCenter(), MercatorBounds::ToLatLon(*pointIt))) - { - ++matched; - geometry.erase(pointIt); - break; - } - } - } + ForEachWaysNode(osmResponse, way, [&matched, &total, &geometry](XMLFeature const & xmlFt) + { + ++total; + for (auto pointIt = begin(geometry); pointIt != end(geometry); ++pointIt) + { + if (LatLonEqual(xmlFt.GetCenter(), MercatorBounds::ToLatLon(*pointIt))) + { + ++matched; + geometry.erase(pointIt); + break; + } + } + }); return static_cast(matched) / total - 0.5; } +} // namespace -pugi::xml_node GetBestOsmNode(pugi::xml_document const & osmResponse, ms::LatLon const latLon) +namespace osm +{ +pugi::xml_node GetBestOsmNode(pugi::xml_document const & osmResponse, ms::LatLon const & latLon) { double bestScore = -1; pugi::xml_node bestMatchNode; @@ -67,18 +79,10 @@ pugi::xml_node GetBestOsmNode(pugi::xml_document const & osmResponse, ms::LatLon { XMLFeature xmlFt(xNode.node()); - // TODO: - // if (!TypesEqual(xmlFt, feature::TypesHolder(ft))) - // continue; - - double nodeScore = ScoreLatLon(xmlFt, latLon); + double const nodeScore = ScoreLatLon(xmlFt, latLon); if (nodeScore < 0) continue; - // TODO: - // nodeScore += ScoreNames(xmlFt, ft.GetNames()); - // nodeScore += ScoreMetadata(xmlFt, ft.GetMetadata()); - if (bestScore < nodeScore) { bestScore = nodeScore; @@ -99,39 +103,24 @@ pugi::xml_node GetBestOsmNode(pugi::xml_document const & osmResponse, ms::LatLon return bestMatchNode; } -pugi::xml_node GetBestOsmWay(pugi::xml_document const & osmResponse, set const & geometry) +pugi::xml_node GetBestOsmWay(pugi::xml_document const & osmResponse, + vector const & geometry) { double bestScore = -1; pugi::xml_node bestMatchWay; for (auto const & xWay : osmResponse.select_nodes("osm/way")) { - try - { - XMLFeature xmlFt(xWay.node()); + XMLFeature xmlFt(xWay.node()); - // TODO: - // if (!TypesEqual(xmlFt, feature::TypesHolder(ft))) - // continue; - - double nodeScore = ScoreGeometry(osmResponse, xWay.node(), geometry); - if (nodeScore < 0) - continue; - - // TODO: - // nodeScore += ScoreNames(xmlFt, ft.GetNames()); - // nodeScore += ScoreMetadata(xmlFt, ft.GetMetadata()); - - if (bestScore < nodeScore) - { - bestScore = nodeScore; - bestMatchWay = xWay.node(); - } - } - catch (editor::XMLFeatureNoLatLonError const & ex) - { - LOG(LWARNING, ("No lat/lon attribute in osm response node.", ex.Msg())); + double const nodeScore = ScoreGeometry(osmResponse, xWay.node(), geometry); + if (nodeScore < 0) continue; + + if (bestScore < nodeScore) + { + bestScore = nodeScore; + bestMatchWay = xWay.node(); } } diff --git a/editor/osm_feature_matcher.hpp b/editor/osm_feature_matcher.hpp index 43393a895a..0c1f05e7bb 100644 --- a/editor/osm_feature_matcher.hpp +++ b/editor/osm_feature_matcher.hpp @@ -3,21 +3,15 @@ #include "editor/xml_feature.hpp" #include "geometry/mercator.hpp" +#include "geometry/point2d.hpp" -#include "std/set.hpp" +#include "std/vector.hpp" namespace osm { -using editor::XMLFeature; - -bool LatLonEqual(ms::LatLon const & a, ms::LatLon const & b); - -double ScoreLatLon(XMLFeature const & xmlFt, ms::LatLon const & latLon); - -double ScoreGeometry(pugi::xml_document const & osmResponse, pugi::xml_node const & way, - set geometry); - -pugi::xml_node GetBestOsmNode(pugi::xml_document const & osmResponse, ms::LatLon const latLon); - -pugi::xml_node GetBestOsmWay(pugi::xml_document const & osmResponse, set const & geometry); +/// @returns closest to the latLon node from osm or empty node if none is close enough. +pugi::xml_node GetBestOsmNode(pugi::xml_document const & osmResponse, ms::LatLon const & latLon); +/// @returns a way from osm with similar geometry or empy node if can't find such way. +pugi::xml_node GetBestOsmWay(pugi::xml_document const & osmResponse, + vector const & geometry); } // namespace osm diff --git a/indexer/osm_editor.cpp b/indexer/osm_editor.cpp index a34f3c5002..9d170ddac3 100644 --- a/indexer/osm_editor.cpp +++ b/indexer/osm_editor.cpp @@ -19,6 +19,7 @@ #include "base/exception.hpp" #include "base/logging.hpp" +#include "base/stl_helpers.hpp" #include "base/string_utils.hpp" #include "base/timer.hpp" @@ -262,30 +263,29 @@ bool AreFeaturesEqualButStreet(FeatureType const & a, FeatureType const & b) XMLFeature GetMatchingFeatureFromOSM(osm::ChangesetWrapper & cw, unique_ptr featurePtr) { - ASSERT(featurePtr->GetFeatureType() != feature::GEOM_LINE, ("Line features are not supported yet.")); + ASSERT_NOT_EQUAL(featurePtr->GetFeatureType(), feature::GEOM_LINE, + ("Line features are not supported yet.")); if (featurePtr->GetFeatureType() == feature::GEOM_POINT) - { return cw.GetMatchingNodeFeatureFromOSM(featurePtr->GetCenter()); - } - else - { - // Set filters out duplicate points for closed ways or triangles' vertices. - set geometry; - featurePtr->ForEachTriangle([&geometry](m2::PointD const & p1, - m2::PointD const & p2, m2::PointD const & p3) - { - geometry.insert(p1); - geometry.insert(p2); - geometry.insert(p3); - // Warning: geometry is cached in FeatureTyped. So if it wasn't BEST_GEOMETRY - // We can never have it. Features here came from editors loader and should - // have BEST_GEOMETRY geometry. - }, FeatureType::BEST_GEOMETRY); - ASSERT_GREATER_OR_EQUAL(geometry.size(), 3, ("Is it an area feature?")); + vector geometry; + featurePtr->ForEachTriangle( + [&geometry](m2::PointD const & p1, m2::PointD const & p2, m2::PointD const & p3) + { + geometry.push_back(p1); + geometry.push_back(p2); + geometry.push_back(p3); + // Warning: geometry is cached in FeatureType. So if it wasn't BEST_GEOMETRY, + // we can never have it. Features here came from editors loader and should + // have BEST_GEOMETRY geometry. + }, + FeatureType::BEST_GEOMETRY); + // Filters out duplicate points for closed ways or triangles' vertices. + my::SortUnique(geometry); - return cw.GetMatchingAreaFeatureFromOSM(geometry); - } + ASSERT_GREATER_OR_EQUAL(geometry.size(), 3, ("Is it an area feature?")); + + return cw.GetMatchingAreaFeatureFromOSM(geometry); } } // namespace @@ -751,8 +751,8 @@ void Editor::UploadChanges(string const & key, string const & secret, TChangeset feature.SetTagValue(kAddrStreetTag, fti.m_street); try { - XMLFeature osmFeature = GetMatchingFeatureFromOSM(changeset, - m_getOriginalFeatureFn(fti.m_feature.GetID())); + XMLFeature osmFeature = + GetMatchingFeatureFromOSM(changeset, m_getOriginalFeatureFn(fti.m_feature.GetID())); XMLFeature const osmFeatureCopy = osmFeature; osmFeature.ApplyPatch(feature); // Check to avoid duplicates.