Snap To Rail #1027
Snap To Rail #1027
Conversation
josh-willis-arcadis
added
the
BLOCKED
josh-willis-arcadis
requested review from
binh-dam-ibigroup and
miles-grant-ibigroup
There was a problem hiding this comment.
I see quite a bit of potential to reduce the number of files touched. I'd start by leaving followStreets untouched. That will also make the edits to valhalla less dangerous. We can leave that file mostly untouched and merely add a short check after the alternates section to switch to rail if followRail is true.
That way we can also configure rail routing within alternates. Just have to add a rail flag on each graphhopper alternate!
| graphHopperKey = KEY | ||
| } else { // no URL or need default value so use graphhopper test environment | ||
| graphHopperUrl = 'https://graphhopper.com/api/1/' | ||
| // include key here??? this will throw 401 i believe |
There was a problem hiding this comment.
You deleted the code that makes it required to have a graphhopper key! That's a required param
| avoidMotorways?: boolean, | ||
| snapToOption: string |
There was a problem hiding this comment.
I'm not sure you're allowed to put an optional before a required parameter
| if (process.env.GRAPH_HOPPER_ALTERNATES) { | ||
| // $FlowFixMe This is a bit of a hack and not how env variables are supposed to work, but the yaml loader supports it. | ||
| const alternates: Array<GraphHopperAlternateServer> = process.env.GRAPH_HOPPER_ALTERNATES | ||
| alternates.forEach(alternative => { | ||
| const { BBOX, ROUTING_TYPE, URL, KEY } = alternative | ||
| if (BBOX.length !== 4) { |
There was a problem hiding this comment.
The edits to Valhalla are quite clunky. Couldn't we instead mostly keep things as is and only tweak the url/key after the existing block?
| function arePointsValid (points, BBOX) { | ||
| return points.every( | ||
| (point) => !coordIsOutOfBounds( | ||
| point, | ||
| L.latLngBounds( | ||
| [BBOX[1], BBOX[0]], | ||
| [BBOX[3], BBOX[2]] | ||
| ) | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Why was this imported into this file?
| @@ -162,17 +162,17 @@ export function handleControlPointDrag ( | |||
| patternCoordinates: any | |||
| ) { | |||
| return function (dispatch: dispatchFn, getState: getStateFn) { | |||
| const {avoidMotorways, currentDragId, followStreets} = getState().editor.editSettings.present | |||
| const {avoidMotorways, currentDragId, snapToOption} = getState().editor.editSettings.present | |||
There was a problem hiding this comment.
Could we call this followOption instead of snapToOption?
| @@ -19,6 +19,11 @@ export const CLICK_OPTIONS: Array<string> = [ | |||
| 'ADD_STOPS_AT_INTERVAL', | |||
| 'ADD_STOPS_AT_INTERSECTIONS' | |||
| ] | |||
| export const SNAP_TO_OPTIONS: Array<string> = [ | |||
There was a problem hiding this comment.
Why is this an array instead of an enum?
| return ( | ||
| <option | ||
| key={v} | ||
| value={v}>{toSentenceCase(v.replace(/_/g, ' '))} |
There was a problem hiding this comment.
You could avoid this by using an enum and then having a map from enum to string
There was a problem hiding this comment.
with JS you could even have one large object that has both enum value and string!
| @@ -83,7 +97,7 @@ export default class EditSettings extends Component<Props, State> { | |||
| // this state would cause the entire shape to disappear). | |||
| disabled={ | |||
| (s.type === 'hideInactiveSegments' && noSegmentIsActive) || | |||
| (s.type === 'avoidMotorways' && !followStreets) | |||
| (s.type === 'avoidMotorways' && snapToOption !== 'STREET') | |||
There was a problem hiding this comment.
No magic strings! Let's use enums instead if we stick with snapToOption
| @@ -104,7 +104,7 @@ export default class EditShapePanel extends Component<Props> { | |||
| return {lng: stop.stop_lon, lat: stop.stop_lat} | |||
| }) | |||
| : [] | |||
| this.drawPatternFromStops(activePattern, stopLocations, editSettings.present.followStreets) | |||
| this.drawPatternFromStops(activePattern, stopLocations) | |||
There was a problem hiding this comment.
Uh oh! I'm not sure followStreets was meant to be removed. Maybe let's keep followStreets as is and add a new boolean for followRail. It makes the UI work a bit more tricky but it helps us avoid touching too many files
| // RAIL-TODO: make sure followStreet is properly set. | ||
| // RAIL-TODO: use in a lot of places, make sure nothing breaks???? | ||
| return update(state, { | ||
| [setting]: {$set: value}, | ||
| followStreets: {$set: value === 'STREET'} | ||
| }) |
There was a problem hiding this comment.
Yes let's definitely leave followStreets alone
miles-grant-ibigroup
removed
the
BLOCKED
Add support for a snap to rail feature when editing trip patterns. Uses OpenRailRouter, a fork of GraphHopper.