From 37aff68a2ab6bfcc85092d7fd4a7b2860705059f Mon Sep 17 00:00:00 2001 From: Vladimir Byko-Ianko Date: Tue, 20 Nov 2018 11:34:32 +0300 Subject: [PATCH] [routing] Section format was changed. Using delta coding for storing feature id in bidirectional case. Review fixes. --- generator/camera_node_processor.hpp | 2 +- generator/generator_tests/maxspeed_tests.cpp | 5 +--- generator/maxspeed_builder.cpp | 7 ++--- generator/maxspeed_builder.hpp | 8 +++--- generator/maxspeed_parser.hpp | 2 +- routing/maxspeeds_serialization.hpp | 28 +++++++++++++++++--- routing_common/car_model.cpp | 4 +-- routing_common/maxspeed_conversion.hpp | 2 +- 8 files changed, 39 insertions(+), 19 deletions(-) diff --git a/generator/camera_node_processor.hpp b/generator/camera_node_processor.hpp index 2715d92ad4..44de7ad57c 100644 --- a/generator/camera_node_processor.hpp +++ b/generator/camera_node_processor.hpp @@ -67,7 +67,7 @@ private: /// \brief Gets text with speed, returns formatted speed string in km per hour. /// \param maxSpeedString - text with speed. Possible format: /// "130" - means 130 km per hour. - /// "130 mph" - means 130 mile per hour. + /// "130 mph" - means 130 miles per hour. /// "130 kmh" - means 130 km per hour. /// See https://wiki.openstreetmap.org/wiki/Key:maxspeed /// for more details about input string. diff --git a/generator/generator_tests/maxspeed_tests.cpp b/generator/generator_tests/maxspeed_tests.cpp index dfff64e9ce..c9e36faa11 100644 --- a/generator/generator_tests/maxspeed_tests.cpp +++ b/generator/generator_tests/maxspeed_tests.cpp @@ -80,7 +80,7 @@ void TestMaxspeedSection(Features const & roads, string const & maxspeedCsvConte // Creating maxspeed section in test.mwm. string const testMwmFullPath = base::JoinPath(testDirFullPath, testMwm); - BuildMaxspeed(testMwmFullPath, featureIdToOsmId, base::JoinPath(testDirFullPath, kCsv)); + BuildMaxspeedSection(testMwmFullPath, featureIdToOsmId, base::JoinPath(testDirFullPath, kCsv)); // Loading maxspeed section. FrozenDataSource dataSource; @@ -137,7 +137,6 @@ UNIT_TEST(ParseMaxspeeds_Smoke) UNIT_TEST(ParseMaxspeeds1) { -// string const maxspeedCsvContent = "10,Metric,60\n11,Metric,90\n"; string const maxspeedCsvContent = R"(10,Metric,60 11,Metric,90)"; OsmIdToMaxspeed const expectedMapping = { @@ -150,7 +149,6 @@ UNIT_TEST(ParseMaxspeeds1) UNIT_TEST(ParseMaxspeeds2) { -// string const maxspeedCsvContent = "10,Metric,60,80\n11,Metric,120\n"; string const maxspeedCsvContent = R"(10,Metric,60,80 11,Metric,120)"; OsmIdToMaxspeed const expectedMapping = { @@ -163,7 +161,6 @@ UNIT_TEST(ParseMaxspeeds2) UNIT_TEST(ParseMaxspeeds3) { -// string const maxspeedCsvContent = "184467440737095516,Imperial,60,80\n184467440737095517,Metric,120\n"; string const maxspeedCsvContent = R"(184467440737095516,Imperial,60,80 184467440737095517,Metric,120)"; diff --git a/generator/maxspeed_builder.cpp b/generator/maxspeed_builder.cpp index 53968c3dd3..b6246c72b5 100644 --- a/generator/maxspeed_builder.cpp +++ b/generator/maxspeed_builder.cpp @@ -171,8 +171,9 @@ void SerializeMaxspeeds(string const & dataPath, vector && spee LOG(LINFO, ("SerializeMaxspeeds(", dataPath, ", ...) serialized:", speeds.size(), "maxspeed tags.")); } -void BuildMaxspeed(string const & dataPath, map const & featureIdToOsmId, - string const & maxspeedFilename) +void BuildMaxspeedSection(string const & dataPath, + map const & featureIdToOsmId, + string const & maxspeedFilename) { MaxspeedMwmCollector collector(dataPath, featureIdToOsmId, maxspeedFilename); SerializeMaxspeeds(dataPath, collector.StealMaxspeeds()); @@ -185,6 +186,6 @@ void BuildMaxspeedSection(string const & dataPath, string const & osmToFeaturePa map featureIdToOsmId; CHECK(ParseFeatureIdToOsmIdMapping(osmToFeaturePath, featureIdToOsmId), ()); - BuildMaxspeed(dataPath, featureIdToOsmId, maxspeedFilename); + BuildMaxspeedSection(dataPath, featureIdToOsmId, maxspeedFilename); } } // namespace routing diff --git a/generator/maxspeed_builder.hpp b/generator/maxspeed_builder.hpp index bbd5e9f569..fe955dea4a 100644 --- a/generator/maxspeed_builder.hpp +++ b/generator/maxspeed_builder.hpp @@ -22,12 +22,12 @@ bool ParseMaxspeeds(std::string const & maxspeedFilename, OsmIdToMaxspeed & osmI /// \brief Writes |speeds| to maxspeed section to mwm with |dataPath|. void SerializeMaxspeeds(std::string const & dataPath, std::vector && speeds); -void BuildMaxspeed(std::string const & dataPath, - std::map const & featureIdToOsmId, - std::string const & maxspeedFilename); +void BuildMaxspeedSection(std::string const & dataPath, + std::map const & featureIdToOsmId, + std::string const & maxspeedFilename); /// \brief Builds maxspeed section in mwm with |dataPath|. This section contains max speed limits -/// if it's available. +/// if they are available in file |maxspeedFilename|. /// \param maxspeedFilename file name to csv file with maxspeed tag values. /// \note To start building the section, the following data must be ready: /// 1. GenerateIntermediateData(). Saves to a file data about maxspeed tags value of road features diff --git a/generator/maxspeed_parser.hpp b/generator/maxspeed_parser.hpp index cf8a7cf74f..b55a7988de 100644 --- a/generator/maxspeed_parser.hpp +++ b/generator/maxspeed_parser.hpp @@ -30,6 +30,6 @@ bool ParseMaxspeedTag(std::string const & maxspeedValue, routing::SpeedInUnits & std::string UnitsToString(measurement_utils::Units units); /// \brief Converts string to measurement_utils::Units. -/// \note |units| should be equale to "Metric" or "Imperial". +/// \note |units| should be "Metric" or "Imperial". measurement_utils::Units StringToUnits(std::string const & units); } // namespace generator diff --git a/routing/maxspeeds_serialization.hpp b/routing/maxspeeds_serialization.hpp index e8de1fb3bd..6bd98bedba 100644 --- a/routing/maxspeeds_serialization.hpp +++ b/routing/maxspeeds_serialization.hpp @@ -96,6 +96,7 @@ public: header.m_bidirectionalMaxspeedOffset = static_cast(sink.Pos() - startOffset); // Keeping bidirectional maxspeeds. + uint32_t prevFeatureId = 0; for (auto const & s : speeds) { if (!s.IsBidirectional()) @@ -107,7 +108,15 @@ public: continue; // No appropriate speed for |s| in SpeedMacro enum. ++header.m_bidirectionalMaxspeedNumber; - WriteToSink(sink, s.GetFeatureId()); + // Note. |speeds| sorted by feature ids and they are unique. So the first item in |speed| + // has feature id 0 or greater. And the second item in |speed| has feature id grater than 0. + // This guarantees that for only first saved |s| the if branch below is called. + if (prevFeatureId != 0) + CHECK_GREATER(s.GetFeatureId(), prevFeatureId, ()); + uint32_t delta = (prevFeatureId == 0 ? s.GetFeatureId() : s.GetFeatureId() - prevFeatureId); + prevFeatureId = s.GetFeatureId(); + + WriteToSink(sink, delta); WriteToSink(sink, static_cast(forwardMacro)); WriteToSink(sink, static_cast(backwardMacro)); } @@ -120,13 +129,23 @@ public: LOG(LINFO, ("Serialized", serializedForwardNumber, "forward maxspeeds and", header.m_bidirectionalMaxspeedNumber, "bidirectional maxspeeds. Section size:", endOffset - startOffset, "bytes.")); + + size_t constexpr kBidirectionalMaxspeedLimit = 2000; + if (header.m_bidirectionalMaxspeedNumber > kBidirectionalMaxspeedLimit) + { + LOG(LWARNING, + ("There's to many bidirectional maxpeed tags:", header.m_bidirectionalMaxspeedNumber, + ". All maxpeed tags are serialized correctly but they may take too large space in mwm. " + "Consider about changing section format to keep bidirectional maxspeeds in another " + "way.")); + } } template static void Deserialize(Source & src, Maxspeeds & maxspeeds) { // Note. Now it's assumed that only little-endian architectures are supported. - // (See a check at the beginning of main method in generator_tools.cpp) If it's necessary + // (See a check at the beginning of main method in generator_tool.cpp) If it's necessary // to support big-endian architectures code below should be modified. CHECK(maxspeeds.IsEmpty(), ()); @@ -151,9 +170,10 @@ public: } maxspeeds.m_bidirectionalMaxspeeds.reserve(header.m_bidirectionalMaxspeedNumber); + uint32_t prevFeatureId = 0; for (size_t i = 0; i < header.m_bidirectionalMaxspeedNumber; ++i) { - auto const fid = ReadPrimitiveFromSource(src); + auto const delta = ReadPrimitiveFromSource(src); auto const forward = ReadPrimitiveFromSource(src); auto const backward = ReadPrimitiveFromSource(src); CHECK(GetMaxspeedConverter().IsValidMacro(forward), (i)); @@ -173,6 +193,8 @@ public: // Note. If neither |forwardSpeed| nor |backwardSpeed| are numeric it means // both of them have value "walk" or "none". So the units are not relevant for this case. + prevFeatureId += delta; + uint32_t fid = (i == 0 ? delta : prevFeatureId); maxspeeds.m_bidirectionalMaxspeeds.emplace_back(fid, units, forwardSpeed.GetSpeed(), backwardSpeed.GetSpeed()); } diff --git a/routing_common/car_model.cpp b/routing_common/car_model.cpp index 174901458a..a1db1510a4 100644 --- a/routing_common/car_model.cpp +++ b/routing_common/car_model.cpp @@ -240,8 +240,8 @@ SpeedKMpH CarModel::GetSpeed(FeatureType & f, SpeedParams const & speedParams) c // Note. It's the first rough attempt using maxspeed tag value for speed calculation. // It's used as a feature speed if it's valid and less then some value. // @TODO maxspeed tag value should be used more sophisticated. - uint16_t const maxspeedBasedspeedKmPH = speedParams.m_maxspeed.GetSpeedKmPH(speedParams.m_forward); - auto const speedKmPH = min(static_cast(maxspeedBasedspeedKmPH), GetMaxWeightSpeed()); + uint16_t const maxspeedBasedSpeedKmPH = speedParams.m_maxspeed.GetSpeedKmPH(speedParams.m_forward); + auto const speedKmPH = min(static_cast(maxspeedBasedSpeedKmPH), GetMaxWeightSpeed()); return {speedKmPH /* weight */, speedKmPH /* eta */}; } diff --git a/routing_common/maxspeed_conversion.hpp b/routing_common/maxspeed_conversion.hpp index a54c1560dc..0886f10578 100644 --- a/routing_common/maxspeed_conversion.hpp +++ b/routing_common/maxspeed_conversion.hpp @@ -10,7 +10,7 @@ namespace routing { -/// \brief This enum class contains most popular maxspeed value according to +/// \brief This enum class contains the most popular maxspeed values according to /// https://taginfo.openstreetmap.org/keys/maxspeed#values. /// \note Value of this enum is saved to mwm. So they should not be changed because of backward /// compatibility. But it's possible to add some new values to this enum.