[Routing] Fixes: #2422 + GetTurnDirection() refactoring #2427

2022-04-20T13:22:30Z

AntonM030481 commented

2022-04-20 13:22:30 +00:00

(Migrated from github.com)

Currently GetTurnDirection() is overcomplicated.
This is an attempt to make it more straightforward.
Also some bugs were found and fixed.
Full list nodes.candidates was used in many cases
but uturn had to be excluded.
The issue #2422 was expected to be handled even twice:

Removing a slight turn if ingoing and outgoing edges
are not links and all other possible ways out are links.
Usage of RightmostDirection() if
the route goes along the rightmost candidate.

But both failed because of mentioned nodes.candidates problem.

Currently GetTurnDirection() is overcomplicated. This is an attempt to make it more straightforward. Also some bugs were found and fixed. Full list nodes.candidates was used in many cases but uturn had to be excluded. The issue #2422 was expected to be handled even twice: - Removing a slight turn if ingoing and outgoing edges are not links and all other possible ways out are links. - Usage of RightmostDirection() if the route goes along the rightmost candidate. But both failed because of mentioned nodes.candidates problem.

biodranik (Migrated from github.com) reviewed 2022-04-20 13:39:13 +00:00

biodranik (Migrated from github.com) left a comment

Хотелось бы ещё больше комментариев там, где их нет )

routing/turns_generator.cpp

					
				@ -197,19 +197,26 @@ bool DiscardTurnByHighwayClass(TurnCandidates const & possibleTurns, TurnInfo co

				}

				/// * \returns false when

				/// * for routes which go straight and don't go to a smaller road:

biodranik (Migrated from github.com) commented

2022-04-20 13:27:25 +00:00

