[Routing] TurnDirections refactoring #2508

2022-05-03T13:49:23Z

AntonM030481 commented

2022-05-03 13:49:23 +00:00

(Migrated from github.com)

Total refactoring of GetTurnDirection() and it's subfunctions.
A lot of new integration tests added (one additional map is required - Cyprus).
Old ones fixed where necessary.
Now only 3 tests failed:
bicycle_turn_test.cpp::RussiaMoscowSevTushinoParkBicycleWayTurnTest (arguable, see todo there)
turn_test.cpp::Russia_Moscow_BolshoyKislovskiyPerBolshayaNikitinskayaUl_TurnTest (bug of other system, see todo there)
turn_test.cpp::Netherlands_Barneveld_TurnTest (complicated new test).

Fixed:
#2417
#2438

Results of tests from pedestrian_directions.cpp are the same as before.

Total refactoring of GetTurnDirection() and it's subfunctions. A lot of new integration tests added (one additional map is required - Cyprus). Old ones fixed where necessary. Now only 3 tests failed: bicycle_turn_test.cpp::RussiaMoscowSevTushinoParkBicycleWayTurnTest (arguable, see todo there) turn_test.cpp::Russia_Moscow_BolshoyKislovskiyPerBolshayaNikitinskayaUl_TurnTest (bug of other system, see todo there) turn_test.cpp::Netherlands_Barneveld_TurnTest (complicated new test). Fixed: #2417 #2438 Results of tests from pedestrian_directions.cpp are the same as before.

biodranik (Migrated from github.com) reviewed 2022-05-03 13:49:23 +00:00

AntonM030481 commented

2022-05-04 08:13:35 +00:00

(Migrated from github.com)

Updated: No, it makes code less consistent here.
A lot of places where turnInfo is used.
Pointer semantic is not the best there.
Mix in different places is not good too.

I had to change struct TurnInfo reference members:

LoadedPathSegment const & m_ingoing;
LoadedPathSegment const & m_outgoing;

to pointers:

LoadedPathSegment const * m_ingoing;
LoadedPathSegment const * m_outgoing;

because of new function, which require assignment operator (incompatible with reference members).

TurnInfo turnInfo;
bool validTurnInfo = GetTurnInfo(result, outgoingSegmentIndex, vehicleSettings, turnInfo);

@biodranik Maybe better solution will be to use std::optional here and go back to references:
std::optional< turnInfo> = GetTurnInfo(result, outgoingSegmentIndex, vehicleSettings);

BTW, std::optional was introduced in C++17, but C++14 is used.
Frankly speaking I don't understand why it compiles now despite of -std=c++14 in CMAKE_CXX_FLAGS.

Updated: No, it makes code less consistent here. A lot of places where turnInfo is used. Pointer semantic is not the best there. Mix in different places is not good too. I had to change struct TurnInfo reference members: ``` LoadedPathSegment const & m_ingoing; LoadedPathSegment const & m_outgoing; ``` to pointers: ``` LoadedPathSegment const * m_ingoing; LoadedPathSegment const * m_outgoing; ``` because of new function, which require assignment operator (incompatible with reference members). ``` TurnInfo turnInfo; bool validTurnInfo = GetTurnInfo(result, outgoingSegmentIndex, vehicleSettings, turnInfo); ``` @biodranik Maybe better solution will be to use std::optional here and go back to references: `std::optional< turnInfo> = GetTurnInfo(result, outgoingSegmentIndex, vehicleSettings);` BTW, std::optional was introduced in C++17, but C++14 is used. Frankly speaking I don't understand why it compiles now despite of -std=c++14 in CMAKE_CXX_FLAGS.

vng (Migrated from github.com) reviewed 2022-05-05 09:06:28 +00:00

routing/turns_generator.cpp

					
				@ -13,58 +14,65 @@

				#include <sstream>

				#include <numeric>

vng (Migrated from github.com) commented

2022-05-05 08:46:49 +00:00

extra "alternative"

vng (Migrated from github.com) commented

