-
Notifications
You must be signed in to change notification settings - Fork 0
Directional Filter and Unnamed Fallback Refinements #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking great! Just the couple small comments
payneBrandon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
chansbtran
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary
Solution