Use voice instructions from server by bsudekum · Pull Request #614 · mapbox/mapbox-navigation-ios

bsudekum · 2017-09-14T19:19:08Z

Need mapbox/mapbox-directions-swift#175 merged first

Todo:

Better handle steps with only less than 3 announcements (right now, a high alert assumes there are 3 announcements)
Figure out the future of Alerts
Test, figure out where there are gaps between server and local instruction creation.

/cc @mapbox/guidance @1ec5 @frederoni @ericrwolfe

1ec5 · 2017-09-14T19:22:08Z

Figure out the future of Alerts

🚮

bsudekum · 2017-09-14T19:23:31Z

@1ec5 should we just do an index?

1ec5 · 2017-09-14T19:24:27Z

The plain text instruction is still needed for the AVSpeechSynthesizer fallback. I think it would be reasonable to pass the whole VoiceInstruction object into this method.

1ec5 · 2017-09-14T19:25:20Z

Is userDistance still needed?

I'd like to keep spokenInstructionFormatter.string around for testing and comparison, so yes still needed.

1ec5 · 2017-09-14T19:31:32Z

should we just do an index?

I guess it depends what our customization story will be. In the future, how will custom milestones interact with the preset ones provided by the API? Would the API be able to add more or fewer than three announcements for a particular step if necessary?

As I see it, the main use case for exposing alert levels publicly was to allow developers to time UI changes around the announcements. We can meet that need by calling a delegate method whenever an announcement will be spoken or has been spoken; we’d need to pass in some way of indicating which announcement was spoken, and that information should be pretty closely tied to the API’s way of referring to the announcements. The API doesn’t currently expose any reliable way to determine the urgency of an announcement, other than its position in the array.

Other current uses of alerts should transition over to the much more flexible RouteProgress class.

1ec5 · 2017-09-15T16:03:10Z

It’s pretty weird that RouteController is observing for notifications posted by itself. Can we call these methods directly wherever we’re posting the notifications?

Observing oneself is a little risky since failing to remove the observer for whatever reason would be guaranteed to leak memory. (We’re correctly calling removeObserver(_:) in suspendNotifications(), but it’s still risky.)

Let's fix this in it's own PR.

1ec5 · 2017-09-15T16:05:36Z

A notification should never have “should” in its name: rename this constant to RouteControllerDidPassVoiceInstructionPoint or something along those lines.

A delegate method can have “should” – that would indicate that the method returns a Boolean that controls whether something happens or not. But a notification observer is never able to control the program flow.

1ec5 · 2017-09-15T16:18:40Z

This is a trap. We don’t currently document that NavigationRouteOptions or includesVoiceInstructions must be used with this library. In fact, the readme currently omits any mention of NavigationRouteOptions. We should either devise a graceful fallback strategy or fail upfront whenever NavigationViewController is created without the necessary route options. Either way, we need to update the documentation to use NavigationRouteOptions.

We don’t currently document that NavigationRouteOptions or includesVoiceInstructions must be used with this library.

Tracking a documentation update in #630. Unless we remove this requirement, I’d consider #630 to block this PR.

1ec5 · 2017-09-15T16:20:07Z

Shouldn’t we continue to track this distance? Seems like it would continue to be useful for developers at least.

Let's cut it if it's not used.

1ec5 · 2017-09-15T16:22:43Z

Why does this code live on RouteLegProgress if it’s specific to the current step?

1ec5 · 2017-09-15T17:16:21Z

This will render the volume and speaking rate options nonfunctional. What should we do about those options until API support for them lands?

ksummerill · 2017-09-15T20:16:58Z

cc @ksummerill to keep 👀 on this for a customer

1ec5 · 2017-09-20T01:00:29Z

With my security hat on, this seems like it would enable someone to break the navigation SDK simply by renaming a road to something like <speak> or </speak>. Using a proper XML parser would be best, although if we’re absolutely certain that the Directions API will be giving us a document with <speak> as the root element, then we could address the security concern by restricting finding-and-replacing only at the beginning and end of the string.

