[routing] Fixed RouteSegment params calculation. #2868

2022-06-29T10:25:05Z

vng commented

2022-06-29 10:25:05 +00:00

(Migrated from github.com)

CC @AntonM030481

biodranik commented

2022-06-29 11:13:52 +00:00

(Migrated from github.com)

Running route_tests.cpp::FinshRouteOnSomeDistanceToTheFinishPointTest
TID(1) ASSERT FAILED
routing/routing_helpers.cpp:26
CHECK(times.size() == routeSegments.size()) 6 5
routing_tests: /home/runner/work/organicmaps/organicmaps/routing/routing_helpers.cpp:26: void routing::FillSegmentInfo(const vector &, vectorrouting::RouteSegment &): Assertion `false' failed.
Aborted (core dumped)
2022-06-29 10:49:42,613 coding_tests --data_path=./data --user_resource_path=./data
2022-06-29 10:49:42,615 Pid: 18309

Running route_tests.cpp::FinshRouteOnSomeDistanceToTheFinishPointTest TID(1) ASSERT FAILED routing/routing_helpers.cpp:26 CHECK(times.size() == routeSegments.size()) 6 5 routing_tests: /home/runner/work/organicmaps/organicmaps/routing/routing_helpers.cpp:26: void routing::FillSegmentInfo(const vector<double> &, vector<routing::RouteSegment> &): Assertion `false' failed. Aborted (core dumped) 2022-06-29 10:49:42,613 coding_tests --data_path=./data --user_resource_path=./data 2022-06-29 10:49:42,615 Pid: 18309

biodranik (Migrated from github.com) approved these changes 2022-06-29 11:34:36 +00:00

biodranik (Migrated from github.com) left a comment

Как-то много правок. Как будем тестировать?

routing/car_directions.cpp

biodranik (Migrated from github.com) commented

2022-06-29 11:14:23 +00:00

Почему закомменчено?

routing/directions_engine.cpp

biodranik (Migrated from github.com) commented

2022-06-29 11:15:39 +00:00

коммент в (), куда копать если свалится?

routing/directions_engine.cpp

biodranik (Migrated from github.com) commented

2022-06-29 11:18:11 +00:00

Будет unused warning в релизе. #ifdef DEBUG? Или в ассерт вставить loadedSegments.front() ?

routing/edge_estimator.cpp

					
				@ -42,2 +41,3 @@

				    return kImpossibleDrivingFactor;

				    // impossible driving factor

				    return 1.0E4;

				  }

biodranik (Migrated from github.com) commented

2022-06-29 11:20:03 +00:00

С константой же было норм. Зачем убрал?

    // Impossible driving factor (поясни тогда уже что это)

С константой же было норм. Зачем убрал? ```suggestion // Impossible driving factor (поясни тогда уже что это) ```

routing/edge_estimator.cpp

biodranik (Migrated from github.com) commented

2022-06-29 11:21:05 +00:00

Зачем убирать именованные переменные? Они же constexpr, компилятор сам подставит и будет оптимально.

routing/geometry.hpp

					
				@ -106,2 +106,4 @@

				  virtual void Load(uint32_t featureId, RoadGeometry & road) = 0;

				  /// Used in client-app only for the final route preparation.

				  virtual SpeedInUnits GetSavedMaxspeed(uint32_t featureId, bool forward);

biodranik (Migrated from github.com) commented

2022-06-29 11:23:44 +00:00

= 0?

routing/geometry.hpp

biodranik (Migrated from github.com) commented

2022-06-29 11:26:57 +00:00

Это обязательно в разных классах делать идентичные методы? Можно запутаться.

routing/index_graph.hpp

biodranik (Migrated from github.com) commented

2022-06-29 11:27:52 +00:00

Что по-русски хотел сказать? Я ничего не понял. flase => false тут и ниже.

routing/index_router.cpp

biodranik (Migrated from github.com) commented

