Skip to content
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

GH# 74: Snap to streets #148

Merged
merged 10 commits into from
May 25, 2024
Merged

GH# 74: Snap to streets #148

merged 10 commits into from
May 25, 2024

Conversation

rsarathy
Copy link

No description provided.

@rsarathy rsarathy changed the title [WIP] GH# 74: Snap to streets GH# 74: Snap to streets May 24, 2024
@rsarathy rsarathy linked an issue May 24, 2024 that may be closed by this pull request
@rsarathy rsarathy marked this pull request as ready for review May 24, 2024 07:17
@rsarathy rsarathy requested review from graue and abhumbla May 24, 2024 07:17
@rsarathy
Copy link
Author

image
We now avoid sidewalks (namely, any ways tagged highway=footway) when snapping start and end locations.

Copy link

@graue graue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it have been harder to just avoid footway=sidewalk?

I suggested that in case there are some footways we might want to use. On the other hand, this is probably fine. Looking at the footway wiki page it's mostly sidewalk-like things and (for example) the O'Shaughnessy Blvd sidepath is tagged highway=path instead, so it's NBD.

final Snap closest = locationIndex.findClosest(point.lat, point.lon, snapFilters.get(i));
EdgeFilter mergedFilter = new SnapPreventionEdgeFilter(
snapFilters.get(i), roadClassEnc, roadEnvEnc,
Collections.singletonList("footway"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we still want to snap to sidewalks for public transit directions with walking as the connecting mode? Probably, right? If so, this should test for the connecting mode. Not currently relevant but matters when we add a transit/no bike directions mode or want to merge changes upstream.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Let me try and implement this at the config-level, using a snap_preventions field underneath our bike2 profile.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, this is now handled via our configs. We can specify snap_preventions as a comma-separated list.

@rsarathy
Copy link
Author

Would it have been harder to just avoid footway=sidewalk?

I suggested that in case there are some footways we might want to use. On the other hand, this is probably fine. Looking at the footway wiki page it's mostly sidewalk-like things and (for example) the O'Shaughnessy Blvd sidepath is tagged highway=path instead, so it's NBD.

I'm not sure right now how difficult this will be to implement, but I'll investigate it in a separate PR. I see that you opened #123, which captures this ask.

@rsarathy rsarathy merged commit a5ed082 into master May 25, 2024
3 checks passed
@rsarathy rsarathy deleted the rohit/gh-74-snap-to-streets branch May 25, 2024 04:54
@graue
Copy link

graue commented May 25, 2024

#123 is different. That issue is about discouraging routing on sidewalks, not about preventing snapping to them for the start point of a route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snap to streets, not separately-mapped sidewalks
2 participants