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

[routing][generator]Save road points access info #7882

Merged
merged 2 commits into from Jan 19, 2018

Conversation

Projects
None yet
4 participants
@tatiana-yan
Copy link
Collaborator

commented Dec 25, 2017

Добавила в генератор генерацию RoadAccess для точечных фичей.

При генерации пользуюсь тем, что в o5m nodes идут раньше чем ways -- предсохраняем информацию о nodes с заполненными тегами доступа, при сохранении информации для ways используем запомненные nodes, сохраняем их вместе, чтобы потом одинаково конвертить osm id в faeture id.

В RoadAccess вместо того чтобы хранить информацию о доступности сегментов храню информацию о доступности фичей и RoadPoints (т.к. именно RoadPoints имеют ограниченный доступ в случае точечных фичей, сегменты по обе стороны от запрещенной RoadPoint можно использовать), такой подход позволит не лепить в дальнейшем костылей для односегментных маршрутов и т.п.
Пока что в IndexGraph не используется информация о доступности RoadPoints, сделаю отдельно.
Для совместимости с существующими mwm номер точки для точечных фичей хранится увеличенным на 1, т.к. номер 0 занят для идентификатора сегмента представляющего полную фичу (другй вариант -- не увеличивать номер, а различать по forward).

@tatiana-yan tatiana-yan requested review from bykoianko and mpimenov Dec 25, 2017

@tatiana-yan

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 25, 2017

JTTP

{OsmElement::Tag("barrier", "log"), RoadAccess::Type::No},
{OsmElement::Tag("barrier", "motorcycle_barrier"), RoadAccess::Type::No},
{OsmElement::Tag("barrier", "swing_gate"), RoadAccess::Type::Private},
};

This comment has been minimized.

Copy link
@bykoianko

bykoianko Dec 26, 2017

Contributor

@Zverik что думаешь о списке барьеров? Может дополнишь?

This comment has been minimized.

Copy link
@Zverik

Zverik Dec 26, 2017

Contributor

А у нас как-то различаются No и Private?
Барьеры, вроде, все нормальные. Но у нас планируется поддержка платных дорог? tool_booth к этому относится: проезжаешь — плати.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

Сейчас у нас private работает как destination: очень много шлагбаумов во дворах потеганы private, мы хотим разрешать выехать из такого шлагбаума и заехать в него, но если возможно избежать шлагбаума -- будем избегать.
access = no не разрешён для проезда вообще.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

Конечно же если у точечной фичи есть явно заданный тег access, или теги vehicle_type = ..., то будут использованы они. Теги из этого map будут использованы только если явно другие теги доступа не заданы:
например, видим bariier = block без каких-либо ещё тегов -- предполагаем что машинам нельзя, а пешеходам и велосипедистам можно
если lift_gate без тегов предполагаем что для машин private, а остальным можно
и т.п.

uint32_t pointIdx;
if (!iter || !strings::to_uint(*iter, pointIdx))
{
LOG(LERROR, ("Error when parsing road access: bad pointIdx at line", lineNo));

This comment has been minimized.

Copy link
@bykoianko

bykoianko Dec 26, 2017

Contributor

Может здесь и выше выводить в случае ошибки содержимое линии, которую не смогли распарсить? |line| Или хотя бы |*iter|.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

done

}

for (size_t i = 0; i < static_cast<size_t>(VehicleType::Count); ++i)
roadAccessByVehicleType[i].SetSegmentTypes(move(segmentType[i]));
roadAccessByVehicleType[i].SetAccessTypes(move(featureType[i]), move(pointType[i]));

This comment has been minimized.

Copy link
@bykoianko

bykoianko Dec 26, 2017

Contributor

#include <utility>?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

done

