Optimal walking routes when walking starts or ends in an area - #4936
Conversation
…area edge with all visibility points.
…sed before graph paths have been fully processed It is a bit unclear why transit routing did not crash before this change
Vertex linking no longer always splits an edge with a new vertex, but sometimes picks an existing vertex which may be alrweady linked. Also area linking may get executed several times.
Area linking runs some geometry.contains() tests, which fail if geometry is void.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #4936 +/- ##
=============================================
- Coverage 63.89% 63.79% -0.10%
+ Complexity 13482 13455 -27
=============================================
Files 1674 1674
Lines 66107 66092 -15
Branches 7143 7152 +9
=============================================
- Hits 42236 42166 -70
- Misses 21488 21533 +45
- Partials 2383 2393 +10
... and 24 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
|
||
| public class LinkStopToPlatformTest { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(LinkStopToPlatformTest.class); |
There was a problem hiding this comment.
It's really, really great to have improved test coverage of this quite hard to understand code. 😻
| @Test | ||
| public void testLinkStopOutsideArea() { | ||
| // test platform is a simple rectangle. It creates a graph of 8 edges. | ||
| Coordinate platform[] = { |
There was a problem hiding this comment.
| Coordinate platform[] = { | |
| Coordinate[] platform = { |
The above line is not wrong but unusual for Java. There are a few others also in the file which would be great if you could change them.
There was a problem hiding this comment.
right! Fixed now.
leonardehrenfried
left a comment
leonardehrenfried
left a comment
There was a problem hiding this comment.
I've tested this and I've consistently seen good results even in complicated areas. I particularly like that there is test coverage of all the complicated cases.
I have a few smaller things that I would like to see changed but all in all I'm quite happy.
There is a bug in graph serialisation. Quick fix until the actual bug is fixed
optionsome
added
the
Digitransit Test
|
I'm not quite sure does this pr cause a need to bump the serialization version or not |
|
I think it does as it modifies AreaEdge so let's add the label just in case. |
leonardehrenfried
added
the
+Bump Serialization Id
|
If you want another review, please click "re-request review". |
|
You can also check |
|
Thanks for advices! This PR is currently in WIP state, because the new area routing finds OSM vertices which have null incoming/outgoing edges after saving and reloading the graph. The root cause must be found. |
4 participants
Summary
Street graph is optimised for walks, which pass through open areas. If an itinerary search starts or ends in a point inside an area ( or a transfer happens within the area ), walk routes are often really strange as the walking gets first linked to a random edge which may travel to totally wrong direction. Path to a desired target point becomes a funny zigzag line.
This pull request fixes those problems by linking points near area edges or inside the area to all so called visibility points. This ensures that street routing can start search to optimal directions. Permanently linked points are added to visibility points, which ensures that direct connections between multiple inserted points become generated. So, transfer path between two area stops is a straight line.
Image below shows a typical area walk before and after this improvement.
Unit tests
Linking is quite complicated business, so many tests are included. The test bench is fairly easily extendable, if need arises.