bool DiscardTurnByNoAlignedAlternatives(std::vector<TurnCandidate> const & candidatesWithoutUturn, TurnInfo const & turnInfo,

Надо соблюдать наше текущее соглашение о форматировании.

```suggestion bool DiscardTurnByNoAlignedAlternatives(std::vector<TurnCandidate> const & candidatesWithoutUturn, TurnInfo const & turnInfo, ``` Надо соблюдать наше текущее соглашение о форматировании.

routing/turns_generator.cpp

					
				@ -208,3 +213,4 @@

				                                        TurnInfo const & turnInfo,

				                                        NumMwmIds const & numMwmIds)

				{

				  double constexpr kMaxAbsAngleSameRoadClass = 70.0;

biodranik (Migrated from github.com) commented

2022-04-20 13:28:04 +00:00

Это можно в одну строку вместо двух, и точку в конце.

routing/turns_generator.cpp

					
				@ -1155,0 +1143,4 @@

				  if (turnInfo.m_ingoing.m_onRoundabout || turnInfo.m_outgoing.m_onRoundabout)

				  {

				    bool const keepTurnByHighwayClass = KeepRoundaboutTurnByHighwayClass(nodes, turnInfo, numMwmIds);

				    turn.m_turn = GetRoundaboutDirection(turnInfo.m_ingoing.m_onRoundabout,

biodranik (Migrated from github.com) commented

2022-04-20 13:29:18 +00:00

  // Check for enter or exit to/from roundabout.

```suggestion // Check for enter or exit to/from roundabout. ```

biodranik (Migrated from github.com) commented

2022-04-20 13:30:18 +00:00

Тогда уже и hasMultiTurns на отдельную строку.

biodranik (Migrated from github.com) commented

2022-04-20 13:30:45 +00:00

Не надо тут переносить на новую строку.

routing/turns_generator.cpp

					
				@ -1205,3 +1168,3 @@

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn) && nodes.candidates.size() != 1)

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn))

				  {

				    if (DiscardTurnByHighwayClass(nodes, turnInfo, numMwmIds, turn.m_turn) ||

biodranik (Migrated from github.com) commented

2022-04-20 13:31:13 +00:00

  auto candidatesWithoutUturn = nodes.candidates;

```suggestion auto candidatesWithoutUturn = nodes.candidates; ```

biodranik (Migrated from github.com) commented

2022-04-20 13:32:31 +00:00

Почему это условие должно выполняться? Надо комментарий в пустых скобках.

biodranik (Migrated from github.com) commented

2022-04-20 13:34:29 +00:00

А по сути — зачем здесь создавать копию nodes.candidates? Если нужно модифицировать, то нужна ссылка:
auto & candidates_without_uturn = nodes.candidates;

А по сути — зачем здесь создавать копию nodes.candidates? Если нужно модифицировать, то нужна ссылка: `auto & candidates_without_uturn = nodes.candidates;`

biodranik (Migrated from github.com) commented

2022-04-20 13:38:52 +00:00

Если часто нужно удалять первый элемент вектора, то имеет смысл использовать не вектор, а другую структуру данных, list или deque. @vng что думаешь?

routing/turns_generator.cpp

					
				@ -1243,1 +1204,3 @@

				      turn.m_turn = CarDirection::None;

				    // If the route goes along the rightmost or the leftmost way among available ones:

				    // 1. RightmostDirection or LeftmostDirection is selected

				    // 2. If the turn direction is |CarDirection::GoStraight|

biodranik (Migrated from github.com) commented

2022-04-20 13:36:55 +00:00

      // Compare with the closest right candidate.

```suggestion // Compare with the closest right candidate. ```

AntonM030481 (Migrated from github.com) reviewed 2022-04-20 15:05:51 +00:00

routing/turns_generator.cpp

					
				@ -197,19 +197,26 @@ bool DiscardTurnByHighwayClass(TurnCandidates const & possibleTurns, TurnInfo co

				}

				/// * \returns false when

				/// * for routes which go straight and don't go to a smaller road:

AntonM030481 (Migrated from github.com) commented

2022-04-20 15:05:51 +00:00

ок

AntonM030481 (Migrated from github.com) reviewed 2022-04-20 15:06:16 +00:00

routing/turns_generator.cpp

					
				@ -208,3 +213,4 @@

				                                        TurnInfo const & turnInfo,

				                                        NumMwmIds const & numMwmIds)

				{

				  double constexpr kMaxAbsAngleSameRoadClass = 70.0;

AntonM030481 (Migrated from github.com) commented

2022-04-20 15:06:16 +00:00

ок

AntonM030481 (Migrated from github.com) reviewed 2022-04-20 15:06:34 +00:00

routing/turns_generator.cpp

					
				@ -1155,0 +1143,4 @@

				  if (turnInfo.m_ingoing.m_onRoundabout || turnInfo.m_outgoing.m_onRoundabout)

				  {

				    bool const keepTurnByHighwayClass = KeepRoundaboutTurnByHighwayClass(nodes, turnInfo, numMwmIds);

				    turn.m_turn = GetRoundaboutDirection(turnInfo.m_ingoing.m_onRoundabout,

AntonM030481 (Migrated from github.com) commented

2022-04-20 15:06:34 +00:00

ок

AntonM030481 (Migrated from github.com) reviewed 2022-04-20 15:06:57 +00:00

routing/turns_generator.cpp

					
				@ -1155,0 +1143,4 @@

				  if (turnInfo.m_ingoing.m_onRoundabout || turnInfo.m_outgoing.m_onRoundabout)

				  {

				    bool const keepTurnByHighwayClass = KeepRoundaboutTurnByHighwayClass(nodes, turnInfo, numMwmIds);

				    turn.m_turn = GetRoundaboutDirection(turnInfo.m_ingoing.m_onRoundabout,

AntonM030481 (Migrated from github.com) commented

2022-04-20 15:06:56 +00:00

ок

AntonM030481 (Migrated from github.com) reviewed 2022-04-20 15:07:43 +00:00

routing/turns_generator.cpp

					
				@ -1155,0 +1143,4 @@

				  if (turnInfo.m_ingoing.m_onRoundabout || turnInfo.m_outgoing.m_onRoundabout)

				  {

				    bool const keepTurnByHighwayClass = KeepRoundaboutTurnByHighwayClass(nodes, turnInfo, numMwmIds);

				    turn.m_turn = GetRoundaboutDirection(turnInfo.m_ingoing.m_onRoundabout,

AntonM030481 (Migrated from github.com) commented

2022-04-20 15:07:43 +00:00

ок

AntonM030481 (Migrated from github.com) reviewed 2022-04-20 15:40:04 +00:00

routing/turns_generator.cpp

					
				@ -1205,3 +1168,3 @@

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn) && nodes.candidates.size() != 1)

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn))

				  {

				    if (DiscardTurnByHighwayClass(nodes, turnInfo, numMwmIds, turn.m_turn) ||

AntonM030481 (Migrated from github.com) commented

2022-04-20 15:40:03 +00:00

это хороший вопрос.
выглядит так, что этот uturn нигде не нужен в GetTurnDirection.
из того, где все еще используется nodes:

KeepRoundaboutTurnByHighwayClass - не нужен (и не мешает)
IsExit - не нужен (и не мешает)
DiscardTurnByIngoingAndOutgoingEdges - очень запутанный метод. Предположительно, там не нужен и мешает!
проверка на nodes.candidates.size() != 1 - предположительно не нужен. Зависит от DiscardTurnByIngoingAndOutgoingEdges
DiscardTurnByHighwayClass - не нужен. Обратить внимание: вызывается также из GetNextCrossSegmentRoutePoint.

Предлагаю вместе разобраться с DiscardTurnByIngoingAndOutgoingEdges и модифицировать оригинал.

это хороший вопрос. выглядит так, что этот uturn нигде не нужен в GetTurnDirection. из того, где все еще используется nodes: - KeepRoundaboutTurnByHighwayClass - не нужен (и не мешает) - IsExit - не нужен (и не мешает) - DiscardTurnByIngoingAndOutgoingEdges - очень запутанный метод. Предположительно, там не нужен и мешает! - проверка на nodes.candidates.size() != 1 - предположительно не нужен. Зависит от DiscardTurnByIngoingAndOutgoingEdges - DiscardTurnByHighwayClass - не нужен. Обратить внимание: вызывается также из GetNextCrossSegmentRoutePoint. Предлагаю вместе разобраться с DiscardTurnByIngoingAndOutgoingEdges и модифицировать оригинал.

AntonM030481 (Migrated from github.com) reviewed 2022-04-20 15:40:21 +00:00

routing/turns_generator.cpp

					
				@ -1205,3 +1168,3 @@

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn) && nodes.candidates.size() != 1)

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn))

				  {

				    if (DiscardTurnByHighwayClass(nodes, turnInfo, numMwmIds, turn.m_turn) ||

AntonM030481 (Migrated from github.com) commented

2022-04-20 15:40:21 +00:00

ок

AntonM030481 (Migrated from github.com) reviewed 2022-04-20 15:40:37 +00:00

routing/turns_generator.cpp

					
				@ -1243,1 +1204,3 @@

				      turn.m_turn = CarDirection::None;

				    // If the route goes along the rightmost or the leftmost way among available ones:

				    // 1. RightmostDirection or LeftmostDirection is selected

				    // 2. If the turn direction is |CarDirection::GoStraight|

AntonM030481 (Migrated from github.com) commented

2022-04-20 15:40:37 +00:00

ок

vng (Migrated from github.com) reviewed 2022-04-20 16:41:17 +00:00

routing/turns_generator.cpp

					
				@ -1204,3 +1167,3 @@

				  // will be made after.

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn) && nodes.candidates.size() != 1)

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn))

				  {

vng (Migrated from github.com) commented

2022-04-20 16:41:16 +00:00

Хм, такое вообще потенциально небезопасно, грубо говоря мы делаем memcpy(dest, src) где src, dest на одном буфере. Я бы написал так:

std::vector candidatesWithoutUturn;
if (...)
{
  auto const & src = nodes.candidates;
  if (..)
    candidatesWithoutUturn.assign(src.begin() + 1, src.end());
  else if (..)
    candidatesWithoutUturn.assign(src.begin(), src.end() - 1);
  else
    candidatesWithoutUturn = src;
}
else
  candidatesWithoutUturn = nodes.candidates;

Хм, такое вообще потенциально небезопасно, грубо говоря мы делаем memcpy(dest, src) где src, dest на одном буфере. Я бы написал так: ``` std::vector candidatesWithoutUturn; if (...) { auto const & src = nodes.candidates; if (..) candidatesWithoutUturn.assign(src.begin() + 1, src.end()); else if (..) candidatesWithoutUturn.assign(src.begin(), src.end() - 1); else candidatesWithoutUturn = src; } else candidatesWithoutUturn = nodes.candidates; ```

biodranik (Migrated from github.com) reviewed 2022-04-20 16:50:13 +00:00

routing/turns_generator.cpp

					
				@ -1204,3 +1167,3 @@

				  // will be made after.

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn) && nodes.candidates.size() != 1)

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn))

				  {

biodranik (Migrated from github.com) commented

2022-04-20 16:50:13 +00:00

Почему бы не использовать candidatesWithoutUturn.erase(candidatesWithoutUturn.begin()); и candidatesWithoutUturn.erase(candidatesWithoutUturn.rbegin());?

Почему бы не использовать `candidatesWithoutUturn.erase(candidatesWithoutUturn.begin());` и `candidatesWithoutUturn.erase(candidatesWithoutUturn.rbegin());`?

vng commented

2022-04-20 16:53:12 +00:00

(Migrated from github.com)

Очевидно, это не для этого PR, но надо бы придумать какую-то инфраструктуру для unit tests для
GetTurnDirection/MakeTurnAnnotation

Очевидно, это не для этого PR, но надо бы придумать какую-то инфраструктуру для unit tests для GetTurnDirection/MakeTurnAnnotation

👍 1

vng (Migrated from github.com) reviewed 2022-04-20 17:07:52 +00:00

routing/turns_generator.cpp

					
				@ -1204,3 +1167,3 @@

				  // will be made after.

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn) && nodes.candidates.size() != 1)

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn))

				  {

vng (Migrated from github.com) commented

2022-04-20 17:07:51 +00:00

Тогда уж, pop_front(), pop_back(). В моем варианте меньше манипуляций с вектором. Он только присваивается, когда erase/pop_front() сделает его переалокацию.

vng commented

2022-04-20 19:58:23 +00:00

(Migrated from github.com)

Some tests failed here (turns_generator_test.cpp), but they are very straightforward like:
TEST_EQUAL(RightmostDirection(-20.), CarDirection::TurnSlightLeft, ());
So feel free to edit them.

To run tests:
./routing_tests
./routing_tests --filter="TestRightmostDirection"

Some tests failed here (turns_generator_test.cpp), but they are very straightforward like: TEST_EQUAL(RightmostDirection(-20.), CarDirection::TurnSlightLeft, ()); So feel free to edit them. To run tests: ./routing_tests ./routing_tests --filter="TestRightmostDirection"

AntonM030481 commented

2022-04-20 20:50:05 +00:00

(Migrated from github.com)

После рефакторинга RighmostDirection поменялся. Если нет возражений - поправлю тесты.

biodranik (Migrated from github.com) reviewed 2022-04-20 22:20:11 +00:00

routing/turns_generator.cpp

					
				@ -1204,3 +1167,3 @@

				  // will be made after.

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn) && nodes.candidates.size() != 1)

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn))

				  {

biodranik (Migrated from github.com) commented

2022-04-20 22:20:11 +00:00

pop_back() да, а вот когда это у вектора появился pop_front() ? :)

