Begin removing PathParsers. #1794 and #2091

src/main/java/org/opentripplanner/routing/algorithm/AStar.java

@@ -14,6 +14,8 @@ the License, or (at your option) any later version.
 package org.opentripplanner.routing.algorithm;
 
 import java.util.Collection;
+import java.util.Collections;
+import java.util.LinkedList;
 import java.util.List;
 
 import org.opentripplanner.common.pqueue.BinHeap;
@@ -65,7 +67,7 @@ class RunState {
         RemainingWeightHeuristic heuristic;
         public RoutingContext rctx;
         public int nVisited;
-        public List<Object> targetAcceptedStates;
+        public List<State> targetAcceptedStates;
         public RunStatus status;
         private RoutingRequest options;
         private SearchTerminationStrategy terminationStrategy;
@@ -276,14 +278,18 @@ void runSearch(long abortTime){
                     runState.rctx.origin, runState.rctx.target, runState.u, runState.spt, runState.options)) {
                     break;
                 }
-            // TODO AMB: Replace isFinal with bicycle conditions in BasicPathParser
             }  else if (!runState.options.batch && runState.u_vertex == runState.rctx.target && runState.u.isFinal() && runState.u.allPathParsersAccept()) {
+                if (runState.options.onlyTransitTrips && !runState.u.isEverBoarded()) {
+                    continue;
+                }
                 runState.targetAcceptedStates.add(runState.u);
                 runState.foundPathWeight = runState.u.getWeight();
                 runState.options.rctx.debugOutput.foundPath();
                 //new GraphPath(runState.u, false).dump();
                 /* Only find one path at a time in long distance mode. */
-                if (runState.options.longDistance) break;
+                if (runState.options.longDistance) {
+                    break;
+                }
                 /* Break out of the search if we've found the requested number of paths. */
                 if (runState.targetAcceptedStates.size() >= runState.options.getNumItineraries()) {
                     LOG.debug("total vertices visited {}", runState.nVisited);
@@ -354,4 +360,14 @@ private boolean isWorstTimeExceeded(State v, RoutingRequest opt) {
     public void setTraverseVisitor(TraverseVisitor traverseVisitor) {
         this.traverseVisitor = traverseVisitor;
     }
+
+    public List<GraphPath> getPathsToTarget() {
+        List<GraphPath> ret = new LinkedList<>();
+        for (State s : runState.targetAcceptedStates) {
+            if (s.isFinal() && s.allPathParsersAccept()) {
+                ret.add(new GraphPath(s, true));
+            }
+        }
+        return ret;
+    }
 }

src/main/java/org/opentripplanner/routing/core/RoutingRequest.java

@@ -411,6 +411,9 @@ public class RoutingRequest implements Cloneable, Serializable {
     /** The function that compares paths converging on the same vertex to decide which ones continue to be explored. */
     public DominanceFunction dominanceFunction = new DominanceFunction.Pareto();
 
+    /** Accept only paths that use transit (no street-only paths). */
+    public boolean onlyTransitTrips = false;
+
     /* CONSTRUCTORS */
 
     /** Constructor for options; modes defaults to walk and transit */

src/main/java/org/opentripplanner/routing/edgetype/PreAlightEdge.java

@@ -123,7 +123,7 @@ public State optimisticTraverse(State s0) {
     }
 
     public String toString() {
-        return "prealight edge at stop " + tov;
+        return "PreAlightEdge at stop " + tov;
     }
 
 }

src/main/java/org/opentripplanner/routing/edgetype/PreBoardEdge.java

@@ -122,7 +122,7 @@ public State optimisticTraverse(State s0) {
     }
 
     public String toString() {
-        return "preboard edge at stop " + fromv;
+        return "PreBoardEdge at stop " + fromv;
     }
 
 }

src/main/java/org/opentripplanner/routing/edgetype/SimpleTransfer.java

@@ -43,6 +43,15 @@ public SimpleTransfer(TransitStop from, TransitStop to, double distance, LineStr
 
     @Override
     public State traverse(State s0) {
+        // Forbid taking shortcuts composed of two transfers in a row
+        if (s0.backEdge instanceof SimpleTransfer) {
+            return null;
+        }
+        // FIXME major algorithmic error: Transfer results can dominate alighting from a vehicle.
+        if (s0.backEdge instanceof StreetTransitLink) {
+            return null;
+        }
+        // Only transfer right after riding a vehicle.
         RoutingRequest rr = s0.getOptions();
         double walkspeed = rr.walkSpeed;
         StateEditor se = s0.edit(this);
@@ -81,4 +90,9 @@ public double getDistance(){
    public LineString getGeometry(){
 	   return this.geometry;
    }
+
+    @Override
+    public String toString() {
+        return "SimpleTransfer " + getName();
+    }
 }

src/main/java/org/opentripplanner/routing/edgetype/StreetTransitLink.java

@@ -82,6 +82,18 @@ public String getName(Locale locale) {
     }
 
     public State traverse(State s0) {
+
+        // Forbid taking shortcuts composed of two street-transit links in a row. Also avoids spurious leg transitions.
+        if (s0.backEdge instanceof StreetTransitLink) {
+            return null;
+        }
+
+        // Do not re-enter the street network following a transfer.
+        // FIXME this is a serious problem: transfer result state can dominate arrivals at a stop on a vehicle and prune the tree!
+        if (s0.backEdge instanceof SimpleTransfer) {
+            return null;
+        }
+
         RoutingRequest req = s0.getOptions();
         if (s0.getOptions().wheelchairAccessible && !wheelchairAccessible) {
             return null;

src/main/java/org/opentripplanner/routing/edgetype/TransitBoardAlight.java

@@ -137,6 +137,11 @@ public State traverse(State s0, long arrivalTimeAtStop) {
         RoutingContext rctx    = s0.getContext();
         RoutingRequest options = s0.getOptions();
 
+        // Forbid taking shortcuts composed of two board-alight edges in a row. Also avoids spurious leg transitions.
+        if (s0.backEdge instanceof TransitBoardAlight) {
+            return null;
+        }
+
         /* If the user requested a wheelchair accessible trip, check whether and this stop is not accessible. */
         if (options.wheelchairAccessible && ! getPattern().wheelchairAccessible(stopIndex)) {
             return null;

src/main/java/org/opentripplanner/routing/impl/GraphPathFinder.java

@@ -101,16 +101,17 @@ public List<GraphPath> getPaths(RoutingRequest options) {
         AStar aStar = new AStar();
         if (options.rctx == null) {
             options.setRoutingContext(router.graph);
-            /* Use a pathparser that constrains the search to use SimpleTransfers. */
-            options.rctx.pathParsers = new PathParser[] { new Parser() };
+            // Use no PathParsers. They should be eliminated.
+            // The special long-distance heuristic should be sufficient to constrain the search to the right area.
+            options.rctx.pathParsers = new PathParser[0];
         }
         // If this Router has a GraphVisualizer attached to it, set it as a callback for the AStar search
         if (router.graphVisualizer != null) {
             aStar.setTraverseVisitor(router.graphVisualizer.traverseVisitor);
             // options.disableRemainingWeightHeuristic = true; // DEBUG
         }
 
-        // without transit, we'd just just return multiple copies of the same on-street itinerary
+        // Without transit, we'd just just return multiple copies of the same on-street itinerary.
         if (!options.modes.isTransit()) {
             options.numItineraries = 1;
         }
@@ -157,28 +158,30 @@ public List<GraphPath> getPaths(RoutingRequest options) {
             timeout -= System.currentTimeMillis(); // absolute to relative
             timeout /= 1000; // msec to seconds
             if (timeout <= 0) {
-                // must catch this case where advancing to the next (lower) timeout value means the search is timed out
-                // before it even begins, because a negative relative timeout will mean "no timeout" in the SPT call.
+                // Catch the case where advancing to the next (lower) timeout value means the search is timed out
+                // before it even begins. Passing a negative relative timeout in the SPT call would mean "no timeout".
                 options.rctx.aborted = true;
                 break;
             }
-            ShortestPathTree spt = aStar.getShortestPathTree(options, timeout);
-            if (spt == null) {
-                LOG.warn("SPT was null."); // unknown failure
-                return null;
-            }
+            aStar.getShortestPathTree(options, timeout);
             if (options.rctx.aborted) {
-                break; // search timed out or was gracefully aborted for some other reason.
+                break; // Search timed out or was gracefully aborted for some other reason.
             }
-            List<GraphPath> newPaths = spt.getPaths();
+            // Don't dig through the SPT object, just ask the A star algorithm for the states that reached the target.
+            List<GraphPath> newPaths = aStar.getPathsToTarget();
             if (newPaths.isEmpty()) {
                 break;
             }
             // Find all trips used in this path and ban them for the remaining searches
             for (GraphPath path : newPaths) {
-                for (State state : path.states) {
-                    AgencyAndId tripId = state.getTripId();
-                    if (tripId != null) options.banTrip(tripId);
+                path.dump();
+                List<AgencyAndId> tripIds = path.getTrips();
+                for (AgencyAndId tripId : tripIds) {
+                    options.banTrip(tripId);
+                }
+                if (tripIds.isEmpty()) {
+                    // This path does not use transit (is entirely on-street). Do not repeatedly find the same one.
+                    options.onlyTransitTrips = true;
                 }
             }
             paths.addAll(newPaths);
@@ -189,107 +192,6 @@ public List<GraphPath> getPaths(RoutingRequest options) {
         return paths;
     }
 
-    /* TODO eliminate the need for pathparsers. They are theoretically efficient but arcane and problematic. */
-
-    public static class Parser extends PathParser {
-
-        static final int STREET       = 1;
-        static final int LINK         = 2;
-        static final int STATION      = 3;
-        static final int ONBOARD      = 4;
-        static final int TRANSFER     = 5;
-        static final int STATION_STOP = 6;
-        static final int STOP_STATION = 7;
-
-        private static final DFA DFA;
-
-        static {
-
-            /* A StreetLeg is one or more street edges. */
-            Nonterminal streetLeg = plus(STREET);
-
-            /* A TransitLeg is a ride on transit, including preboard and prealight edges at its 
-             * ends. It begins and ends at a TransitStop vertex. 
-             * Note that these are STATION* rather than STATION+ because some transfer edges
-             * (timed transfer edges) connect arrival and depart vertices. Requiring a STATION
-             * edge would prevent them from being traversed. */
-            Nonterminal transitLeg = seq(star(STATION), plus(ONBOARD), star(STATION));
-
-            /* A beginning gets us from the path's initial vertex to the first transit stop it 
-             * passes through (its first board location). We may want to transfer at the beginning 
-             * of an itinerary that begins at a station or stop, and does not use streets. */
-            Nonterminal beginning = choice(seq(optional(streetLeg), LINK), seq(optional(STATION_STOP), optional(TRANSFER)));
-
-            /* Begin on board transit, ending up at another stop where the "middle" can take over. */
-            Nonterminal onboardBeginning = seq(plus(ONBOARD), plus(STATION), optional(TRANSFER));
-
-            /* Ride transit at least one time, chaining transit legs together with single transfer edges. */
-            Nonterminal middle = seq(transitLeg, star(optional(TRANSFER), transitLeg));
-
-            /* And end gets us from the last stop to the final vertex. It is the same as a beginning, 
-             * but with the sub-sequences reversed. This must cover 6 different cases: 
-             * 1. leave the station and optionally walk, 
-             * 2. stay at the stop where we are, 
-             * 3. stay at the stop where we are but go to its parent station, 
-             * 4. transfer and stay at the target stop, 
-             * 5. transfer and move to the target stop's parent station. */
-            Nonterminal end = choice(seq(LINK, optional(streetLeg)), seq(optional(TRANSFER), optional(STOP_STATION)));
-
-            /* An itinerary that includes a ride on public transit. It might begin on- or offboard. 
-             * if it begins onboard, it doesn't necessarily have subsequent transit legs. */
-            Nonterminal transitItinerary = choice( 
-                    seq(beginning, middle, end),
-                    seq(onboardBeginning, optional(middle), end));
-
-            /* A streets-only itinerary, which might begin or end at a stop or its station, 
-             * but does not actually ride transit. */
-            Nonterminal streetItinerary = choice(TRANSFER, seq(
-                    optional(STATION_STOP), optional(LINK), 
-                    streetLeg,
-                    optional(LINK), optional(STOP_STATION)));
-
-            Nonterminal itinerary = choice(streetItinerary, transitItinerary);
-
-            DFA = itinerary.toDFA().minimize();
-            // System.out.println(DFA.toGraphViz());
-            // System.out.println(DFA.dumpTable());
-        }
-
-        @Override
-        protected DFA getDFA() {
-            return DFA;
-        }
-
-        /**
-         * The terminal is normally based exclusively on the backEdge, i.e. each terminal represents
-         * exactly one edge in the path. In case of @link{StationStopEdge}, however, the type of the
-         * current vertex also determines what kind of terminal this is.
-         */
-        @Override
-        public int terminalFor(State state) {
-            Edge e = state.getBackEdge();
-            if (e == null) {
-                throw new RuntimeException ("terminalFor should never be called on States without back edges!");
-            }
-            /* OnboardEdge currently includes BoardAlight edges. */
-            if (e instanceof OnboardEdge)       return ONBOARD;
-            if (e instanceof StationEdge)       return STATION;
-            if (e instanceof StationStopEdge) {
-                return state.getVertex() instanceof TransitStop ? STATION_STOP : STOP_STATION;
-            }
-            // There should perhaps be a shared superclass of all transfer edges to simplify this. 
-            if (e instanceof SimpleTransfer)    return TRANSFER;
-            if (e instanceof TransferEdge)      return TRANSFER;
-            if (e instanceof TimedTransferEdge) return TRANSFER;
-            if (e instanceof StreetTransitLink) return LINK;
-            if (e instanceof IntersectionTransitLink) return LINK;
-            if (e instanceof PathwayEdge)       return LINK;
-            // Is it really correct to clasify all other edges as STREET?
-            return STREET;
-        }
-
-    }
-
     /* Try to find N paths through the Graph */
     public List<GraphPath> graphPathFinderEntryPoint (RoutingRequest request) {
 

src/main/java/org/opentripplanner/routing/spt/DominanceFunction.java

@@ -39,6 +39,12 @@ public abstract class DominanceFunction implements Serializable {
      */
     public boolean betterOrEqualAndComparable(State a, State b) {
 
+        // States before boarding transit and after riding transit are incomparable.
+        // This allows returning transit options even when walking to the destination is the optimal strategy.
+        if (a.isEverBoarded() != b.isEverBoarded()) {
+            return false;
+        }
+
         // Does one state represent riding a rented bike and the other represent walking before/after rental?
         if (a.isBikeRenting() != b.isBikeRenting()) {
             return false;

src/main/java/org/opentripplanner/routing/spt/GraphPath.java

@@ -185,10 +185,14 @@ public int hashCode() {
     public void dump() {
         System.out.println(" --- BEGIN GRAPHPATH DUMP ---");
         System.out.println(this.toString());
-        for (State s : states)
-            System.out.println(s + " via " + s.getBackEdge());
+        for (State s : states) {
+            //System.out.println(s.getBackEdge() + " leads to " + s);
+            if (s.getBackEdge() != null) {
+                System.out.println(s.getBackEdge().getClass().getSimpleName() + " --> " + s.getVertex().getClass().getSimpleName());
+            }
+        }
         System.out.println(" --- END GRAPHPATH DUMP ---");
-        System.out.println("Total meters walked in this graphpath: " + 
+        System.out.println("Total meters walked in the preceding graphpath: " +
                states.getLast().getWalkDistance());
     }
 

src/main/java/org/opentripplanner/routing/spt/ShortestPathTree.java

@@ -59,8 +59,9 @@ public List<GraphPath> getPaths(Vertex dest, boolean optimize) {
             return Collections.emptyList();
         List<GraphPath> ret = new LinkedList<GraphPath>();
         for (State s : stateList) {
-            if (s.isFinal() && s.allPathParsersAccept())
+            if (s.isFinal() && s.allPathParsersAccept()) {
                 ret.add(new GraphPath(s, optimize));
+            }
         }
         return ret;
     }
@@ -127,7 +128,7 @@ public boolean add(State newState) {
 
         // if the vertex has no states, add one and return
         if (states == null) {
-            states = new ArrayList<State>();
+            states = new ArrayList<>();
             stateSets.put(vertex, states);
             states.add(newState);
             return true;