Improve voice instructions #4696

Trietes · 2024-04-19T11:39:28Z

Issue

Trying to do more work on #4640.

Tasklist

Add tests
Do some more real-world testing
Update the changelog

Requirements / Relations

Also contains the changes made in #4644. So we can either skip #4644 and wait for this one or quickly merge as soon as we know that it works with Maplibre and later on merge this one.

Trietes

It just a draft right now. Even if it works fine in my case it should be tested by somebody else to verify if it works in reality on the road.

Trietes · 2024-04-19T11:44:35Z

src/tyr/route_serializer_osrm.cc

@@ -12,6 +12,8 @@
 #include "midgard/util.h"
 #include "odin/enhancedtrippath.h"
 #include "odin/util.h"
+#include "odin/narrative_builder_factory.h"
+#include "odin/narrativebuilder.h"


Having these here is ofc debatable. We have following problem here: Currently we only build instructions during building the maneuver. But we decide the distance when a maneuver is played in the osrm_serializer and the instructions do depend on the distance (e.g. In 200 meter do XY). So the two options in my mind are:

Calculate the distances here and add the 'IN 200 meters' information here. This needs the dependency and that's how I did it.

Inject the distance information during maneuver building. So we later on just call here e.g. verbal_transition_alert_instruction() and we also get the distance when to play it.

Trietes · 2024-04-19T11:45:34Z

src/tyr/route_serializer_osrm.cc

-const constexpr double SECONDS_BEFORE_VERBAL_TRANSITION_ALERT_INSTRUCTION = 15.0;
-const constexpr double SECONDS_BEFORE_VERBAL_PRE_TRANSITION_INSTRUCTION = 5.0;
+const constexpr double SECONDS_BEFORE_VERBAL_TRANSITION_ALERT_INSTRUCTION = 35.0;
+const constexpr double SECONDS_BEFORE_VERBAL_PRE_TRANSITION_INSTRUCTION = 10.0;


Just increased those values. 5 and 15 seconds sound nice in theory but on the road I experienced that this is way too late. Would be cool to get some feedback of somebody else on this

Also 35 seconds is the way better option for alert instructions when enhancing it by the 'IN 200 meters ...' information.

I agree, these are much better values.

I think that longer term (maybe not this PR), these values be tunable at request time. We can set sensible defaults, but different people / use cases may have different preferences.

Trietes · 2024-04-19T11:51:09Z

src/tyr/route_serializer_osrm.cc

-const constexpr double SECONDS_BEFORE_VERBAL_PRE_TRANSITION_INSTRUCTION = 5.0;
+const constexpr double SECONDS_BEFORE_VERBAL_TRANSITION_ALERT_INSTRUCTION = 35.0;
+const constexpr double SECONDS_BEFORE_VERBAL_PRE_TRANSITION_INSTRUCTION = 10.0;
+const constexpr double MIN_DISTANCE_VERBAL_PRE_TRANSITION_INSTRUCTION = 40;


Adding a minimum distance for the pre transition alert here. I experienced that on low speed roads we will just announce right before the turn due to the low speed. That also often feels way too late. On such low speed roads its even fine to announce 30 seconds before the transition if you can already can see it.
One thing to take into account here is that a routing graph only reflects (ideally) the center of the road and doesn't represent the width of a road.
I'm not sure if I 100% like this concept, but it helped in my case

Trietes · 2024-04-19T11:51:57Z

src/tyr/route_serializer_osrm.cc

