Skip to content

Otp2 redesign api modes - #3011

Merged
gmellemstrand merged 45 commits into
opentripplanner:dev-2.xfrom
entur:otp2_redesign_api_modes
Apr 21, 2020
Merged

Otp2 redesign api modes#3011
gmellemstrand merged 45 commits into
opentripplanner:dev-2.xfrom
entur:otp2_redesign_api_modes

Conversation

@gmellemstrand

@gmellemstrand gmellemstrand commented Mar 19, 2020

Copy link
Copy Markdown
Contributor

To be completed by pull request submitter:

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 first step in reimplementing and improving how street routing works in OTP2. This builds on #3010, and should be merged after.

I have done the following:

  • Moved the RoutingRequest class to the org.opentripplanner.routing.request package.
  • Created a new RequestModes class that allows access, egress, direct and transit modes to be specified individually.
  • Created new TransitMode and StreetMode enums. Transit modes are specified as before, but street modes are a high level grouping, like WALKor BIKE_RENTAL which may include several smaller transitions. (Walk -> bike -> walk with bike -> bike).
  • The TraverseModeSet field is kept in the RoutingRequest, but it is only set by the getStreetSearchRequest method. This should be refactored so we have a separate request object for the street routing.
  • The Transmodel API has been changed to reflect the new way to specify modes.
  • The direct street routing has been extracted to its own class. A new subRequest with the correct settings for the applicable StreetMode is created.
  • The access and egress routing has been refactored to not create the same objects as transfer routing, but new AccessEgress objects that implement the same interface, but contain the states found during routing.
  • A timeshift method has been added to the Itinerary class and is used when mapping access/egress legs in the RaptorPathToItineraryMapper.
  • Made a best effort at mapping the REST API modes to the new format. This can be seen in the QualifiedModeSet.getRequestModes class. TraverseModes are now completely separated from the REST API, except the debug output at the bottom part of the PlannerResource class.
  • The NearbyStopFinder now has a new method that takes a RoutingRequest as a parameter. This allows access/egress searches using other modes than walk.
  • Tested the supported access/egress/direct modes. Which ones are supported for which searches is specified in the StreetMode class.
  • Added a check for bike rental that a bike has actually been rented before reaching the destination. This is in the State.isFinal method. This method is also called in the NearbyStopFinder in order to get the same functionality for the access/egress searches.

@gmellemstrand
gmellemstrand requested a review from a team March 19, 2020 14:19
@sven4all

Copy link
Copy Markdown
Contributor

I would like to help test this PR.

Is there already some layering in place, for example that a train or metrostation is preferred over a bus or tram stop when accessing or egressing transit by bike?

@gmellemstrand

Copy link
Copy Markdown
Contributor Author

I would like to help test this PR.

Is there already some layering in place, for example that a train or metrostation is preferred over a bus or tram stop when accessing or egressing transit by bike?

Great! For now I'm mostly working on getting the structure in place, and then I will start working on the modes one by one. The priority will be on the access/egress modes that start/end with walking, and then bringing the bike with you on transit will be a later step. It should be possible to favor specific stations using cost.

@t2gran t2gran removed the Entur On Entur Roadmap label Mar 20, 2020
Comment thread src/main/java/org/opentripplanner/routing/request/AllowedModes.java Outdated
@gmellemstrand
gmellemstrand force-pushed the otp2_redesign_api_modes branch from 9693505 to d40f0c8 Compare March 24, 2020 14:22
@gmellemstrand
gmellemstrand force-pushed the otp2_redesign_api_modes branch from d40f0c8 to eec7006 Compare March 24, 2020 15:29
@sven4all

Copy link
Copy Markdown
Contributor

After the last commit it is working, but the travel advices I retrieve is on foot and not with the bicycle.

@gmellemstrand

Copy link
Copy Markdown
Contributor Author

After the last commit it is working, but the travel advices I retrieve is on foot and not with the bicycle.

What does your request look like? I tested with the OTP client and it does give you a bike-only result if your origin/destination are close enough so it does not go over the distance limit.