1ec5 · 2017-09-20T01:01:28Z

Since alert levels are no more, describe this notification in terms of the user passing an ideal point at which to announce an upcoming maneuver.

1ec5 · 2017-09-20T01:02:14Z

Rename to currentSpokenInstruction for consistency with currentStep, currentLeg, etc.

1ec5 · 2017-09-20T01:03:30Z

Rename to RouteControllerDidPassSpokenInstructionPoint for consistency. Can you look through the PR for other places that should say “spoken” instead of “voice”?

1ec5 · 2017-09-20T01:04:44Z

Is it possible to nest <prosody> tags in SSML? I could imagine a future in which the Directions API decides to pronounce some portion of an instruction slightly louder than another portion.

1ec5 · 2017-09-28T22:41:30Z

Pull this logic out into an extension on Locale, then get the current locale’s measurement system:

mapbox-navigation-ios/MapboxCoreNavigation/DistanceFormatter.swift

Lines 31 to 37 in e1b588b

var usesMetric: Bool {

let locale = numberFormatter.locale as NSLocale

guard let measurementSystem = locale.object(forKey: .measurementSystem) as? String else {

return false

}

return measurementSystem == "Metric"

}

bsudekum · 2017-10-04T16:04:25Z

In order how this PR is blocked:

Add fallback code for unsupported locales Project-OSRM/osrm-text-instructions#168
Updating the directions api
Add language/locale mapbox-directions-swift#176
Add voice instructions mapbox-directions-swift#175

Let's start powering through this list. @1ec5 @mcwhittemore can I get your eyes on Project-OSRM/osrm-text-instructions#168 today?

bsudekum · 2017-10-04T18:06:14Z

Began working on prefetching audio instructions here. Blocked by a few PRs. voice-instructions...prefetch-audio

1ec5 · 2017-10-07T02:16:31Z

This branch is pretty far from compiling. Are you working within a CocoaPods workspace? If so, please try developing directly against the project in this repository.

bsudekum · 2017-10-10T23:37:49Z

~~Alright this is ready to go. @frederoni @1ec5 @JThramer could you 👀 this?~~

Scratch that, still some work to do.

bsudekum · 2017-10-12T00:03:23Z

@1ec5 @frederoni @JThramer okay, this is now ready for review.

1ec5

Everything looks good to go, although I’m not sure if we’re planning to hold this PR until the Directions API catches up to the latest dependencies. Please test against the production API before merging.

1ec5 · 2017-10-13T19:01:48Z

-        input.text = "<speak><amazon:effect name=\"drc\"><prosody volume='\(instructionVoiceVolume)' rate='\(instructionVoiceSpeedRate)'>\(text)</prosody></amazon:effect></speak>"
+        let wrappedSSML = text
+            .replacingOccurrences(of: "^<speak\\s*(?:\\s+[^>]*)?>", with: "$0<amazon:effect name=\"drc\"><prosody volume='\(instructionVoiceVolume)' rate='\(instructionVoiceSpeedRate)'>", options: .regularExpression)
+            .replacingOccurrences(of: "</speak>$", with: "</prosody></amazon:effect></speak>",options: .regularExpression)


Noting for posterity that we need to remove this code once the Directions API begins to add these tags itself.

@1ec5 what do you think about making this future proof-able and adding an xml parsing dependency? It might make sense to check and make sure we have all necessary keys.

No, I think we should introduce an XML parsing library only when we need it for a particular purpose, so that we can evaluate each candidate library based on how well it’d handle our use case. As it stands, we’re planning to have the Directions API add these tags, so the SDK won’t have any XML parsing needs for now.

frederoni · 2017-10-13T20:09:58Z

-     */
-    case arrive
-}
+import AVFoundation


Do we use AVFoundation in RouteProgress?

bsudekum · 2017-10-17T15:05:19Z

Removed our custom tag addition code in 7254395. We'll need the new tags deployed to the Directions API before merging this.

