fix: support Barrier Nodes for NavigateResponse #2975
Conversation
|
Converted to DRAFT. There seem to be errors due to rebasing |
Supplying a route with Intersections located on the same spot, connected with a zero length edge seems to confuse Mapbox like APIs. This can happen if there are barriers on the route, which are split into two intersections connected by a zero length edge. The idea is to find these intersections and merge all the edges of both intersections and remove the out outgoing edge of the connecting edge from the first and the in edge of the incoming edge.
OlafFlebbeBosch
force-pushed
the
barrier
branch
from
6ea91ff to
551fc0d
Compare
|
Fixed rebaseing issue |
|
From my side this looks good and can be merged. Do you have some examples where this currently happens? @boldtrn any objections? |
|
The newly introduced unittest barrierTest in NavigateResponseConverterTest is triggering the issue while However I have to admit that I have issues to reproduce the issue ad hoc on routes like https://graphhopper.com/maps/?point=55.720661%2C13.213068&point=55.720375%2C13.213033&profile=bike&layer=Omniscale |
|
Was tripping over my own feet :) The issue can be seen in sweden: The 2nd/3rd and 4th/5th are located on the same point . The PR fixes this. |
|
Thanks a lot for providing this PR @OlafFlebbeBosch. I can also reproduce the issue at different places on GraphHopper.com 👍. IMHO we should not have two intersections at the same coordinate with incorrect bearings, so this fix makes sense to me. While I also think that clients should not crash due to that, it is good if we can fix this issue here as well :). |
core/src/main/java/com/graphhopper/util/details/IntersectionValues.java
Outdated
Show resolved
Hide resolved
| Integer out = (Integer) intersectionMap.get("out"); | ||
| List<Boolean> entry = (List<Boolean>) intersectionMap.get("entries"); | ||
|
|
||
| if (bearings.size() != entry.size()) { |
There was a problem hiding this comment.
I am not sure about this case, so this is an error case, right? Should we return silent? Or should we at least log the error or even throw an IllegalStateException? This should never happen right?
There was a problem hiding this comment.
AFAIK this should never happen. Not sure what is an appropriate reaction on that. I wasn't aware that IllegalStateException is a RuntimeException, so we do not need to attribute each throw , which is good.
But I can create an Logger instance and Log it as well... Please advise me here.
There was a problem hiding this comment.
Yes, this should result in a HTTP request with response code 400, as Dropwizard catches this. Since we believe this should never happen, I'd say let's throw an exception at least (and maybe even log the result).
| intersection.put("bearings", | ||
| list.stream().map(x -> x.bearing).collect(Collectors.toList())); | ||
| intersection.put("entries", | ||
| list.stream().map(x -> x.entry).collect(Collectors.toList())); |
There was a problem hiding this comment.
Not that it matters that much, but the old implementation is more efficient. This adds two more list iterations.
There was a problem hiding this comment.
Indeed. But I think the runtime costs it will be overshadowed by list allocation, which is needed anyway.
…lues.java Co-authored-by: Robin <boldtrn@users.noreply.github.com>
Throwing now an IllegalstateException for this "should not happen" case |
Supplying a route with Intersections located on the same spot, connected with a zero length edge seems to confuse Mapbox like APIs. This can happen if there are barriers on the route, which are split into two intersections connected by a zero length edge. The idea is to find these intersections and merge all the edges of both intersections and remove the out outgoing edge of the connecting edge from the first and the in edge of the incoming edge for the connection.
Patch is rather large unfortunately because of reformatting based on editorconfig.