Revert "change phantom node to store an appx speed regardless of direction. use this speed to estimate an accel/decel penalty and scale the penalty based on the distance traveled up to a cutoff. only used in the table plugin at the moment. TODO still lives in the guidance assemble_leg area"

This reverts commit ca55521c87.
2019-02-10 00:42:10 -08:00 · 2019-02-10 00:42:10 -08:00 · 823d7e45e7
commit 823d7e45e7
parent 6b961dccfc
9 changed files with 78 additions and 77 deletions
--- a/include/engine/api/base_parameters.hpp
+++ b/include/engine/api/base_parameters.hpp
@ -123,8 +123,8 @@ struct BaseParameters
                           });
    }
 };
-} // namespace api
-} // namespace engine
-} // namespace osrm
+}
+}
+}

 #endif // ROUTE_PARAMETERS_HPP
--- a/include/engine/geospatial_query.hpp
+++ b/include/engine/geospatial_query.hpp
@ -466,7 +466,6 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
        return distance_and_phantoms;
    }

-    // TODO: remove stoppage penalty as its not needed here anymore
    PhantomNodeWithDistance MakePhantomNode(const util::Coordinate input_coordinate,
                                            const EdgeData &data,
                                            const double min_stoppage_penalty = 0,
@ -556,11 +555,36 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
            point_on_segment,
            datafacade.GetCoordinateOfNode(forward_geometry(data.fwd_segment_position + 1)));

+        // where does this speed lie with respect to the min/max penalizable speeds
+        auto penalty_range = min_stoppage_penalty != INVALID_MINIMUM_STOPPAGE_PENALTY &&
+                                     max_stoppage_penalty != INVALID_MAXIMUM_STOPPAGE_PENALTY
+                                 ? max_stoppage_penalty - min_stoppage_penalty
+                                 : 0;
+        auto stoppage_penalty =
+            [penalty_range, min_stoppage_penalty, max_stoppage_penalty](double speed) -> double {
+            // You're so slow already you don't get a penalty
+            if (speed < MINIMAL_ACCEL_DECEL_PENALIZABLE_SPEED)
+                return 0;
+
+            // Find where it is on the scale
+            constexpr auto max =
+                MAXIMAL_ACCEL_DECEL_PENALIZABLE_SPEED - MINIMAL_ACCEL_DECEL_PENALIZABLE_SPEED;
+            auto ratio = (speed - MINIMAL_ACCEL_DECEL_PENALIZABLE_SPEED) / max;
+
+            // You're faster than the max so you get the max
+            if (ratio >= 1)
+                return max_stoppage_penalty;
+
+            // You're in between so you get a linear combination
+            return min_stoppage_penalty + ratio * penalty_range;
+        };
+
        // We may end up adding a stoppage penalty
-        double forward_speed = 0;
-        double reverse_speed = 0;
-        auto total_distance = 0;
-        auto penalty_range = max_stoppage_penalty - min_stoppage_penalty;
+        EdgeDuration forward_stoppage_penalty = 0;
+        EdgeDuration reverse_stoppage_penalty = 0;
+        auto total_distance = penalty_range > 0 ? appx_distance(forward_geometry.begin(),
+                                                                std::prev(forward_geometry.end()))
+                                                : 0.0;
        ratio = std::min(1.0, std::max(0.0, ratio));
        if (data.forward_segment_id.id != SPECIAL_SEGMENTID)
        {
@ -569,12 +593,11 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
            // Stoppage penalty based on speed
            if (data.forward_segment_id.enabled && penalty_range > 0)
            {
-                total_distance =
-                    appx_distance(forward_geometry.begin(), std::prev(forward_geometry.end()));
                const auto total_duration = std::accumulate(
                    forward_durations.begin(), forward_durations.end(), EdgeDuration{0});
-                forward_speed = total_distance / (total_duration * 0.1);
-                reverse_speed = forward_speed;
+                const auto speed = total_distance / (total_duration * 0.1);
+                forward_stoppage_penalty =
+                    static_cast<EdgeDuration>((stoppage_penalty(speed) * 10) + .5);
            }
        }
        if (data.reverse_segment_id.id != SPECIAL_SEGMENTID)
@ -584,14 +607,11 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
            // Stoppage penalty based on speed
            if (data.reverse_segment_id.enabled && penalty_range > 0)
            {
-                if (total_distance == 0)
-                    total_distance =
-                        appx_distance(forward_geometry.begin(), std::prev(forward_geometry.end()));
                const auto total_duration = std::accumulate(
                    reverse_durations.begin(), reverse_durations.end(), EdgeDuration{0});
-                reverse_speed = total_distance / (total_duration * 0.1);
-                if (forward_speed == 0)
-                    forward_speed = reverse_speed;
+                const auto speed = total_distance / (total_duration * 0.1);
+                reverse_stoppage_penalty =
+                    static_cast<EdgeDuration>((stoppage_penalty(speed) * 10) + .5);
            }
        }