2022-06-29 11:29:27 +00:00

    // By VNG: I believe that we should convert the segment to a real one first, and fetch

```suggestion // By VNG: I believe that we should convert the segment to a real one first, and fetch ```

routing/loaded_path_segment.hpp

biodranik (Migrated from github.com) commented

2022-06-29 11:31:19 +00:00

Чтобы меньше памяти занимала структура?

vng (Migrated from github.com) reviewed 2022-06-29 11:43:54 +00:00

routing/car_directions.cpp

vng (Migrated from github.com) commented

2022-06-29 11:43:54 +00:00

Потому что валится тут.
Я так понимаю, @AntonM030481 расставлял ассерты по "наитию", чтобы понять алгоритмы. Но тут это не работает.

Потому что валится тут. Я так понимаю, @AntonM030481 расставлял ассерты по "наитию", чтобы понять алгоритмы. Но тут это не работает.

vng (Migrated from github.com) reviewed 2022-06-29 11:44:38 +00:00

routing/directions_engine.cpp

vng (Migrated from github.com) commented

2022-06-29 11:44:38 +00:00

Здесь строки вылета достаточно, и какие дополнительные коменты не имеют смысла. Копать везде :)

vng (Migrated from github.com) reviewed 2022-06-29 11:47:06 +00:00

routing/edge_estimator.cpp

					
				@ -42,2 +41,3 @@

				    return kImpossibleDrivingFactor;

				    // impossible driving factor

				    return 1.0E4;

				  }

vng (Migrated from github.com) commented

2022-06-29 11:47:06 +00:00

Не понимаю для чего добавлять переменную, которая используется сразу же и только в следующей строке.
Переменная стала комментарием, других пояснений нет.

- Не понимаю для чего добавлять переменную, которая используется сразу же и только в следующей строке. - Переменная стала комментарием, других пояснений нет.

vng (Migrated from github.com) reviewed 2022-06-29 11:48:22 +00:00

routing/edge_estimator.cpp

vng (Migrated from github.com) commented

2022-06-29 11:48:22 +00:00

Потому что она не несет никакой смысловой нагрузки. Лучше бы развернутое пояснение, тебе стало понятнее от "kTimePenalty"?

vng (Migrated from github.com) reviewed 2022-06-29 11:48:45 +00:00

routing/geometry.hpp

					
				@ -106,2 +106,4 @@

				  virtual void Load(uint32_t featureId, RoadGeometry & road) = 0;

				  /// Used in client-app only for the final route preparation.

				  virtual SpeedInUnits GetSavedMaxspeed(uint32_t featureId, bool forward);

vng (Migrated from github.com) commented

2022-06-29 11:48:45 +00:00

много добавлять лишних заглушек

vng (Migrated from github.com) reviewed 2022-06-29 11:49:16 +00:00

routing/geometry.hpp

vng (Migrated from github.com) commented

2022-06-29 11:49:16 +00:00

Потом один вызывает второй, и все логично

vng (Migrated from github.com) reviewed 2022-06-29 11:50:52 +00:00

routing/index_graph.hpp

vng (Migrated from github.com) commented

2022-06-29 11:50:52 +00:00

prevWeight - время когда юзер будет в конце from (to)

vng (Migrated from github.com) reviewed 2022-06-29 12:18:33 +00:00

routing/car_directions.cpp

vng (Migrated from github.com) commented

2022-06-29 12:18:32 +00:00

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

AntonM030481 (Migrated from github.com) reviewed 2022-06-29 13:27:31 +00:00

routing/index_router.cpp

AntonM030481 (Migrated from github.com) commented

2022-06-29 13:27:31 +00:00

Тогда уже и точка в конце)

biodranik (Migrated from github.com) reviewed 2022-06-29 13:34:21 +00:00

routing/car_directions.cpp

biodranik (Migrated from github.com) commented

2022-06-29 13:34:21 +00:00