2022-05-05 08:50:28 +00:00

align function params in column

routing/turns_generator.cpp

					
				@ -40,1 +32,3 @@

				bool IsHighway(ftypes::HighwayClass hwClass, bool isLink)

				// Min difference of route and alternative turn abs angles in degrees 

				// to ignore this alternative candidate (when alternative road is the same or smaller).

				double constexpr kMinAbsAngleDiffForSameOrSmallerRoad = 35.0;

vng (Migrated from github.com) commented

2022-05-05 08:48:12 +00:00

If so, I'd put main namespaces here to limit using scope and remove useless
using namespace routing;
using namespace routing::turns;
like this:

namespace routing
{
namespace turns
{
using namespace std;
using namespace ftypes;
...

If so, I'd put main namespaces here to limit using scope and remove useless using namespace routing; using namespace routing::turns; like this: ``` namespace routing { namespace turns { using namespace std; using namespace ftypes; ... ```

routing/turns_generator.cpp

vng (Migrated from github.com) commented

2022-05-05 08:57:57 +00:00

I'd prefer to see simple speedKmph = 100;
And make transform in the end:
distanceMeters / (speedKmph * 10.0 / 36.0);

I'd prefer to see simple speedKmph = 100; And make transform in the end: distanceMeters / (speedKmph * 10.0 / 36.0);

routing/turns_generator.cpp

					
				@ -1111,189 +1012,225 @@ void GetTurnDirection(IRoutingResult const & result, size_t outgoingSegmentIndex

				      result, outgoingSegmentIndex, numMwmIds, vehicleSettings.m_maxOutgoingPointsCount,

				      vehicleSettings.m_minOutgoingDistMeters, true /* forward */);

vng (Migrated from github.com) commented

2022-05-05 09:05:47 +00:00

if (!GetTurnInfo(...))

AntonM030481 (Migrated from github.com) reviewed 2022-05-05 09:12:30 +00:00

routing/turns_generator.cpp

					
				@ -13,58 +14,65 @@

				#include <sstream>

				#include <numeric>

AntonM030481 (Migrated from github.com) commented

2022-05-05 09:12:30 +00:00

ок

AntonM030481 (Migrated from github.com) reviewed 2022-05-05 09:30:47 +00:00

routing/turns_generator.cpp

					
				@ -13,58 +14,65 @@

				#include <sstream>

				#include <numeric>

AntonM030481 (Migrated from github.com) commented

2022-05-05 09:30:47 +00:00

ok

AntonM030481 (Migrated from github.com) reviewed 2022-05-05 09:48:48 +00:00

routing/turns_generator.cpp

AntonM030481 (Migrated from github.com) commented

2022-05-05 09:48:48 +00:00

Agree. Even better - found
measurement_utils::KmphToMps()
and used it.
BTW there is duplicating function transit::KmphToMps()

Agree. Even better - found measurement_utils::KmphToMps() and used it. BTW there is duplicating function transit::KmphToMps()

AntonM030481 (Migrated from github.com) reviewed 2022-05-05 10:10:54 +00:00

routing/turns_generator.cpp

					
				@ -40,1 +32,3 @@

				bool IsHighway(ftypes::HighwayClass hwClass, bool isLink)

				// Min difference of route and alternative turn abs angles in degrees 

				// to ignore this alternative candidate (when alternative road is the same or smaller).