AntonM030481 (Migrated from github.com) reviewed 2022-04-21 13:34:34 +00:00

routing/turns_generator.cpp

					
				@ -1204,3 +1167,3 @@

				  // will be made after.

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn) && nodes.candidates.size() != 1)

				  if (!turn.m_keepAnyway && IsGoStraightOrSlightTurn(turn.m_turn))

				  {

AntonM030481 (Migrated from github.com) commented

2022-04-21 13:34:34 +00:00

Исправил

AntonM030481 commented

2022-04-21 15:59:12 +00:00

(Migrated from github.com)

Тесты RightmostDirection и LeftmostDirection исправлены.

Кстати говоря, у меня
3 tests failed:
opening_hours_serdes_tests.cpp::BitReaderWriter_OpeningHoursSerDes_Weekday_Usage_2
opening_hours_serdes_tests.cpp::BitReaderWriter_OpeningHoursSerDes_MonthHours_Usage
road_access_test.cpp::RoadAccess_WayBlockedWhenStartButOpenWhenReach

Тесты RightmostDirection и LeftmostDirection исправлены. Кстати говоря, у меня 3 tests failed: opening_hours_serdes_tests.cpp::BitReaderWriter_OpeningHoursSerDes_Weekday_Usage_2 opening_hours_serdes_tests.cpp::BitReaderWriter_OpeningHoursSerDes_MonthHours_Usage road_access_test.cpp::RoadAccess_WayBlockedWhenStartButOpenWhenReach

vng commented

2022-04-21 17:19:55 +00:00

(Migrated from github.com)

Странно что у вас не проходят эти тесты, на GA все прошло хорошо. Что у вас за ошибки?

vng (Migrated from github.com) reviewed 2022-04-21 17:29:46 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

vng (Migrated from github.com) commented

2022-04-21 17:29:42 +00:00

Could you explain why 90 degrees is TurnRight and -90 degrees is GoStraight?
The same with others .. Am I miss something with these angles?

Could you explain why 90 degrees is TurnRight and -90 degrees is GoStraight? The same with others .. Am I miss something with these angles?

AntonM030481 (Migrated from github.com) reviewed 2022-04-21 18:01:44 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

AntonM030481 (Migrated from github.com) commented

2022-04-21 18:01:44 +00:00

I was trying to explain it in RighmostDirection() definition:

// For sure it's incorrect to give directions TurnLeft or TurnSlighLeft if we need the rightmost turn.
// So GoStraight direction is given even for needed sharp left turn when other turns on the left and are more sharp.
// The reason: the rightmost turn is the most straight one here.

If necessary I can make it more clear or move comments to \brief section.

I was trying to explain it in RighmostDirection() definition: ``` // For sure it's incorrect to give directions TurnLeft or TurnSlighLeft if we need the rightmost turn. // So GoStraight direction is given even for needed sharp left turn when other turns on the left and are more sharp. // The reason: the rightmost turn is the most straight one here. ``` If necessary I can make it more clear or move comments to \brief section.

vng (Migrated from github.com) reviewed 2022-04-21 18:12:23 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

vng (Migrated from github.com) commented

2022-04-21 18:12:23 +00:00

Ok, but could you also provide an example (in brief or description or when RighmostDirection/LeftmostDirection functions are called), who decided that the turn is "rightmost" or "leftmost"?
What does "rightmost/leftmost" actually mean?

Ok, but could you also provide an example (in brief or description or when RighmostDirection/LeftmostDirection functions are called), who decided that the turn is "rightmost" or "leftmost"? What does "rightmost/leftmost" actually mean?

vng (Migrated from github.com) reviewed 2022-04-21 18:14:26 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

vng (Migrated from github.com) commented

2022-04-21 18:14:26 +00:00

I see this block but can't understand what does it mean?

if (nodes.candidates.front().m_segment == firstOutgoingSeg)
  turn.m_turn = LeftmostDirection(turnAngle);
else if (nodes.candidates.back().m_segment == firstOutgoingSeg)
  turn.m_turn = RightmostDirection(turnAngle);
else
  turn.m_turn = intermediateDirection;

I see this block but can't understand what does it mean? ``` if (nodes.candidates.front().m_segment == firstOutgoingSeg) turn.m_turn = LeftmostDirection(turnAngle); else if (nodes.candidates.back().m_segment == firstOutgoingSeg) turn.m_turn = RightmostDirection(turnAngle); else turn.m_turn = intermediateDirection; ```

vng (Migrated from github.com) reviewed 2022-04-21 18:16:06 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

vng (Migrated from github.com) commented

2022-04-21 18:16:06 +00:00

Ah, if nodes.candidates are sorted by direction angle, then it makes sense :)
Ok, please, add some comments, but now I got the logic!