Если условие не выполняется, это баг, надо искать причину и фиксить, а не комментить.

vng (Migrated from github.com) reviewed 2022-06-29 13:38:59 +00:00

routing/car_directions.cpp

vng (Migrated from github.com) commented

2022-06-29 13:38:58 +00:00

Я не уверен, что тут нет баги в самом условии. Оно никак не проверяет, что выполняется потом.

AntonM030481 (Migrated from github.com) reviewed 2022-06-29 14:13:18 +00:00

routing/edge_estimator.cpp

AntonM030481 (Migrated from github.com) commented

2022-06-29 14:13:18 +00:00

Сейчас CarEstimator::CalcSegment() используется только в CarEstimator::CalcSegmentWeight()
может, отдельный метод вообще не нужен?

Сейчас CarEstimator::CalcSegment() используется только в CarEstimator::CalcSegmentWeight() может, отдельный метод вообще не нужен?

AntonM030481 (Migrated from github.com) reviewed 2022-06-29 14:29:51 +00:00

routing/geometry.hpp

					
				@ -106,2 +106,4 @@

				  virtual void Load(uint32_t featureId, RoadGeometry & road) = 0;

				  /// Used in client-app only for the final route preparation.

				  virtual SpeedInUnits GetSavedMaxspeed(uint32_t featureId, bool forward);

AntonM030481 (Migrated from github.com) commented

2022-06-29 14:29:49 +00:00

Я так и не понял, почему от дополнительного = 0 класс становится абстрактным и не хочет инстанцироваться. Почему для Load() все ОК?

vng (Migrated from github.com) reviewed 2022-06-29 14:32:33 +00:00

routing/geometry.hpp

					
				@ -106,2 +106,4 @@

				  virtual void Load(uint32_t featureId, RoadGeometry & road) = 0;

				  /// Used in client-app only for the final route preparation.

				  virtual SpeedInUnits GetSavedMaxspeed(uint32_t featureId, bool forward);

vng (Migrated from github.com) commented

2022-06-29 14:32:32 +00:00

Load нужен во всех производных, GetSavedMaxspeed достаточно только в одной (по крайней мере пока)

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

routing/index_router.cpp

					
				@ -1559,3 +1560,4 @@

				  for (size_t i = 0; i <= segsCount; ++i)

				    junctions.emplace_back(starter.GetRouteJunction(segments, i).ToPointWithAltitude());

				  IndexRoadGraph roadGraph(starter, segments, junctions, m_dataSource);

AntonM030481 (Migrated from github.com) commented

2022-06-29 15:04:53 +00:00

может
geometry::PointWithAltitude LatLonWithAltitude::ToPointWithAltitude()
превратить в
operator geometry::PointWithAltitude()
?

может geometry::PointWithAltitude LatLonWithAltitude::ToPointWithAltitude() превратить в operator geometry::PointWithAltitude() ?

AntonM030481 (Migrated from github.com) reviewed 2022-06-29 15:13:17 +00:00

routing/loaded_path_segment.hpp

AntonM030481 (Migrated from github.com) commented

2022-06-29 15:13:16 +00:00

тут вообще больше не нужен m_maxSpeed.

AntonM030481 (Migrated from github.com) reviewed 2022-06-29 15:31:26 +00:00

routing/turns_generator.cpp

AntonM030481 (Migrated from github.com) commented

2022-06-29 15:31:26 +00:00

У меня это все перестало вылетать после фикса
7f23b24c5e/routing/car_directions.cpp (L105)

У меня это все перестало вылетать после фикса https://github.com/organicmaps/organicmaps/blob/7f23b24c5e2f8c475292616d4b744b2762d3a021/routing/car_directions.cpp#L105

vng (Migrated from github.com) reviewed 2022-06-29 15:40:29 +00:00

routing/turns_generator.cpp

vng (Migrated from github.com) commented

2022-06-29 15:40:28 +00:00

ну давай попробуем

AntonM030481 (Migrated from github.com) reviewed 2022-06-29 15:43:42 +00:00

routing/edge_estimator.cpp

AntonM030481 (Migrated from github.com) commented

2022-06-29 15:43:41 +00:00

@vng оставляем как есть?

AntonM030481 commented

2022-06-29 16:12:33 +00:00

(Migrated from github.com)

Running bicycle_turn_test.cpp::RussiaMoscowSevTushinoParkBicycleOnePointOnewayRoadTurnTest
Avoid next roads: RoutingOptions: {}
routing section for Russia_Moscow loaded in 1.00523 seconds
Routing in mode: Joints
Route build time: 0 ms
TID(1) ASSERT FAILED
routing/directions_engine.cpp:217
CHECK(m_adjacentEdges.emplace(segmentRange, move(adjacentEdges)).second)
Assertion failed: (false), function FillPathSegmentsAndAdjacentEdgesMap, file directions_engine.cpp, line 217.

Data 220515, 220613 - the same.

Running bicycle_turn_test.cpp::RussiaMoscowSevTushinoParkBicycleOnePointOnewayRoadTurnTest Avoid next roads: RoutingOptions: {} routing section for Russia_Moscow loaded in 1.00523 seconds Routing in mode: Joints Route build time: 0 ms TID(1) ASSERT FAILED routing/directions_engine.cpp:217 CHECK(m_adjacentEdges.emplace(segmentRange, move(adjacentEdges)).second) Assertion failed: (false), function FillPathSegmentsAndAdjacentEdgesMap, file directions_engine.cpp, line 217. Data 220515, 220613 - the same.

AntonM030481 commented

2022-06-29 16:23:25 +00:00

(Migrated from github.com)

assert in AsyncGuiThreadTestWithRoutingSession_TestRouteRebuildingMovingAway

Remove 1st element
ed7bae9081/routing/routing_tests/routing_session_test.cpp (L41)

assert in AsyncGuiThreadTestWithRoutingSession_TestRouteRebuildingMovingAway Remove 1st element https://github.com/organicmaps/organicmaps/blob/ed7bae90818ba74b02bf48769dcc2bdae20a0827/routing/routing_tests/routing_session_test.cpp#L41

AntonM030481 (Migrated from github.com) approved these changes 2022-06-29 17:09:50 +00:00

biodranik (Migrated from github.com) approved these changes 2022-06-29 18:43:12 +00:00

routing/index_router.cpp

biodranik (Migrated from github.com) commented

2022-06-29 18:39:53 +00:00

todo?

routing/world_graph.cpp

					
				@ -42,0 +41,4 @@

				std::unique_ptr<TransitInfo> WorldGraph::GetTransitInfo(Segment const &)

				{

				  return nullptr;

				}

biodranik (Migrated from github.com) commented

2022-06-29 18:43:01 +00:00

Чисто виртуальные методы были нагляднее и более безопасные. Чем пустые значения лучше?

vng (Migrated from github.com) reviewed 2022-06-29 19:39:05 +00:00

routing/index_router.cpp

vng (Migrated from github.com) commented

2022-06-29 19:39:05 +00:00

Я позапускал интеграционные тесты, тут мой believe откатывется назад и ставится todo.

vng (Migrated from github.com) reviewed 2022-06-29 19:43:36 +00:00

routing/world_graph.cpp

					
				@ -42,0 +41,4 @@

				std::unique_ptr<TransitInfo> WorldGraph::GetTransitInfo(Segment const &)

				{

				  return nullptr;

				}

vng (Migrated from github.com) commented

2022-06-29 19:43:36 +00:00

Тем, что вместо 3-х заглушек пишется одна. По сути, это все равно не фонтан интерфейсное решение, потому не вижу смысла плодить заглушки пусть и ради наглядного = 0.

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

No reviewers

AntonM030481

biodranik