Comment thread docs/OTP2-MigrationGuide.md Outdated
Comment thread src/ext/java/org/opentripplanner/ext/siri/SiriFuzzyTripMatcher.java Outdated
@sven4all

sven4all commented Apr 16, 2020

Copy link
Copy Markdown
Contributor

After the last commit it is working, but the travel advices I retrieve is on foot and not with the bicycle.

What does your request look like? I tested with the OTP client and it does give you a bike-only result if your origin/destination are close enough so it does not go over the distance limit.

This is what I tried, should I specify a maximum walk distance?

{
  trip(
    dateTime: "2020-04-18T11:14:00Z", 
    from: {coordinates: {latitude: 51.907708, longitude:4.454966 }},
    to: {coordinates: {latitude: 52.053137, longitude:4.328905}},
  	modes: {accessMode:bicycle, egressMode:bicycle}
  )
  {
    tripPatterns {
      startTime
      duration
      walkDistance
      legs {
        mode
        distance
        line {
          id
          publicCode
        	presentation {
            textColour
            colour
          }
          
        }
        aimedEndTime
        aimedStartTime
        expectedEndTime
        expectedStartTime
        realtime
        fromPlace {
          name
          vertexType
        }
        toPlace {
          name
          vertexType
        }
        
        pointsOnLink{ 
          points
        }
        realtime
        
        
      }
    }
  }
}

@gmellemstrand

Copy link
Copy Markdown
Contributor Author

@sven4all Taking the bike with you on public transport is currently not supported. Look at the StreetMode enum to have a look at the supported combinations. As you can see in RequestModes class, it will default to null if the mode is not supported for that part of the trip.

Making the bike->transit->bike functionality available should be possible, but it requires separate pre-calculated edges for biking (assuming you actually bike during the transfers).

Some combinations that I have tested between two coordinates:
{directMode:bicycle}
{directMode:walk}
{directMode:car}

{accessMode:bike_rental, egressMode:foot}
{accessMode:bike_park, egressMode:foot}
{accessMode:car_park, egressMode:foot}
{accessMode:taxi, egressMode:foot}

Most of these will also work as egressMode, and in any combination with accessMode. There is still work to be done to get all modes working properly. For example taxi should include walking at both ends in order to be used as both access and egress mode. Also, we need to decide what to do when doing a search that does not start with walking from a stop that is only connected by walk-only streets.

This is a search I have been testing with on the Oslo graph.

  trip(from: {coordinates: {latitude: 59.9171598, longitude:10.7116047}}, to: {coordinates:{latitude:59.9184624,longitude:10.7379756}}, modes: {directMode:bicycle, accessMode:taxi, egressMode:foot}) {
    messageEnums
    tripPatterns {
      startTime
      endTime
      legs {
        fromPlace {
          quay {
            id
          }
        }
        toPlace {
          quay {
            id
          }
        }
        duration
        mode
      }
    }
  }
}

@t2gran

t2gran commented Apr 17, 2020

Copy link
Copy Markdown
Member

I this as a fist step it would be nice to support bike-transit(walk transfers)-bike. I see it is not correct, but it is better than not supporting it at all. The long term fix is then to precalculate bike transfers the same way we do walking - this should be toggled on/off for those who need it.

@gmellemstrand

gmellemstrand commented Apr 17, 2020

Copy link
Copy Markdown
Contributor Author

There is no harm in leaving it in, but as you said, it will only support walk transfers. The fix is as simple as access/egress on the bike element in StreetMode and add some documentation. Also, as long as the transfers aren't too long, this will still be quite useful.

@gmellemstrand
gmellemstrand requested a review from t2gran April 17, 2020 12:23
@hannesj

hannesj commented Apr 17, 2020

Copy link
Copy Markdown
Contributor

I just noticed that this breaks arriveBy-routing. I don't know whether it worked that well previously, but in this branch it fails with error 500 and the following stack trace.

