Skip to content

Turn by turn directions#73

Draft
wrangtangle wants to merge 89 commits into
jeancochrane:masterfrom
wrangtangle:turn-by-turn-contd
Draft

Turn by turn directions#73
wrangtangle wants to merge 89 commits into
jeancochrane:masterfrom
wrangtangle:turn-by-turn-contd

Conversation

@wrangtangle

Copy link
Copy Markdown
Contributor

Expands on #48 and #68, and optionally incorporates #61, to implement #69.

Changes

  • Adds turn by turn directions to the UI in the left column on desktop, or below the map on mobile. Individual directions can merge multiple chicago_ways; adding ?debug=true to the URL adds a panel below each direction that shows the ways it's composed of.
image

(As you can see from this screenshot, there are still issues with routing onto sidewalks, which I've opened a separate draft PR for, and with turns onto unnamed streets that could be more specifically identified, though many of the unnamed streets now at least have descriptions.)

  • Describes unnamed ways in directions based on tags:
image
  • Stores Chicago park geometries in DB, uses them to label chicago_ways that are in parks on build, and uses this information to describe unnamed ways in directions as being in parks:
image
  • Add a script to fuzz turn by turn directions between random vertices to find issues. Instructions on running in readme.

To do

  • Still doing some refactoring.
  • There are still many "unnamed streets" that show up in directions. Fuzz directions more to find more combinations of tags that straightforwardly can resolve into descriptions for unnamed streets.
  • I think we could improve the characterization of alleys, e.g. "the alley parallel to x street".
  • As discussed in Classify unnamed streets #62, there's more we could do to disambiguate turns on unnamed streets.
  • Potentially hide this feature behind a setting: checkbox for "Turn by turn directions (beta)". Or, I'm not sure if there's a staging site? We'd want only a smaller group of users to be seeing this until the kinks are worked out.
  • Potentially add little maps to each direction. I was experimenting a bit with doing this with SVG, and it seems potentially doable. I think the minimal version of this that would be useful would represent the previous segment (turning off of), the new segment (turning onto), the roads/paths connected to that vertex, and have labels for anything with a name. I'd expect little maps would be helpful disambiguating turns where several unnamed paths come together.

Kalil and others added 30 commits August 24, 2022 12:58
E.g. http://localhost:8000/from/my-saved-location/to/19%20East%20Chicago%20Avenue%2C%20Chicago%2C%20IL%2060611/
View in map changes from that focused segment back to whole map

@jeancochrane jeancochrane left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Capturing some of our feedback from our in-person UX testing:

  • Remove buffer around nodes when highlighting a route segment (keep the buffer around the ways)
  • Handle navbar sizing on small screens (reduce logo text size, then ultimately remove logo text, leaving just the bike emoji)
  • When clicking on a turn-by-turn route segment, if the directions modal is expanded to full screen, un-expand the modal so that the user can see the segment they've selected
  • When clicking on a turn-by-turn route segment, if the directions modal is not expanded, then the operation to center the map viewport should exclude the portion of the map that is obscured by the directions modal

@wrangtangle

Copy link
Copy Markdown
Contributor Author

Made the changes we discussed, with one difference: I made the header text "Mellow Bike Map" collapse to "MBM" on small viewports, which wasn't an option we'd considered, but seemed to me to preserve the site's branding a bit better than the bike emoji. If you'd rather stick to just getting rid of the site title and showing the bike emoji, lmk.

@jeancochrane jeancochrane left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looking great! Lots of small thoughts, but nothing disqualifying just yet.

Comment thread db/raw/chicago_parks.geojson
Comment thread db/raw/chicago_parks.geojson
Comment thread db/label-parks.sql Outdated
Comment on lines +64 to +69
return (
rows[0]["component"],
rows[0]["component_node_count"],
rows[0]["total_components"],
rows[0]["total_nodes"],
)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Suggestion, optional] It's probably fine since this is a management command and not module code, but situations like these where we're returning a bunch of values in a tuple often call for a dataclass.

Comment thread app/mbm/static/js/turnbyturn/displayDirections.js
Comment thread app/mbm/directions.py Outdated
Comment on lines +192 to +194
total_distance = sum(
feature.get("properties", {}).get("length_m", 0) for feature in features
)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Question, non-blocking] Isn't calculate_route() using distance as this property name instead of length_m? (source)

                'distance': row['length_m'],

Comment thread app/mbm/directions.py
Comment on lines +116 to +130
def _format_distance(meters: float) -> str:
meters_per_mile = 1609.344
meters_per_foot = 0.3048
miles = meters / meters_per_mile
def round_half_up(value: float) -> int:
return int(value + 0.5)

if miles < 0.09:
feet = round_half_up(meters / meters_per_foot)
unit = 'foot' if feet == 1 else 'feet'
return f"{feet} {unit}"

rounded_miles = round_half_up(miles * 10) / 10
unit = 'mile' if rounded_miles == 1 else 'miles'
return f"{rounded_miles} {unit}"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Question, non-blocking] This seems like a dupe of mbm.routing._format_distance(), albeit with slightly different logic. Do they really need to be two different functions? If so, is there a way we can distinguish their function names to make the difference clearer?

Comment on lines +18 to +22
let listItemHtml = `<li class="direction-item" data-direction-index="${index}" data-color="${color}"><span class="direction-icon-wrapper">${icon}</span><span class="direction-text">${directionText}`

listItemHtml += `</span></li>`

$directionsList.append(listItemHtml)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Nitpick, optional] I'm not terribly concerned about it, but I think it's technically possible to insert malicious code into an OSM street name in order to get it templated in here via directionText. Whether or not that's a realistic attack vector is a separate question, but it's easy enough to use the JQuery text() API to sanitize the input (code not tested):

Suggested change
let listItemHtml = `<li class="direction-item" data-direction-index="${index}" data-color="${color}"><span class="direction-icon-wrapper">${icon}</span><span class="direction-text">${directionText}`
listItemHtml += `</span></li>`
$directionsList.append(listItemHtml)
const $li = $('<li>', {
class: 'direction-item',
'data-direction-index': index,
'data-color': color,
})
// icon is a static SVG string we control, so innerHTML is fine here
const $iconWrapper = $('<span>', { class: 'direction-icon-wrapper' }).html(icon)
const $textWrapper = $('<span>', { class: 'direction-text' })
$textWrapper.text(directionText)
$li.append($iconWrapper).append($textWrapper)
$directionsList.append($li)

Comment thread app/mbm/static/js/app.js
onEachFeature: function (feature, layer) {
// Bind popup to allow tooltips on highlighted segments
layer.bindPopup(
`<strong>Name:</strong> ${feature.properties.name}<br>` +

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Nitpick, optional] Here we are also potentially directly rendering untrusted input from OSM, I think. Not totally sure the best way to do so but we should probably escape it if we want to be maximally defensive.

@jeancochrane jeancochrane left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Oh, I also forgot to mention -- there are some merge conflicts with the main branch, do you mind resolving those?

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.

2 participants