Do not overload user with info at roundabouts#561
Conversation
| } else if alertLevel == .high && upcomingStepDuration < linkedInstructionMultiplier { | ||
| text = String.localizedStringWithFormat(NSLocalizedString("LINKED_UTTERANCE_FORMAT", bundle: .mapboxCoreNavigation, value: "%@, then %@", comment: "Format for speech string; 1 = current instruction; 2 = the following linked instruction"), upComingInstruction, followOnInstruction) | ||
| // If the user is entering the roundabout and there is exit information, don't link instruction | ||
| if let upcomingStep = routeProgress.currentLegProgress.upComingStep, (upcomingStep.maneuverType == .takeRotary || upcomingStep.maneuverType == .takeRoundabout) && upcomingStep.exitIndex != nil { |
There was a problem hiding this comment.
Can we simplify this logic by waiting until Project-OSRM/osrm-backend#4358 is deployed and removing this special case altogether?
There was a problem hiding this comment.
@1ec5 yep, that's the idea. This PR would be a regression until then. We'll also need to update osrm-text-instructions to account for the new type.
There was a problem hiding this comment.
I think this code would read more naturally as:
if followOnInstruction.maneuverType == .exitRoundabout || followOnInstruction.maneuverType == .exitRotaryThat way, if the developer fails to specify includesRoundaboutExitManeuver in the original route options and there’s a turn close to the exit, that turn won’t be omitted entirely.
bsudekum
force-pushed
the
roundabout-exit
branch
from
2beb699 to
dfd7699
Compare
|
@1ec5 this is mergeable now that |
| if let upcomingStep = routeProgress.currentLegProgress.upComingStep, upcomingStep.maneuverType == .exitRoundabout || upcomingStep.maneuverType == .exitRotary { | ||
| text = upComingInstruction | ||
| } else { | ||
| text = String.localizedStringWithFormat(NSLocalizedString("LINKED_UTTERANCE_FORMAT", bundle: .mapboxCoreNavigation, value: "%@, %@", comment: "Format for speech string; 1 = current instruction; 2 = the following linked instruction"), upComingInstruction, followOnInstruction) |
There was a problem hiding this comment.
The value changed from %@, then %@ to %@, %@. Is that intentional?
| } else if alertLevel == .high && upcomingStepDuration < linkedInstructionMultiplier { | ||
| text = String.localizedStringWithFormat(NSLocalizedString("LINKED_UTTERANCE_FORMAT", bundle: .mapboxCoreNavigation, value: "%@, then %@", comment: "Format for speech string; 1 = current instruction; 2 = the following linked instruction"), upComingInstruction, followOnInstruction) | ||
| // If the upcoming step is an .exitRoundabout or .exitRotary, don't link the instruction | ||
| if let upcomingStep = routeProgress.currentLegProgress.upComingStep, upcomingStep.maneuverType == .exitRoundabout || upcomingStep.maneuverType == .exitRotary { |
There was a problem hiding this comment.
Why upComingStep? Wouldn’t the upComingStep be takeRoundabout and followOnStep be exitRoundabout?
There was a problem hiding this comment.
- If
upComingStepistakeRoundaboutandfollowOnStepisexitRoundabout, we announcetakeRoundaboutthenexitRoundabout. 🙅♂️ - If
upComingStepisexitRoundaboutandfollowOnStepisturn, we announce just theexitRoundaboutand never announce theturn. 🙅♂️
|
@1ec5 good call, fixed to look at the |
3 participants
Roundabouts will soon have an exit maneuver. In the past, we have tried to make up for the fact that it is not there by over loading the enter roundabout maneuver with as much info as we could grab onto.
Now, when the user is entering the roundabout, we don't need to link the instruction if the instruction contains exit information.
Before:
Now
Note: when the new maneuver type is added, this PR needs to account for it.
/cc @miccolis @danpat @1ec5