From 51f01fb05559f89de38686dacc318f9d94b89e86 Mon Sep 17 00:00:00 2001 From: Sergey Magidovich Date: Wed, 3 Aug 2016 22:42:28 +0300 Subject: [PATCH] Code review. --- generator/booking_dataset.cpp | 57 +++++++--------- generator/booking_dataset.hpp | 22 ++++--- .../booking_quality_check.cpp | 17 ++--- .../booking_quality_check.pro | 38 +++++++++++ generator/osm_source.cpp | 65 ++++++++++--------- generator/osm_source.hpp | 7 ++ generator/osm_translator.hpp | 4 +- indexer/classificator_loader.cpp | 42 ++++++------ omim.pro | 12 ++-- 9 files changed, 151 insertions(+), 113 deletions(-) create mode 100644 generator/booking_quality_check/booking_quality_check.pro diff --git a/generator/booking_dataset.cpp b/generator/booking_dataset.cpp index d93b3bac31..f2f2bb3373 100644 --- a/generator/booking_dataset.cpp +++ b/generator/booking_dataset.cpp @@ -16,7 +16,7 @@ #include "std/fstream.hpp" #include "std/iostream.hpp" -#include "std/limits.hpp" + #include "std/sstream.hpp" namespace generator @@ -125,13 +125,15 @@ size_t BookingDataset::GetMatchingHotelIndex(FeatureBuilder1 const & fb) const { if (CanBeBooking(fb)) return MatchWithBooking(fb); - return numeric_limits::max(); + return kInvalidHotelIndex; } -// TODO(mgsergio): rename to ... or delete. -bool BookingDataset::TourismFilter(FeatureBuilder1 const & fb) const +bool BookingDataset::CanBeBooking(FeatureBuilder1 const & fb) const { - return CanBeBooking(fb); + if (fb.GetName(StringUtf8Multilang::kDefaultCode).empty()) + return false; + + return ftypes::IsHotelChecker::Instance()(fb.GetTypes()); } BookingDataset::Hotel const & BookingDataset::GetHotel(size_t index) const @@ -146,17 +148,17 @@ BookingDataset::Hotel & BookingDataset::GetHotel(size_t index) return m_hotels[index]; } -vector BookingDataset::GetNearestHotels(double lat, double lon, size_t limit, - double maxDistance /* = 0.0 */) const +vector BookingDataset::GetNearestHotels(ms::LatLon const & latLon, size_t const limit, + double const maxDistance /* = 0.0 */) const { namespace bgi = boost::geometry::index; vector indexes; - for_each(bgi::qbegin(m_rtree, bgi::nearest(TPoint(lat, lon), limit)), bgi::qend(m_rtree), - [&](TValue const & v) + for_each(bgi::qbegin(m_rtree, bgi::nearest(TPoint(latLon.lat, latLon.lon), limit)), + bgi::qend(m_rtree), [&](TValue const & v) { auto const & hotel = GetHotel(v.second); - double const dist = ms::DistanceOnEarth(lat, lon, hotel.lat, hotel.lon); + double const dist = ms::DistanceOnEarth(latLon.lat, latLon.lon, hotel.lat, hotel.lon); if (maxDistance != 0.0 && dist > maxDistance /* max distance in meters */) return; @@ -166,15 +168,15 @@ vector BookingDataset::GetNearestHotels(double lat, double lon, size_t l return indexes; } -void BookingDataset::BuildFeature(size_t const hotelIndex, - function const & fn) const +void BookingDataset::BuildHotel(size_t const hotelIndex, + function const & fn) const { auto const & hotel = m_hotels[hotelIndex]; - FeatureBuilder1 bookingFb; + FeatureBuilder1 fb; FeatureParams params; - bookingFb.SetCenter(MercatorBounds::FromLatLon(hotel.lat, hotel.lon)); + fb.SetCenter(MercatorBounds::FromLatLon(hotel.lat, hotel.lon)); auto & metadata = params.GetMetadata(); metadata.Set(feature::Metadata::FMD_SPONSORED_ID, strings::to_string(hotel.id)); @@ -187,10 +189,10 @@ void BookingDataset::BuildFeature(size_t const hotelIndex, // TODO(mgsergio): addr:full ??? if (!hotel.street.empty()) - bookingFb.AddStreet(hotel.street); + fb.AddStreet(hotel.street); if (!hotel.houseNumber.empty()) - bookingFb.AddHouseNumber(hotel.houseNumber); + fb.AddHouseNumber(hotel.houseNumber); params.AddName(StringUtf8Multilang::GetLangByCode(StringUtf8Multilang::kDefaultCode), hotel.name); @@ -199,7 +201,7 @@ void BookingDataset::BuildFeature(size_t const hotelIndex, // TODO(mgsergio): Move parsing to the hotel costruction stage. vector parts; strings::ParseCSVRow(hotel.translations, '|', parts); - CHECK(parts.size() % 3 == 0, ()); + CHECK_EQUAL(parts.size() % 3, 0, ("Invalid translation string:", hotel.translations)); for (auto i = 0; i < parts.size(); i += 3) { auto const langCode = StringUtf8Multilang::GetLangIndex(parts[i]); @@ -261,9 +263,9 @@ void BookingDataset::BuildFeature(size_t const hotelIndex, default: params.AddType(clf.GetTypeByPath({"tourism", "hotel"})); break; } - bookingFb.SetParams(params); + fb.SetParams(params); - fn(bookingFb); + fn(fb); } void BookingDataset::LoadHotels(istream & src, string const & addressReferencePath) @@ -315,10 +317,9 @@ size_t BookingDataset::MatchWithBooking(FeatureBuilder1 const & fb) const if (name.empty()) return false; - auto const center = MercatorBounds::ToLatLon(fb.GetKeyPoint()); // Find |kMaxSelectedElements| nearest values to a point. - auto const bookingIndexes = - GetNearestHotels(center.lat, center.lon, kMaxSelectedElements, kDistanceLimitInMeters); + auto const bookingIndexes = GetNearestHotels(MercatorBounds::ToLatLon(fb.GetKeyPoint()), + kMaxSelectedElements, kDistanceLimitInMeters); for (size_t const j : bookingIndexes) { @@ -326,16 +327,6 @@ size_t BookingDataset::MatchWithBooking(FeatureBuilder1 const & fb) const return j; } - return numeric_limits::max(); -} - -bool BookingDataset::CanBeBooking(FeatureBuilder1 const & fb) const -{ - // TODO(mgsergio): Remove me after refactoring is done and tested. - // Or remove the entire filter func. - if (fb.GetName(StringUtf8Multilang::kDefaultCode).empty()) - return false; - - return ftypes::IsHotelChecker::Instance()(fb.GetTypes()); + return kInvalidHotelIndex; } } // namespace generator diff --git a/generator/booking_dataset.hpp b/generator/booking_dataset.hpp index 0193d4f001..9eb13e778f 100644 --- a/generator/booking_dataset.hpp +++ b/generator/booking_dataset.hpp @@ -10,6 +10,7 @@ #include "boost/geometry/index/rtree.hpp" #include "std/function.hpp" +#include "std/limits.hpp" #include "std/string.hpp" class FeatureBuilder1; @@ -19,8 +20,9 @@ namespace generator class BookingDataset { public: - double static constexpr kDistanceLimitInMeters = 150; - size_t static constexpr kMaxSelectedElements = 3; + static double constexpr kDistanceLimitInMeters = 150; + static size_t constexpr kMaxSelectedElements = 3; + static size_t constexpr kInvalidHotelIndex = numeric_limits::max(); struct Hotel { @@ -77,33 +79,33 @@ public: explicit BookingDataset(string const & dataPath, string const & addressReferencePath = string()); explicit BookingDataset(istream & dataSource, string const & addressReferencePath = string()); - /// @returns an index of a matched hotel or numeric_limits::max on failure. - size_t GetMatchingHotelIndex(FeatureBuilder1 const & e) const; - bool TourismFilter(FeatureBuilder1 const & e) const; + /// @return an index of a matched hotel or kInvalidHotelIndex on failure. + size_t GetMatchingHotelIndex(FeatureBuilder1 const & fb) const; + /// @return true if |fb| is a hotel with a name. + bool CanBeBooking(FeatureBuilder1 const & fb) const; inline size_t Size() const { return m_hotels.size(); } Hotel const & GetHotel(size_t index) const; Hotel & GetHotel(size_t index); - vector GetNearestHotels(double lat, double lon, size_t limit, + vector GetNearestHotels(ms::LatLon const & latLon, size_t limit, double maxDistance = 0.0) const; bool MatchByName(string const & osmName, vector const & bookingIndexes) const; - void BuildFeature(size_t hotelIndex, function const & fn) const; + void BuildHotel(size_t hotelIndex, function const & fn) const; protected: vector m_hotels; - // create the rtree using default constructor using TPoint = boost::geometry::model::point; using TBox = boost::geometry::model::box; using TValue = pair; + // Create the rtree using default constructor. boost::geometry::index::rtree> m_rtree; void LoadHotels(istream & path, string const & addressReferencePath); - /// @returns an index of a matched hotel or numeric_limits::max() on failure. + /// @return an index of a matched hotel or numeric_limits::max() on failure. size_t MatchWithBooking(FeatureBuilder1 const & e) const; - bool CanBeBooking(FeatureBuilder1 const & e) const; }; ostream & operator<<(ostream & s, BookingDataset::Hotel const & h); diff --git a/generator/booking_quality_check/booking_quality_check.cpp b/generator/booking_quality_check/booking_quality_check.cpp index 3257ffe5f4..e6dc0acb9b 100644 --- a/generator/booking_quality_check/booking_quality_check.cpp +++ b/generator/booking_quality_check/booking_quality_check.cpp @@ -7,6 +7,8 @@ #include "geometry/distance_on_sphere.hpp" +#include "coding/file_name_utils.hpp" + #include "std/fstream.hpp" #include "std/iostream.hpp" #include "std/numeric.hpp" @@ -15,8 +17,6 @@ #include "3party/gflags/src/gflags/gflags.h" -// TODO(mgsergio):Unused: DEFINE_bool(generate_classif, false, "Generate classificator."); - DEFINE_string(osm_file_name, "", "Input .o5m file"); DEFINE_string(booking_data, "", "Path to booking data in .tsv format"); DEFINE_string(sample_data, "", "Sample output path"); @@ -72,7 +72,7 @@ struct Emitter : public EmitterBase void operator()(FeatureBuilder1 & fb) override { - if (m_bookingDataset.TourismFilter(fb)) + if (m_bookingDataset.CanBeBooking(fb)) m_features.emplace_back(fb); } @@ -96,9 +96,8 @@ struct Emitter : public EmitterBase for (size_t i : elementIndexes) { auto const & fb = m_features[i]; - auto const center = MercatorBounds::ToLatLon(fb.GetKeyPoint()); auto const bookingIndexes = m_bookingDataset.GetNearestHotels( - center.lat, center.lon, + MercatorBounds::ToLatLon(fb.GetKeyPoint()), BookingDataset::kMaxSelectedElements, BookingDataset::kDistanceLimitInMeters); @@ -167,12 +166,10 @@ feature::GenerateInfo GetGenerateInfo() info.SetNodeStorageType("map"); info.SetOsmFileType("o5m"); - auto const lastSlash = FLAGS_sample_data.rfind("/"); - if (lastSlash == string::npos) - info.m_intermediateDir = "."; - else - info.m_intermediateDir = FLAGS_sample_data.substr(0, lastSlash); + info.m_intermediateDir = my::GetDirectory(FLAGS_sample_data); + // ... + return info; } } // namespace diff --git a/generator/booking_quality_check/booking_quality_check.pro b/generator/booking_quality_check/booking_quality_check.pro new file mode 100644 index 0000000000..2e667d44ea --- /dev/null +++ b/generator/booking_quality_check/booking_quality_check.pro @@ -0,0 +1,38 @@ +# Base functions tests. + +TARGET = booking_quality_check +CONFIG += console warn_on +CONFIG -= app_bundle +TEMPLATE = app + +ROOT_DIR = ../.. +DEPENDENCIES = \ + generator \ + search \ + routing \ + indexer \ + geometry \ + editor \ + platform \ + coding \ + base \ + tomcrypt \ + jansson \ + pugixml \ + stats_client \ + opening_hours \ + gflags \ + oauthcpp \ + expat \ + protobuf \ + +include($$ROOT_DIR/common.pri) + +INCLUDEPATH *= $$ROOT_DIR/3party/gflags/src + +QT *= core + +SOURCES += \ + booking_quality_check.cpp \ + +HEADERS += diff --git a/generator/osm_source.cpp b/generator/osm_source.cpp index 4c6a401ea6..2fe3cb9b57 100644 --- a/generator/osm_source.cpp +++ b/generator/osm_source.cpp @@ -20,10 +20,11 @@ #include "geometry/mercator.hpp" #include "geometry/tree4d.hpp" +#include "base/stl_helpers.hpp" + #include "coding/parse_xml.hpp" #include "std/fstream.hpp" -#include "std/limits.hpp" #include "defines.hpp" @@ -188,21 +189,6 @@ public: /// Used to make a "good" node for a highway graph with OSRM for low zooms. class Place { - FeatureBuilder1 m_ft; - m2::PointD m_pt; - uint32_t m_type; - double m_thresholdM; - - bool IsPoint() const { return (m_ft.GetGeomType() == feature::GEOM_POINT); } - static bool IsEqualTypes(uint32_t t1, uint32_t t2) - { - // Use 2-arity places comparison for filtering. - // ("place-city-capital-2" is equal to "place-city") - ftype::TruncValue(t1, 2); - ftype::TruncValue(t2, 2); - return (t1 == t2); - } - public: Place(FeatureBuilder1 const & ft, uint32_t type) : m_ft(ft), m_pt(ft.GetKeyPoint()), m_type(type) { @@ -228,7 +214,7 @@ public: bool IsEqual(Place const & r) const { - return (IsEqualTypes(m_type, r.m_type) && + return (AreTypesEqual(m_type, r.m_type) && m_ft.GetName() == r.m_ft.GetName() && (IsPoint() || r.IsPoint()) && MercatorBounds::DistanceOnEarth(m_pt, r.m_pt) < m_thresholdM); @@ -241,20 +227,38 @@ public: uint8_t const r1 = m_ft.GetRank(); uint8_t const r2 = r.m_ft.GetRank(); if (r1 != r2) - return (r2 < r1); + return r1 > r2; // Check types length. // ("place-city-capital-2" is better than "place-city"). uint8_t const l1 = ftype::GetLevel(m_type); uint8_t const l2 = ftype::GetLevel(r.m_type); if (l1 != l2) - return (l2 < l1); + return l1 > l2; // Assume that area places has better priority than point places at the very end ... /// @todo It was usefull when place=XXX type has any area fill style. /// Need to review priority logic here (leave the native osm label). return !IsPoint(); } + +private: + bool IsPoint() const { return (m_ft.GetGeomType() == feature::GEOM_POINT); } + + static bool AreTypesEqual(uint32_t t1, uint32_t t2) + { + // Use 2-arity places comparison for filtering. + // ("place-city-capital-2" is equal to "place-city") + ftype::TruncValue(t1, 2); + ftype::TruncValue(t2, 2); + return (t1 == t2); + } + + FeatureBuilder1 m_ft; + m2::PointD m_pt; + uint32_t m_type; + double m_thresholdM; + }; class MainFeaturesEmitter : public EmitterBase @@ -294,9 +298,9 @@ class MainFeaturesEmitter : public EmitterBase public: MainFeaturesEmitter(feature::GenerateInfo const & info) - : m_skippedElementsPath(info.GetIntermediateFileName("skipped_elements", ".lst")) - , m_failOnCoasts(info.m_failOnCoasts) - , m_bookingDataset(info.m_bookingDatafileName, info.m_bookingReferenceDir) + : m_skippedElementsPath(info.GetIntermediateFileName("skipped_elements", ".lst")) + , m_failOnCoasts(info.m_failOnCoasts) + , m_bookingDataset(info.m_bookingDatafileName, info.m_bookingReferenceDir) { Classificator const & c = classif(); @@ -338,7 +342,7 @@ public: static uint32_t const placeType = classif().GetTypeByPath({"place"}); uint32_t const type = fb.GetParams().FindType(placeType, 1); - auto hotelIndex = numeric_limits::max(); + auto hotelIndex = generator::BookingDataset::kInvalidHotelIndex; if (type != ftype::GetEmptyValue() && !fb.GetName().empty()) { @@ -348,11 +352,11 @@ public: [](Place const & p1, Place const & p2) { return p1.IsBetterThan(p2); }); } else if ((hotelIndex = m_bookingDataset.GetMatchingHotelIndex(fb)) != - numeric_limits::max()) + generator::BookingDataset::kInvalidHotelIndex) { m_skippedElements << DebugPrint(fb.GetMostGenericOsmId()) << endl; - // Make a hotel a simple building. + // Turn a hotel into a simple building. if (fb.GetGeomType() == feature::GEOM_AREA) { // Remove all information about a hotel. @@ -363,14 +367,13 @@ public: meta.Drop(feature::Metadata::EType::FMD_WEBSITE); meta.Drop(feature::Metadata::EType::FMD_PHONE_NUMBER); - auto & types = params.m_Types; - types.erase(remove_if(begin(types), end(types), [](uint32_t type) + auto const & c = classif(); + auto const tourism = c.GetTypeByPath({"tourism"}); + my::EraseIf(params.m_Types, [&c, tourism](uint32_t type) { - static auto const & c = classif(); - static auto tourism = c.GetTypeByPath({"tourism"}); ftype::TruncValue(type, 1); return type == tourism; - })); + }); fb.SetParams(params); Emit(fb); @@ -389,7 +392,7 @@ public: // Emit all booking objecs to the map. for (size_t hotelIndex = 0; hotelIndex < m_bookingDataset.Size(); ++hotelIndex) - m_bookingDataset.BuildFeature(hotelIndex, [this](FeatureBuilder1 & fb) { Emit(fb); }); + m_bookingDataset.BuildHotel(hotelIndex, [this](FeatureBuilder1 & fb) { Emit(fb); }); m_places.ForEach([this](Place const & p) { diff --git a/generator/osm_source.hpp b/generator/osm_source.hpp index 90dea9e702..d5688043e9 100644 --- a/generator/osm_source.hpp +++ b/generator/osm_source.hpp @@ -32,12 +32,19 @@ public: class FeatureBuilder1; +// Emitter is used in OsmElemen to FeatureBuilder translation process. class EmitterBase { public: virtual ~EmitterBase() = default; + + /// This method is used by OsmTranslator to pass |fb| to Emitter for further processing. virtual void operator()(FeatureBuilder1 & fb) = 0; + /// Finish is used in GenerateFeatureImpl to make whatever work is needed after + /// all OmsElements are processed. virtual bool Finish() { return true; } + /// Sets buckets (mwm names). + // TODO(syershov): Make this topic clear. virtual void GetNames(vector & names) const = 0; }; diff --git a/generator/osm_translator.hpp b/generator/osm_translator.hpp index a0b171da11..ffa9c641fb 100644 --- a/generator/osm_translator.hpp +++ b/generator/osm_translator.hpp @@ -483,8 +483,8 @@ public: } public: - OsmToFeatureTranslator(TEmitter & emitter, TCache & holder, - uint32_t coastType, string const & addrFilePath = {}) + OsmToFeatureTranslator(TEmitter & emitter, TCache & holder, uint32_t coastType, + string const & addrFilePath = {}) : m_emitter(emitter), m_holder(holder), m_coastType(coastType) { if (!addrFilePath.empty()) diff --git a/indexer/classificator_loader.cpp b/indexer/classificator_loader.cpp index 29dcd831fa..d996235d07 100644 --- a/indexer/classificator_loader.cpp +++ b/indexer/classificator_loader.cpp @@ -40,30 +40,30 @@ void ReadCommon(unique_ptr classificator, namespace classificator { - void Load() +void Load() +{ + LOG(LDEBUG, ("Reading of classificator started")); + + Platform & p = GetPlatform(); + + MapStyle const originMapStyle = GetStyleReader().GetCurrentStyle(); + + for (size_t i = 0; i < MapStyleCount; ++i) { - LOG(LDEBUG, ("Reading of classificator started")); - - Platform & p = GetPlatform(); - - MapStyle const originMapStyle = GetStyleReader().GetCurrentStyle(); - - for (size_t i = 0; i < MapStyleCount; ++i) + MapStyle const mapStyle = static_cast(i); + // Read the merged style only if it was requested. + if (mapStyle != MapStyleMerged || originMapStyle == MapStyleMerged) { - MapStyle const mapStyle = static_cast(i); - // Read the merged style only if it was requested. - if (mapStyle != MapStyleMerged || originMapStyle == MapStyleMerged) - { - GetStyleReader().SetCurrentStyle(mapStyle); - ReadCommon(p.GetReader("classificator.txt"), - p.GetReader("types.txt")); + GetStyleReader().SetCurrentStyle(mapStyle); + ReadCommon(p.GetReader("classificator.txt"), + p.GetReader("types.txt")); - drule::LoadRules(); - } + drule::LoadRules(); } - - GetStyleReader().SetCurrentStyle(originMapStyle); - - LOG(LDEBUG, ("Reading of classificator finished")); } + + GetStyleReader().SetCurrentStyle(originMapStyle); + + LOG(LDEBUG, ("Reading of classificator finished")); +} } // namespace classificator diff --git a/omim.pro b/omim.pro index 4a16aece48..57719405db 100644 --- a/omim.pro +++ b/omim.pro @@ -34,18 +34,18 @@ SUBDIRS = 3party base coding geometry editor indexer routing search generator_tool.subdir = generator/generator_tool generator_tool.depends = $$SUBDIRS SUBDIRS *= generator_tool - - # Booking quality check - booking_quality_check.subdir = generator/booking_quality_check - booking_quality_check.depends = $$SUBDIRS - SUBDIRS *= booking_quality_check - } + } # Integration tests dependencies for gtool. # TODO(AlexZ): Avoid duplication for routing_integration_tests. CONFIG(gtool):!CONFIG(no-tests) { SUBDIRS *= map + # Booking quality check + booking_quality_check.subdir = generator/booking_quality_check + booking_quality_check.depends = $$SUBDIRS + SUBDIRS *= booking_quality_check + routing_integration_tests.subdir = routing/routing_integration_tests routing_integration_tests.depends = $$SUBDIRS routing_consistency_tests.subdir = routing/routing_consistency_tests