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.

yeah i think the long term way we should do it is the second way primarily because it is odins main function, deciding where maneuvers are. deciding when to warn about them is very closely related in my opinion.

having said that, you already did all the work and there has been slow progress on this effort in the past so i think it could stay this way until someone has time for a larger refactor. i would wager that others would agree especially because there are other odin deps mixed in here as well (which was already not great and was for the same reason).

as an aside.. maybe we should have had the actor stuff that tyr provides be part of the valhalla namespace exposed in the main valhalla header and then just have odin subsume all the serialization stuff (essentially dividing up tyrs parts and then removing tyr completely). i think it would be reasonably appropriate for odin to generate maneuvers narrative and do serialization. its essentially serializing the path into maneuvers and then serializing maneuvers into narrative. may as well do the final serialization into json or pbf bytes. this of course ignores that there are some endpoints that have no directions or narrative and maybe those would seem strange calling into odin.. meh just some thoughts 😄

Thanks for having a look! I completely agree. The second way feels more correct, but would need some more time. Would also be better so the normal valhalla json answer could be enhanced by the same distance information which might be helpful.

But then let me clean up this one and write some test. I'll also open a cleanup issue for doing it the second way. Maybe I'll have some time in the future to clean it up.

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

…la#4698) Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

…4705)

…ections_type is set to none (valhalla#4706)

Co-authored-by: Christian <58629404+chrstnbwnkl@users.noreply.github.com>

…alhalla#4710) Co-authored-by: Christian <58629404+chrstnbwnkl@users.noreply.github.com>

Co-authored-by: Christian <58629404+chrstnbwnkl@users.noreply.github.com>

Co-authored-by: Martin Schlossarek <martin.schlossarek@gmail.com>

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.

…uncements (valhalla#4644) Co-authored-by: Christian <58629404+chrstnbwnkl@users.noreply.github.com>

…es/valhalla into improve-voice-instructions

Trietes · 2024-06-09T09:15:15Z

Seems like GitHub doesn't like my rebase. I'm just done with adapting the tests and cleaning the stuff up. I'll open a clean new pull request now

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

kevinkreiser and others added 15 commits April 19, 2024 09:39

typo in docs

c835e2c

PBF support for /expansion (valhalla#4614)

c8f4773

label third_party includes as -isystem in test/CMakeLists.txt (valhal…

56785d0

…la#4698) Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

Address some "unused code" errors (valhalla#4701)

812383f

rm unused import fixes building tests without prime_server (valhalla#…

9888d8e

…4705)

Correctly set has_highway, has_ferry and has_toll attributes when dir…

16fecc1

…ections_type is set to none (valhalla#4706)

Added elapsed_cost to map matching json output (valhalla#4709)

0636e37

test-add workflow to auto-update the fossgis instances (valhalla#4716)

651a515

always save vcpkg cache (valhalla#4719)

629f0dd

Remove tile_extract from config when creating stats (valhalla#4722)

1594fe9

add error when we fail to find any connection in matrix (valhalla#4718)

76f316c

auto-update the fossgis instances (valhalla#4717)

6fbe96e

Co-authored-by: Christian <58629404+chrstnbwnkl@users.noreply.github.com>

add some error communication & handling to valhalla_ingest_transit (v…

26cfae4

…alhalla#4710) Co-authored-by: Christian <58629404+chrstnbwnkl@users.noreply.github.com>

Fix inconsistency in graph.lua for motor_vehicle_node (valhalla#4724)

3463f1a

Co-authored-by: Christian <58629404+chrstnbwnkl@users.noreply.github.com>

fix echo statement for linux/macos in building.md (valhalla#4730)

c14d3ac

Co-authored-by: Martin Schlossarek <martin.schlossarek@gmail.com>

ianthetechie reviewed May 16, 2024

View reviewed changes

Removed voice & banner instructions from last step and added ssmlAnno…

78c1701

…uncements (valhalla#4644) Co-authored-by: Christian <58629404+chrstnbwnkl@users.noreply.github.com>

Trietes mentioned this pull request Jun 4, 2024

Removed voice & banner instructions from last step and added ssmlAnnouncements #4644

Merged

3 tasks

eikes and others added 5 commits June 5, 2024 08:49

Adds documentation for ignore_oneways option (valhalla#4743)

4f6ef05

add deadend to directededge json (valhalla#4751)

48b339b

first improved instructions

f4ce0db

Merge branch 'improve-voice-instructions' of https://github.com/Triet…

e8d387e

…es/valhalla into improve-voice-instructions

cleanup and fixed tests

66e7428

Trietes closed this Jun 9, 2024

Trietes mentioned this pull request Jun 9, 2024

Improve voice instructions #4756

Open

3 tasks

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

kevinkreiser Jun 4, 2024

Trietes Jun 4, 2024

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

Trietes commented Jun 9, 2024

Improve voice instructions #4696

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trietes Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trietes Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trietes commented May 16, 2024

Trietes commented Jun 9, 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