Landmark Routing 6 PR: Update narrative generation with landmarks #4301

vesperlou · 2023-09-19T19:59:13Z

Issue

Update narrative generation process to support the use of landmarks in text guidance.

Tasklist

Update the maneuver types in locales to support landmark replacements. (update the en-us one, and wait for translations)
Update the narrative generation to pick phrases that support the use of landmarks.
Update tests and the changelog accordingly.

Requirements / Relations

Landmark Routing 6: Narrative Generation Updates #4138

nilsnolde · 2023-09-20T07:54:02Z

Reminder to ourselves: since we're updating (possibly) stale translation files here, we should first pull whatever transifex has currently, merge to master, then merge master in this PR, to avoid tons of conflicts. Let's do that right before merging in a few weeks.

nilsnolde · 2023-09-20T08:08:04Z

locales/en-US.json

@@ -2036,6 +2188,7 @@
        "6": "Make a <RELATIVE_DIRECTION> U-turn at <JUNCTION_NAME>.",
        "7": "Make a <RELATIVE_DIRECTION> U-turn toward <TOWARD_SIGN>."
      },
+      "landmark_types": [],


for u-turn we also want landmark narratives

nilsnolde · 2023-09-20T08:30:45Z

src/odin/narrativebuilder.cc

@@ -4531,6 +4581,52 @@ NarrativeBuilder::FormRelativeThreeDirection(DirectionsLeg_Maneuver_Type type,
  }
 }

+std::string NarrativeBuilder::FormLandmarkType(RouteLandmark_Type type,


instead of this function, we can cast the RouteLandmark_Type to uint8_t and index into the phrases directly

nilsnolde · 2023-09-20T08:39:52Z

src/odin/narrativebuilder.cc

+// close, than we pick the nearest left handed one.
+valhalla::RouteLandmark select_landmark(const std::vector<valhalla::RouteLandmark>& landmarks,
+                                        const std::string& relative_direction) {
+  bool direction = relative_direction == "right" ? 1 : 0;


Suggested change

bool direction = relative_direction == "right" ? 1 : 0;

bool turn_direction = relative_direction == "right";

nilsnolde · 2023-09-20T08:46:01Z

src/odin/narrativebuilder.cc

+  int idx = -1;
+  double min_dist = -1;
+  for (auto i = 0; i < landmarks.size(); ++i) {
+    // adjust landmark distance with compensation based on their direction
+    auto dist = landmarks[i].right() == direction
+                    ? landmarks[i].distance()
+                    : (landmarks[i].distance() + kLandmarkOppositeDirectionCompensation);
+    if (idx == -1) {
+      idx = i;
+      min_dist = dist;
+    } else if (dist < min_dist) {
+      idx = i;
+      min_dist = dist;
+    }
+  }
+  return landmarks[idx];
+}


Suggested change

int idx = -1;

double min_dist = -1;

for (auto i = 0; i < landmarks.size(); ++i) {

// adjust landmark distance with compensation based on their direction

auto dist = landmarks[i].right() == direction

? landmarks[i].distance()

: (landmarks[i].distance() + kLandmarkOppositeDirectionCompensation);

if (idx == -1) {

idx = i;

min_dist = dist;

} else if (dist < min_dist) {

idx = i;

min_dist = dist;

}

}

return landmarks[idx];

}

size_t idx = 0;

double min_dist = std::numeric_limits<double>::max();

for (auto i = 0; i < landmarks.size(); i++) {

// adjust landmark distance with compensation based on their direction

auto dist = landmarks[i].right() == direction

? landmarks[i].distance()

: (landmarks[i].distance() + kLandmarkOppositeDirectionCompensation);

if (dist < min_dist) {

idx = i;

min_dist = dist;

}

}

return landmarks[idx];

}

nilsnolde · 2023-09-20T08:49:10Z

src/odin/narrativebuilder.cc