ERROR (PlannerError.java:48) exception planning trip: 
java.lang.NullPointerException: null
	at org.opentripplanner.routing.core.State.getStates(State.java:52) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.opentripplanner.routing.algorithm.astar.AStar.startSearch(AStar.java:128) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.opentripplanner.routing.algorithm.astar.AStar.startSearch(AStar.java:92) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.opentripplanner.routing.algorithm.astar.AStar.getShortestPathTree(AStar.java:295) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.opentripplanner.routing.algorithm.astar.AStar.getShortestPathTree(AStar.java:79) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.opentripplanner.graph_builder.module.NearbyStopFinder.findNearbyStopsViaStreets(NearbyStopFinder.java:152) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.opentripplanner.routing.algorithm.raptor.router.street.AccessEgressRouter.streetSearch(AccessEgressRouter.java:48) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.opentripplanner.routing.algorithm.RoutingWorker.routeTransit(RoutingWorker.java:115) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.opentripplanner.routing.algorithm.RoutingWorker.route(RoutingWorker.java:73) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.opentripplanner.routing.RoutingService.route(RoutingService.java:49) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.opentripplanner.api.resource.PlannerResource.plan(PlannerResource.java:76) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:na]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:na]
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:na]
	at java.base/java.lang.reflect.Method.invoke(Method.java:567) ~[na:na]
	at org.glassfish.jersey.server.model.internal.ResourceMethodInvocationHandlerFactory$1.invoke(ResourceMethodInvocationHandlerFactory.java:81) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher$1.run(AbstractJavaResourceMethodDispatcher.java:144) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher.invoke(AbstractJavaResourceMethodDispatcher.java:161) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.server.model.internal.JavaResourceMethodDispatcherProvider$TypeOutInvoker.doDispatch(JavaResourceMethodDispatcherProvider.java:205) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher.dispatch(AbstractJavaResourceMethodDispatcher.java:99) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.invoke(ResourceMethodInvoker.java:389) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.apply(ResourceMethodInvoker.java:347) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.apply(ResourceMethodInvoker.java:102) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.server.ServerRuntime$2.run(ServerRuntime.java:308) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:271) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:267) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:315) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:297) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:267) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:317) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.server.ServerRuntime.process(ServerRuntime.java:291) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.server.ApplicationHandler.handle(ApplicationHandler.java:1140) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpContainer.service(GrizzlyHttpContainer.java:375) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.grizzly.http.server.HttpHandler$1.run(HttpHandler.java:224) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.doWork(AbstractThreadPool.java:565) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.run(AbstractThreadPool.java:545) ~[otp-2.0.0-SNAPSHOT-shaded.jar:1.1]
	at java.base/java.lang.Thread.run(Thread.java:830) ~[na:na]

t2gran
t2gran previously approved these changes Apr 17, 2020

@t2gran t2gran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is 5 classes in OTP2 (after this is merged) that represent modes in slightly different scopes with overlapping types - like TransitMode and TraverseMode, witch both include mode like BUS. I think this is confusing. I will not insist that we change this, just express a bit of concern about it.

If we make two sub-types for itinerary Leg, a TransitLeg and a StreetLeg, then maybe we could get rid of the TraverseMode. But are modes really static structures? In StreetMode there is more than one CAR and BIKE mode, to me this is a signal that we are missing something? The set (RENTED, BIKE, FOR_ACCESS) is not a mode. The mode is BIKE, the fact that it is rented and used for access tells us that the next mode transition is going to happen in a place where we can park the bike. Modes are in many contexts a hierarchical structure (e.g. NeTEx), so maybe it would be better to represent them in another way. I do not have a complete overview of what we use modes for - so it is hard to suggest something concrete.

- `searchWindow` Limit the departure window or arrival window for the routing search.
- `boardSlackByMode` How much time boarding a vehicle takes for each given mode.
- `alightSlackByMode` How much time alighting a vehicle takes for each given mode.
- `modes` The REST API is unchanged, but is mapped into a new structure in the RoutingRequest. This means not all combinations of non-transit modes that was available in OTP1 is available in OTP2.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to keep all lines 100 characters wide, but I am not sure if we should apply it on markdown files. From what I see in the IDE and here on GitHub, it probably better to not do it (both wraps nicely, and Intelij do NOT wrap lines when I try to do auto reformatting). You can do as you like on this one, but we should state something in the developer guideline - I will do that.