{
// todo(@m) Add support for non-way elements, such as barrier=gate.
// We will proccess all nodes before ways because of o5m format:
// all nodes are first, than all ways, than all relations.

This comment has been minimized.

Copy link
@bykoianko

bykoianko Dec 26, 2017

Contributor

You rely on this fact. Is it a part of o5m format?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

Yes:

Format description
The structure of the new .o5m format is similar to the structure of conventional .osm
format: Objects are stored using a strict sequence. First all nodes, then all ways, and
finally all relations. Within each of these three groups, the objects are sequenced by their
ids, in ascending order.

http://wiki.openstreetmap.org/wiki/O5m

This comment has been minimized.

Copy link
@bykoianko

bykoianko Dec 26, 2017

Contributor

Ok

template <typename MF>
void SetFeatureTypesForTests(MF && mf)
{
m_featureTypes = std::forward<MF>(mf);

This comment has been minimized.

Copy link
@bykoianko

bykoianko Dec 26, 2017

Contributor

#include <utility>

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

done


roadAccess.SetSegmentTypes(std::move(m));
roadAccess.SetAccessTypes(std::move(mf), std::move(mp));

This comment has been minimized.

Copy link
@bykoianko

bykoianko Dec 26, 2017

Contributor

#include <utility>?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

done

segmentsByRoadAccessType[static_cast<size_t>(kv.second)].push_back(kv.first);
for (auto const & kv : mf)
segmentsByRoadAccessType[static_cast<size_t>(kv.second)].push_back(
Segment(kFakeNumMwmId, kv.first, 0, true));

This comment has been minimized.

Copy link
@bykoianko

bykoianko Dec 26, 2017

Contributor

{}?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

done

// segmentIdx.
for (auto const & kv : mp)
segmentsByRoadAccessType[static_cast<size_t>(kv.second)].push_back(
Segment(kFakeNumMwmId, kv.first.GetFeatureId(), kv.first.GetPointId() + 1, true));

This comment has been minimized.

Copy link
@bykoianko

bykoianko Dec 26, 2017

Contributor

{}?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

done

@tatiana-yan tatiana-yan force-pushed the tatiana-yan:nodes_access branch from df672f5 to 909ec3f Dec 26, 2017

@tatiana-yan

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 26, 2017

@tatiana-yan tatiana-yan force-pushed the tatiana-yan:nodes_access branch from 909ec3f to 4acd844 Dec 26, 2017

@@ -67,6 +81,10 @@ TagMapping const kBicycleTagMapping = {
{OsmElement::Tag("bicycle", "destination"), RoadAccess::Type::Destination},
};

TagMapping const kBicycleBarriersTagMapping = {
{OsmElement::Tag("barrier", "cycle-barrier"), RoadAccess::Type::No},

This comment has been minimized.

Copy link
@Zverik

Zverik Dec 26, 2017

Contributor

Также стоит добавить gate, kissing_gate и turnstile. Надеюсь, у нас учитываются модификаторы bicycle=yes и bicycle=no?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

да, у нас приоритетно учитываются сначала теги для конкретного вида транспорта, потом теги access, потом уже то что мы предположили для данного типа барьеров

сейчас gate, kissing_gate, turnstile будут учтены, если они потеганы bicycle = yes/no/private/destination, vehicle = yes/no/private/destination или access = yes/no/private/destination

но можно сюда что-то добавить, чтобы мы их учитывали в прокладке велосипедного маршрута даже если у них нет тегов bicycle, vehicle, access.

как ты думаешь, что для какого типа нужно добавить?

This comment has been minimized.

Copy link
@Zverik

Zverik Dec 26, 2017

Contributor

Я написал типы, которые No по умолчанию. Остальное велосипедистов не трогает. Хорошо, что bicycle=yes сработает.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

barrier=gate часто бывают воротами, которые открыты, например:
https://www.openstreetmap.org/node/1562455692

я бы не стала безусловно все barrier=gate без тегов access и bicycle запрещать велосипедистам

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

turnstile тоже добавила, а kissing_gate больше от животных -- чтобы через него проехать надо слезть с велика, но пройти можно -- не потятно надо ли безусловно запрещать
в исландии например они есть там, где вполне можно на велике передвигаться (чтобы ограничивать передвижение барашков)

This comment has been minimized.

Copy link
@Zverik

Zverik Dec 26, 2017

Contributor

Насчёт gate — как раз наоборот, почти всегда они закрыты, но иногда картографы забывают проставлять поясняющие теги. Их нужно безусловно запретить, чтобы искать подобные ошибки. OSRM не роутит по барьерам без дополнительных атрибутов.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

для gate давайте великам хотя бы private по умолчанию поставим, чтобы дать из парка с не потеганным gate выехать (их реально много не потеганных, но при этом разрешенных для великов, посмотрела центр москвы и европейские города которые знаю)
@Zverik @bykoianko что думаете об этом?

This comment has been minimized.

Copy link
@bykoianko

bykoianko Dec 26, 2017

Contributor

Я думаю, что если старт или финиш окружен не потеганными (а может и потеганными) гейтами, то лучше проложить маршрут. Т.е. если вариантов нет, то наверно лучше, хоть какой-то вариант, чем не какого. Люди часто хотят проехать на закрытие территории, к которым имеют доступ. По хорошему, конечно об этом стоит предупреждать, что на маршруте, есть участок, который закрыт для всех или который платный.

This comment has been minimized.

Copy link
@Zverik

Zverik Dec 26, 2017

Contributor

Насчёт kissing_gate: я смотрю на вывод google images по этому слову, и через некоторые из корот с байком перебраться проблематично. Разве что перекидывать байк через стену.

Про гейты и Private — хорошо. В принципе, для велосипедов можно всё Private сделать.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Dec 26, 2017

Author Collaborator

давай сделаем gate и kissing_gate private (через них поедет если иначе нельзя), а turnstile и cycle-barrier no, потому что они как раз для того чтобы только пешеходы могли ходить

а дальше если что адаптируемся под требования пользователей

ну и конечно приоритетно будут учитываться теги bicycle и access

@tatiana-yan tatiana-yan force-pushed the tatiana-yan:nodes_access branch from 4acd844 to 90bcbb0 Dec 26, 2017

@tatiana-yan tatiana-yan force-pushed the tatiana-yan:nodes_access branch from 90bcbb0 to 35a4838 Jan 12, 2018

@tatiana-yan

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 12, 2018

JTTP

@tatiana-yan

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 12, 2018

Добавила в этот же PR отдельным коммитом учёт точечных ограничений в IndexGraph при поиске входящих/исходящих + тест к нему. Это всё что нужно было сделать по учёту access для барьеров.

@Zverik

Zverik approved these changes Jan 12, 2018

@@ -21,6 +21,13 @@ class RoadPoint final

uint32_t GetPointId() const { return m_pointId; }

bool operator<(RoadPoint const & rp) const

This comment has been minimized.

Copy link
@bykoianko

bykoianko Jan 15, 2018

Contributor

What do you think about moving operator< and operator== to .cpp?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 17, 2018

Author Collaborator

сейчас нет road_point.cpp и не хочется ради этого его заводить -- класс очень компактный (30 строк), один файл читать удобнее

This comment has been minimized.

Copy link
@bykoianko

bykoianko Jan 18, 2018

Contributor

Ок

}
featureTypes[request.m_id] = request.m_accessType;

// All features have 1 segment. |from| has point index 0, |to| has point index 1.

This comment has been minimized.

Copy link
@bykoianko

bykoianko Jan 15, 2018

Contributor

Is this comment is relevant to the current code?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 15, 2018

Author Collaborator

да, это то как TestIndexGraphTopology, который задан набором рёбер, превращается в IndexGraph -- каждое ребро превращается в односегментную фичу, id которой равен порядковому номеру ребра, mwmId -- тестовый mwmId, а сегмент в фиче 1 с pointIndex 0 и 1.

This comment has been minimized.

Copy link
@bykoianko

bykoianko Jan 18, 2018

Contributor

Ок

{OsmElement::Tag("barrier", "block"), RoadAccess::Type::No},
{OsmElement::Tag("barrier", "bollard"), RoadAccess::Type::No},
{OsmElement::Tag("barrier", "chain"), RoadAccess::Type::No},
{OsmElement::Tag("barrier", "cycle-barrier"), RoadAccess::Type::No},

This comment has been minimized.

Copy link
@bykoianko

bykoianko Jan 17, 2018

Contributor

cycle_barrier?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 17, 2018

Author Collaborator

спасибо! fixed

@@ -67,6 +81,13 @@ TagMapping const kBicycleTagMapping = {
{OsmElement::Tag("bicycle", "destination"), RoadAccess::Type::Destination},
};

TagMapping const kBicycleBarriersTagMapping = {
{OsmElement::Tag("barrier", "cycle-barrier"), RoadAccess::Type::No},

This comment has been minimized.

Copy link
@bykoianko

bykoianko Jan 17, 2018

Contributor

cycle_barrier?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 17, 2018

Author Collaborator

fixed

@tatiana-yan tatiana-yan force-pushed the tatiana-yan:nodes_access branch from f63fc7a to c698bf3 Jan 17, 2018

@tatiana-yan

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 17, 2018

@bykoianko
Copy link
Contributor

left a comment

LGTM

@tatiana-yan

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 18, 2018

JTTP

{
// todo(@m) Add support for non-way elements, such as barrier=gate.
// We will proccess all nodes before ways because of o5m format:
// all nodes are first, than all ways, than all relations.

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

s/than/then/g

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

fixed

}
else
{
// We store |pointIdx + 1| instead of |pointIdx| for backward compatability because

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

s/compatability/compatibility/
Но я не очень понимаю этот комментарий: изначально же так и задумывалось (разве что были индексы сегментов, а не точек), о какой обратной совместимости речь?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

переписала коммент

segmentsByRoadAccessType[static_cast<size_t>(kv.second)].push_back(
Segment(kFakeNumMwmId, kv.first, 0, true));
}
// For backward campatability we store pointId + 1 because 0 is reserves for wildcars

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

s/campatability/compatibility/

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

переписала коммент

segmentsByRoadAccessType[static_cast<size_t>(kv.second)].push_back(
Segment(kFakeNumMwmId, kv.first, 0, true));
}
// For backward campatability we store pointId + 1 because 0 is reserves for wildcars

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

s/reserves/reserved/

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

fixed

} // namespace

namespace routing
{
// RoadAccess --------------------------------------------------------------------------------------
RoadAccess::Type RoadAccess::GetSegmentType(Segment const & segment) const
{
// todo(@m) This may or may not be too slow. Consider profiling this and using

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

Почему? По-моему, комментарий по-прежнему валиден.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

вернула


oss << "]";
oss << "RoadAccess { FeatureTypes [";
PrintKV(r.GetFeatureTypes(), oss, kMaxIdsToShow);

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

Я бы передал oss первым параметром в этом случае.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

ok, done

}
if (kvs.size() > maxKVToShow)
oss << ", ...";
}
} // namespace