+    // Make sure the landmark type is legal
+    if (static_cast<int>(landmark.type()) > 0 &&
+        static_cast<int>(landmark.type()) < subset->landmark_types.size()) {
+      // increment the phrase id by 6 to get the corresponding landmark phrase
+      phrase_id += 6;
+    }


IMO we don't need a check like this, bcs we're in full control of the landmark types and we do make sure in the data building pipeline that nothing enters that's outside of the type bounds. If we want to check this at this point, then let's throw an exception, bcs that'd be a very serious flaw.

…fine code

…ot added in case of lack of vacancy, and inconsistent input in valhalla_add_landmarks and AddLandmarks

nilsnolde · 2023-09-22T12:28:12Z

One thing we noticed is that the overall graph size is shrinking after adding landmarks.

vesperlou · 2023-09-22T12:56:55Z

src/mjolnir/valhalla_add_landmarks.cc

@@ -51,7 +49,7 @@ int main(int argc, char** argv) {
    valhalla::midgard::logging::Configure(logging_config);
  }

-  if (!valhalla::mjolnir::AddLandmarks(pt.get_child("mjolnir"))) {
+  if (!valhalla::mjolnir::AddLandmarks(pt)) {


Suggested change

if (!valhalla::mjolnir::AddLandmarks(pt)) {

if (!valhalla::mjolnir::AddLandmarks(pt.get_child("mjolnir"))) {

I've edited AddLandmarks to handle the input as child mjolnir, maybe we want to change this back to get child mjolnir. it works for me :)

Yeah I must not've seen that for some reason. Weird though that I could push at all if I didn't pull your changes before.. I'll revert this!

I guess you can push because there is no conflict caused by this commit

nilsnolde · 2024-02-12T13:13:33Z

I started looking at this again, but didn't get very far yet. When I tried a route request, it wouldn't work, because we mess smth up when adding landmarks and make the tiles invalid somehow (as said before, tile size shrinks when adding landmarks!). It's not much code, but still annoying to debug.

kevinkreiser · 2024-02-12T15:22:03Z

yeah i was hoping we could patch it up to the point that it could somewhat work sometimes and be a thing we opt into. maybe in the coming weeks we can pair on it for an hour at a time a couple times a week

…o landmark_narrative

…o reproduce in tests

nilsnolde · 2024-02-24T19:25:18Z

my shortcut PR to remove names/speed is just tripping over the landmarks stuff: @kevinkreiser reminder for us to check how shortcuts are behaving with landmarks.. might be very well the key..

kevinkreiser · 2024-02-25T02:58:45Z

interesting hypothesis. imho we shouldn't put them on shortcuts anyway its a duplication without purpose

nilsnolde · 2024-02-25T08:49:21Z

Yeah we don’t want that. But seems like we are, and while not being aware of it, ~~maybe we’re forgetting about the base edges in the process and they get lost..~~ meeh, that’s very unlikely

kevinkreiser · 2024-02-25T11:15:45Z

also we use loki search iirc which doesn't return shortcuts

nilsnolde · 2024-02-25T12:15:38Z

Right, I think that was a fluke.. the test was going over the tileset and exactly, didn’t expect to see shortcuts, so all good there

…o landmark_narrative

vesperlou added 2 commits September 19, 2023 21:39

update locales with landmark name and landmark type

3bf1d1f

update narrative generation to add landmarks, and update test

3b5c08a

nilsnolde reviewed Sep 20, 2023

View reviewed changes

vesperlou added 3 commits September 20, 2023 13:02

address code reviews, update uturn instruction with landmarks, and re…

05e346d

…fine code

update Changelog and code cleaning

78f15a7

spell

b13a0d3

vesperlou requested review from nilsnolde and kevinkreiser September 20, 2023 13:19

fix previous bugs: fix incorrect offset increment when name info is n…

9e78ff3

…ot added in case of lack of vacancy, and inconsistent input in valhalla_add_landmarks and AddLandmarks

vesperlou changed the title ~~Update narrative generation with landmarks~~ Landmark Routing 6 PR: Update narrative generation with landmarks Sep 21, 2023

fix valhalla_add_landmarks

2f914b0

vesperlou commented Sep 22, 2023

View reviewed changes

nilsnolde mentioned this pull request Oct 21, 2023

prepare for 3.4.1 #4347

Open

9 tasks

nilsnolde added 2 commits February 12, 2024 09:51

Merge branch 'master' into landmark_narrative

fc07d9b

gurka_landmarks is warning-free

0c7374a

kevinkreiser and others added 4 commits February 19, 2024 09:19

resolve conflict

584eaa6

Merge branch 'landmark_narrative' of github.com:valhalla/valhalla int…

8067a10

…o landmark_narrative

add a failing test, adding landmarks should not shrink the tiles

978bfb1

add correct test but its not failing yet. real world does fail need t…

6657259

…o reproduce in tests

nilsnolde added 2 commits March 23, 2024 21:28

Merge branch 'master' into landmark_narrative

7ebc5fd

Merge branch 'landmark_narrative' of github.com:valhalla/valhalla int…

9b4ee2b

…o landmark_narrative

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Landmark Routing 6 PR: Update narrative generation with landmarks #4301

Landmark Routing 6 PR: Update narrative generation with landmarks #4301

vesperlou commented Sep 19, 2023 •

edited

nilsnolde commented Sep 20, 2023 •

edited

nilsnolde Sep 20, 2023

nilsnolde Sep 20, 2023

nilsnolde Sep 20, 2023 •

edited

nilsnolde Sep 20, 2023

nilsnolde Sep 20, 2023

nilsnolde commented Sep 22, 2023

vesperlou Sep 22, 2023

nilsnolde Sep 22, 2023

vesperlou Sep 22, 2023

nilsnolde commented Feb 12, 2024

kevinkreiser commented Feb 12, 2024

nilsnolde commented Feb 24, 2024

kevinkreiser commented Feb 25, 2024 •

edited

nilsnolde commented Feb 25, 2024 •

edited

kevinkreiser commented Feb 25, 2024

nilsnolde commented Feb 25, 2024

	bool direction = relative_direction == "right" ? 1 : 0;
	bool turn_direction = relative_direction == "right";

	if (!valhalla::mjolnir::AddLandmarks(pt)) {
	if (!valhalla::mjolnir::AddLandmarks(pt.get_child("mjolnir"))) {

Landmark Routing 6 PR: Update narrative generation with landmarks #4301

Are you sure you want to change the base?

Landmark Routing 6 PR: Update narrative generation with landmarks #4301

Conversation

vesperlou commented Sep 19, 2023 • edited

Issue

Tasklist

Requirements / Relations

nilsnolde commented Sep 20, 2023 • edited

nilsnolde Sep 20, 2023

Choose a reason for hiding this comment

nilsnolde Sep 20, 2023

Choose a reason for hiding this comment

nilsnolde Sep 20, 2023 • edited

Choose a reason for hiding this comment

nilsnolde Sep 20, 2023

Choose a reason for hiding this comment

nilsnolde Sep 20, 2023

Choose a reason for hiding this comment

nilsnolde commented Sep 22, 2023

vesperlou Sep 22, 2023

Choose a reason for hiding this comment

nilsnolde Sep 22, 2023

Choose a reason for hiding this comment

vesperlou Sep 22, 2023

Choose a reason for hiding this comment

nilsnolde commented Feb 12, 2024

kevinkreiser commented Feb 12, 2024

nilsnolde commented Feb 24, 2024

kevinkreiser commented Feb 25, 2024 • edited

nilsnolde commented Feb 25, 2024 • edited

kevinkreiser commented Feb 25, 2024

nilsnolde commented Feb 25, 2024

vesperlou commented Sep 19, 2023 •

edited

nilsnolde commented Sep 20, 2023 •

edited

nilsnolde Sep 20, 2023 •

edited

kevinkreiser commented Feb 25, 2024 •

edited

nilsnolde commented Feb 25, 2024 •

edited