@ -623,7 +643,8 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
                        reverse_duration,
                        forward_duration_offset,
                        reverse_duration_offset,
-                        static_cast<float>((forward_speed + reverse_speed) / 2.0),
+                        forward_stoppage_penalty,
+                        reverse_stoppage_penalty,
                        is_forward_valid_source,
                        is_forward_valid_target,
                        is_reverse_valid_source,
--- a/include/engine/guidance/assemble_leg.hpp
+++ b/include/engine/guidance/assemble_leg.hpp
@ -182,8 +182,12 @@ inline RouteLeg assembleLeg(const datafacade::BaseDataFacade &facade,
        duration = std::max(0, duration);
    }

-    // TODO: Add start and stop penalties to the duration to simulate accel/deceleration
-    //
+    // Add start and stop penalties
+    if (distance > 0)
+        duration +=
+            (target_traversed_in_reverse
+                 ? source_node.reverse_duration_penalty + target_node.reverse_duration_penalty
+                 : source_node.forward_duration_penalty + target_node.forward_duration_penalty);

    std::string summary;
    if (needs_summary)
--- a/include/engine/hint.hpp
+++ b/include/engine/hint.hpp
@ -64,7 +64,7 @@ struct Hint
 };

 static_assert(sizeof(Hint) == sizeof(PhantomNode) + 4, "Hint is bigger than expected");
-constexpr std::size_t ENCODED_HINT_SIZE = 120;
+constexpr std::size_t ENCODED_HINT_SIZE = 124;
 static_assert(ENCODED_HINT_SIZE / 4 * 3 >= sizeof(Hint),
              "ENCODED_HINT_SIZE does not match size of Hint");
 } // namespace engine
