From f6ecc3c5167079a03271493e31128007e301299d Mon Sep 17 00:00:00 2001 From: Daniel Patterson Date: Tue, 23 Jan 2018 16:38:04 -0800 Subject: [PATCH] Address @karenzshea feedback, and other tidy up. --- features/guidance/maneuver-tag.feature | 4 -- include/engine/api/route_api.hpp | 57 ++++++++++--------- .../contiguous_internalmem_datafacade.hpp | 29 ++++------ include/extractor/serialization.hpp | 2 - src/extractor/edge_based_graph_factory.cpp | 19 ------- src/extractor/extraction_containers.cpp | 6 -- .../maneuver_override_relation_parser.cpp | 13 +---- 7 files changed, 45 insertions(+), 85 deletions(-) diff --git a/features/guidance/maneuver-tag.feature b/features/guidance/maneuver-tag.feature index 165641098dc..c2326aa85dd 100644 --- a/features/guidance/maneuver-tag.feature +++ b/features/guidance/maneuver-tag.feature @@ -33,10 +33,6 @@ Feature: Maneuver tag support # Testing directly connected from/to | a,j | A Street,C Street,J Street,J Street | depart,turn sharp right,turn left,arrive | | b,g | A Street,C Street,C Street | depart,turn sharp right,arrive | - # Testing disconnected via ways (first turn is only modified if you end up - # on a particular way) - # | h,a | J Street,C Street,A Street,A Street | depart,turn left,turn left,arrive | - # | h,e | J Street,C Street,B Street,B Street | depart,turn sharp left,turn left,arrive | # Testing re-awakening suppressed turns | a,e | A Street,B Street,B Street | depart,turn slight left,arrive | diff --git a/include/engine/api/route_api.hpp b/include/engine/api/route_api.hpp index 0a2f110712c..62f1964bd3a 100644 --- a/include/engine/api/route_api.hpp +++ b/include/engine/api/route_api.hpp @@ -107,24 +107,24 @@ class RouteAPI : public BaseAPI { for (const auto &a : unpacked_path_segments) { - std::cout << "Route: "; - std::cout << (source_traversed_in_reverse[0] + util::Log(logDEBUG) << "Route: "; + util::Log(logDEBUG) << (source_traversed_in_reverse[0] ? segment_end_coordinates[0].source_phantom.reverse_segment_id.id : segment_end_coordinates[0].source_phantom.forward_segment_id.id) << " "; for (const auto &b : a) { - std::cout << "("; - std::cout << b.from_edge_based_node << " "; - std::cout << static_cast(b.turn_instruction.type) << " "; - std::cout << static_cast(b.turn_instruction.direction_modifier) << ") "; + util::Log(logDEBUG) << "("; + util::Log(logDEBUG) << b.from_edge_based_node << " "; + util::Log(logDEBUG) << static_cast(b.turn_instruction.type) << " "; + util::Log(logDEBUG) << static_cast(b.turn_instruction.direction_modifier) << ") "; } - std::cout << (target_traversed_in_reverse[0] + util::Log(logDEBUG) << (target_traversed_in_reverse[0] ? segment_end_coordinates[0].target_phantom.reverse_segment_id.id : segment_end_coordinates[0].target_phantom.forward_segment_id.id) << " "; - std::cout << std::endl; + util::Log(logDEBUG) << std::endl; } std::vector legs; std::vector leg_geometries; @@ -154,7 +154,7 @@ class RouteAPI : public BaseAPI reversed_target, parameters.steps); - std::cout << "Assembling steps " << std::endl; + util::Log(logDEBUG) << "Assembling steps " << std::endl; if (parameters.steps) { auto steps = guidance::assembleSteps(BaseAPI::facade, @@ -171,35 +171,35 @@ class RouteAPI : public BaseAPI for (auto current_step_it = steps.begin(); current_step_it != steps.end(); ++current_step_it) { - std::cout << "Searching for " << current_step_it->from_id << std::endl; + util::Log(logDEBUG) << "Searching for " << current_step_it->from_id << std::endl; const auto overrides = BaseAPI::facade.GetOverridesThatStartAt(current_step_it->from_id); if (overrides.empty()) continue; - std::cout << "~~~~ GOT A HIT, checking the rest ~~~" << std::endl; + util::Log(logDEBUG) << "~~~~ GOT A HIT, checking the rest ~~~" << std::endl; for (const extractor::ManeuverOverride &maneuver_relation : overrides) { - std::cout << "Override sequence is "; + util::Log(logDEBUG) << "Override sequence is "; for (auto &n : maneuver_relation.node_sequence) { - std::cout << n << " "; + util::Log(logDEBUG) << n << " "; } - std::cout << std::endl; - std::cout << "Override type is " + util::Log(logDEBUG) << std::endl; + util::Log(logDEBUG) << "Override type is " << osrm::guidance::internalInstructionTypeToString( maneuver_relation.override_type) << std::endl; - std::cout << "Override direction is " + util::Log(logDEBUG) << "Override direction is " << osrm::guidance::instructionModifierToString( maneuver_relation.direction) << std::endl; - std::cout << "Route sequence is "; + util::Log(logDEBUG) << "Route sequence is "; for (auto it = current_step_it; it != steps.end(); ++it) { - std::cout << it->from_id << " "; + util::Log(logDEBUG) << it->from_id << " "; } - std::cout << std::endl; + util::Log(logDEBUG) << std::endl; auto search_iter = maneuver_relation.node_sequence.begin(); auto route_iter = current_step_it; @@ -215,6 +215,9 @@ class RouteAPI : public BaseAPI continue; } // Skip over duplicated EBNs in the step array + // EBNs are sometime duplicated because guidance code inserts + // "fake" steps that it later removes. This hasn't happened yet + // at this point, but we can safely just skip past the dupes. if ((route_iter - 1)->from_id == route_iter->from_id) { ++route_iter; @@ -228,7 +231,7 @@ class RouteAPI : public BaseAPI // We got a match, update using the instruction_node if (search_iter == maneuver_relation.node_sequence.end()) { - std::cout << "Node sequence matched, looking for the step " + util::Log(logDEBUG) << "Node sequence matched, looking for the step " << "that has the via node" << std::endl; const auto via_node_coords = BaseAPI::facade.GetCoordinateOfNode( maneuver_relation.instruction_node); @@ -237,7 +240,7 @@ class RouteAPI : public BaseAPI current_step_it, route_iter, [&leg_geometry, &via_node_coords](const auto &step) { - std::cout << "Leg geom from " << step.geometry_begin << " to " + util::Log(logDEBUG) << "Leg geom from " << step.geometry_begin << " to " << step.geometry_end << std::endl; // iterators over geometry of current step @@ -250,12 +253,12 @@ class RouteAPI : public BaseAPI }); if (via_match != end) { - std::cout << "Found geometry match at " + util::Log(logDEBUG) << "Found geometry match at " << (std::distance(begin, end) - std::distance(via_match, end)) << std::endl; } - std::cout << ((*(leg_geometry.locations.begin() + + util::Log(logDEBUG) << ((*(leg_geometry.locations.begin() + step.geometry_begin) == via_node_coords) ? "true" : "false") @@ -269,14 +272,14 @@ class RouteAPI : public BaseAPI if (step_to_update != route_iter) { // Don't update the last step (it's an arrive instruction) - std::cout << "Updating step " + util::Log(logDEBUG) << "Updating step " << std::distance(steps.begin(), steps.end()) - std::distance(step_to_update, steps.end()) << std::endl; if (maneuver_relation.override_type != osrm::guidance::TurnType::MaxTurnType) { - std::cout << " instruction was " + util::Log(logDEBUG) << " instruction was " << osrm::guidance::internalInstructionTypeToString( step_to_update->maneuver.instruction.type) << " now " @@ -289,7 +292,7 @@ class RouteAPI : public BaseAPI if (maneuver_relation.direction != osrm::guidance::DirectionModifier::MaxDirectionModifier) { - std::cout << " direction was " + util::Log(logDEBUG) << " direction was " << osrm::guidance::instructionModifierToString( step_to_update->maneuver.instruction .direction_modifier) @@ -304,7 +307,7 @@ class RouteAPI : public BaseAPI } } } - std::cout << "Done tweaking steps" << std::endl; + util::Log(logDEBUG) << "Done tweaking steps" << std::endl; } /* Perform step-based post-processing. diff --git a/include/engine/datafacade/contiguous_internalmem_datafacade.hpp b/include/engine/datafacade/contiguous_internalmem_datafacade.hpp index b4ab52856bc..d0cc1fe7286 100644 --- a/include/engine/datafacade/contiguous_internalmem_datafacade.hpp +++ b/include/engine/datafacade/contiguous_internalmem_datafacade.hpp @@ -916,20 +916,17 @@ class ContiguousInternalMemoryDataFacadeBase : public BaseDataFacade { std::vector results; - bool found = false; - // Copy all the overrides into the results array. - // m_maneuver_overrides is sorted by node_sequence.front(), - // so we can stop searching after we get to a node > edge_based_node_id - for (const auto & override : m_maneuver_overrides) + // heterogeneous comparison: + struct Comp { - if (found && override.start_node != edge_based_node_id) - break; - - if (override.start_node == edge_based_node_id) - { - std::cout << "Found start node " << override.start_node << std::endl; - std::cout << "Copying from " << override.node_sequence_offset_begin << " to " - << override.node_sequence_offset_end << std::endl; + bool operator() ( const extractor::StorageManeuverOverride& s, NodeID i ) const { return s.start_node < i; } + bool operator() ( NodeID i, const extractor::StorageManeuverOverride& s ) const { return i < s.start_node; } + }; + + auto found_range = std::equal_range(m_maneuver_overrides.begin(), m_maneuver_overrides.end(), + edge_based_node_id, Comp{}); + + std::for_each(found_range.first, found_range.second, [&](const auto &override) { std::vector sequence(m_maneuver_override_node_sequences.begin() + override.node_sequence_offset_begin, m_maneuver_override_node_sequences.begin() + @@ -938,11 +935,9 @@ class ContiguousInternalMemoryDataFacadeBase : public BaseDataFacade override.instruction_node, override.override_type, override.direction}); - found = true; - } - } - + }); return results; + } }; diff --git a/include/extractor/serialization.hpp b/include/extractor/serialization.hpp index 03a8e3fc578..fb05225c2f3 100644 --- a/include/extractor/serialization.hpp +++ b/include/extractor/serialization.hpp @@ -304,8 +304,6 @@ inline void write(storage::io::FileWriter &writer, const std::vector &maneuver_overrides, const std::vector &node_sequences) { - std::cout << "###### Writing " << maneuver_overrides.size() << " overrides, and " - << node_sequences.size() << " sequence points" << std::endl; writer.WriteElementCount64(maneuver_overrides.size()); writer.WriteElementCount64(node_sequences.size()); writer.WriteFrom(maneuver_overrides); diff --git a/src/extractor/edge_based_graph_factory.cpp b/src/extractor/edge_based_graph_factory.cpp index dba28aef6e7..4b54cfefa77 100644 --- a/src/extractor/edge_based_graph_factory.cpp +++ b/src/extractor/edge_based_graph_factory.cpp @@ -1041,10 +1041,6 @@ void EdgeBasedGraphFactory::GenerateEdgeExpandedEdges( std::for_each(buffer->turn_to_ebn_map.begin(), buffer->turn_to_ebn_map.end(), [&global_turn_to_ebn_map](const auto &p) { - std::cout << "Copying " << p.first.from << "," << p.first.via - << "," << p.first.to << " (" << p.second.first << "," - << p.second.second << ") to the cache list" - << std::endl; // TODO: log conflicts here global_turn_to_ebn_map.insert(p); }); @@ -1078,25 +1074,10 @@ void EdgeBasedGraphFactory::GenerateEdgeExpandedEdges( const auto v = global_turn_to_ebn_map.find(unresolved_override.turn_sequence[i]); if (v != global_turn_to_ebn_map.end()) { - std::clog << "Adding pair " << v->second.first << "->" << v->second.second - << std::endl; node_sequence[i] = v->second.first; node_sequence[i + 1] = v->second.second; } - else - { - std::cout << "Turn " << unresolved_override.turn_sequence[i].from << "," - << unresolved_override.turn_sequence[i].via << "," - << unresolved_override.turn_sequence[i].to - << " not found in our cache list" << std::endl; - } - } - std::cout << "Generated node sequence: "; - for (const auto &n : node_sequence) - { - std::cout << " " << n; } - std::cout << std::endl; storage_override.node_sequence_offset_begin = maneuver_override_sequences.size(); storage_override.node_sequence_offset_end = maneuver_override_sequences.size() + node_sequence.size(); diff --git a/src/extractor/extraction_containers.cpp b/src/extractor/extraction_containers.cpp index 80c29407dd9..c58bbec7729 100644 --- a/src/extractor/extraction_containers.cpp +++ b/src/extractor/extraction_containers.cpp @@ -793,12 +793,6 @@ void ExtractionContainers::PrepareManeuverOverrides() auto result = find_turn_from_way_tofrom_nodes(from_segment_itr->second, to_segment_itr->second); - std::cout << "Turn from " << from_segment_itr->second.way_id << " to " - << to_segment_itr->second.way_id << std::endl; - - std::cout << " traverses nodes from " << result.from << " via " << result.via << " to " - << result.to << std::endl; - return result; }; diff --git a/src/extractor/maneuver_override_relation_parser.cpp b/src/extractor/maneuver_override_relation_parser.cpp index 37ebd7833d0..3dbc1896f95 100644 --- a/src/extractor/maneuver_override_relation_parser.cpp +++ b/src/extractor/maneuver_override_relation_parser.cpp @@ -24,12 +24,9 @@ namespace extractor ManeuverOverrideRelationParser::ManeuverOverrideRelationParser() {} /** - * Tries to parse a relation as a turn restriction. This can fail for a number of - * reasons. The return type is a boost::optional. - * - * Some restrictions can also be ignored: See the ```get_restrictions``` function - * in the corresponding profile. We use it for both namespacing restrictions, as in - * restriction:motorcar as well as whitelisting if its in except:motorcar. + * Parses the `type=maneuver` relation. Reads the fields, and puts data + * into an InputManeuverOverride object, if the relation is considered + * valid (i.e. has the minimum tags we expect). */ boost::optional ManeuverOverrideRelationParser::TryParse(const osmium::Relation &relation) const @@ -55,9 +52,6 @@ ManeuverOverrideRelationParser::TryParse(const osmium::Relation &relation) const maneuver_override.maneuver = relation.tags().get_value_by_key("maneuver", ""); maneuver_override.direction = relation.tags().get_value_by_key("direction", ""); - std::cout << "maneuver " << maneuver_override.maneuver << std::endl; - std::cout << "direction " << maneuver_override.direction << std::endl; - boost::optional from = boost::none, via = boost::none, to = boost::none; std::vector via_ways; @@ -119,7 +113,6 @@ ManeuverOverrideRelationParser::TryParse(const osmium::Relation &relation) const } for (const auto &n : via_ways) { - std::cout << "Adding via way: " << n << std::endl; maneuver_override.via_ways.push_back(OSMWayID{n}); } }