Ah, if nodes.candidates are sorted by direction angle, then it makes sense :) Ok, please, add some comments, but now I got the logic!

AntonM030481 (Migrated from github.com) reviewed 2022-04-21 18:27:17 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

AntonM030481 (Migrated from github.com) commented

2022-04-21 18:27:17 +00:00

Running road_access_test.cpp::RoadAccess_WayBlockedWhenStartButOpenWhenReach
FAILED
routing_tests/index_graph_tools.cpp:606 TEST(base::AlmostEqualAbs(pathWeight, expectedWeight, kEpsilon)) 10804 12802 [5: (0, 1) (1, 2) (2, 3) (3, 4) (4, 5) ]

Running opening_hours_serdes_tests.cpp::BitReaderWriter_OpeningHoursSerDes_MonthHours_Usage
FAILED
routing_tests/opening_hours_serdes_tests.cpp:823 TEST(oh.IsClosed(GetUnixtimeByDate(2020, Month::May, 6, 01 , 00 )))

Running opening_hours_serdes_tests.cpp::BitReaderWriter_OpeningHoursSerDes_Weekday_Usage_2
FAILED
routing_tests/opening_hours_serdes_tests.cpp:750 TEST(oh.IsOpen(GetUnixtimeByDate(2023, Month::Apr, Weekday::Saturday, 13 , 30 )))

