organicmaps/organicmaps-tmp

Archived

Fork 1

[Navigation] Refactoring of CarDirectionsEngine and PedestrianDirectionsEngine #2547

Merged

AntonM030481 merged 11 commits from navigation-refactoring into master

2022-05-16 16:29:59 +00:00

AntonM030481 commented

2022-05-12 16:25:12 +00:00

(Migrated from github.com)

Reason:
A lot of duplicated code which is not easy to read.
Results are not easy to understand.

Preparation by updating pedestrian turn tests.
Make functions the same, but hold different names. Line by line comparison can be seen here.
Remove MakeTurnAnnotationPedestrian.
Added comments for input and output data structures.

Logical continuation is changing of CarDirectionsEngine and PedestrianDirectionsEngine (childs of DirectionsEngine).
Currently the only method they have - almost identical Generate().

Suggested changes:
MakeTurnAnnotation() - becomes method of DirectionsEngine.
All VehicleType dependent logic is incapsulated in virtual methods GetTurnDirection(), FixupTurns().
Existing functions become methods of CarDirectionsEngine and PedestrianDirectionsEngine.

If this next step is OK, I will make changes in this PR.

Reason: A lot of duplicated code which is not easy to read. Results are not easy to understand. 1. Preparation by updating pedestrian turn tests. 2. Make functions the same, but hold different names. Line by line comparison can be seen here. 3. Remove MakeTurnAnnotationPedestrian. 4. Added comments for input and output data structures. Logical continuation is changing of CarDirectionsEngine and PedestrianDirectionsEngine (childs of DirectionsEngine). Currently the only method they have - almost identical Generate(). Suggested changes: MakeTurnAnnotation() - becomes method of DirectionsEngine. All VehicleType dependent logic is incapsulated in virtual methods GetTurnDirection(), FixupTurns(). Existing functions become methods of CarDirectionsEngine and PedestrianDirectionsEngine. If this next step is OK, I will make changes in this PR.

👍 1 🚀 1

vng commented

2022-05-12 17:04:28 +00:00

(Migrated from github.com)

@AntonM030481 You are doing great, so do not hesitate to refactor according to your vision of a better code.

AntonM030481 commented

2022-05-13 17:02:12 +00:00

(Migrated from github.com)

No further changes are planned. Ready for review.

vng commented

2022-05-13 17:31:34 +00:00

(Migrated from github.com)

Is it necessary to update icu and boost here?

👍 1

vng (Migrated from github.com) reviewed 2022-05-13 18:09:49 +00:00

routing/car_directions.cpp

vng (Migrated from github.com) commented

2022-05-13 17:40:07 +00:00

Sure that junctions is not empty here?

vng (Migrated from github.com) commented

2022-05-13 17:48:02 +00:00

So if validFirstOutgoingSeg == false, we can return false immediately before this loop.
Or there is an error here in logic ..

So if validFirstOutgoingSeg == false, we can return false immediately before this loop. Or there is an error here in logic ..

vng (Migrated from github.com) commented

2022-05-13 17:50:52 +00:00

Should we take out named constant = 70?

routing/directions_engine.cpp

vng (Migrated from github.com) commented

2022-05-13 18:00:53 +00:00

Can assign directly without intermediate outgoingSegmentDist here

routing/turns_generator.cpp

vng (Migrated from github.com) commented

2022-05-13 18:07:55 +00:00

Split on 2 lines

vng (Migrated from github.com) commented

2022-05-13 18:08:56 +00:00

Split like normal block:
{
...
};

Split like normal block: { ... };

AntonM030481 commented

2022-05-16 07:49:01 +00:00

(Migrated from github.com)

Reset + push --force to remove accidental icu and boost changes.

AntonM030481 (Migrated from github.com) reviewed 2022-05-16 08:24:12 +00:00

routing/car_directions.cpp