+                                  const valhalla::Options& options) {
+  // narrative builder for custom pre alert instructions
+  MarkupFormatter nullFormatter;
+  std::unique_ptr<NarrativeBuilder> narrative_builder = NarrativeBuilderFactory::Create(options, etp, nullFormatter);


Also not 100% happy with this. The functions which we need don't use the markup formatter, so I'm just passing a null formatter.

Trietes · 2024-04-19T11:52:55Z

src/tyr/route_serializer_osrm.cc

@@ -1494,65 +1503,94 @@ json::ArrayPtr voice_instructions(const valhalla::DirectionsLeg::Maneuver* prev_
  // shortly (10 meters at the given speed) after the maneuver has started.
  // The voice_instruction_beginning starts shortly after the beginning of the step.
  // The voice_instruction_end starts shortly before the end of the step.
-  float distance_before_verbal_transition_alert_instruction = -1;
-  float distance_before_verbal_pre_transition_instruction = -1;
-  if (prev_maneuver) {


Just moved this if(prev_maneuver) out of the function. We should always have it.

Trietes · 2024-04-19T11:54:52Z

src/tyr/route_serializer_osrm.cc

+  // TODO: don't add alert instruction if there was already a pre/post transition aleart which is close by
+  if (!maneuver.verbal_transition_alert_instruction().empty() 
+    && distance_before_verbal_transition_alert_instruction > -1.0
+    && prev_maneuver->time() > SECONDS_BEFORE_VERBAL_TRANSITION_ALERT_INSTRUCTION + APPROXIMATE_VERBAL_PRERANSITION_SECONDS) {


This is one of the most important changes. We will not have the alert instructions for short maneuvers. Doesn't make sense in my mind to alert something on a 50 meter road. Just play the transition instruction in that case is fine enough. Two instructions on that small maneuver are way too much.
Actually we should even go further and collapse multiple maneuver into a single one if the maneuver itself is too short

Yeah, this is the exact approach I took in a hacked up previous implementation.

Trietes · 2024-04-19T11:55:37Z

src/tyr/route_serializer_osrm.cc

-    voice_instruction_end->emplace("announcement", maneuver.verbal_transition_alert_instruction());
+    //building voice instructions for the alert
+    float distance_km = (float) distance_along_geometry / 1000.0f; 
+    std::string instruction = narrative_builder->FormVerbalAlertApproachInstruction(distance_km, maneuver.verbal_transition_alert_instruction());


Here is the part where I'm enhancing the alert instruction by the distance information. This needs the narrative_builder. In english language it will add some prefix like 'In x meters ...'

Trietes · 2024-04-19T11:56:04Z

src/tyr/route_serializer_osrm.cc

@@ -1832,29 +1871,28 @@ json::ArrayPtr serialize_legs(const google::protobuf::RepeatedPtrField<valhalla:

      // Add banner instructions if the user requested them
      if (options.banner_instructions()) {
-        if (prev_step) {
+        if (prev_step && prev_maneuver) {


Just ensuring here that we have the prev_maneuver, since I moved it out of the build voice instruction function. Actually it should always be present

ianthetechie

We'll need a review from one of the core maintainers to comment on the parts that you flagged as needing a design review, but throwing in my 2 cents, this 1) does appear to work (running it on some servers now after some local testing), and 2) is a significant improvement.

I'd suggest marking this ready for review @Trietes unless you're still waiting on something I missed.

ianthetechie · 2024-05-16T04:05:41Z

src/tyr/route_serializer_osrm.cc

-const constexpr double SECONDS_BEFORE_VERBAL_TRANSITION_ALERT_INSTRUCTION = 15.0;
-const constexpr double SECONDS_BEFORE_VERBAL_PRE_TRANSITION_INSTRUCTION = 5.0;
+const constexpr double SECONDS_BEFORE_VERBAL_TRANSITION_ALERT_INSTRUCTION = 35.0;
+const constexpr double SECONDS_BEFORE_VERBAL_PRE_TRANSITION_INSTRUCTION = 10.0;


I agree, these are much better values.

I think that longer term (maybe not this PR), these values be tunable at request time. We can set sensible defaults, but different people / use cases may have different preferences.

ianthetechie · 2024-05-16T04:21:16Z

src/tyr/route_serializer_osrm.cc

+  // TODO: don't add alert instruction if there was already a pre/post transition aleart which is close by
+  if (!maneuver.verbal_transition_alert_instruction().empty() 
+    && distance_before_verbal_transition_alert_instruction > -1.0
+    && prev_maneuver->time() > SECONDS_BEFORE_VERBAL_TRANSITION_ALERT_INSTRUCTION + APPROXIMATE_VERBAL_PRERANSITION_SECONDS) {


Yeah, this is the exact approach I took in a hacked up previous implementation.

Trietes · 2024-05-16T18:35:40Z

Thanks for checking it out and giving it a test. I've been waiting for somebody to test it out and saying it is at least better than before :D
I'm only concerned of including the narrative builder here. Works fine, but I don't know if it is good practice. If the maintainers are fine with it, I can add some tests and make the PR ready.

Trietes added 2 commits April 13, 2024 11:26

first improved instructions

af6af8e

Merge branch 'valhalla:master' into improve-voice-instructions

13ef437

Trietes commented Apr 19, 2024

View reviewed changes

ianthetechie reviewed May 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve voice instructions #4696

Improve voice instructions #4696

Trietes commented Apr 19, 2024

Trietes left a comment

Trietes Apr 19, 2024 •

edited

Trietes Apr 19, 2024 •

edited

ianthetechie May 16, 2024

Trietes Apr 19, 2024

Trietes Apr 19, 2024 •

edited

Trietes Apr 19, 2024

Trietes Apr 19, 2024

ianthetechie May 16, 2024

Trietes Apr 19, 2024 •

edited

Trietes Apr 19, 2024 •

edited

ianthetechie left a comment

ianthetechie May 16, 2024

ianthetechie May 16, 2024

Trietes commented May 16, 2024

Improve voice instructions #4696

Are you sure you want to change the base?

Improve voice instructions #4696

Conversation

Trietes commented Apr 19, 2024

Issue

Tasklist

Requirements / Relations

Trietes left a comment

Choose a reason for hiding this comment

Trietes Apr 19, 2024 • edited

Choose a reason for hiding this comment

Trietes Apr 19, 2024 • edited

Choose a reason for hiding this comment

ianthetechie May 16, 2024

Choose a reason for hiding this comment

Trietes Apr 19, 2024

Choose a reason for hiding this comment

Trietes Apr 19, 2024 • edited

Choose a reason for hiding this comment

Trietes Apr 19, 2024

Choose a reason for hiding this comment

Trietes Apr 19, 2024

Choose a reason for hiding this comment

ianthetechie May 16, 2024

Choose a reason for hiding this comment

Trietes Apr 19, 2024 • edited

Choose a reason for hiding this comment

Trietes Apr 19, 2024 • edited

Choose a reason for hiding this comment

ianthetechie left a comment

Choose a reason for hiding this comment

ianthetechie May 16, 2024

Choose a reason for hiding this comment

ianthetechie May 16, 2024

Choose a reason for hiding this comment

Trietes commented May 16, 2024

Trietes Apr 19, 2024 •

edited

Trietes Apr 19, 2024 •

edited

Trietes Apr 19, 2024 •

edited

Trietes Apr 19, 2024 •

edited

Trietes Apr 19, 2024 •

edited