				double constexpr kMinAbsAngleDiffForSameOrSmallerRoad = 35.0;

AntonM030481 (Migrated from github.com) commented

2022-05-05 10:10:54 +00:00

Do we need to keep anonymous namespace? Removed it.

AntonM030481 (Migrated from github.com) reviewed 2022-05-05 10:17:29 +00:00

routing/turns_generator.cpp

					
				@ -1111,189 +1012,225 @@ void GetTurnDirection(IRoutingResult const & result, size_t outgoingSegmentIndex

				      result, outgoingSegmentIndex, numMwmIds, vehicleSettings.m_maxOutgoingPointsCount,

				      vehicleSettings.m_minOutgoingDistMeters, true /* forward */);

AntonM030481 (Migrated from github.com) commented

2022-05-05 10:17:29 +00:00

Can I use organicmaps/organicmaps#2508 (comment)?

Can I use https://git.omaps.dev/organicmaps/organicmaps/pulls/2508#issuecomment-1117040390?

vng (Migrated from github.com) reviewed 2022-05-05 14:07:36 +00:00

routing/turns_generator.cpp

					
				@ -1111,189 +1012,225 @@ void GetTurnDirection(IRoutingResult const & result, size_t outgoingSegmentIndex

				      result, outgoingSegmentIndex, numMwmIds, vehicleSettings.m_maxOutgoingPointsCount,

				      vehicleSettings.m_minOutgoingDistMeters, true /* forward */);

vng (Migrated from github.com) commented

2022-05-05 14:07:36 +00:00

ok

AntonM030481 (Migrated from github.com) reviewed 2022-05-05 15:31:32 +00:00

routing/turns_generator.cpp

					
				@ -1111,189 +1012,225 @@ void GetTurnDirection(IRoutingResult const & result, size_t outgoingSegmentIndex

				      result, outgoingSegmentIndex, numMwmIds, vehicleSettings.m_maxOutgoingPointsCount,

				      vehicleSettings.m_minOutgoingDistMeters, true /* forward */);

AntonM030481 (Migrated from github.com) commented

2022-05-05 15:31:32 +00:00

Changed to
if (!GetTurnInfo(...))

Changed to if (!GetTurnInfo(...))

AntonM030481 commented

2022-05-05 15:59:27 +00:00

(Migrated from github.com)

If we had enough tests for pedestrians i would change MakeTurnAnnotationPedestrian() too.
Now it's almost full copy of MakeTurnAnnotation(). And both are not trivial.

If we had enough tests for pedestrians i would change MakeTurnAnnotationPedestrian() too. Now it's almost full copy of MakeTurnAnnotation(). And both are not trivial.

AntonM030481 (Migrated from github.com) reviewed 2022-05-05 16:39:50 +00:00

routing/turns_generator.cpp

					
				@ -40,1 +32,3 @@

				bool IsHighway(ftypes::HighwayClass hwClass, bool isLink)

				// Min difference of route and alternative turn abs angles in degrees 

				// to ignore this alternative candidate (when alternative road is the same or smaller).

				double constexpr kMinAbsAngleDiffForSameOrSmallerRoad = 35.0;

AntonM030481 (Migrated from github.com) commented

2022-05-05 16:39:50 +00:00

Is it OK?

vng (Migrated from github.com) reviewed 2022-05-05 17:46:24 +00:00

routing/turns_generator.cpp

					
				@ -40,1 +32,3 @@

				bool IsHighway(ftypes::HighwayClass hwClass, bool isLink)

				// Min difference of route and alternative turn abs angles in degrees 

				// to ignore this alternative candidate (when alternative road is the same or smaller).

				double constexpr kMinAbsAngleDiffForSameOrSmallerRoad = 35.0;

vng (Migrated from github.com) commented

2022-05-05 17:46:24 +00:00

ok

vng commented

2022-05-05 17:48:14 +00:00

(Migrated from github.com)

@AntonM030481 I do merge? Or you gonna append the PR?

AntonM030481 commented

2022-05-05 17:56:21 +00:00

(Migrated from github.com)

@vng Merge please.

Let's add more tests next time and continue to improve in next PR, if you don't mind.

@vng Merge please. Let's add more tests next time and continue to improve in next PR, if you don't mind.

AntonM030481 commented

2022-05-05 18:35:05 +00:00

(Migrated from github.com)

@vng Sorry for huge amount of changes.
But I hope now it is more easy to understand and modify.
So next time I suppose changes will be not so dramatic.
Thank you for quick review.

@vng Sorry for huge amount of changes. But I hope now it is more easy to understand and modify. So next time I suppose changes will be not so dramatic. Thank you for quick review.

👍 1 🚀 1

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

No reviewers

biodranik