diff --git a/generator/generator_tests/maxspeeds_tests.cpp b/generator/generator_tests/maxspeeds_tests.cpp index 114987de06..f4f1d2aa26 100644 --- a/generator/generator_tests/maxspeeds_tests.cpp +++ b/generator/generator_tests/maxspeeds_tests.cpp @@ -47,6 +47,7 @@ namespace { + using namespace generator; using namespace generator_tests; using namespace measurement_utils; @@ -136,6 +137,8 @@ bool ParseCsv(string const & maxspeedsCsvContent, OsmIdToMaxspeed & mapping) return ParseMaxspeeds(base::JoinPath(testDirFullPath, kCsv), mapping); } +} // namespace + UNIT_TEST(MaxspeedTagValueToSpeedTest) { SpeedInUnits speed; @@ -398,4 +401,40 @@ UNIT_TEST(MaxspeedCollector_Merge) TEST_EQUAL(osmIdToMaxspeed[base::MakeOsmWay(3)].GetForward(), static_cast(70), ()); TEST_EQUAL(osmIdToMaxspeed[base::MakeOsmWay(4)].GetForward(), static_cast(80), ()); } -} // namespace + +UNIT_TEST(MaxspeedCollector_Smoke) +{ + classificator::Load(); + auto const filename = GetFileName(); + SCOPE_GUARD(_, std::bind(Platform::RemoveFileIfExists, std::cref(filename))); + + FeatureBuilder builder; + + auto c1 = std::make_shared(filename); + c1->CollectFeature(builder, MakeOsmElement(1 /* id */, {{"maxspeed:forward", "50"}} /* tags */, OsmElement::EntityType::Way)); + c1->CollectFeature(builder, MakeOsmElement(2 /* id */, {{"maxspeed:backward", "50"}} /* tags */, OsmElement::EntityType::Way)); + + builder.SetType(classif().GetTypeByPath({"highway", "motorway_link"})); + c1->CollectFeature(builder, MakeOsmElement(3 /* id */, {{"maxspeed:advisory", "70"}} /* tags */, OsmElement::EntityType::Way)); + + builder.SetType(classif().GetTypeByPath({"highway", "trunk"})); + c1->CollectFeature(builder, MakeOsmElement(4 /* id */, {{"maxspeed:advisory", "70"}} /* tags */, OsmElement::EntityType::Way)); + + builder.SetType(classif().GetTypeByPath({"highway", "trunk_link"})); + c1->CollectFeature(builder, MakeOsmElement(5 /* id */, {{"maxspeed:advisory", "10"}, {"maxspeed", "20"}} /* tags */, OsmElement::EntityType::Way)); + + c1->Finish(); + c1->Finalize(); + + OsmIdToMaxspeed osmIdToMaxspeed; + ParseMaxspeeds(filename, osmIdToMaxspeed); + TEST_EQUAL(osmIdToMaxspeed.size(), 3, ()); + + TEST_EQUAL(osmIdToMaxspeed[base::MakeOsmWay(1)].GetForward(), static_cast(50), ()); + TEST(osmIdToMaxspeed.find(base::MakeOsmWay(2)) == osmIdToMaxspeed.end(), ()); + + TEST_EQUAL(osmIdToMaxspeed[base::MakeOsmWay(3)].GetForward(), static_cast(70), ()); + TEST(osmIdToMaxspeed.find(base::MakeOsmWay(4)) == osmIdToMaxspeed.end(), ()); + + TEST_EQUAL(osmIdToMaxspeed[base::MakeOsmWay(5)].GetForward(), static_cast(20), ()); +} diff --git a/generator/maxspeeds_collector.cpp b/generator/maxspeeds_collector.cpp index e913737dbc..47cd405f14 100644 --- a/generator/maxspeeds_collector.cpp +++ b/generator/maxspeeds_collector.cpp @@ -28,7 +28,7 @@ namespace { bool ParseMaxspeedAndWriteToStream(string const & maxspeed, SpeedInUnits & speed, ostringstream & ss) { - if (!ParseMaxspeedTag(maxspeed, speed)) + if (maxspeed.empty() || !ParseMaxspeedTag(maxspeed, speed)) return false; ss << UnitsToString(speed.GetUnits()) << "," << strings::to_string(speed.GetSpeed()); @@ -51,7 +51,7 @@ shared_ptr MaxspeedsCollector::Clone( return make_shared(GetFilename()); } -void MaxspeedsCollector::CollectFeature(FeatureBuilder const &, OsmElement const & p) +void MaxspeedsCollector::CollectFeature(FeatureBuilder const & ft, OsmElement const & p) { if (!p.IsWay()) return; @@ -59,22 +59,20 @@ void MaxspeedsCollector::CollectFeature(FeatureBuilder const &, OsmElement const ostringstream ss; ss << p.m_id << ","; - auto const & tags = p.Tags(); - string maxspeedForwardStr; - string maxspeedBackwardStr; + string maxspeedForwardStr, maxspeedBackwardStr, maxspeedAdvisoryStr; bool isReverse = false; - for (auto const & t : tags) + SpeedInUnits maxspeed; + for (auto const & t : p.Tags()) { - if (t.m_key == "maxspeed") + if (t.m_key == "maxspeed" && ParseMaxspeedAndWriteToStream(t.m_value, maxspeed, ss)) { - SpeedInUnits dummySpeed; - if (!ParseMaxspeedAndWriteToStream(t.m_value, dummySpeed, ss)) - return; m_stream << ss.str() << '\n'; return; } + if (t.m_key == "maxspeed:advisory") + maxspeedAdvisoryStr = t.m_value; if (t.m_key == "maxspeed:forward") maxspeedForwardStr = t.m_value; else if (t.m_key == "maxspeed:backward") @@ -91,28 +89,31 @@ void MaxspeedsCollector::CollectFeature(FeatureBuilder const &, OsmElement const if (isReverse) maxspeedForwardStr.swap(maxspeedBackwardStr); - if (maxspeedForwardStr.empty()) - return; - - SpeedInUnits maxspeedForward; - if (!ParseMaxspeedAndWriteToStream(maxspeedForwardStr, maxspeedForward, ss)) - return; - - if (!maxspeedBackwardStr.empty()) + if (ParseMaxspeedAndWriteToStream(maxspeedForwardStr, maxspeed, ss)) { - SpeedInUnits maxspeedBackward; - if (!ParseMaxspeedTag(maxspeedBackwardStr, maxspeedBackward)) - return; - // Note. Keeping only maxspeed:forward and maxspeed:backward if they have the same units. // The exception is maxspeed:forward or maxspeed:backward have a special values // like "none" or "walk". In that case units mean nothing an the values should // be processed in a special way. - if (!HaveSameUnits(maxspeedForward, maxspeedBackward)) - return; - - ss << "," << strings::to_string(maxspeedBackward.GetSpeed()); + SpeedInUnits maxspeedBackward; + if (ParseMaxspeedTag(maxspeedBackwardStr, maxspeedBackward) && + HaveSameUnits(maxspeed, maxspeedBackward)) + { + ss << "," << strings::to_string(maxspeedBackward.GetSpeed()); + } } + else if (ftypes::IsLinkChecker::Instance()(ft.GetTypes()) && + ParseMaxspeedAndWriteToStream(maxspeedAdvisoryStr, maxspeed, ss)) + { + // Assign maxspeed:advisory for highway:xxx_link types to avoid situation when + // not defined link speed is predicted bigger than defined parent ingoing highway speed. + // https://www.openstreetmap.org/way/5511439#map=18/45.55465/-122.67787 + + /// @todo Best solution is to correct not defined link speed according to the defined parent highway speed + /// in graph building process, but it needs much more efforts. + } + else + return; m_stream << ss.str() << '\n'; } diff --git a/routing_common/vehicle_model.cpp b/routing_common/vehicle_model.cpp index 56579aadf9..f9c0aaa2ff 100644 --- a/routing_common/vehicle_model.cpp +++ b/routing_common/vehicle_model.cpp @@ -244,17 +244,7 @@ SpeedKMpH VehicleModel::GetSpeedWihtoutMaxspeed(FeatureType & f, bool VehicleModel::IsOneWay(FeatureType & f) const { - // It's a hotfix for release and this code shouldn't be merge to master. - // According to osm documentation on roundabout it's implied that roundabout is one way - // road execpt for rare cases. Only 0.3% (~1200) of roundabout in the world are two-way road. - // (https://wiki.openstreetmap.org/wiki/Tag:junction%3Droundabout) - // It should be processed on map generation stage together with other implied one way features - // rules like: motorway_link (if not set oneway == "no") - // motorway (if not set oneway == "no"). Please see - // https://github.com/organicmaps/organicmaps/blob/master/3party/osrm/osrm-backend/profiles/car.lua#L392 - // for further details. - // TODO(@Zverik, @bykoianko) Please process the rules on map generation stage. - return HasOneWayType(feature::TypesHolder(f)) || ftypes::IsRoundAboutChecker::Instance()(f); + return HasOneWayType(feature::TypesHolder(f)); } bool VehicleModel::HasOneWayType(feature::TypesHolder const & types) const