--- a/include/engine/phantom_node.hpp
+++ b/include/engine/phantom_node.hpp
@ -44,16 +44,16 @@ namespace engine
 struct PhantomNode
 {
    PhantomNode()
-        : forward_segment_id{SPECIAL_SEGMENTID, false}, reverse_segment_id{SPECIAL_SEGMENTID,
-                                                                           false},
-          forward_weight(INVALID_EDGE_WEIGHT), reverse_weight(INVALID_EDGE_WEIGHT),
-          forward_weight_offset(0), reverse_weight_offset(0),
+        : forward_segment_id{SPECIAL_SEGMENTID, false},
+          reverse_segment_id{SPECIAL_SEGMENTID, false}, forward_weight(INVALID_EDGE_WEIGHT),
+          reverse_weight(INVALID_EDGE_WEIGHT), forward_weight_offset(0), reverse_weight_offset(0),
          forward_distance(INVALID_EDGE_DISTANCE), reverse_distance(INVALID_EDGE_DISTANCE),
          forward_distance_offset(0), reverse_distance_offset(0),
          forward_duration(MAXIMAL_EDGE_DURATION), reverse_duration(MAXIMAL_EDGE_DURATION),
-          forward_duration_offset(0), reverse_duration_offset(0), speed_approximation(0),
-          fwd_segment_position(0), is_valid_forward_source{false}, is_valid_forward_target{false},
-          is_valid_reverse_source{false}, is_valid_reverse_target{false}, bearing(0)
+          forward_duration_offset(0), reverse_duration_offset(0), forward_duration_penalty(0),
+          reverse_duration_penalty(0), fwd_segment_position(0), is_valid_forward_source{false},
+          is_valid_forward_target{false}, is_valid_reverse_source{false},
+          is_valid_reverse_target{false}, bearing(0)

    {
    }
@ -110,9 +110,8 @@ struct PhantomNode

    bool IsValid(const unsigned number_of_nodes) const
    {
-        return location.IsValid() &&
-               ((forward_segment_id.id < number_of_nodes) ||
-                (reverse_segment_id.id < number_of_nodes)) &&
+        return location.IsValid() && ((forward_segment_id.id < number_of_nodes) ||
+                                      (reverse_segment_id.id < number_of_nodes)) &&
               ((forward_weight != INVALID_EDGE_WEIGHT) ||
                (reverse_weight != INVALID_EDGE_WEIGHT)) &&
               ((forward_duration != MAXIMAL_EDGE_DURATION) ||
@ -169,7 +168,8 @@ struct PhantomNode
                         EdgeDuration reverse_duration,
                         EdgeDuration forward_duration_offset,
                         EdgeDuration reverse_duration_offset,
-                         EdgeDistance speed_approximation,
+                         EdgeDuration forward_duration_penalty,
+                         EdgeDuration reverse_duration_penalty,
                         bool is_valid_forward_source,
                         bool is_valid_forward_target,
                         bool is_valid_reverse_source,
@ -185,9 +185,10 @@ struct PhantomNode
          reverse_distance_offset{reverse_distance_offset}, forward_duration{forward_duration},
          reverse_duration{reverse_duration}, forward_duration_offset{forward_duration_offset},
          reverse_duration_offset{reverse_duration_offset},
-          speed_approximation{speed_approximation}, component{component.id, component.is_tiny},
-          location{location}, input_location{input_location},
-          fwd_segment_position{other.fwd_segment_position},
+          forward_duration_penalty{forward_duration_penalty},
+          reverse_duration_penalty{reverse_duration_penalty},
+          component{component.id, component.is_tiny}, location{location},
+          input_location{input_location}, fwd_segment_position{other.fwd_segment_position},
          is_valid_forward_source{is_valid_forward_source},
          is_valid_forward_target{is_valid_forward_target},
          is_valid_reverse_source{is_valid_reverse_source},
@ -209,7 +210,8 @@ struct PhantomNode
    EdgeDuration reverse_duration;
    EdgeDuration forward_duration_offset; // TODO: try to remove -> requires path unpacking changes
    EdgeDuration reverse_duration_offset; // TODO: try to remove -> requires path unpacking changes
-    EdgeDistance speed_approximation;     // m/s
+    EdgeDuration forward_duration_penalty;
+    EdgeDuration reverse_duration_penalty;

    ComponentID component;

@ -225,7 +227,7 @@ struct PhantomNode
    unsigned short bearing : 12;
 };

-static_assert(sizeof(PhantomNode) == 84, "PhantomNode has more padding then expected");
+static_assert(sizeof(PhantomNode) == 88, "PhantomNode has more padding then expected");

 using PhantomNodePair = std::pair<PhantomNode, PhantomNode>;

--- a/include/engine/routing_algorithms/routing_base.hpp
+++ b/include/engine/routing_algorithms/routing_base.hpp
@ -86,7 +86,7 @@ void insertSourceInHeap(ManyToManyQueryHeap &heap, const PhantomNode &phantom_no
        heap.Insert(phantom_node.forward_segment_id.id,
                    -phantom_node.GetForwardWeightPlusOffset(),
                    {phantom_node.forward_segment_id.id,
-                     -phantom_node.GetForwardDuration(),
+                     -phantom_node.GetForwardDuration() + phantom_node.forward_duration_penalty,
                     -phantom_node.GetForwardDistance()});
    }
    if (phantom_node.IsValidReverseSource())
@ -94,7 +94,7 @@ void insertSourceInHeap(ManyToManyQueryHeap &heap, const PhantomNode &phantom_no
        heap.Insert(phantom_node.reverse_segment_id.id,
                    -phantom_node.GetReverseWeightPlusOffset(),
                    {phantom_node.reverse_segment_id.id,
-                     -phantom_node.GetReverseDuration(),
+                     -phantom_node.GetReverseDuration() + phantom_node.reverse_duration_penalty,
                     -phantom_node.GetReverseDistance()});
    }
 }
@ -107,7 +107,7 @@ void insertTargetInHeap(ManyToManyQueryHeap &heap, const PhantomNode &phantom_no
        heap.Insert(phantom_node.forward_segment_id.id,
                    phantom_node.GetForwardWeightPlusOffset(),
                    {phantom_node.forward_segment_id.id,
-                     phantom_node.GetForwardDuration(),
+                     phantom_node.GetForwardDuration() + phantom_node.forward_duration_penalty,
                     phantom_node.GetForwardDistance()});
    }
    if (phantom_node.IsValidReverseTarget())
@ -115,7 +115,7 @@ void insertTargetInHeap(ManyToManyQueryHeap &heap, const PhantomNode &phantom_no
        heap.Insert(phantom_node.reverse_segment_id.id,
                    phantom_node.GetReverseWeightPlusOffset(),
                    {phantom_node.reverse_segment_id.id,
-                     phantom_node.GetReverseDuration(),
+                     phantom_node.GetReverseDuration() + phantom_node.reverse_duration_penalty,
                     phantom_node.GetReverseDistance()});
    }
 }