/cc @mcwhittemore @1ec5 @ericrwolfe

1ec5 reviewed Sep 14, 2017

View reviewed changes

ericrwolfe added the general: experimental label Sep 14, 2017

1ec5 reviewed Sep 15, 2017

View reviewed changes

bsudekum force-pushed the voice-instructions branch 3 times, most recently from 10fb598 to f2c31c1 Compare September 19, 2017 18:30

1ec5 reviewed Sep 20, 2017

View reviewed changes

bsudekum force-pushed the voice-instructions branch from f2c31c1 to 08bfe22 Compare September 20, 2017 20:02

bsudekum mentioned this pull request Sep 25, 2017

Timeout polly requests sooner #652

Merged

bsudekum force-pushed the voice-instructions branch 3 times, most recently from bda2dcd to 109bb42 Compare September 28, 2017 21:47

1ec5 reviewed Sep 28, 2017

View reviewed changes

bsudekum force-pushed the voice-instructions branch 3 times, most recently from 46ab7e9 to 7472ed8 Compare October 2, 2017 23:47

bsudekum force-pushed the voice-instructions branch from 7472ed8 to 79f20ec Compare October 4, 2017 16:16

bsudekum force-pushed the voice-instructions branch from 79f20ec to 4394192 Compare October 6, 2017 17:36

bsudekum force-pushed the voice-instructions branch from dcf1328 to ebc163f Compare October 10, 2017 19:47

bsudekum mentioned this pull request Oct 10, 2017

Update MapboxDirections.swift and release v0.4.0 Project-OSRM/osrm-text-instructions.swift#45

Closed

bsudekum force-pushed the voice-instructions branch from 2c84966 to 2de56a9 Compare October 11, 2017 23:12

1ec5 approved these changes Oct 13, 2017

View reviewed changes

frederoni reviewed Oct 13, 2017

View reviewed changes

bsudekum mentioned this pull request Oct 17, 2017

Prefetch audio instructions #724

Merged

Bobby Sudekum added 18 commits October 18, 2017 13:35

Use voice instructions from server

9f002f1

Update

c11c5d5

remove alert

8469664

Obj-c

2e612e0

Fix

40edefe

rm

2adcb0c

Consolidate

718cd37

Change

688316a

Keep index

bcaf052

Upgrade MapboxDirections.swift

7e24c35

Update func names

da3b3f4

Use regex

129da49

rm

d6ab9a6

Use right key

1fda5d8

wrong key

dbb2130

remove

634df8c

remove regex

b9640d7

handle waypoints better

9521078

bsudekum force-pushed the voice-instructions branch from 696cf6c to 9521078 Compare October 18, 2017 20:35

bsudekum merged commit f44f1b5 into master Oct 18, 2017

1ec5 mentioned this pull request Nov 9, 2017

Spoken instructions ignore application locale #821

Closed

1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018

1ec5 added this to the v0.10.0 milestone Dec 18, 2018

1ec5 mentioned this pull request Apr 3, 2019

Will Mapbox navigation keep the compatibility with OSRM? #2072

Closed

1ec5 deleted the voice-instructions branch December 5, 2019 18:11

1ec5 mentioned this pull request Sep 9, 2021

Fix inconsistent route progress and index after rerouting #3345

Merged

	var usesMetric: Bool {
	let locale = numberFormatter.locale as NSLocale
	guard let measurementSystem = locale.object(forKey: .measurementSystem) as? String else {
	return false
	}
	return measurementSystem == "Metric"
	}

Conversation

bsudekum commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1ec5 commented Sep 14, 2017

Uh oh!

bsudekum commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksummerill commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsudekum commented Oct 4, 2017

Uh oh!

1ec5 commented Oct 7, 2017

Uh oh!

bsudekum commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsudekum commented Oct 12, 2017

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

bsudekum commented Sep 14, 2017 •

edited

Loading

bsudekum commented Oct 4, 2017 •

edited

Loading

bsudekum commented Oct 10, 2017 •

edited

Loading