-
Notifications
You must be signed in to change notification settings - Fork 85
Additional TraversalPermissionLabelers #976
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
base: dev
Are you sure you want to change the base?
Conversation
and set NETWORK_FORMAT_VERSION to previous commit hash to force network rebuild
| case "sidewalk" -> new SidewalkTraversalPermissionLabeler(); | ||
| case "noSidewalkCycling" -> new NoSidewalkCyclingTraversalPermissionLabeler(); | ||
| case null -> new USTraversalPermissionLabeler(); | ||
| case null -> new NoSidewalkCyclingTraversalPermissionLabeler(); |
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.
Is the intent here to make the default NoSidewalkCycling? Does this mean that now there's no way to select USTraversalPermissionLabeler?
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.
No, the intent is to keep USTraversalPermissionLabeler. This default case was just set for testing but should be reverted before final review of this PR:
49a205a forces use of the NoSidewalkCyclingTraversalPermissionLabeler and should be reverted before merging.
…yal/r5 into sidewalk-traversal-options
Prevents traversing ways with highway=steps and nodes with kerb=raised
60c0ea8 to
552975a
Compare
to force network rebuild
and reorganize custom TraversalPermissionLabelers
0195be3 to
dc178d5
Compare
| } | ||
|
|
||
| if (permissionLabeler instanceof StepFreeTraversalPermissionLabeler) { | ||
| stepFree = ((StepFreeTraversalPermissionLabeler) permissionLabeler).requireStepFree; |
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.
It feels strange to set this via a way permission labeler, as the labeler instance can't take the action itself and we need to detect its type here. Could stepFree just be considered another separate TransportNetworkConfig option? The boolean field could be pulled up out of StepFreeTraversalPermissionLabeler into TransportNetworkConfig itself. This would then become stepFree = config.stepFree with no instanceof or conditional. The purpose of the field here seems to be only blocking kerbs (not stair/step ways) so perhaps it should be renamed accordingly (e.g. kerbFree or requireKerbCuts).
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.
Yes, it makes sense to make stepFree another TransportNetworkConfig option.
Ways with highway=steps tags are filtered out where appropriate in the custom TraversalPermissionLabelers.
| * USTraversalPermissionLabeler, except walking is disallowed on ways that allow cars, ways with inclines | ||
| * steeper than a specified limit, and ways with steps. | ||
| */ | ||
| public class StepFreeTraversalPermissionLabeler extends USTraversalPermissionLabeler { |
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.
Ideally we should avoid the duplicate code between this and NoSteepInclinesTraversalPermissionLabeler. Could this class just extend NoSteepInclinesTraversalPermissionLabeler? Could they just be a single class with a boolean for disallowing steps (stairs) passed into the constructor? This might be further simplified by pulling the requireStepFree field up to the enclosing NetworkConfig.
and reduce duplicate code
We've had a few requests for custom routing profiles, including:
NoSidewalkCyclingTraversalPermissionLabelerin 7f4282a)NoSteepInclinesTraversalPermissionLabeler)This PR adds these capabilities as custom TraversalPermissionLabelers, which can be applied at network build using TransportNetworkConfig options.