Running road_access_test.cpp::RoadAccess_WayBlockedWhenStartButOpenWhenReach FAILED routing_tests/index_graph_tools.cpp:606 TEST(base::AlmostEqualAbs(pathWeight, expectedWeight, kEpsilon)) 10804 12802 [5: (0, 1) (1, 2) (2, 3) (3, 4) (4, 5) ] Running opening_hours_serdes_tests.cpp::BitReaderWriter_OpeningHoursSerDes_MonthHours_Usage FAILED routing_tests/opening_hours_serdes_tests.cpp:823 TEST(oh.IsClosed(GetUnixtimeByDate(2020, Month::May, 6, 01 , 00 ))) Running opening_hours_serdes_tests.cpp::BitReaderWriter_OpeningHoursSerDes_Weekday_Usage_2 FAILED routing_tests/opening_hours_serdes_tests.cpp:750 TEST(oh.IsOpen(GetUnixtimeByDate(2023, Month::Apr, Weekday::Saturday, 13 , 30 )))

vng (Migrated from github.com) reviewed 2022-04-21 18:38:28 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

vng (Migrated from github.com) commented

2022-04-21 18:38:28 +00:00

What is your env (Linux/MacOS) and toolchain (gcc/clang --version)?
Looks like OpeningHours (boost/spirit) is buggy on your system.
Do you have boost submodule latest and updated?

