diff --git a/generator/generator_tests_support/test_with_custom_mwms.cpp b/generator/generator_tests_support/test_with_custom_mwms.cpp index bd840c2582..cfea1dbbc0 100644 --- a/generator/generator_tests_support/test_with_custom_mwms.cpp +++ b/generator/generator_tests_support/test_with_custom_mwms.cpp @@ -69,5 +69,25 @@ void TestWithCustomMwms::RegisterLocalMapsInViewport(m2::RectD const & viewport) } } +void TestWithCustomMwms::RegisterLocalMapsByPrefix(std::string const & prefix) +{ + std::vector localFiles; + FindAllLocalMapsAndCleanup(std::numeric_limits::max() /* latestVersion */, localFiles); + + for (auto const & file : localFiles) + { + // Always load World.mwm, important for search. + auto const & name = file.GetCountryName(); + if (name != WORLD_FILE_NAME && !strings::StartsWith(name, prefix)) + continue; + + auto const res = m_dataSource.RegisterMap(file); + CHECK_EQUAL(res.second, MwmSet::RegResult::Success, ()); + + auto const & info = res.first.GetInfo(); + OnMwmBuilt(*info); + } +} + } // namespace tests_support } // namespace generator diff --git a/generator/generator_tests_support/test_with_custom_mwms.hpp b/generator/generator_tests_support/test_with_custom_mwms.hpp index f10b35511d..0854f64e0e 100644 --- a/generator/generator_tests_support/test_with_custom_mwms.hpp +++ b/generator/generator_tests_support/test_with_custom_mwms.hpp @@ -72,6 +72,7 @@ public: void SetMwmVersion(uint32_t version) { m_version = version; } void RegisterLocalMapsInViewport(m2::RectD const & viewport); + void RegisterLocalMapsByPrefix(std::string const & prefix); protected: static void Cleanup(platform::LocalCountryFile const & file); diff --git a/search/geocoder.cpp b/search/geocoder.cpp index 169e56d06d..a57c405c0c 100644 --- a/search/geocoder.cpp +++ b/search/geocoder.cpp @@ -13,6 +13,8 @@ #include "search/tracer.hpp" #include "search/utils.hpp" +#include "storage/country_info_getter.hpp" + #include "indexer/data_source.hpp" #include "indexer/feature_decl.hpp" #include "indexer/ftypes_matcher.hpp" @@ -36,10 +38,6 @@ #include "base/stl_helpers.hpp" #include -#include -#include -#include -#include #include "defines.hpp" @@ -248,9 +246,10 @@ void JoinQueryTokens(QueryParams const & params, TokenRange const & range, UniSt double Area(m2::RectD const & rect) { return rect.IsValid() ? rect.SizeX() * rect.SizeY() : 0; } -// Computes the average similarity between |rect| and |pivot|. By -// similarity between two rects we mean a fraction of the area of -// rects intersection to the area of the smallest rect. +/// @brief Computes the average similarity between |rect| and |pivot|. +/// By similarity between two rects we mean a fraction of the area of +/// rects intersection to the area of the smallest rect. +/// @return [0, 1] double GetSimilarity(m2::RectD const & pivot, m2::RectD const & rect) { double const area = min(Area(pivot), Area(rect)); @@ -297,14 +296,6 @@ unique_ptr GetWorldContext(DataSource const & dataSource) SCOPE_GUARD(tracerGuard, [&] { m_resultTracer.LeaveMethod(ResultTracer::Branch::branch); }) } // namespace -// Geocoder::ExtendedMwmInfos::ExtendedMwmInfo ----------------------------------------------------- -bool Geocoder::ExtendedMwmInfos::ExtendedMwmInfo::operator<( - Geocoder::ExtendedMwmInfos::ExtendedMwmInfo const & rhs) const -{ - if (m_distance == 0.0 && rhs.m_distance == 0.0) - return m_similarity > rhs.m_similarity; - return m_distance < rhs.m_distance; -} // Geocoder::LocalitiesCaches ---------------------------------------------------------------------- Geocoder::LocalitiesCaches::LocalitiesCaches(base::Cancellable const & cancellable) @@ -406,7 +397,7 @@ void Geocoder::GoEverywhere() if (m_params.GetNumTokens() == 0) return; - vector> infos; + vector infos; m_dataSource.GetMwmsInfo(infos); GoImpl(infos, false /* inViewport */); @@ -419,10 +410,10 @@ void Geocoder::GoInViewport() if (m_params.GetNumTokens() == 0) return; - vector> infos; + vector infos; m_dataSource.GetMwmsInfo(infos); - base::EraseIf(infos, [this](shared_ptr const & info) { + base::EraseIf(infos, [this](MwmInfoPtr const & info) { return !m_params.m_pivot.IsIntersect(info->m_bordersRect); }); @@ -458,31 +449,7 @@ void Geocoder::SetParamsForCategorialSearch(Params const & params) LOG(LDEBUG, (static_cast(m_params))); } -Geocoder::ExtendedMwmInfos::ExtendedMwmInfo Geocoder::GetExtendedMwmInfo( - shared_ptr const & info, bool inViewport, - function const &)> const & isMwmWithMatchedCity, - function const &)> const & isMwmWithMatchedState) const -{ - ExtendedMwmInfos::ExtendedMwmInfo extendedInfo; - extendedInfo.m_info = info; - - auto const & rect = info->m_bordersRect; - extendedInfo.m_type.m_viewportIntersected = m_params.m_pivot.IsIntersect(rect); - extendedInfo.m_type.m_containsUserPosition = - m_params.m_position && rect.IsPointInside(*m_params.m_position); - extendedInfo.m_type.m_containsMatchedCity = isMwmWithMatchedCity(info); - extendedInfo.m_type.m_containsMatchedState = isMwmWithMatchedState(info); - - extendedInfo.m_similarity = GetSimilarity(m_params.m_pivot, rect); - if (!inViewport && extendedInfo.m_type.m_containsUserPosition) - extendedInfo.m_distance = 0.0; - else - extendedInfo.m_distance = GetDistanceMeters(m_params.m_pivot.Center(), rect); - return extendedInfo; -} - -Geocoder::ExtendedMwmInfos Geocoder::OrderCountries(bool inViewport, - vector> const & infos) +Geocoder::ExtendedMwmInfos Geocoder::OrderCountries(bool inViewport, vector const & infos) { set mwmsWithCities; set mwmsWithStates; @@ -502,29 +469,72 @@ Geocoder::ExtendedMwmInfos Geocoder::OrderCountries(bool inViewport, } } - ExtendedMwmInfos res; - res.m_infos.reserve(infos.size()); - auto const hasMatchedCity = [&mwmsWithCities](auto const & i) { + auto const hasMatchedCity = [&mwmsWithCities](auto const & i) + { return mwmsWithCities.count(i->GetCountryName()) != 0; }; - auto const hasMatchedState = [&mwmsWithStates](auto const & i) { + auto const hasMatchedState = [&mwmsWithStates](auto const & i) + { return mwmsWithStates.count(i->GetCountryName()) != 0; }; + + std::string locationMwm; + if (m_params.m_position) + locationMwm = m_infoGetter.GetRegionCountryId(*m_params.m_position); + + auto const viewportCenter = m_params.m_pivot.Center(); + std::string const viewportMwm = m_infoGetter.GetRegionCountryId(viewportCenter); + + ExtendedMwmInfos res; + res.m_infos.reserve(infos.size()); for (auto const & info : infos) { - res.m_infos.push_back(GetExtendedMwmInfo(info, inViewport, hasMatchedCity, hasMatchedState)); + ExtendedMwmInfos::ExtendedMwmInfo ei; + ei.m_info = info; + + auto const & rect = info->m_bordersRect; + ei.m_type.m_viewportIntersected = m_params.m_pivot.IsIntersect(rect); + ei.m_type.m_containsUserPosition = m_params.m_position && rect.IsPointInside(*m_params.m_position); + ei.m_type.m_containsMatchedCity = hasMatchedCity(info); + ei.m_type.m_containsMatchedState = hasMatchedState(info); + + // Order MWMs like: + // - World + // - containing viewport center + // - containing user's position (except viewport search mode) + // - less by viewport center or rect similarity + + if (info->GetType() == MwmInfo::WORLD) + ei.m_score = -6; + else if (info->GetCountryName() == viewportMwm) + ei.m_score = -4; + else if (!inViewport && ei.m_type.m_containsUserPosition) + { + ei.m_score = 0; + if (info->GetCountryName() == locationMwm) + ei.m_score = -2; + } + else + ei.m_score = GetDistanceMeters(viewportCenter, rect); + + // Subtract [0, 1] similarity from the score in case of equal viewport distances. + ei.m_score -= GetSimilarity(m_params.m_pivot, rect); + + res.m_infos.push_back(std::move(ei)); } + sort(res.m_infos.begin(), res.m_infos.end()); - auto const firstBatch = [&](auto const & extendedInfo) { + auto const sep = stable_partition(res.m_infos.begin(), res.m_infos.end(), [&](auto const & extendedInfo) + { return extendedInfo.m_type.IsFirstBatchMwm(inViewport); - }; - auto const sep = stable_partition(res.m_infos.begin(), res.m_infos.end(), firstBatch); + }); res.m_firstBatchSize = distance(res.m_infos.begin(), sep); + return res; } -void Geocoder::GoImpl(vector> const & infos, bool inViewport) +void Geocoder::GoImpl(vector const & infos, bool inViewport) { // base::PProf pprof("/tmp/geocoder.prof"); diff --git a/search/geocoder.hpp b/search/geocoder.hpp index 0925f44a70..bd6a3ec52d 100644 --- a/search/geocoder.hpp +++ b/search/geocoder.hpp @@ -14,32 +14,22 @@ #include "search/mode.hpp" #include "search/model.hpp" #include "search/mwm_context.hpp" -#include "search/nested_rects_cache.hpp" #include "search/postcode_points.hpp" -#include "search/pre_ranking_info.hpp" #include "search/query_params.hpp" -#include "search/ranking_utils.hpp" #include "search/streets_matcher.hpp" #include "search/token_range.hpp" #include "search/tracer.hpp" #include "indexer/mwm_set.hpp" -#include "storage/country_info_getter.hpp" - -#include "coding/compressed_bit_vector.hpp" - #include "geometry/point2d.hpp" #include "geometry/rect2d.hpp" #include "base/cancellable.hpp" #include "base/dfa_helpers.hpp" #include "base/levenshtein_dfa.hpp" -#include "base/string_utils.hpp" -#include -#include -#include +#include #include #include #include @@ -147,16 +137,19 @@ private: Count }; + using MwmInfoPtr = std::shared_ptr; struct ExtendedMwmInfos { struct ExtendedMwmInfo { - bool operator<(ExtendedMwmInfo const & rhs) const; + bool operator<(ExtendedMwmInfo const & rhs) const { return m_score < rhs.m_score; } - std::shared_ptr m_info; + MwmInfoPtr m_info; MwmContext::MwmType m_type; - double m_similarity; - double m_distance; + + // Score is a rect distance, with exceptions for World, viewport and users's position. + // Less score is better for search priority. + double m_score; }; std::vector m_infos; @@ -189,7 +182,7 @@ private: // Sets search query params for categorial search. void SetParamsForCategorialSearch(Params const & params); - void GoImpl(std::vector> const & infos, bool inViewport); + void GoImpl(std::vector const & infos, bool inViewport); template using TokenToLocalities = std::map>; @@ -292,13 +285,7 @@ private: // This is a faster wrapper around SearchModel::GetSearchType(), as // it uses pre-loaded lists of streets and villages. - [[nodiscard]] bool GetTypeInGeocoding(BaseContext const & ctx, uint32_t featureId, - Model::Type & type); - - ExtendedMwmInfos::ExtendedMwmInfo GetExtendedMwmInfo( - std::shared_ptr const & info, bool inViewport, - std::function const &)> const & isMwmWithMatchedCity, - std::function const &)> const & isMwmWithMatchedState) const; + [[nodiscard]] bool GetTypeInGeocoding(BaseContext const & ctx, uint32_t featureId, Model::Type & type); // Reorders maps in a way that prefix consists of "best" maps to search and suffix consists of all // other maps ordered by minimum distance from pivot. Returns ExtendedMwmInfos structure which @@ -308,8 +295,7 @@ private: // For non-viewport search mode prefix consists of maps intersecting with pivot, map with user // location and maps with cities matched to the query, sorting prefers mwms that contain the // user's position. - ExtendedMwmInfos OrderCountries(bool inViewport, - std::vector> const & infos); + ExtendedMwmInfos OrderCountries(bool inViewport, std::vector const & infos); DataSource const & m_dataSource; storage::CountryInfoGetter const & m_infoGetter; diff --git a/search/search_quality/helpers.cpp b/search/search_quality/helpers.cpp index 9f83f49e6d..2f78851aee 100644 --- a/search/search_quality/helpers.cpp +++ b/search/search_quality/helpers.cpp @@ -173,10 +173,9 @@ unique_ptr InitSearchEngine( params.m_locale = locale; params.m_numThreads = base::checked_cast(numThreads); - auto infoGetter = storage::CountryInfoReader::CreateCountryInfoGetter(GetPlatform()); - infoGetter->SetAffiliations(&affiliations); - - return make_unique(dataSource, move(infoGetter), params); + auto res = make_unique(dataSource, params); + res->GetCountryInfoGetter().SetAffiliations(&affiliations); + return res; } } // namespace search_quality } // namespace search diff --git a/search/search_quality/search_quality_tests/real_mwm_tests.cpp b/search/search_quality/search_quality_tests/real_mwm_tests.cpp index afa98d66a9..272c2027b8 100644 --- a/search/search_quality/search_quality_tests/real_mwm_tests.cpp +++ b/search/search_quality/search_quality_tests/real_mwm_tests.cpp @@ -7,11 +7,11 @@ namespace real_mwm_tests { -class MwmTestsFixture : public search::tests_support::SearchTest +class MwmTestsFixture : public search::tests_support::SearchTestBase { public: // Pass LDEBUG to verbose logs for debugging. - MwmTestsFixture() : search::tests_support::SearchTest(LINFO) {} + MwmTestsFixture() : search::tests_support::SearchTestBase(LINFO, false /* mockCountryInfo */) {} // Default top POIs count to check types or distances. static size_t constexpr kTopPoiResultsCount = 5; @@ -109,6 +109,11 @@ public: TEST(rect.IsPointInside({ll.m_lon, ll.m_lat}), (r)); } } + + double GetDistanceM(search::Result const & r, ms::LatLon const & ll) const + { + return ms::DistanceOnEarth(ll, mercator::ToLatLon(r.GetFeatureCenter())); + } }; // https://github.com/organicmaps/organicmaps/issues/3026 @@ -306,4 +311,19 @@ UNIT_CLASS_TEST(MwmTestsFixture, Barcelona_Carrers) } } +// https://github.com/organicmaps/organicmaps/issues/3307 +// "Karlstraße" is a common street name in Germany. +UNIT_CLASS_TEST(MwmTestsFixture, Germany_Karlstraße_3) +{ + RegisterLocalMapsByPrefix("Germany"); + // Ulm + SetViewport({48.40420, 9.98604}, 3000); + + auto request = MakeRequest("Karlstraße 3"); + auto const & results = request->Results(); + TEST_GREATER(results.size(), kTopPoiResultsCount, ()); + + // First expected result in Ulm: https://www.openstreetmap.org/node/2293529605#map=19/48.40419/9.98615 + TEST_LESS(GetDistanceM(results[0], {48.4042014, 9.9860426}), 500, ()); +} } // namespace real_mwm_tests diff --git a/search/search_tests_support/helpers.cpp b/search/search_tests_support/helpers.cpp index 006b90e048..279173cbef 100644 --- a/search/search_tests_support/helpers.cpp +++ b/search/search_tests_support/helpers.cpp @@ -13,26 +13,18 @@ namespace tests_support { using namespace std; -SearchTest::SearchTest(base::LogLevel logLevel /* = base::LDEBUG */) - : m_scopedLog(logLevel) - , m_engine(m_dataSource, make_unique(), {}) +SearchTestBase::SearchTestBase(base::LogLevel logLevel, bool mockCountryInfo) + : m_scopedLog(logLevel), m_engine(m_dataSource, {}, mockCountryInfo) { SetViewport(mercator::Bounds::FullRect()); } -void SearchTest::SetViewport(ms::LatLon const & ll, double radiusM) +void SearchTestBase::SetViewport(ms::LatLon const & ll, double radiusM) { SetViewport(mercator::MetersToXY(ll.m_lon, ll.m_lat, radiusM)); } -void SearchTest::RegisterCountry(string const & name, m2::RectD const & rect) -{ - auto & infoGetter = - static_cast(m_engine.GetCountryInfoGetter()); - infoGetter.AddCountry(storage::CountryDef(name, rect)); -} - -bool SearchTest::CategoryMatch(std::string const & query, Rules const & rules, string const & locale) +bool SearchTestBase::CategoryMatch(std::string const & query, Rules const & rules, string const & locale) { TestSearchRequest request(m_engine, query, locale, Mode::Everywhere, m_viewport); request.SetCategorial(); @@ -41,7 +33,7 @@ bool SearchTest::CategoryMatch(std::string const & query, Rules const & rules, s return MatchResults(m_dataSource, rules, request.Results()); } -bool SearchTest::ResultsMatch(std::string const & query, Rules const & rules, +bool SearchTestBase::ResultsMatch(std::string const & query, Rules const & rules, std::string const & locale /* = "en" */, Mode mode /* = Mode::Everywhere */) { @@ -50,45 +42,45 @@ bool SearchTest::ResultsMatch(std::string const & query, Rules const & rules, return MatchResults(m_dataSource, rules, request.Results()); } -bool SearchTest::ResultsMatch(vector const & results, Rules const & rules) +bool SearchTestBase::ResultsMatch(vector const & results, Rules const & rules) { return MatchResults(m_dataSource, rules, results); } -bool SearchTest::ResultsMatch(SearchParams const & params, Rules const & rules) +bool SearchTestBase::ResultsMatch(SearchParams const & params, Rules const & rules) { TestSearchRequest request(m_engine, params); request.Run(); return ResultsMatch(request.Results(), rules); } -bool SearchTest::IsResultMatches(search::Result const & result, Rule const & rule) +bool SearchTestBase::IsResultMatches(search::Result const & result, Rule const & rule) { return tests_support::ResultMatches(m_dataSource, rule, result); } -bool SearchTest::AlternativeMatch(string const & query, vector const & rulesList) +bool SearchTestBase::AlternativeMatch(string const & query, vector const & rulesList) { TestSearchRequest request(m_engine, query, "en", Mode::Everywhere, m_viewport); request.Run(); return tests_support::AlternativeMatch(m_dataSource, rulesList, request.Results()); } -size_t SearchTest::GetResultsNumber(string const & query, string const & locale) +size_t SearchTestBase::GetResultsNumber(string const & query, string const & locale) { TestSearchRequest request(m_engine, query, locale, Mode::Everywhere, m_viewport); request.Run(); return request.Results().size(); } -unique_ptr SearchTest::MakeRequest(SearchParams const & params) +unique_ptr SearchTestBase::MakeRequest(SearchParams const & params) { auto request = make_unique(m_engine, params); request->Run(); return request; } -unique_ptr SearchTest::MakeRequest( +unique_ptr SearchTestBase::MakeRequest( string const & query, string const & locale /* = "en" */) { SearchParams params; @@ -106,7 +98,7 @@ unique_ptr SearchTest::MakeRequest( return request; } -size_t SearchTest::CountFeatures(m2::RectD const & rect) +size_t SearchTestBase::CountFeatures(m2::RectD const & rect) { size_t count = 0; auto counter = [&count](const FeatureType & /* ft */) { ++count; }; @@ -114,7 +106,12 @@ size_t SearchTest::CountFeatures(m2::RectD const & rect) return count; } -// static +void SearchTest::RegisterCountry(string const & name, m2::RectD const & rect) +{ + auto & infoGetter = dynamic_cast(m_engine.GetCountryInfoGetter()); + infoGetter.AddCountry(storage::CountryDef(name, rect)); +} + void SearchTest::OnMwmBuilt(MwmInfo const & info) { switch (info.GetType()) diff --git a/search/search_tests_support/helpers.hpp b/search/search_tests_support/helpers.hpp index 4b4ff960da..2c7378a9e6 100644 --- a/search/search_tests_support/helpers.hpp +++ b/search/search_tests_support/helpers.hpp @@ -18,17 +18,13 @@ namespace search namespace tests_support { -class SearchTest : public TestWithCustomMwms +class SearchTestBase : public TestWithCustomMwms { public: using Rule = std::shared_ptr; using Rules = std::vector; - explicit SearchTest(base::LogLevel logLevel = base::LDEBUG); - ~SearchTest() override = default; - - // Registers country in internal records. Note that physical country file may be absent. - void RegisterCountry(std::string const & name, m2::RectD const & rect); + SearchTestBase(base::LogLevel logLevel, bool mockCountryInfo); inline void SetViewport(m2::RectD const & viewport) { m_viewport = viewport; } void SetViewport(ms::LatLon const & ll, double radiusM); @@ -54,8 +50,6 @@ public: size_t CountFeatures(m2::RectD const & rect); protected: - void OnMwmBuilt(MwmInfo const & /* info */) override; - base::ScopedLogLevelChanger m_scopedLog; TestSearchEngine m_engine; @@ -63,6 +57,21 @@ protected: m2::RectD m_viewport; }; +class SearchTest : public SearchTestBase +{ +public: + explicit SearchTest(base::LogLevel logLevel = base::LDEBUG) + : SearchTestBase(logLevel, true /* mockCountryInfo*/) + { + } + + // Registers country in internal records. Note that physical country file may be absent. + void RegisterCountry(std::string const & name, m2::RectD const & rect); + +protected: + void OnMwmBuilt(MwmInfo const & /* info */) override; +}; + class TestCafe : public generator::tests_support::TestPOI { public: diff --git a/search/search_tests_support/test_search_engine.cpp b/search/search_tests_support/test_search_engine.cpp index f993d88144..010116fc74 100644 --- a/search/search_tests_support/test_search_engine.cpp +++ b/search/search_tests_support/test_search_engine.cpp @@ -12,16 +12,10 @@ namespace tests_support { using namespace std; -TestSearchEngine::TestSearchEngine(DataSource & dataSource, - unique_ptr infoGetter, - Engine::Params const & params) - : m_infoGetter(move(infoGetter)) - , m_engine(dataSource, GetDefaultCategories(), *m_infoGetter, params) -{ -} - -TestSearchEngine::TestSearchEngine(DataSource & dataSource, Engine::Params const & params) - : m_infoGetter(storage::CountryInfoReader::CreateCountryInfoGetter(GetPlatform())) +TestSearchEngine::TestSearchEngine(DataSource & dataSource, Engine::Params const & params, bool mockCountryInfo) + : m_infoGetter(mockCountryInfo ? + make_unique() : + storage::CountryInfoReader::CreateCountryInfoGetter(GetPlatform())) , m_engine(dataSource, GetDefaultCategories(), *m_infoGetter, params) { } diff --git a/search/search_tests_support/test_search_engine.hpp b/search/search_tests_support/test_search_engine.hpp index aa76886c24..676617ed05 100644 --- a/search/search_tests_support/test_search_engine.hpp +++ b/search/search_tests_support/test_search_engine.hpp @@ -18,9 +18,7 @@ namespace tests_support class TestSearchEngine { public: - TestSearchEngine(DataSource & dataSource, std::unique_ptr infoGetter, - Engine::Params const & params); - TestSearchEngine(DataSource & dataSource, Engine::Params const & params); + TestSearchEngine(DataSource & dataSource, Engine::Params const & params, bool mockCountryInfo = false); void SetLocale(std::string const & locale) { m_engine.SetLocale(locale); }