namespace routing
{
// RoadAccess --------------------------------------------------------------------------------------
RoadAccess::Type RoadAccess::GetSegmentType(Segment const & segment) const

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

Думаю, если мы отказываемся от идеи блокировать отдельные сегменты, надо менять и интерфейс, принимая сразу FeatureId.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

оставила сначала GetSegmentType, потому что иначе все места вызова выглядят
GetFeatureType(segment.GetFeatureId()), типа убрала GetFeatureId внутрь, но всё же GetFeatureType действительно лучше, переделала

for (auto const & kv : mf)
{
segmentsByRoadAccessType[static_cast<size_t>(kv.second)].push_back(
Segment(kFakeNumMwmId, kv.first, 0, true));

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

inline comments for 0 and true

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

done

@tatiana-yan tatiana-yan force-pushed the tatiana-yan:nodes_access branch from c698bf3 to 0b4243b Jan 18, 2018

@tatiana-yan

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 18, 2018

@mpimenov PTAL

@@ -138,7 +171,15 @@ bool ParseRoadAccess(string const & roadAccessPath, map<osm::Id, uint32_t> const
uint64_t osmId;
if (!iter || !strings::to_uint64(*iter, osmId))
{
LOG(LERROR, ("Error when parsing road access: bad osm id at line", lineNo));
LOG(LERROR, ("Error when parsing road access: bad osm id at line. Line:", line, "Line number:", lineNo));

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

LOG(LERROR, ("Error when parsing road access: bad osm id at line", lineNo, "Line contents:", line));
Чтобы слово "line" встречалось пореже.
Это в странном предположении, что это действительно нужно и номера строки не хватает

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

done

VehicleType m_vehicleType;
// Order of tag mappings in m_tagMappings is from more to less specific.
// e.g. for car: motorcar, motorvehicle, vehicle, general access tags.
std::vector<TagMapping const *> m_tagMappings;
std::map<uint64_t, RoadAccess::Type> m_barriers;

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

Комментарий к полю, пожалуйста.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

done

int32_t const accessPenalty = fromAccessAllowed == toAccessAllowed ? 0 : 1;
int32_t accessPenalty = fromAccessAllowed == toAccessAllowed ? 0 : 1;

// RoadPoint between u and v is front of u.

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

Разве не "back of u" или "front of v"?

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

вроде нет, у нас тут u это from, v это to, точка между ними это front сегмента from, то есть front of u

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

Да, точно.

} // namespace

namespace routing
{
// RoadAccess --------------------------------------------------------------------------------------
RoadAccess::Type RoadAccess::GetSegmentType(Segment const & segment) const
RoadAccess::Type RoadAccess::GetFeatureType(uint32_t featureId) const
// todo(@m) This may or may not be too slow. Consider profiling this and using

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

Комментарий ближе к комментируемому месту (после скобки), пожалуйста

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

done

@tatiana-yan tatiana-yan force-pushed the tatiana-yan:nodes_access branch from 0b4243b to 6f2cd2d Jan 18, 2018

for (size_t i = 0; i < static_cast<size_t>(RoadAccess::Type::Count); ++i)
{
std::vector<Segment> segs;
DeserializeSegments(src, segs);
for (auto const & seg : segs)
m[seg] = static_cast<RoadAccess::Type>(i);
{
if (seg.GetSegmentIdx() == 0)

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

Где-то здесь стоит дописать что-то типа

// An earlier version allowed blocking any segment of a feature (or the entire feature
// by providing a wildcard segment index).
// At present, we either block the feature entirely or block any of its road points. The
// the serialization code remains the same, although its semantics changes as we now
// work with point indices instead of segment indices.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 18, 2018

Author Collaborator

у нас не было ни одной mwm-ки сгенерировано с не wildcard сегментами, поэтому мне кажется, что писать коммент о том что был когда-то такой план -- только запутывать

This comment has been minimized.

Copy link
@mpimenov

mpimenov Jan 18, 2018

Contributor

Я имею в виду, что код был для этого предназначен. В частности, именование переменных. Сейчас в десериализаторе segment то, segment это, а на выходе две мапки без сегментов.

This comment has been minimized.

Copy link
@tatiana-yan

tatiana-yan Jan 19, 2018

Author Collaborator

ok, done

@tatiana-yan tatiana-yan force-pushed the tatiana-yan:nodes_access branch from 6f2cd2d to f906110 Jan 19, 2018

@tatiana-yan

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 19, 2018

@mpimenov PTAL

@tatiana-yan

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 19, 2018

JTTP

@mpimenov
Copy link
Contributor

left a comment

LGTM

@mpimenov mpimenov merged commit f998668 into mapsme:master Jan 19, 2018

1 check was pending

GitHubWatchCat WatchCat checking your PR now.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.