-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Wheelchair routing profile #1726
Conversation
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.
Thanks! See my comments
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
removed unnecessary comments and code changed handlePriority method be valuable for wheelchair fixed wheelchair svg
Do I need to do something more, additionally to committing and pushing the requested changes? |
Thanks! Somehow I wasn't notified about this. Will review in the next hours&days. |
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.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.
I additionally found wheelchair:no
or wheelchair:limited
- is this already considered? See https://wiki.openstreetmap.org/wiki/Wheelchair_routing#Requirements_for_Wheelchair_Accessible_Routes
Also it really needs two directions support to avoid steep elevation. This is now easy, see Bike2WeightFlagEncoder (new UnsignedDecimalEncodedValue("xy", bits, factor, true)
)
Also it would be important to consider the slope tag.
Edit: Ok, when viewing the tests it seems skipping surface:sand etc is already skipped. Can you comment on this?
Edit2: Ah, you do not "avoid" these tags them but exclude them. Then it would be better to rename avoidHighwayTags and avoidSurface and avoidSmoothness to excludeXY
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
renamed variables to better reflect their usage exclude steep incline and high kerbs
I'm not sure what you mean with that. Isn't that already be solved by skipping ways that are too steep (see below)? Or do you mean that the speed should be adjusted to the incline?
As long as the |
👍
Yes, but you are right that this is not necessary for a first version
OSM tags are very rare for this and I would prefer skipping too steep roads via elevation. See here of how we do this for Bike2WeightFlagEncoder |
I assumed, that elevation data from Now the |
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.
Thanks for the improvements! Looks better. See my new comments
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Show resolved
Hide resolved
core/src/test/java/com/graphhopper/routing/util/WheelchairFlagEncoderTest.java
Outdated
Show resolved
Hide resolved
It is in meter. |
calculate slopes correctly now make two directions for speed encoder configurable for FootFlagEncoder
Sorry for disturbing you again, but can you please have a look at the changes? |
Not a problem. Please leave a comment as I do not track of the commits in the PRs, especially I do not know when something is ready etc Will review this again later. For now can you send us the signed CLA (or just your email address) and add yourself as a contributor in CONTRIBUTORS.md? |
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Show resolved
Hide resolved
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Show resolved
Hide resolved
Thanks again :) ! |
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Show resolved
Hide resolved
I managed to enable wheelchair routing thanks to this pull! I think it’s really awesome that wheelchair routing is available. Thank you so much @don-philipe and @karussell ! I wasn’t aware that it’s only in later releases. I used 0.13 first (as described in the quick start guide), now I’m using 1.0-pre13. Unfortunally, I’m too much of a newbie especially in Java, so I’d need to learn a lot before I can contribute myself :-(. But I’ll definitivly look into it and will see what I can do. For now I hope it's helpful if I do my best as a test user. I already found some issues. Maybe you can help. e. g. for the following request I get: Cannot find point 0: 50.439114,7.655696 Bike2 and foot routing works for the two points. How can I check what's going wrong and maybe solve the issue? |
Another issue with this route: Going via Bahnhofstr. to avoid Löhrstr. seems odd (I can see no reason), also this strange diversion to cross Am Plan/Altengraben... Plus, at the end(after turning right to Gebrüder-Dommermuth-Weg), there're actually (in rl) steps, as far as I remember. |
And another one: yields "Connection between locations not found" although there is one (long way, but possible). |
I think the problems can be divided into two causes:
What do you think? |
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.
Some input from me as an actual wheelchair user...
core/src/main/java/com/graphhopper/routing/util/WheelchairFlagEncoder.java
Show resolved
Hide resolved
excludeHighwayTags.add("steps"); | ||
excludeHighwayTags.add("track"); | ||
|
||
excludeSurfaces.add("cobblestone"); |
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.
I'd prefer not to exclude so strictly. Sometimes, it is unavoidable. Is t possible to give a very bad speed penalty, like 1km/h?
excludeSurfaces.add("gravel"); | ||
excludeSurfaces.add("sand"); | ||
|
||
excludeSmoothness.add("bad"); |
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.
same as with surfaces. The worse the slower...
private final Set<String> excludeSurfaces = new HashSet<>(); | ||
private final Set<String> excludeSmoothness = new HashSet<>(); | ||
private final Set<String> excludeHighwayTags = new HashSet<>(); | ||
private final int maxInclinePercent = 6; |
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.
That's iso norm for ramps, but for ways very restrictive and thus unrealistic. I would raise it to 15, giving it a speed penalty over 6.
excludeHighwayTags.add("primary_link"); | ||
excludeHighwayTags.add("secondary"); | ||
excludeHighwayTags.add("secondary_link"); | ||
excludeHighwayTags.add("tertiary"); |
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 tertiary a Kreisstraße/Landstraße? Then please don't exclude. Sometimes, that's the only available way in rural areas - except for forrest/agricultural roads (Feldwege), wich are much worse.
|
||
double eleDelta = pl.getElevation(pl.size() - 1) - prevEle; | ||
double elePercent = eleDelta / fullDist2D * 100; | ||
int smallInclinePercent = 3; |
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.
I'd raise that to 5,giving a speed penalty over 3.
excludeHighwayTags.add("tertiary"); | ||
excludeHighwayTags.add("tertiary_link"); | ||
excludeHighwayTags.add("steps"); | ||
excludeHighwayTags.add("track"); |
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.
tracktype=grade1 would be possible
Maybe we should add excludeHighwayTags.add("path"); |
In the meantime I changed a bit of code in the WheelchairFlagEncoder and now the links which I posted above produce better results as they point to my GH instance. Basically I removed some restrictions like excluding cobblestones and others which were too strict from my point of view and raised the max slope limit to 15%. The changes are very rudimentary (commenting out lines and replacing 6 with 15 ) and rather suboptimal in a way so that there is a lot of room for improvement. For example it would be better to give a speed penalty to bad surfaces. Unfortunally I didn't have time and knowledge yet to implement that, but at least I wrote some code reviews in regards to this. Maybe we can continue working on that topic. I would be very happy about that and will look more deeply into the matter myself next year. For now Guten Rutsch! |
This pull request adds a new routing profile for wheelchairs. It takes surfaces and smoothness into account. It mainly uses values and rules from the foot routing profile (as, at least in Germany, wheelchair users are equivalent to pedestrians). But in some cases it is more restrictive.
The icon is just a placeholder. I'm sure there are people who can do that better than me.