AStar multiple start vertices refactor

[gmellemstrand]

To be completed by pull request submitter:

• issue: #2813
• roadmap: #2626
• tests: I have deleted many of the GenericLocationTests and moved the ones that are still relevant to the LocationMatcher
• formatting: Have you followed the suggested code style?
• documentation: Added javadoc to the refactored GenericLocation class
• changelog: add a bullet point to the changelog file with description and link to the linked issue

To be completed by @opentripplanner/plc:

• reviews and approvals by 2 members, ideally from different organizations
• after merging: update the relevant card on the roadmap

This is a refactor that is needed to support NeTEx extensions to stops and stations, but it should also make the process of specifying a from/to place from the API and resolving them to vertices cleaner.

This builds on #2840 and #2868

What I have done:

• Simplified the main RoutingContext constructor. Removed transit-related logic and moved some logic out in separate methods.
• Changed the RoutingContext to contain a set of to/from vertices instead of a single one of each. This is needed because there is no edge between Station and Stop anymore.
• Removed origin/destination fields from RoutingContext, as they were used for reverse optimization.
• Changed the A Star implementation so that it can start from multiple initial states (one per start vertex).
• Refactored and simplified the GenericLocation. It now only contains a (pass-through) label, a FeedScopeId and coordinates. All pattern matching has been moved to a new LocationMatcher class, as this is only used in the REST API.
• Changed the getVerticesForLocation method in StreetVertexIndexServiceImpl to resolve the GenericLocation to a set of vertices by first trying to match the FeedScopeId to a Stop or StopCollection, and then matching the coordinate (via SimpleStreetSplitter) as a fallback.

[abyrd]

Initial observations routing on a GTFS-derived graph via the REST API: I had to merge the fares fix PR to get any routing results. The routing itself with fromPlace and toPlace as lat,lon points works fine. I continued issuing the same request over the REST API as follows:

$ curl -H "accept:application/json" "http://localhost:8080/otp/routers/default/plan?fromPlace=myFromPlace::-1,-4&toPlace=label::ignoregarbage-12,15&time=6%3A39pm&date=08-21-2019&mode=TRANSIT%2CWALK&maxWalkDistance=804.672&arriveBy=false&wheelchair=false&locale=en" | jq .

trying different fromPlace and toPlace strings and tracing execution. Some combinations work as expected and others don't. I think what we want to do in getGenericLocation is more like:

            if (matcher.find()) {
                lat = Double.parseDouble(matcher.group(1));
                lon = Double.parseDouble(matcher.group(4));
            } else if (FeedScopedId.isValidString(place)) {
                placeId = FeedScopedId.convertFromString(place);
            }

meaning we should parse the entire place string into a FeedScopedId only when it doesn't match the lat,lon matcher.

We can see from GtfsTest line 114 the format it's expecting when stops are specified: "label::feedId:stopId", with a colon rather than an underscore. Interestingly none of the subclasses of this test fail, but I think that's just the transit routing is no longer in place, so we disabled all of the tests that actually call this routing method. I suggest we expand LocationMatcherTest with a few more cases, since it doesn't check the case where a stop ID is included, or a station ID, or extraneous noise characters before a lat,lon (not sure why OTP accepts that anyway).

OneBusAway uses the underscore separator but OTP shifted to colons in FeedScopedIds because underscores are common in identifiers and useful in feedIds, and colons are commonly used as scope separators. It's currently using the wrong feedId separator (from the OneBusAway source).

This leads me to think about some possible changes to FeedScopedId: We can replace the separator constant with a colon, and we should rename fields and methods of feedScopedId to "feed" and "getFeed" rather than "agency". We can update the FeedScopedIdSerializer and FeedScopedIdKeyDeserializer to all use the same logic as FeedScopedId.toString(), with the new separator.

Also, in GenericLocation, the field placeId is always interpreted as a Stop id, so we should rename it accordingly (FeedScopedId stopId or stopCollectionId).

I think we can probably merge GenericLocation#fromOldStyleString and LocationMatcher#getGenericLocation.

LocationMatcher Javadoc says: "parse input strings and match them to a GenericLocation." It doesn't exactly match to a GenericLocation, it constructs a GenericLocation containing the parsed strings and numbers.

I can provide commits to fix/update most of these things.

[abyrd]

@gmellemstrand I made most of the changes mentioned in my last comment-review, eliminated the StreetVertexIndex interface and factory, and in b0a287a also fixed a problem where we were repeatedly searching for the non-transit itinerary due to the old trip-banning loop. This should improve response times somewhat.

Everything seems to work well in testing, though I do have to comment out itinerary.fare = fareService.getCost(path); in the ItineraryMapper until we get #2896 merged.

[gmellemstrand]

@abyrd The changes look good to me. I have not reviewed the exact functionality of the LocationStringParser or the FareService implementation, but it does not affect routing otherwise.

I added another change to the NearbyStopFinder, to ensure that inital stop vertices are also added as StopForDistance objects.

[gmellemstrand]

This fixes #2770

[gmellemstrand]

@abyrd You asked me about the NearbyStopFinder also including the origin vertices. This seems to already be accounted for before this pull request.

The NearbyStopFinder does include the origin vertices. This is what we need for the AccessEgressRouter. The DirectTransferGenerator, however does not want them to be included. This is accounted for by this check.

https://github.com/entur/OpenTripPlanner/blob/rutebanken_develop/src/main/java/org/opentripplanner/graph_builder/module/DirectTransferGenerator.java#L101

[t2gran]

@gmellemstrand and @abyrd have worked together on this and reviewed each other, so I will approve this so @gmellemstrand gan get it merged (without the approval from @abyrd ).