Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If configured, add subway station entrances from OSM to walk steps #6076

Closed
Closed
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
1d19a05
Add subway station entrances to walk steps
HenrikSundell Sep 18, 2024
67f4b1b
Add entity to walk steps
HenrikSundell Oct 2, 2024
9d18269
Add more parameters to Entrance
HenrikSundell Oct 3, 2024
3b88288
Move StepEntity classes
HenrikSundell Oct 7, 2024
0a62bf8
Remove default name for subway station entrances
HenrikSundell Oct 16, 2024
528ab55
Add option to turn on osm subway entrances in osmDefaults
HenrikSundell Oct 18, 2024
3e4ae96
Merge remote-tracking branch 'otp/dev-2.x' into station-entrances
HenrikSundell Oct 19, 2024
401a405
Fix walk step generation
HenrikSundell Oct 21, 2024
438bc31
Add step entity to graphql tests
HenrikSundell Oct 23, 2024
97c2de6
Rename variables to match with graphql
HenrikSundell Oct 25, 2024
d844561
Rename variables
HenrikSundell Oct 25, 2024
02a07ea
Rename function
HenrikSundell Oct 27, 2024
5dc74dd
Fix comments
HenrikSundell Oct 28, 2024
5d55646
Remove println
HenrikSundell Oct 28, 2024
d1067c6
Remove unnecessary imports
HenrikSundell Oct 28, 2024
5c97ad1
Add accessibilty information to entrances
HenrikSundell Oct 28, 2024
e10e0a2
Use existing entrance class for walk steps
HenrikSundell Nov 7, 2024
34761fe
Fix EntranceImpl
HenrikSundell Nov 7, 2024
c84b7cf
Add id to walk step entrances
HenrikSundell Nov 8, 2024
2ea8a52
Remove old file
HenrikSundell Nov 8, 2024
39b0db3
Fix otp version
HenrikSundell Nov 8, 2024
3b6bf3f
Remove unused import
HenrikSundell Nov 8, 2024
36be3dc
Merge remote-tracking branch 'otp/dev-2.x' into station-entrances
HenrikSundell Nov 8, 2024
c9df52d
Require entranceId
HenrikSundell Nov 10, 2024
7a9a8f6
Rename methods
HenrikSundell Nov 10, 2024
c9139e3
Update dosumentation
HenrikSundell Nov 11, 2024
7b21024
Update documentation
HenrikSundell Nov 11, 2024
ce7719c
Remove redundant null check
HenrikSundell Nov 11, 2024
b737411
Add feature union to steps
HenrikSundell Nov 14, 2024
f547e07
Return feature based on relativeDirection
HenrikSundell Nov 14, 2024
18b84f0
Remove StepFeature class
HenrikSundell Nov 14, 2024
2060016
Reuse transit entrance vertex
HenrikSundell Dec 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Use existing entrance class for walk steps
HenrikSundell committed Nov 7, 2024
commit e10e0a2fae17e16cfebb1e888652a531e7fef493
Original file line number Diff line number Diff line change
@@ -25,6 +25,7 @@ public static ApiRelativeDirection mapRelativeDirection(RelativeDirection domain
case UTURN_RIGHT -> ApiRelativeDirection.UTURN_RIGHT;
case ENTER_STATION -> ApiRelativeDirection.ENTER_STATION;
case EXIT_STATION -> ApiRelativeDirection.EXIT_STATION;
case ENTER_OR_EXIT_STATION -> ApiRelativeDirection.ENTER_OR_EXIT_STATION;
case FOLLOW_SIGNS -> ApiRelativeDirection.FOLLOW_SIGNS;
};
}
Original file line number Diff line number Diff line change
@@ -21,5 +21,6 @@ public enum ApiRelativeDirection {
UTURN_RIGHT,
ENTER_STATION,
EXIT_STATION,
ENTER_OR_EXIT_STATION,
FOLLOW_SIGNS,
}
Original file line number Diff line number Diff line change
@@ -37,6 +37,7 @@
import org.opentripplanner.apis.gtfs.datafetchers.CurrencyImpl;
import org.opentripplanner.apis.gtfs.datafetchers.DefaultFareProductImpl;
import org.opentripplanner.apis.gtfs.datafetchers.DepartureRowImpl;
import org.opentripplanner.apis.gtfs.datafetchers.EntranceImpl;
import org.opentripplanner.apis.gtfs.datafetchers.FareProductTypeResolver;
import org.opentripplanner.apis.gtfs.datafetchers.FareProductUseImpl;
import org.opentripplanner.apis.gtfs.datafetchers.FeedImpl;
@@ -58,7 +59,6 @@
import org.opentripplanner.apis.gtfs.datafetchers.RouteImpl;
import org.opentripplanner.apis.gtfs.datafetchers.RouteTypeImpl;
import org.opentripplanner.apis.gtfs.datafetchers.RoutingErrorImpl;
import org.opentripplanner.apis.gtfs.datafetchers.StepEntityTypeResolver;
import org.opentripplanner.apis.gtfs.datafetchers.StopGeometriesImpl;
import org.opentripplanner.apis.gtfs.datafetchers.StopImpl;
import org.opentripplanner.apis.gtfs.datafetchers.StopOnRouteImpl;
@@ -125,7 +125,6 @@ protected static GraphQLSchema buildSchema() {
.type("Node", type -> type.typeResolver(new NodeTypeResolver()))
.type("PlaceInterface", type -> type.typeResolver(new PlaceInterfaceTypeResolver()))
.type("StopPosition", type -> type.typeResolver(new StopPosition() {}))
.type("StepEntity", type -> type.typeResolver(new StepEntityTypeResolver()))
.type("FareProduct", type -> type.typeResolver(new FareProductTypeResolver()))
.type("AlertEntity", type -> type.typeResolver(new AlertEntityTypeResolver()))
.type(typeWiring.build(AgencyImpl.class))
@@ -181,6 +180,7 @@ protected static GraphQLSchema buildSchema() {
.type(typeWiring.build(FareProductUseImpl.class))
.type(typeWiring.build(DefaultFareProductImpl.class))
.type(typeWiring.build(TripOccupancyImpl.class))
.type(typeWiring.build(EntranceImpl.class))
.build();
SchemaGenerator schemaGenerator = new SchemaGenerator();
return schemaGenerator.makeExecutableSchema(typeRegistry, runtimeWiring);
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package org.opentripplanner.apis.gtfs.datafetchers;

import graphql.schema.DataFetcher;
import graphql.schema.DataFetchingEnvironment;
import org.opentripplanner.apis.gtfs.GraphQLUtils;
import org.opentripplanner.apis.gtfs.generated.GraphQLDataFetchers;
import org.opentripplanner.apis.gtfs.generated.GraphQLTypes;
import org.opentripplanner.transit.model.site.Entrance;

public class EntranceImpl implements GraphQLDataFetchers.GraphQLEntrance {

@Override
public DataFetcher<String> code() {
return environment -> {
Entrance entrance = getEntrance(environment);
return entrance != null && entrance.getCode() != null ? entrance.getCode() : null;
};
}

@Override
public DataFetcher<String> gtfsId() {
return environment -> {
Entrance entrance = getEntrance(environment);
return entrance != null && entrance.getId() != null ? entrance.getId().toString() : null;
};
}

@Override
public DataFetcher<String> name() {
return environment -> {
Entrance entrance = getEntrance(environment);
return entrance != null && entrance.getName() != null ? entrance.getName().toString() : null;
};
}

@Override
public DataFetcher<GraphQLTypes.GraphQLWheelchairBoarding> wheelchairAccessible() {
return environment -> {
Entrance entrance = getEntrance(environment);
return entrance != null
? GraphQLUtils.toGraphQL(entrance.getWheelchairAccessibility())
: null;
};
}

/**
* Helper method to retrieve the Entrance object from the DataFetchingEnvironment.
*/
private Entrance getEntrance(DataFetchingEnvironment environment) {
Object source = environment.getSource();
return source instanceof Entrance ? (Entrance) source : null;
}
}
Original file line number Diff line number Diff line change
@@ -4,7 +4,7 @@
import graphql.schema.GraphQLObjectType;
import graphql.schema.GraphQLSchema;
import graphql.schema.TypeResolver;
import org.opentripplanner.model.plan.Entrance;
import org.opentripplanner.transit.model.site.Entrance;

public class StepEntityTypeResolver implements TypeResolver {

Original file line number Diff line number Diff line change
@@ -54,8 +54,8 @@ public DataFetcher<String> exit() {
}

@Override
public DataFetcher<Object> entity() {
return environment -> getSource(environment).getEntity();
public DataFetcher<Object> entrance() {
return environment -> getSource(environment).getEntrance();
}

@Override
Original file line number Diff line number Diff line change
@@ -359,13 +359,13 @@ public interface GraphQLEmissions {

/** Station entrance or exit. */
public interface GraphQLEntrance {
public DataFetcher<Boolean> accessible();

public DataFetcher<String> code();

public DataFetcher<String> gtfsId();

public DataFetcher<String> name();

public DataFetcher<org.opentripplanner.apis.gtfs.generated.GraphQLTypes.GraphQLWheelchairBoarding> wheelchairAccessible();
}

/** A 'medium' that a fare product applies to, for example cash, 'Oyster Card' or 'DB Navigator App'. */
@@ -984,9 +984,6 @@ public interface GraphQLRoutingError {
public DataFetcher<GraphQLInputField> inputField();
}

/** Entity to a step */
public interface GraphQLStepEntity extends TypeResolver {}

/**
* Stop can represent either a single public transport stop, where passengers can
* board and/or disembark vehicles, or a station, which contains multiple stops.
@@ -1438,7 +1435,7 @@ public interface GraphQLStep {

public DataFetcher<Iterable<org.opentripplanner.model.plan.ElevationProfile.Step>> elevationProfile();

public DataFetcher<Object> entity();
public DataFetcher<Object> entrance();

public DataFetcher<String> exit();

Original file line number Diff line number Diff line change
@@ -3942,6 +3942,7 @@ public enum GraphQLRelativeDirection {
CONTINUE,
DEPART,
ELEVATOR,
ENTER_OR_EXIT_STATION,
ENTER_STATION,
EXIT_STATION,
FOLLOW_SIGNS,
Original file line number Diff line number Diff line change
@@ -38,6 +38,7 @@ public static GraphQLRelativeDirection map(RelativeDirection relativeDirection)
case UTURN_RIGHT -> GraphQLRelativeDirection.UTURN_RIGHT;
case ENTER_STATION -> GraphQLRelativeDirection.ENTER_STATION;
case EXIT_STATION -> GraphQLRelativeDirection.EXIT_STATION;
case ENTER_OR_EXIT_STATION -> GraphQLRelativeDirection.ENTER_OR_EXIT_STATION;
case FOLLOW_SIGNS -> GraphQLRelativeDirection.FOLLOW_SIGNS;
};
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -21,6 +21,7 @@ public enum RelativeDirection {
UTURN_RIGHT,
ENTER_STATION,
EXIT_STATION,
ENTER_OR_EXIT_STATION,
FOLLOW_SIGNS;

public static RelativeDirection calculate(

This file was deleted.

Original file line number Diff line number Diff line change
@@ -8,9 +8,9 @@
import org.opentripplanner.framework.i18n.I18NString;
import org.opentripplanner.framework.lang.DoubleUtils;
import org.opentripplanner.framework.tostring.ToStringBuilder;
import org.opentripplanner.model.plan.StepEntity;
import org.opentripplanner.street.model.edge.Edge;
import org.opentripplanner.street.model.note.StreetNote;
import org.opentripplanner.transit.model.site.Entrance;

/**
* Represents one instruction in walking directions. Three examples from New York City:
@@ -45,7 +45,7 @@ public final class WalkStep {
private final boolean walkingBike;

private final String exit;
private final StepEntity entity;
private final Entrance entrance;
private final ElevationProfile elevationProfile;
private final boolean stayOn;

@@ -58,7 +58,7 @@ public final class WalkStep {
I18NString directionText,
Set<StreetNote> streetNotes,
String exit,
StepEntity entity,
Entrance entrance,
ElevationProfile elevationProfile,
boolean bogusName,
boolean walkingBike,
@@ -79,7 +79,7 @@ public final class WalkStep {
this.walkingBike = walkingBike;
this.area = area;
this.exit = exit;
this.entity = entity;
this.entrance = entrance;
this.elevationProfile = elevationProfile;
this.stayOn = stayOn;
this.edges = List.copyOf(Objects.requireNonNull(edges));
@@ -135,10 +135,10 @@ public String getExit() {
}

/**
* Entity related to a step e.g. building entrance/exit.
* Get information about building entrance or exit.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should say "subway stations's entrance" instead of building here. Also since there is already getExit here, maybe rename this and the highway exit rename methods so it's really clear that one is for highways, and the other is for stations.

*/
public Object getEntity() {
return entity;
public Entrance getEntrance() {
return entrance;
}

Copy link
Member

@t2gran t2gran Oct 24, 2024

Choose a reason for hiding this comment

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

I would like to se a diagram with boxes - forget about the class design for a moment and just draw the things we want to add in the future (next 1- 2 years). Then we can discuss how to group them and map the relationship.

Return an Object is no-go, and so is Entrance extends StepEntity.

Copy link
Member

Choose a reason for hiding this comment

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

I drew some sort of a process diagram with boxes together with @HenrikSundell.

entity-diagram

Copy link
Member

Choose a reason for hiding this comment

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

I think this method should probably still return StepEntity instead of Object but I think the step impl must return an Object for the entity which is then handled by StepEntityTypeResolver. It's possible that instead of doing the instanceof checks that are currently done to return correct schema types, we could cast the object to a StepEntity and then check if entrance field is non-null, or escalator field is non-null etc., but I'm not sure how much cleaner that solution is.

Copy link
Member

Choose a reason for hiding this comment

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

I just quickly tested and seems like it's possible to define a class that is used for an union from GraphQL (through codegen configuration). So with this current setup where the different entity types would have a shared base class, you could use that for the union. However, if they don't, then you need to return an Object from the stepImpl#entity() method.

Copy link
Member

Choose a reason for hiding this comment

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

New diagram
step-elements

/**
Original file line number Diff line number Diff line change
@@ -9,10 +9,9 @@
import org.opentripplanner.framework.i18n.I18NString;
import org.opentripplanner.framework.lang.DoubleUtils;
import org.opentripplanner.framework.lang.IntUtils;
import org.opentripplanner.model.plan.Entrance;
import org.opentripplanner.model.plan.StepEntity;
import org.opentripplanner.street.model.edge.Edge;
import org.opentripplanner.street.model.note.StreetNote;
import org.opentripplanner.transit.model.site.Entrance;

public class WalkStepBuilder {

@@ -27,7 +26,7 @@ public class WalkStepBuilder {
private RelativeDirection relativeDirection;
private ElevationProfile elevationProfile;
private String exit;
private StepEntity entity;
private Entrance entrance;
private boolean stayOn = false;
/**
* Distance used for appending elevation profiles
@@ -78,7 +77,7 @@ public WalkStepBuilder withExit(String exit) {
}

public WalkStepBuilder withEntrance(Entrance entrance) {
this.entity = entrance;
this.entrance = entrance;
return this;
}

@@ -164,7 +163,7 @@ public WalkStep build() {
directionText,
streetNotes,
exit,
entity,
entrance,
elevationProfile,
bogusName,
walkingBike,
Original file line number Diff line number Diff line change
@@ -13,7 +13,6 @@
import org.opentripplanner.framework.geometry.WgsCoordinate;
import org.opentripplanner.framework.i18n.I18NString;
import org.opentripplanner.model.plan.ElevationProfile;
import org.opentripplanner.model.plan.Entrance;
import org.opentripplanner.model.plan.RelativeDirection;
import org.opentripplanner.model.plan.WalkStep;
import org.opentripplanner.model.plan.WalkStepBuilder;
@@ -30,6 +29,8 @@
import org.opentripplanner.street.model.vertex.Vertex;
import org.opentripplanner.street.search.TraverseMode;
import org.opentripplanner.street.search.state.State;
import org.opentripplanner.transit.model.basic.Accessibility;
import org.opentripplanner.transit.model.site.Entrance;

/**
* Process a list of states into a list of walking/driving instructions for a street leg.
@@ -528,10 +529,20 @@ private WalkStepBuilder createStationEntranceWalkStep(
// don't care what came before or comes after
var step = createWalkStep(forwardState, backState);

step.withRelativeDirection(RelativeDirection.CONTINUE);
step.withRelativeDirection(RelativeDirection.ENTER_OR_EXIT_STATION);
Copy link
Member

Choose a reason for hiding this comment

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

You could add a comment here to explain that figuring out if something is an entrance or exit is difficult here.


StationEntranceVertex vertex = (StationEntranceVertex) backState.getVertex();
step.withEntrance(Entrance.withCodeAndAccessible(vertex.getCode(), vertex.isAccessible()));

Entrance entrance = Entrance
.of(null)
.withCode(vertex.getCode())
.withCoordinate(new WgsCoordinate(vertex.getCoordinate()))
.withWheelchairAccessibility(
vertex.isAccessible() ? Accessibility.POSSIBLE : Accessibility.NOT_POSSIBLE
)
.build();

step.withEntrance(entrance);
return step;
}

Original file line number Diff line number Diff line change
@@ -30,7 +30,7 @@ public abstract class AbstractTransitEntity<
private final FeedScopedId id;

public AbstractTransitEntity(FeedScopedId id) {
this.id = Objects.requireNonNull(id);
this.id = id; // TODO IS THIS WORNG
}

public final FeedScopedId getId() {
Original file line number Diff line number Diff line change
@@ -73,9 +73,6 @@ interface PlaceInterface {
"Entity related to an alert"
union AlertEntity = Agency | Pattern | Route | RouteType | Stop | StopOnRoute | StopOnTrip | Trip | Unknown

"Entity to a step"
union StepEntity = Entrance

union StopPosition = PositionAtStop | PositionBetweenStops

"A public transport agency"
@@ -449,14 +446,14 @@ type Emissions {

"Station entrance or exit."
Copy link
Member

Choose a reason for hiding this comment

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

You could clarify here that this can originate either from GTFS or from OSM.

type Entrance {
"True if the entrance is wheelchair accessible."
accessible: Boolean
"Short text or a number that identifies the entrance or exit for passengers. For example, `A` or `B`."
code: String
"ID of the entrance in the format of `FeedId:EntranceId`."
Copy link
Member

Choose a reason for hiding this comment

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

Same here, you could mention that if the feed is osm, the entrance is from OSM data.

gtfsId: String
"Name of the entrance or exit."
name: String
"Whether the entrance or exit is accessible by wheelchair"
wheelchairAccessible: WheelchairBoarding
}

"A 'medium' that a fare product applies to, for example cash, 'Oyster Card' or 'DB Navigator App'."
@@ -2660,8 +2657,8 @@ type step {
distance: Float
"The elevation profile as a list of { distance, elevation } values."
elevationProfile: [elevationProfileComponent]
"Step entity, e.g. an entrance."
entity: StepEntity
"Information about an station entrance or exit"
entrance: Entrance
Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm totally mistaken, I think on this API level we still want to have the entity field which returns an entity union.

"When exiting a highway or traffic circle, the exit name/number."
exit: String
"The latitude of the start of the step."
@@ -3329,6 +3326,7 @@ enum RelativeDirection {
CONTINUE
Copy link
Member

Choose a reason for hiding this comment

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

Add description for this enum value. Maybe we could add descriptions for some other values in this enum as well as not all of them are clear.

DEPART
ELEVATOR
ENTER_OR_EXIT_STATION
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to expose this enum value to users, it should be mapped to CONTINUE.

ENTER_STATION
EXIT_STATION
FOLLOW_SIGNS
Loading