What is your env (Linux/MacOS) and toolchain (gcc/clang --version)? Looks like OpeningHours (boost/spirit) is buggy on your system. Do you have boost submodule latest and updated?

vng (Migrated from github.com) reviewed 2022-04-21 18:50:38 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

vng (Migrated from github.com) commented

2022-04-21 18:50:38 +00:00

BTW, try to run ./opening_hours_tests

AntonM030481 (Migrated from github.com) reviewed 2022-04-21 19:16:13 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

AntonM030481 (Migrated from github.com) commented

2022-04-21 19:16:13 +00:00

MacOS 12.13.1
Apple clang version 13.1.6 (clang-1316.0.21.2)
Target: x86_64-apple-darwin21.4.0
Thread model: posix

Boost just pulled.

*** 12 failures are detected in the test module "OpeningHours"

MacOS 12.13.1 Apple clang version 13.1.6 (clang-1316.0.21.2) Target: x86_64-apple-darwin21.4.0 Thread model: posix Boost just pulled. *** 12 failures are detected in the test module "OpeningHours"

AntonM030481 (Migrated from github.com) reviewed 2022-04-21 19:19:10 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

AntonM030481 (Migrated from github.com) commented

2022-04-21 19:19:10 +00:00

Added:

// turnCandidates are sorted by angle from leftmost to rightmost.
if (turnCandidates.front().m_segment == firstOutgoingSeg)

Added: ``` // turnCandidates are sorted by angle from leftmost to rightmost. if (turnCandidates.front().m_segment == firstOutgoingSeg) ```

vng (Migrated from github.com) approved these changes 2022-04-21 19:22:50 +00:00

vng (Migrated from github.com) reviewed 2022-04-21 19:23:40 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

vng (Migrated from github.com) commented

2022-04-21 19:23:40 +00:00

Strange, because I have exactly the same toolchain ..

AntonM030481 (Migrated from github.com) reviewed 2022-04-21 20:23:56 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

AntonM030481 (Migrated from github.com) commented

2022-04-21 20:23:56 +00:00

Do I need any additional data to run the tests?
I used "--filter=blob:limit=128k"

Do I need any additional data to run the tests? I used "--filter=blob:limit=128k"

vng (Migrated from github.com) reviewed 2022-04-21 20:36:29 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

vng (Migrated from github.com) commented

2022-04-21 20:36:29 +00:00

No, you don't .. Could you show me:
cat 3party/boost/boost/version.hpp | grep "VERSION"
cat 3party/boost/boost/spirit/include/version.hpp | grep "VERSION"
cat 3party/boost/boost/phoenix/version.hpp | grep "VERSION"

No, you don't .. Could you show me: cat 3party/boost/boost/version.hpp | grep "VERSION" cat 3party/boost/boost/spirit/include/version.hpp | grep "VERSION" cat 3party/boost/boost/phoenix/version.hpp | grep "VERSION"

vng commented

2022-04-22 06:16:03 +00:00

(Migrated from github.com)

@biodranik Approve?

biodranik (Migrated from github.com) approved these changes 2022-04-22 08:09:31 +00:00

vng commented

2022-04-22 13:10:31 +00:00

(Migrated from github.com)

@AntonM030481
Confirm that turn instructions engine now works better.
Checked and fixed on new generated data with routing_integration_tests master build.

There are still some turn issues. Please, check these tests if you have free time and feel free to ask.

This is "by-design-bug", because here is shuttle_train road, but will be good if we can do something here (skip turns in shuttle train and ferry).
route_test.cpp::GermanyShuttleTrainTest
Screenshot 2022-04-22 at 16 20 36

There are minor bugs:
bicycle_turn_test.cpp::RussiaMoscowTatishchevaOnewayCarRoadTurnTest
bicycle_turn_test.cpp::TurnsNearAltufievskoeShosseLongFakeSegmentTest
bicycle_turn_test.cpp::TurnsNearKhladkombinatTest

turn_test.cpp::RussiaMoscowVarshavskoeShosseMKAD
turn_test.cpp::RussiaMoscowSvobodaStTest

@AntonM030481 Confirm that turn instructions engine now works better. Checked and fixed on new generated data with routing_integration_tests master build. There are still some turn issues. Please, check these tests if you have free time and feel free to ask. This is "by-design-bug", because here is shuttle_train road, but will be good if we can do something here (skip turns in shuttle train and ferry). route_test.cpp::GermanyShuttleTrainTest <img width="1246" alt="Screenshot 2022-04-22 at 16 20 36" src="https://user-images.githubusercontent.com/175612/164722472-f1bffad5-9a4d-41fa-84f1-cc49c90e2fc1.png"> There are minor bugs: bicycle_turn_test.cpp::RussiaMoscowTatishchevaOnewayCarRoadTurnTest bicycle_turn_test.cpp::TurnsNearAltufievskoeShosseLongFakeSegmentTest bicycle_turn_test.cpp::TurnsNearKhladkombinatTest turn_test.cpp::RussiaMoscowVarshavskoeShosseMKAD turn_test.cpp::RussiaMoscowSvobodaStTest

👍 1

AntonM030481 (Migrated from github.com) reviewed 2022-04-26 08:43:32 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

AntonM030481 (Migrated from github.com) commented

2022-04-26 08:43:31 +00:00

#define BOOST_VERSION 107900
#define BOOST_LIB_VERSION "1_79"

#define SPIRIT_VERSION 0x2059
#define SPIRIT_PIZZA_VERSION SUPER_HOT_SPANISH_SARDINES  // :-O

#define BOOST_PHOENIX_VERSION   0x3200    // 3.2.0
#define BOOST_PHOENIX_VERSION_NUMBER = BOOST_VERSION_NUMBER(3,2,0)

``` #define BOOST_VERSION 107900 #define BOOST_LIB_VERSION "1_79" #define SPIRIT_VERSION 0x2059 #define SPIRIT_PIZZA_VERSION SUPER_HOT_SPANISH_SARDINES // :-O #define BOOST_PHOENIX_VERSION 0x3200 // 3.2.0 #define BOOST_PHOENIX_VERSION_NUMBER = BOOST_VERSION_NUMBER(3,2,0) ```

biodranik (Migrated from github.com) reviewed 2022-04-26 11:52:45 +00:00

routing/routing_tests/turns_generator_test.cpp

					
				@ -334,3 +333,4 @@

				  TEST_EQUAL(RightmostDirection(-170.), CarDirection::GoStraight, ());

				}

				UNIT_TEST(TestLeftmostDirection)

biodranik (Migrated from github.com) commented

2022-04-26 11:52:45 +00:00