@t2gran
t2gran self-requested a review April 17, 2020 16:50
@t2gran
t2gran dismissed their stale review April 17, 2020 16:53

Someone from another organization needs to approve this, sorry for hitting the wrong button.

@gmellemstrand

Copy link
Copy Markdown
Contributor Author

There is 5 classes in OTP2 (after this is merged) that represent modes in slightly different scopes with overlapping types - like TransitMode and TraverseMode, witch both include mode like BUS. I think this is confusing. I will not insist that we change this, just express a bit of concern about it.

If we make two sub-types for itinerary Leg, a TransitLeg and a StreetLeg, then maybe we could get rid of the TraverseMode. But are modes really static structures? In StreetMode there is more than one CAR and BIKE mode, to me this is a signal that we are missing something? The set (RENTED, BIKE, FOR_ACCESS) is not a mode. The mode is BIKE, the fact that it is rented and used for access tells us that the next mode transition is going to happen in a place where we can park the bike. Modes are in many contexts a hierarchical structure (e.g. NeTEx), so maybe it would be better to represent them in another way. I do not have a complete overview of what we use modes for - so it is hard to suggest something concrete.

I agree that StreetMode is not a mode in the same sense as the others, but I'm struggling to find a good name for it. In some ways it represents a state-machine with modes and allowed transitions.

@sven4all

sven4all commented Apr 21, 2020

Copy link
Copy Markdown
Contributor

@sven4all Taking the bike with you on public transport is currently not supported. Look at the StreetMode enum to have a look at the supported combinations. As you can see in RequestModes class, it will default to null if the mode is not supported for that part of the trip.

Making the bike->transit->bike functionality available should be possible, but it requires separate pre-calculated edges for biking (assuming you actually bike during the transfers).

Some combinations that I have tested between two coordinates:
{directMode:bicycle}
{directMode:walk}
{directMode:car}

{accessMode:bike_rental, egressMode:foot}
{accessMode:bike_park, egressMode:foot}
{accessMode:car_park, egressMode:foot}
{accessMode:taxi, egressMode:foot}

Most of these will also work as egressMode, and in any combination with accessMode. There is still work to be done to get all modes working properly. For example taxi should include walking at both ends in order to be used as both access and egress mode. Also, we need to decide what to do when doing a search that does not start with walking from a stop that is only connected by walk-only streets.

This is a search I have been testing with on the Oslo graph.

  trip(from: {coordinates: {latitude: 59.9171598, longitude:10.7116047}}, to: {coordinates:{latitude:59.9184624,longitude:10.7379756}}, modes: {directMode:bicycle, accessMode:taxi, egressMode:foot}) {
    messageEnums
    tripPatterns {
      startTime
      endTime
      legs {
        fromPlace {
          quay {
            id
          }
        }
        toPlace {
          quay {
            id
          }
        }
        duration
        mode
      }
    }
  }
}

Maybe I do not understand it correctly. But what I try to do is plan a trip where the user cycles to the station, then parks it's bike at the station, take the train and afterwards take a bicycle again. (maybe it's a very dutch way to use public transport that causes some misunderstanding)

@gmellemstrand
gmellemstrand merged commit c9c7a2b into opentripplanner:dev-2.x Apr 21, 2020
@gmellemstrand
gmellemstrand deleted the otp2_redesign_api_modes branch April 21, 2020 08:51
@gmellemstrand

Copy link
Copy Markdown
Contributor Author

@sven4all

Maybe I do not understand it correctly. But what I try to do is plan a trip where the user cycles to the station, then parks it's bike at the station, take the train and afterwards take a bicycle again. (maybe it's a very dutch way to use public transport that causes some misunderstanding)

This is also a Norwegian way to use public transport (but probably not to the same extent as the Dutch). You can use bike_park as access mode, and then foot as egress mode (because you don't have your bike with you anymore). This requires there to be a bike parking point defined.

As this pull request has been merged, I think you should create a new issue if you wish to discuss this further.

@abyrd abyrd added this to the 2.0 milestone Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants