Insert via points at closest road segment #153
Conversation
|
Nice, thank you also for this fix! |
| * @param routes one or more routes to be considered | ||
| * @param location the location for which the closest route should be found | ||
| */ | ||
| export function findNextWayPoint( |
There was a problem hiding this comment.
Can we have a utility class and also export calcDist? (I need/have this for the navi branch already)
src/map/Popup.tsx
Outdated
| }), | ||
| } | ||
| }) | ||
| // we silently assume the way points are the same for all routes (and they are) |
There was a problem hiding this comment.
Aren't we always using the way points for the correct route? (Also there should be only one route and no alternative when using one or more via-points. Or maybe I do not understand what you mean here :) )
There was a problem hiding this comment.
I was thinking along this line:
- the GH API returns one or multiple paths (if there are alternative routes)
- each of the paths has the
snapped_waypointsproperty, even though there is only one set of waypoints (and it is the same for all alternative routes) - findNextWayPoint finds the index of the next way point for the route that is closest to the clicked location and it returns the index of the route alternative as well
- (only) because the way points are the same for all route alternatives can we use this way point index without even checking which route alternative is the closest to the clicked location
So basically there is a mismatch between the UI (one list of way points) and the API (a list of way points for each route alternative). Since we know that the way points are the same for all alternatives we can just ignore this, that is all I wanted to say.
Aren't we always using the way points for the correct route?
Yes, they are automatically correct, because they are the same for each route alternative.
Also there should be only one route and no alternative when using one or more via-points
Also yes, when there are alternatives there can only ever be two way points, because as soon as there are via points we do not calculate alternatives at all (at least for now).
There was a problem hiding this comment.
Thanks! I think, I got it. More complicated than I thought :)
So this feature also introduces a certain dependency between query points and the route(s).
So basically there is a mismatch between the UI (one list of way points) and the API (a list of way points for each route alternative)
Just for my understanding: will the assumption get wrong if e.g. the same query points snap to different waypoints? (e.g. when we implement the via point routing via map matching this could indeed happen)
There was a problem hiding this comment.
So this feature also introduces a certain dependency between query points and the route(s).
There is a (small) dependency, but I would not say that this feature introduces this. For this feature we need to find out between which two current way points we want to introduce the new via point. And the answer is: The two way points at the beginning and end of the route segment that is closest to the clicked location. So we need both: The route coordinates to figure out which route segment is closest to the clicked location, and the way points to find out the way point index where the new via point should be added. This is the exact same we do in GH maps 1 by the way.
Just for my understanding: will the assumption get wrong if e.g. the same query points snap to different waypoints? (e.g. when we implement the via point routing via map matching this could indeed happen)
No, not at all. Maybe the confusion comes from the distinction between way points and query points. I think I was referring to the snapped points the whole time. Let's say our query points are A, B and C. And these query points are snapped to the snapped points a, b and c. Then the API returns a route with coordinates:
a - x1 - x2 - x3 - b - x4 - x5 - x6 - c
and snapped points:
a, b, c
x1...x6 are just points belonging to the route (the route geometry), but importantly the snapped points are also part of the route. If a-x1-x2-x3-b is the route segment that comes closest to the clicked location we know that the new via point should be added in between the query points A and B (because they correspond to the snapped points a and b, which mark the beginning and end of the route segment). For b-x4-x5-x6-c it would be the query points B and C. You can see more such examples in findNextWayPoint.test.ts. So actually very simple and it does not matter at all how much the query points differ from the snapped locations for this logic. The only thing that might be relevant is that there is one snapped point for each query point, but this is always true for the /route endpoint.
There was a problem hiding this comment.
I think I understand what findNextWayPoint does :). But I still do not understand this code comment:
we silently assume the way points are the same for all routes (and they are)
Do you mean the count of the snapped_waypoints must be the same? Because the algorithm should even work when B snaps to a different snapped_waypoints b' in the alternative route (?) and so we don't need to assume that "the [snapped] waypoints are the same for all routes".
There was a problem hiding this comment.
Because the algorithm should even work when B snaps to a different snapped_waypoints b' in the alternative route (?) and so we don't need to assume that "the [snapped] waypoints are the same for all routes".
Yes, this is true. I probably rather meant "we silently assume all routes were calculated for the same query points", or "we silently assume the (query!) way points were the same for all routes (even though all we see here is the server response)". And we assume this even without this feature, because there is only one list of query points in the first place. I was just trying to say that we get a list of alternative routes from the server, and each one is carrying a list of 'snapped_waypoints', but these way points are not independent from each other. They are based on the same query points. And the count is also the same (exactly one snapped point for each query point). I did not intend to emphasize that the snapped points are the same for each route, but rather that there is a 1:1 correspondence between each list of snapped waypoints and the query points, and that this relation is the same for all lists of snapped waypoints.
Or to put it another way. Imagine I gave you three lists of coordinates (just routes) and a list of snapped_waypoints for each of them. And then I would tell you that the clicked location was between index 3 and 4 of the second of the routes. If you wanted to associate these indices to the query points without taking into account that the second route was the closest you would have to know that index 3 and 4 of the snapped_waypoints list correspond to query point 3 and 4 for all routes.
But you are right there could be different snapped points for each route and we could still simply take the index from findNextWayPoint to determine the query points between which we introduce the new point. I'll change the comment to "note that we can use the index returned by findNextWayPoint no matter which route alternative was found to be closest to the clicked location, because for every route the n-th snapped_waypoint corresponds to the n-th query point". Does this make it more clear?
This discussion makes this matter sound a lot more complicated than it actually is :D
There was a problem hiding this comment.
I'll change the comment to "note that we can use the index returned by findNextWayPoint no matter which route alternative was found to be closest to the clicked location, because for every route the n-th snapped_waypoint corresponds to the n-th query point". Does this make it more clear?
Yes, thank you :)
| onSelect, | ||
| }: { | ||
| coordinate: Coordinate | ||
| queryPoints: QueryPoint[] | ||
| route: RouteStoreState |
There was a problem hiding this comment.
I'm not sure if carrying around the route state here is the right thing to do. Should the popup component be coupled to the route store state like this? We need the route coordinates to figure out where a new via point should be inserted, but not sure if the PopupComponent is the right place to do this (but no idea what would be a better place either).
There was a problem hiding this comment.
You could move that locig into the store, but I don't know whether this would improve things. I guess you could also pass a subset of properties instead of the whole state, but this would not be such a big difference I guess.
There was a problem hiding this comment.
Ok, let's merge this then so we can move on :)
Currently new via points that are added using the context menu are added as last (just before the destination), no matter where we click. This PR inserts new via points based on the click location. More precisely, we insert via points between the two way points that are connected by the road segment that comes closest to the click location.
I ported this functionality form GH maps 1 (see graphhopper/graphhopper#791, graphhopper/graphhopper#795, code).
This is part of #37