--- a/include/server/api/base_parameters_grammar.hpp
+++ b/include/server/api/base_parameters_grammar.hpp
@ -25,7 +25,7 @@ namespace
 {
 namespace ph = boost::phoenix;
 namespace qi = boost::spirit::qi;
-} // namespace
+}

 template <typename T, char... Fmt> struct no_trailing_dot_policy : qi::real_policies<T>
 {
@ -222,8 +222,8 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar<Iterator, Signature>
    qi::symbols<char, engine::Approach> approach_type;
    qi::symbols<char, engine::api::BaseParameters::SnappingType> snapping_type;
 };
-} // namespace api
-} // namespace server
-} // namespace osrm
+}
+}
+}

 #endif
--- a/include/util/typedefs.hpp
+++ b/include/util/typedefs.hpp
@ -119,7 +119,8 @@ static const EdgeDistance INVALID_EDGE_DISTANCE = std::numeric_limits<EdgeDistan
 static const EdgeDistance INVALID_FALLBACK_SPEED = std::numeric_limits<EdgeDistance>::max();
 constexpr EdgeDuration INVALID_MINIMUM_STOPPAGE_PENALTY = std::numeric_limits<EdgeDuration>::max();
 constexpr EdgeDuration INVALID_MAXIMUM_STOPPAGE_PENALTY = std::numeric_limits<EdgeDuration>::max();
-constexpr float MAXIMAL_ACCEL_DECEL_PENALIZABLE_SPEED_INVERSE = 40.f; // seconds/metre
+constexpr double MINIMAL_ACCEL_DECEL_PENALIZABLE_SPEED = 10; // metres/second
+constexpr double MAXIMAL_ACCEL_DECEL_PENALIZABLE_SPEED = 40; // metres/sec

 // FIXME the bitfields we use require a reduced maximal duration, this should be kept consistent
 // within the code base. For now we have to ensure that we don't case 30 bit to -1 and break any
--- a/src/engine/plugins/table.cpp
+++ b/src/engine/plugins/table.cpp
@ -97,22 +97,6 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms,

    std::vector<api::TableAPI::TableCellRef> estimated_pairs;

-    auto penalize = [&params](float speed, EdgeDuration duration) -> EdgeDuration {
-        // this is the penalty before adjusting for the length of the route
-        auto speed_ratio = speed * MAXIMAL_ACCEL_DECEL_PENALIZABLE_SPEED_INVERSE;
-        auto penalty = speed_ratio * (params.max_stoppage_penalty - params.min_stoppage_penalty) +
-                       params.min_stoppage_penalty;
-        // how much distance would we cover if we took half of it at this speed
-        // scale the penalty based on this distance, if its small we dont get much
-        // of the penalty, buf if its large we get up to the whole penalty
-        // .05 is to scale the duration to second and cut it in half
-        // .005 is like / 200m, so the estimated distance as a ratio of 200m
-        // so you get the full penalty if your route is 200m or more
-        auto distance_ratio = (speed * duration * .05) * 0.008;
-        return static_cast<EdgeDuration>(
-            (distance_ratio < 1.f ? distance_ratio * penalty : penalty) * 10.);
-    };
-
    // Scan table for null results - if any exist, replace with distance estimates
    if (params.fallback_speed != INVALID_FALLBACK_SPEED || params.scale_factor != 1 ||
        (params.min_stoppage_penalty != INVALID_MINIMUM_STOPPAGE_PENALTY &&
@ -124,20 +108,9 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms,
            {
                const auto &table_index = row * num_destinations + column;
                BOOST_ASSERT(table_index < result_tables_pair.first.size());
-                // apply an accel/deceleration penalty to the duration
-                if (result_tables_pair.first[table_index] != MAXIMAL_EDGE_DURATION && row != column)
-                {
-                    const auto &source =
-                        snapped_phantoms[params.sources.empty() ? row : params.sources[row]];
-                    const auto &destination =
-                        snapped_phantoms[params.destinations.empty() ? column
-                                                                     : params.destinations[column]];
-                    result_tables_pair.first[table_index] +=
-                        penalize(source.speed_approximation,
-                                 result_tables_pair.first[table_index]) +
-                        penalize(destination.speed_approximation,
-                                 result_tables_pair.first[table_index]);
-                }
+                // Zero out the diagonal
+                if (result_tables_pair.first[table_index] != MAXIMAL_EDGE_DURATION && row == column)
+                    result_tables_pair.first[table_index] = 0;
                // Estimate null results based on fallback_speed (if valid) and distance
                if (params.fallback_speed != INVALID_FALLBACK_SPEED && params.fallback_speed > 0 &&
                    result_tables_pair.first[table_index] == MAXIMAL_EDGE_DURATION)