Pronunciation for names and destinations #3132

gknisely · 2021-06-08T14:43:01Z

Issue

Process the pronunciation for names and destinations in OSM. Store the pronunciation with the associated name or destination(sign).

Tasklist

Add tests
Update the changelog
If you made changes to the lua files, update the taginfo too.

…s and pronunciation logic.

…e file. Way data is saved to new pronunciation file

…nt_. A sign can have multiple phoneme types.

kevinkreiser · 2021-06-08T20:25:33Z

there was another pr that @danpaz started that was attempting to do this, the size of this PR is surprizing compared to what i remember that one was. do you guys mind filling out the description a bit when you are ready?

src/baldr/edgeinfo.cc

kevinkreiser · 2021-06-08T20:33:27Z

i know its probably not ready yet but, i thought i saw at different places in the code, different names being used for the same concept: phoneme, pronounciation, verbal would we be able to use just one of these for the sake of reducing confusion? or maybe these are different things and i didnt review it closely enough yet?

dgearhart · 2021-10-06T21:28:53Z

test/gurka/test_route_with_pronunciations.cc

+            "ˈgɹænvɪl");
+
+  // Verify guide sign pronunciation instructions
+  gurka::assert::raw::expect_instructions_at_maneuver_index(


Verify guide sign pronunciation instructions

dgearhart · 2021-10-06T21:29:52Z

test/gurka/test_route_with_pronunciations.cc

+            "ɛm ˈʤʌŋkʃən");
+
+  // Verify junction name pronunciation instructions
+  gurka::assert::raw::expect_instructions_at_maneuver_index(


Verify junction name pronunciation instructions

dgearhart · 2021-10-06T21:33:27Z

valhalla/baldr/pronunciation.h

+namespace valhalla {
+namespace baldr {
+
+class Pronunciation {


class to use with street names and signs through odin

this is completely trivial, could we just make it a struct with public members and no constructor?

dgearhart · 2021-10-06T21:36:36Z

valhalla/odin/markup_formatter.h

+namespace valhalla {
+namespace odin {
+
+class MarkupFormatter {


new class that will handle the formatting of our markup strings

dgearhart · 2021-10-06T21:37:23Z

valhalla/odin/narrativebuilder.h

@@ -20,7 +21,8 @@ class NarrativeBuilder {
 public:
  NarrativeBuilder(const Options& options,
                   const EnhancedTripLeg* trip_path,
-                   const NarrativeDictionary& dictionary);
+                   const NarrativeDictionary& dictionary,
+                   const MarkupFormatter& markup_formatter);


pass markup formatter to the narrative builder

dgearhart · 2021-10-06T21:38:41Z

valhalla/odin/sign.h

@@ -50,6 +63,7 @@ class Sign {
  std::string text_;
  bool is_route_number_;
  uint32_t consecutive_count_;
+  boost::optional<baldr::Pronunciation> pronunciation_;


optional pronunciation for a sign

dgearhart · 2021-10-06T21:40:21Z

valhalla/proto_conversions.h

@@ -127,6 +127,24 @@ inline TripLeg_Node_Type GetTripLegNodeType(const baldr::NodeType node_type) {
                           " Unhandled NodeType: " + std::to_string(num));
 }

+inline Pronunciation_Alphabet
+GetTripPronunciationAlphabet(const valhalla::baldr::PronunciationAlphabet pronunciation_alphabet) {


transform to proto Pronunciation Alphabet

dgearhart

LGTM - would appreciate @kevinkreiser 👀 for final review

kevinkreiser · 2021-10-07T15:07:43Z

Yep I'll review later today!

…onemes

CHANGELOG.md

src/baldr/edgeinfo.cc

…phonemes

kevinkreiser · 2021-10-07T19:22:48Z

src/baldr/graphtile.cc

+            std::unordered_map<uint32_t, std::pair<uint8_t, std::string>>::iterator iter =
+                index_pronunciation_map.find(header.name_index_);
+
+            if (iter == index_pronunciation_map.end())
+              index_pronunciation_map.emplace(
+                  std::make_pair(header.name_index_,
+                                 std::make_pair(header.phonetic_alphabet_,
+                                                std::string((text + pos), header.length_))));
+            else {
+              if (header.phonetic_alphabet_ > (iter->second).first) {
+                iter->second = std::make_pair(header.phonetic_alphabet_,
+                                              std::string((text + pos), header.length_));
+              }


you can use std::unordered_map<>::insert and if it already existed it will give you back the iterator where it already existed. that said, why do we want to overwrite it?

Ok. And yes; we use Jeita over Katakanna if it exists

kevinkreiser · 2021-10-07T19:26:02Z

src/baldr/pronunciation.cc

+// Constructor
+Pronunciation::Pronunciation(const valhalla::Pronunciation_Alphabet alphabet,
+                             const std::string& value)
+    : alphabet_(alphabet), value_(value) {
+}
+
+valhalla::Pronunciation_Alphabet Pronunciation::alphabet() const {
+  return alphabet_;
+}
+
+const std::string& Pronunciation::value() const {
+  return value_;
+}


why have this? if you make the header a struct we dont need to have this implementation at all. you can intialize it with the brace initializer and you can just use .value etc to get the information. it just seems not useful to me to have an impelmentation file which does nothing that you cant achieve with just the header and a simple struct. maybe this is just a style thing but i prefer less files to manage than more. if there is only one place this struct is referenced i might even just put it in that files header and not even make a new header, its already hard to manage all the source files imho

@kevinkreiser can we refactor this for the next release? so we have more time to test the refactor

kevinkreiser · 2021-10-07T19:29:40Z

src/mjolnir/graphbuilder.cc

-                                                     tagged_values, types, added, dual_refs);
+            edge_info_offset =
+                graphtile.AddEdgeInfo(edge_pair.second, (*nodes[source]).graph_id,
+                                      (*nodes[target]).graph_id, w.way_id(), 1234, bike_network,


i thought we fixed this random elevation bug previously....

kevinkreiser · 2021-10-07T19:54:00Z

src/mjolnir/graphbuilder.cc

+    std::vector<std::string> ipa_tokens, nt_sampa_tokens, katakana_tokens, jeita_tokens;
+    GetPronunciationTokens(osmdata, node.ref_pronunciation_ipa_index(),
+                           node.ref_pronunciation_nt_sampa_index(),
+                           node.ref_pronunciation_katakana_index(),
+                           node.ref_pronunciation_jeita_index(), ipa_tokens, nt_sampa_tokens,
+                           katakana_tokens, jeita_tokens, true);
+
+    bool add_ipa = (ipa_tokens.size() && n_refs.size() == ipa_tokens.size());
+    bool add_nt_sampa = (nt_sampa_tokens.size() && n_refs.size() == nt_sampa_tokens.size());
+    bool add_katakana = (katakana_tokens.size() && n_refs.size() == katakana_tokens.size());
+    bool add_jeita = (jeita_tokens.size() && n_refs.size() == jeita_tokens.size());
+
+    for (size_t i = 0; i < n_refs.size(); ++i) {
+      if (add_ipa || add_nt_sampa || add_katakana || add_jeita) {
+        uint32_t count = 0;
+        const size_t size = pronunciations.size();
+        BuildPronunciations(ipa_tokens, nt_sampa_tokens, katakana_tokens, jeita_tokens, i,
+                            pronunciations, add_ipa, add_nt_sampa, add_katakana, add_jeita, count);
+        exit_list.emplace_back(Sign::Type::kExitNumber, false, false, (count != 0), size, count,
+                               n_refs[i]);
+      } else
+        exit_list.emplace_back(Sign::Type::kExitNumber, false, false, false, 0, 0, n_refs[i]);


too bad this isnt in a lambda would be nice to have less duplication

kevinkreiser · 2021-10-07T20:00:56Z

src/mjolnir/graphvalidator.cc

@@ -311,7 +311,7 @@ void validate(

      // Validate signs
      if (ni->named_intersection()) {
-        if (tile->GetSigns(i, true).size() == 0) {
+        if (tile->ProcessSigns(i, true).size() == 0) {


nit: but i liked the previous name way better

kevinkreiser · 2021-10-07T20:10:15Z

src/mjolnir/osmway.cc

+    names.insert(names.end(), tokens.begin(), tokens.end());
+
+    size_t key = names.size() - tokens.size();
+    AddPronunciations(pronunciations, name_offset_map,


i feel like inside AddPronunciations we could have done all 4 rather than call it 4 times?

kevinkreiser · 2021-10-07T20:13:23Z

src/odin/maneuversbuilder.cc

-        StreetNamesFactory::Create(trip_path_->GetCountryCode(node_index), prev_edge->GetNameList()));
+        StreetNamesFactory::Create(trip_path_->GetCountryCode(node_index), prev_edge->name()));


this one is not obvious to me

kevinkreiser · 2021-10-07T20:14:39Z

src/odin/maneuversbuilder.cc

@@ -1357,7 +1377,7 @@ void ManeuversBuilder::FinalizeManeuver(Maneuver& maneuver, int node_index) {
  if (!curr_edge->IsHighway() && !curr_edge->internal_intersection() &&
      (curr_edge->name_size() > 1)) {
    std::unique_ptr<StreetNames> curr_edge_names =
-        StreetNamesFactory::Create(trip_path_->GetCountryCode(node_index), curr_edge->GetNameList());
+        StreetNamesFactory::Create(trip_path_->GetCountryCode(node_index), curr_edge->name());


there are more, was it a function name change or just over complicated for no reason or?

@kevinkreiser had to change the method since we need the pronunciation too - it just had a list of name, is route num pair before

kevinkreiser

I think we could streamline this a bit more but good for now

gknisely · 2021-10-07T20:52:41Z

@dgearhart and I will have a follow up PR to clean things up. TY @kevinkreiser

gknisely and others added 27 commits May 21, 2021 11:00

added GetPronunciationsMap() and only_pronunciations to GetTaggedName…

a1b5882

…s and pronunciation logic.

updated getsign logic for pronunciations

42b6925

updated call to addedgeinfo

0431b98

added logic to process pronunciation_file and pronunciations.

380342a

updated call to .GetNamesAndTypes

88287ff

updated call to addedgeinfo

e616ec2

added logic to write out pronunciations. Using tagged bit on signs now.

f3db0c8

updated call to addedgeinfo

bbdb6b5

added logic to process pronunciations for ways.

8d8add1

added logic to parse pronunciations. Nodes data saved to existing nod…

56b8fc8

…e file. Way data is saved to new pronunciation file

updated call to addedgeinfo

994a6e4

updated call to addedgeinfo

9e2e8c6

just temp printing out pronunciations

f8d4894

updated call to addedgeinfo

c5c78b8

added new pronunciation file

498f2dc

added new kVerbal type.

575a8c7

tagged bit is now in use.

af3796c

added is_tagged_, has_phoneme_, phoneme_start_index_, and phoneme_cou…

bf4fd4b

…nt_. A sign can have multiple phoneme types.

updated comment

e7d4109

added pronunciations to AddEdgeInfo

618161f

added pronunciation indexes.

22a6ac6

added pronunciation_file

cf6f5e7

init copy

2191bf4

init copy

d357d62

lint

acac388

Merge remote-tracking branch 'origin/master' into phonemes

13cbedc

Added verbal type and value to street name and sign element

2f333af

kevinkreiser reviewed Jun 8, 2021

View reviewed changes

src/baldr/edgeinfo.cc Outdated Show resolved Hide resolved

dgearhart reviewed Oct 6, 2021

View reviewed changes

gknisely mentioned this pull request Oct 7, 2021

release 3.1.4 #3348

Merged

dgearhart reviewed Oct 7, 2021

View reviewed changes

Merge branch 'master' of https://github.com/valhalla/valhalla into ph…

7b92a2d

…onemes

kevinkreiser reviewed Oct 7, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kevinkreiser reviewed Oct 7, 2021

View reviewed changes

src/baldr/edgeinfo.cc Outdated Show resolved Hide resolved

gknisely added 3 commits October 7, 2021 15:07

updated.

d2b06e2

cleanup

f108c60

Merge branch 'phonemes' of https://github.com/valhalla/valhalla into …

2fea8e6

…phonemes

kevinkreiser reviewed Oct 7, 2021

View reviewed changes

kevinkreiser approved these changes Oct 7, 2021

View reviewed changes

gknisely merged commit 3652aca into master Oct 7, 2021

gknisely added the hacktoberfest-accepted label Oct 15, 2021

dgearhart mentioned this pull request Nov 9, 2021

Add support for name:pronunciation tag #2639

Closed

4 tasks

gknisely deleted the phonemes branch February 22, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pronunciation for names and destinations #3132

Pronunciation for names and destinations #3132

gknisely commented Jun 8, 2021 •

edited

Loading

kevinkreiser commented Jun 8, 2021

kevinkreiser commented Jun 8, 2021

dgearhart Oct 6, 2021

dgearhart Oct 6, 2021

dgearhart Oct 6, 2021

kevinkreiser Oct 7, 2021

dgearhart Oct 6, 2021

dgearhart Oct 6, 2021

dgearhart Oct 6, 2021

dgearhart Oct 6, 2021

dgearhart left a comment

kevinkreiser commented Oct 7, 2021

kevinkreiser Oct 7, 2021

gknisely Oct 7, 2021

kevinkreiser Oct 7, 2021

dgearhart Oct 7, 2021

kevinkreiser Oct 7, 2021

kevinkreiser Oct 7, 2021

kevinkreiser Oct 7, 2021

kevinkreiser Oct 7, 2021

kevinkreiser Oct 7, 2021

kevinkreiser Oct 7, 2021

dgearhart Oct 7, 2021

kevinkreiser left a comment

gknisely commented Oct 7, 2021

		StreetNamesFactory::Create(trip_path_->GetCountryCode(node_index), prev_edge->GetNameList()));
		StreetNamesFactory::Create(trip_path_->GetCountryCode(node_index), prev_edge->name()));

Pronunciation for names and destinations #3132

Pronunciation for names and destinations #3132

Conversation

gknisely commented Jun 8, 2021 • edited Loading

Issue

Tasklist

kevinkreiser commented Jun 8, 2021

kevinkreiser commented Jun 8, 2021

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

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

dgearhart left a comment

Choose a reason for hiding this comment

kevinkreiser commented Oct 7, 2021

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser left a comment

Choose a reason for hiding this comment

gknisely commented Oct 7, 2021

gknisely commented Jun 8, 2021 •

edited

Loading