Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collect road restrictions in feature id term on generation step. #4567

Merged

Conversation

bykoianko
Copy link
Contributor

@bykoianko bykoianko commented Oct 27, 2016

PR реализует следующее.
1.
На первом и втором проходе генерации собираются ограничения проездов в osm id и mapping из osm id в feature id. Примерно вот так:

file: restrictions_in_osm_id.csv
No, 157616943, 157616943,
No, 5185062, 395192830,
No, 37914683, 37914683,
Only, 265752848, 265752852,
No, 48601078, 5185021,
No, 48601078, 48601078,

file: feature_id_to_osm_ids.csv
778192, 421826190,
778193, 421829987,
778194, 421829988,
778195, 421829989,
778196, 421829990,
778197, 421829991,
778198, 421829992,
778199, 421829993,

Названия файлов задаются параметрами generator_tool примерно вот так:
--restriction_name="restrictions.csv"
--feature_id_to_osm_ids_name="feature_id_to_osm_ids.csv"

  1. Если у generator_tool задан параметр --generate_routing, то будет создана секция routing и в нее будут положены restrictions. В дальнейшем в нее же будет добавлен и граф дорог в PR-те @dobriy-eeh (ожидается 7-го ноября).

  2. Рестрикшены уже хранятся в компактном виде. В Москве 10K рестрикшенов. Для них потребовалось 37KB. Update: 32KB.

@ygorshenin @mpimenov @syershov @Zverik @dobriy-eeh PTAL

@mapsmetest
Copy link
Contributor

DUDE JTTP

return m_links < restriction.m_links;
}

RestrictionCollector::~RestrictionCollector() {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Он ничего не делает, можно удалить

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Спасибо, удалил.

{
vector<string> const restrictionTypesNo = {"no_right_turn", "no_left_turn", "no_u_turn",
"no_straight_on", "no_entry", "no_exit"};
vector<string> const restrictionTypesOnly = {"only_right_turn", "only_left_turn", "only_straight_on"};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А зачем? Они же в анонимном namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Поправил

@bykoianko bykoianko force-pushed the master-adding-relation-restriction-to-mwm branch from 8cda6d0 to 4427652 Compare October 27, 2016 14:35
@bykoianko
Copy link
Contributor Author

@syershov @Zverik @dobriy-eeh PTAL

@bykoianko bykoianko force-pushed the master-adding-relation-restriction-to-mwm branch from 89eb291 to f0fcaab Compare October 28, 2016 08:03
@@ -134,6 +150,9 @@ class RelationTagsWay : public RelationTagsBase
if (TBase::IsSkipRelation(type) || type == "route")
return;

if (type == "restriction")
m_restrictionCollector.AddRestriction(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут точно не надо добавить return ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Добавил

@@ -289,11 +308,12 @@ class OsmToFeatureTranslator
}
}

void EmitLine(FeatureBuilder1 & ft, FeatureParams params, bool isCoastLine) const
void EmitLine(FeatureBuilder1 & ft, FeatureParams params, bool isCoastLine, osm::Id id) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Может не надо передавать osm::Id, а сразу выставлять его для FeatureBuilder1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вообще удалил AddOsmId. И так добавляется.

protected:
void Process(RelationElement const & e) override
{
string const & type = e.GetType();
if (TBase::IsSkipRelation(type))
return;

if (type == "restriction")
m_restrictionCollector.AddRestriction(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return;

LOG(LINFO, ("Saving intermediate file with restrictions to", fullPath));
ofstream ofs(fullPath, std::ofstream::out);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Добавить проверку на ошибки открытия

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Добавил

}
}

string DebugPrint(RestrictionCollector::Type const & type)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToString

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


class RelationElement;

namespace std
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не хорошо пихать что-то в std

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Оказывается можно без namespace std

@dobriy-eeh
Copy link
Contributor

LGTM

@bykoianko bykoianko force-pushed the master-adding-relation-restriction-to-mwm branch from f0fcaab to 513a9ff Compare October 28, 2016 11:54
@bykoianko
Copy link
Contributor Author

@syershov PTAL

@@ -167,6 +170,10 @@ namespace feature
m_currentNames += country->m_name;

(*(m_Buckets[country->m_index]))(fb);
uint32_t const nextFeatureId = m_Buckets[country->m_index]->GetNextFeatureId();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Для этой строчки и выше можно обобщить

auto & bucket = *(m_Buckets[country->m_index]);
bucket(fb);
uint32_t const nextFeatureId = bucket.GetNextFeatureId();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сделал

if (fid == kInvalidFeatureId)
return false;
}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return find(begin(m_links), end(m_links), kInvalidFeatureId) == end(m_links);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да