$ ../omim-build-debug/opening_hours_tests
Running 23 test cases...
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1348: error: in "OpeningHours_TestIsActive": check IsActive(rules[0], time) has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1351: error: in "OpeningHours_TestIsActive": check IsActive(rules[1], time) has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1458: error: in "OpeningHours_TestIsActive": check IsActive(rules[0], time) has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1478: error: in "OpeningHours_TestIsActive": check IsActive(rules[0], time) has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1553: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2021-05-08 19:40") has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1592: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2015-07-09 16:50") has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1615: error: in "OpeningHours_TestIsOpen": check IsClosed(rules, "2012-10-08 15:59") has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1628: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2015-04-10 07:15") has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1629: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2015-05-01 07:15") has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1630: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2015-04-11 07:15") has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1649: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2017-05-31 23:35") has failed
/Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1659: error: in "OpeningHours_TestIsOpen": check !IsOpen(rules, "2017-05-31 23:35") has failed

*** 12 failures are detected in the test module "OpeningHours"

``` $ ../omim-build-debug/opening_hours_tests Running 23 test cases... /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1348: error: in "OpeningHours_TestIsActive": check IsActive(rules[0], time) has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1351: error: in "OpeningHours_TestIsActive": check IsActive(rules[1], time) has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1458: error: in "OpeningHours_TestIsActive": check IsActive(rules[0], time) has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1478: error: in "OpeningHours_TestIsActive": check IsActive(rules[0], time) has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1553: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2021-05-08 19:40") has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1592: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2015-07-09 16:50") has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1615: error: in "OpeningHours_TestIsOpen": check IsClosed(rules, "2012-10-08 15:59") has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1628: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2015-04-10 07:15") has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1629: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2015-05-01 07:15") has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1630: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2015-04-11 07:15") has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1649: error: in "OpeningHours_TestIsOpen": check IsOpen(rules, "2017-05-31 23:35") has failed /Users/alex/Developer/omim/omim/3party/opening_hours/opening_hours_tests/opening_hours_tests.cpp:1659: error: in "OpeningHours_TestIsOpen": check !IsOpen(rules, "2017-05-31 23:35") has failed *** 12 failures are detected in the test module "OpeningHours" ```

AntonM030481 commented

2022-04-26 18:05:48 +00:00

(Migrated from github.com)

How can I see the test routes in desktop?
I try to

Copy coordinates from UNIT_TEST
Paste it in Routing Settings in Desktop
Select appropriate router and show turns
Press OK
Build Road icon?...

How can I see the test routes in desktop? I try to 1. Copy coordinates from UNIT_TEST 2. Paste it in Routing Settings in Desktop 3. Select appropriate router and show turns 4. Press OK 5. Build Road icon?...

vng commented

2022-04-26 19:51:13 +00:00

(Migrated from github.com)

Yes, this is not obvious .. After closing Routing Settings dialog, press ALT + LMB (current position), then SHIFT + LMB (destination point). Instead of mouse points, the app will take coordinates from dialog.

👍 1

biodranik commented

2022-04-27 08:55:51 +00:00

(Migrated from github.com)

Maybe we can add a comment text in the dialog with these hints?

👍 1

AntonM030481 commented

2022-04-27 09:00:27 +00:00

(Migrated from github.com)

What is "Build Route" icon is responsible for?
Sounds natural that it will build road according to Routing Settings.
Second part of its tooltip is not clear for me in the context of the first part.

What is "Build Route" icon is responsible for? Sounds natural that it will build road according to Routing Settings. Second part of its tooltip is not clear for me in the context of the first part.

vng commented

2022-04-27 09:28:04 +00:00

(Migrated from github.com)

Well, not a good hint there.

build route with ALT + LMB (current position) and any destination point
press this button, the app will start "Follow route" mode
use ALT + LMB to simulate current position and move along the route

Well, not a good hint there. - build route with ALT + LMB (current position) and any destination point - press this button, the app will start "Follow route" mode - use ALT + LMB to simulate current position and move along the route

vng commented

2022-04-27 09:29:28 +00:00

(Migrated from github.com)

So the tooltip should be: "Follow route mode only if route built from the current position".

Or always move current position to the start of a route when clicked.

So the tooltip should be: "Follow route mode only if route built from the current position". Or always move current position to the start of a route when clicked.

👍 1

This repo is archived. You cannot comment on pull requests.

No reviewers

vng

biodranik