Directional Filter and Unnamed Fallback Refinements#25
Conversation
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Show resolved
Hide resolved
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Outdated
Show resolved
Hide resolved
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Outdated
Show resolved
Hide resolved
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Show resolved
Hide resolved
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR refines buffer path selection by improving direction-based filtering and adjusting unnamed-road fallback behavior, while extracting the direction logic into a dedicated helper for better testability.
Changes:
- Introduces
DirectionFilterHelperand unit tests (including a regression case) for direction-based path filtering. - Updates direction filtering behavior to default to the primary path when path terminal deltas are below a threshold-derived cutoff.
- Adjusts unnamed fallback traversal to choose the straightest continuation among both named and unnamed candidate edges.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java |
New helper encapsulating direction filtering logic, including new threshold behavior. |
web-bundle/src/main/java/com/graphhopper/resources/BufferResource.java |
Uses DirectionFilterHelper and refines unnamed fallback edge selection toward straightest continuation. |
web-bundle/src/test/java/com/graphhopper/util/DirectionFilterHelperTest.java |
Adds unit and regression tests for the extracted helper logic. |
web-bundle/src/test/resources/test-examples/regression-direction-filter-test.json |
Adds GeoJSON input for the regression test case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Outdated
Show resolved
Hide resolved
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Show resolved
Hide resolved
web-bundle/src/main/java/com/graphhopper/resources/BufferResource.java
Outdated
Show resolved
Hide resolved
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Outdated
Show resolved
Hide resolved
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Show resolved
Hide resolved
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Outdated
Show resolved
Hide resolved
| * Filters a list of LineStrings based on the specified cardinal or intercardinal direction. | ||
| * Compares the furthest points of the first and last LineStrings to determine which aligns | ||
| * best with the desired direction, returning only the selected LineString. | ||
| * @param lineStrings list of LineStrings to filter |
There was a problem hiding this comment.
question: is this assuming only 2 lineString values? would it be more clear to pass them in as path1/2 instead?
There was a problem hiding this comment.
ah I see we have a case for both of them...maybejust update the param description to indicate we're expecting 2 and don't compare across an entire list
There was a problem hiding this comment.
The normal case is to have two LineStrings. It is actually possible to have just one LineString but the code still works with one LineString (although it is doing unnecessary work with one LineString).
It used to be the case that it was always two and never one.
It is never possible to have more than two. It would make sense to have a check for 'just one' and to simply take the one and not call filterPathsByDirection if it was just one. If that were done, then it would be safe to use the indices to specify first and second inside filterPathsByDirection but it would not be safe IF it was still possible to call the method with just one.
Important note is that the final return there:
return createGeoJsonResponse(filteredLineStrings, stopWatch);
DOES need to accept a List of LineString because filterPathsByDirection will return two LineStrings if it receives two LineStrings AND ALSO has a direction of 'BOTH'.
web-bundle/src/main/java/com/graphhopper/util/DirectionFilterHelper.java
Outdated
Show resolved
Hide resolved
payneBrandon
left a comment
There was a problem hiding this comment.
looking great! Just the couple small comments
Summary
Solution