m_restrictions.emplace_back(type, links.size());
size_t const restrictionCount = m_restrictions.size() - 1;
for (size_t i = 0; i < links.size(); ++i)
m_restrictionIndex.push_back(make_pair(links[i], Index({restrictionCount, i})));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emplace_back?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тогда будет вызван конструктор make_pair непосредственно. А так можно использовать make_pair.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Поправил:

  for (size_t i = 0; i < osmIds.size(); ++i)
    m_restrictionIndex.emplace_back(osmIds[i], Index({restrictionCount, i}));

if (!restriction.IsValid())
return false;
}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return find_if(begin(m_restrictions), end(m_restrictions, [](Restriction const & r){ return !r.IsValid(); }))) == end(m_restrictions);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

size_t const restrictionSz = m_restrictions.size();
for (pair<osm::Id, Index> const & osmIdAndIndex : m_restrictionIndex)
{
Index const index = osmIdAndIndex.second;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const &

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Restriction & restriction = m_restrictions[index.m_restrictionNumber];
CHECK_LESS(index.m_linkNumber, restriction.m_links.size(), ());

osm::Id const osmId = osmIdAndIndex.first;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const &

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Можно

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Хотя, лучше оставить по значению. По тем тестам, что известны мне - int64 было чуть быстрее, чем ссылка.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сделал по ссылке

if (distance(rangeId.first, rangeId.second) != 1)
continue; // |osmId| mentioned in restrictions was included in more than one feature.

FeatureId const featureId = rangeId.first->second;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const &

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int32 будет быстрее, чем ссылка

@mpimenov
Copy link
Contributor

mpimenov commented Oct 28, 2016

NOT LGTM
Обсудили голосом и пришли к следующему.
Надо делать в два независимых приёма: один строит таблицу соответствия osm_id <-> feature_id (которая пригодится как минимум ещё и для отладки и которая,
как ни странно, не существовала до сих пор);
второй вытягивает информацию по тэгу "restriction" в отдельную структуру,
работая с osm_id (которые и записаны в значениях тэга).
Потом отдельным проходом ограничения восстанавливают feature_id, к
которым они должны применяться, и дописываются в секцию к дорожному графу.
В результате работа с ограничениями в генераторе сводится к обработке
соответствующих тэгов, а на базовый уровень (вплоть до EmitFeature) выносится
вполне себе базовая идея соответствия наших и осмовских номеров.

Используются ограничения на лету на клиенте: мы не можем менять граф в генераторе,
т.к. граф хранится один для всех типов маршрутизации, а ограничения для
них могут быть разные.

{
size_t operator()(osm::Id const & id) const
{
return hash<double>()(id.OsmId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем double?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут должно быть uint64_t

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Введу изменения в RestrictionCollector надобность в этих строчка отпала.

/// their road feature in text file for using later.
class RestrictionCollector
{
friend void UnitTest_RestrictionTest_ValidCase();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем? Почему бы не сделать CheckCorrectness() и прочее public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. RestrictionCollector расчитан на многопоточную среду. Мьютексы расставлены так, что он готово только к вызовам открытых методов;
  2. Тесты иногда обращаются к данным членам этого RestrictionCollector;
  3. CheckCorrectness и другие вызываются из открытых методов и текущее использование кода (кроме тестов) не подразумевает их вызов не зависимо.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А нельзя ли здесь использовать UNIT_CLASS_TEST и сделать ровно один friend?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Для чего здесь нужна многопоточная среда? Почему нельзя в один поток? Можешь объяснить, откуда берутся несколько потоков?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Наверно не стоит. Можно сделать другом класс - первый параметр в UNIT_CLASS_TEST и реализовать в нем методы, которые будут обращаться к закрытым полям/методам RestrictionCollector. Но код будет выглядит не очень. Примерно так:

class RestrictionCollectorTest
{
  RestrictionCollector m_restrictionCollector;

public:
  RestrictionCollectorTest() : m_restrictionCollector("", "") {}
  void AddRestriction(RestrictionCollector::Type type, vector<uint64_t> const & osmIds)
  {
    m_restrictionCollector.AddRestriction(type, osmIds);
  }

  void ComposeRestrictions()
  {
    m_restrictionCollector.ComposeRestrictions();
  }
....
  RestrictionCollector & Inst() { return m_restrictionCollector; }
};

UNIT_CLASS_TEST(RestrictionCollectorTest, RestrictionTest_ValidCase)
{
  // Adding restrictions and feature ids to restrictionCollector in mixed order.
  AddRestriction(RestrictionCollector::Type::No, {1, 2} /* osmIds */);
  Inst().AddFeatureId(30 /* featureId */, {3} /* osmIds */);
  AddRestriction(RestrictionCollector::Type::No, {2, 3} /* osmIds */);
  Inst().AddFeatureId(10 /* featureId */, {1} /* osmIds */);
  Inst().AddFeatureId(50 /* featureId */, {5} /* osmIds */);
  AddRestriction(RestrictionCollector::Type::Only, {5, 7} /* osmIds */);
  Inst().AddFeatureId(70 /* featureId */, {7} /* osmIds */);
  Inst().AddFeatureId(20 /* featureId */, {2} /* osmIds */);
...

Мне кажется, что при этом тест теряет наглядность. Т.е. мы то обращаемся к Inst(), то непосредственно к this через переопределенные методы. Можно, конечно переопределеить все методы (и открытые) в RestrictionCollectorTest. Но не слишком ли это дорогая цена за несколько френдов у RestrictionCollector?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excuse me, I didn't get it, why do we need Inst()? You may create a helper method in RestrictionCollectorTest, named AddFeatureId. Or, you may make m_restrictionCollector protected, and access it directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Действительно,
(1) я могу добавить зактрытые методы RestrictionCollector в RestrictionCollectorTest. Тогда тест сохранит наглядность и мы избавимся растущего набора в friend-ов в RestrictionCollector, оставим там только friend RestrictionCollectorTest. Цена: повторение интефейса RestrictionCollectorTest в RestrictionCollector. Т.е. надо добавить в RestrictionCollectorTest не только AddFeatureId, но и закрытые ComposeRestrictions(), RemoveInvalidRestrictions(), ParseFeatureId2OsmIdsMapping() и т.д.

(2) можно оставить как есть сейчас. Цена: постоянно растущий список friends.

Вариант с protected m_restrictionCollector не подходит, поскольку я должен буду в классе наследнике от RestrictionCollectorTest обращаться к private методам RestrictionCollector. А класс наследник RestrictionCollectorTest не является другом RestrictionCollector.

Думаешь вариант (1) будет лучше?

@bykoianko bykoianko force-pushed the master-adding-relation-restriction-to-mwm branch 2 times, most recently from 7e9a07c to aee1508 Compare October 31, 2016 11:58
@@ -48,6 +51,17 @@ class EmitterBase
virtual void GetNames(vector<string> & names) const = 0;
};

class SyncOfstream
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, move this out to the separate class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SyncOfstream is not a nested class. Do you suggest to move it a separate file? At first let's decide if we need SyncOfstream at all. Have a look please as Polygonizer. There's a method in it:

    void operator () (FeatureBuilder1 & fb, SyncOfstream & featureId2osmIds)
    {
      buffer_vector<borders::CountryPolygons const *, 32> vec;
      m_countries.ForEachInRect(fb.GetLimitRect(), InsertCountriesPtr(vec));

      switch (vec.size())
      {
      case 0:
        break;
      case 1:
        EmitFeature(vec[0], fb, featureId2osmIds);
        break;
      default:
        {
#if PARALLEL_POLYGONIZER
          m_ThreadPoolSemaphore.acquire();
          m_ThreadPool.start(new PolygonizerTask(this, vec, fb, featureId2osmIds));
#else
          PolygonizerTask task(this, vec, fb, featureId2osmIds);
          task.RunBase();
#endif
        }
      }
    }

As far as I see in case 2 PolygonizerTask(s) could be executed in a separate thread. So EmitFeature could be called from different threads. To be on the save side I decided to use mutex (SyncOfstream). Do you think EmitFeature() couldn't be called from different threads?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, seems that PARALLEL_POLYGONIZER is set here. But in any case, you may just collect pass to EmitFeature some kind of holder instead of sync ofstream, i.e. map from feature id to a vector of osm ids. And, at the end, just dump one or two maps to the file. No need in SyncOfstream.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no. It seems that things are more complex. OK, need to think about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syershov Could you please comment it? As far as I remember you're well aware of this part of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, move this out to the separate class.

I moved SyncOfstream to a separate hpp/cpp.

@@ -524,6 +532,30 @@ class MainFeaturesEmitter : public EmitterBase
};
} // anonymous namespace

void SyncOfstream::Open(string const & fullPath)
{
lock_guard<mutex> gard(m_mutex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/gard/guard/g

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -40,6 +40,8 @@ class FeaturesCollector

string const & GetFilePath() const { return m_datFile.GetName(); }

uint32_t GetNextFeatureId() const { return m_featureID; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, get rid of this method. Modify operator() to return feature id instead.

Copy link
Contributor Author

@bykoianko bykoianko Nov 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return;

lock_guard<mutex> gard(m_mutex);
m_stream << featureId << ", ";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that ASCII is good here? Why not to binary-encode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least at development stage I'd prefer to leave it as ASCII text files. They are not very large (up to 10K lines) and it's convenient to have them as text for debugging restrictions. Then we can change serializing/deserializing to binary encoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides that, it's easier to write tests if we use ASCII.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The third advantage of keeping the mapping in text files is the mapping from feature id to osm ids could be useful for other parts of our system. (@mgsergio has mentioned that it could be useful for him.)

lock_guard<mutex> gard(m_mutex);
m_stream << featureId << ", ";
for (osm::Id const & osmId : osmIds)
m_stream << osmId.OsmId() << ", ";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea to write "," at the end of the line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it to make code easier. Anyway deserializer is ready if there's no comma at the end of line (ParseLineOfNumbers). Do you think it'll be better to add some lines of code and stop writing commas at the end?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what? A list of items separated by commas is more natural than list of items with trailing comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -524,6 +532,30 @@ class MainFeaturesEmitter : public EmitterBase
};
} // anonymous namespace

void SyncOfstream::Open(string const & fullPath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain why do we need this class at all?

Copy link
Contributor Author

@bykoianko bykoianko Nov 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my detailed explanation and questions above, near your comment "Please, move this out to the separate class."


namespace
{
string const kNoStr = "No";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/kNoStr/kNo/g
s/kOnlyStr/kOnly/g

BTW, char[] is almost the same but cheaper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Let's do it as char[].

{
Restriction(Type type, size_t linkNumber);
// Constructor for testing.
Restriction(Type type, vector<FeatureId> const & links);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's for testing, create a static method like "MakeForTesting".

Copy link
Contributor Author

@bykoianko bykoianko Nov 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the situation is changed. The construction is used for initializing Restriction after Deserializtion. The comment was removed.

/// \brief Addresses a link in vector<Restriction>.
struct Index
{
size_t m_restrictionNumber; // Restriction number in restriction vector.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default initialization, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Constructor with two params is enough.

    Index(size_t restrictionNumber, size_t linkNumber)
      : m_restrictionNumber(restrictionNumber), m_linkNumber(linkNumber) {}

@bykoianko bykoianko force-pushed the master-adding-relation-restriction-to-mwm branch 2 times, most recently from e7d8b29 to d825090 Compare November 3, 2016 10:12
@bykoianko
Copy link
Contributor Author

Обсудили голосом и пришли к следующему. Надо делать в два независимых приёма

Сделал, как договаривались. См. детальный ответ в описании PR.

@bykoianko
Copy link
Contributor Author

@ygorshenin @mpimenov @syershov @Zverik @dobriy-eeh PTAL
DUDE JTTP

@bykoianko
Copy link
Contributor Author

DUDE JTTP

@bykoianko bykoianko force-pushed the master-adding-relation-restriction-to-mwm branch from 2b3e568 to 8931c6a Compare November 7, 2016 08:25
@bykoianko
Copy link
Contributor Author

bykoianko commented Nov 10, 2016

Отребейзил на свежий мастер.
DUDE JTTP
@ygorshenin @mpimenov @syershov PTAL

string const writableDir = platform.WritableDir();

// Building empty mwm.
LocalCountryFile country(my::JoinFoldersToPath(writableDir, kTestDir), CountryFile(kTestMwm), 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have LocalCountryFile::MakeForTesting() for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work in this case because |kTestMwm| are placed in writableDir/kTestDir but not in writableDir.

if (!ParseFeatureId2OsmIdsMapping(featureIdToOsmIdsPath)
|| !ParseRestrictions(restrictionPath))
{
LOG(LWARNING, ("An error happened while parsing", featureIdToOsmIdsPath, "or", restrictionPath));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is not very informative. I suggest you to replace it by more specific error messages in corresponding Parse...() functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return false;

if (osmIds.size() < 2)
return false; // Every line should contain feature id and at least one osm id.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above - an error message would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, done


try
{
m_reader = make_unique<FilesContainerR::TReader>(mwmValue.m_cont.GetReader(ROUTING_FILE_TAG));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You use only mwmValue.m_cont. So, I suggest you to get rid of mwmValue and pass FileContainerR instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use mwmValue directly in initialization list:

RestrictionLoader::RestrictionLoader(MwmValue const & mwmValue)
  : m_countryFileName(mwmValue.GetCountryFileName())

CHECK(restriction.IsValid(), ());
CHECK_LESS(1, restriction.m_featureIds.size(), ("No sense in zero or one link restrictions."));

BitWriter<FileWriter> bits(sink);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, why do you need to re-create bits on each iteration? Why not to create it once, before the loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Thanks. The same the BitReader bits(src); Fixed!

@bykoianko
Copy link
Contributor Author

DUDE JTTP
@ygorshenin @mpimenov @syershov PTAL

RestrictionCollector::RestrictionCollector(string const & restrictionPath,
string const & featureIdToOsmIdsPath)
{
auto const restrictionCleaner = [this](){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/restrictionCleaner/clean/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


bool RestrictionCollector::ParseFeatureId2OsmIdsMapping(string const & featureIdToOsmIdsPath)
{
ifstream featureId2OsmIdsStream(featureIdToOsmIdsPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/2/To/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ygorshenin
Copy link
Contributor

LGTM

if (!ParseLineOfNumbers(iter, osmIds))
{
LOG(LWARNING, ("Cannot parse feature id to osm ids mapping from file:",
featureIdToOsmIdsPath, ". line:", line));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Обычно нотация file+line означает, что будет написано имя файла и номер строки, а не содержимое.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, я понимаю. Но у мне не так просто получить номер строки здесь. Теперь имя файла переехало выше (в вызывающий метод). Так что теперь это выглядит иначе.

void RestrictionCollector::AddFeatureId(uint32_t featureId, vector<uint64_t> const & osmIds)
{
// Note. One |featureId| could correspond to several osm ids.
// but for road feature |featureId| corresponds exactly one osm id.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/but/But/
s/corresponds/corresponds to/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, done

@@ -0,0 +1,76 @@
#pragma once

#include "routing/routing_serializer.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the file name if it is not too hard. Compare with existing:
base/std_serialization.hpp
indexer/geometry_serialization.hpp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename the two files for uniformity

// Extracting type of restriction.
auto const tagIt = relationElement.tags.find("restriction");
if (tagIt == relationElement.tags.end())
return; // Type of the element is different from "restriction".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лиший комментарий.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да. Удалил.


Restriction::Type type = Restriction::Type::No;
if (!TagToType(tagIt->second, type))
return; // Unsupported restriction type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лишний комментарий.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это полезный, думаю. В osm добавляется большое кол-во вариантов мусорных типов restrictions. Если мы в этой строчке, то значит обрабатываем restriction не документированного типа.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Удалил

return; // Unsupported restriction type.

// Adding restriction.
m_stream << ToString(type) << "," << fromIt->first << ", " << toIt->first << endl;
Copy link
Contributor

@mpimenov mpimenov Nov 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using '\n': recall that endl flushes the stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Let's save some times while saving.

@bykoianko bykoianko force-pushed the master-adding-relation-restriction-to-mwm branch from 3b56e90 to 6615363 Compare November 11, 2016 13:16
@bykoianko
Copy link
Contributor Author

DUDE JTTP
@mpimenov @syershov PTAL

uint32_t const biasedFirstFeatureId = coding::DeltaCoder::Decode(bits);
if (biasedFirstFeatureId == 0)
{
LOG(LERROR, ("Decoded first link restriction feature id delta is zero.."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лишняя точка.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Готово

@bykoianko bykoianko force-pushed the master-adding-relation-restriction-to-mwm branch from 6615363 to 527000b Compare November 11, 2016 14:28
@mpimenov
Copy link
Contributor

LGTM

@bykoianko
Copy link
Contributor Author

DUDE JTTP

@syershov
Copy link
Contributor

LGTM

@bykoianko
Copy link
Contributor Author

Всем спасибо за ревью!

@dobriy-eeh dobriy-eeh merged commit ac83492 into mapsme:master Nov 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants