WIP: Helicopter navigation #4917

strump · 2023-04-04T15:03:13Z

strump commented

2023-04-04 15:03:13 +00:00

Added new type of navigation. It could be used as a ruller (maybe rename it to "Ruller").

How to use: select point, start navigation to the point, pick Helicopter icon.

Video:

https://user-images.githubusercontent.com/720808/229831475-470db2dd-2e1c-4e77-886b-b05ffd7ff6d3.mp4

There is no navigation along the route. There's no elevation info.
No need to download intermediate maps to build a route:

Android routing UI changes

If you need to build a route with multiple points: Home → School → Coffee → Work. With current UI user needs to select Home as start then select Work as finish point and then select School and Coffee as intermediate points. I added new button which continues route to selected point instead of replaceing finish point. So user can pick Home as start and School as finish then Coffee as new finish and Work as final finish.

On UI it looks like dropdown menu on "Route to" button with two options: old one "Route to" button and new "Continue route to". Selected option is saved in preferences.

Known issues:

Issue 1:

Line is drawn below buildings:

Issue 2 [Android]:

New dropdown menu icon is shifted:

Issue 3 [iOS]:

We don't need to show time in bottom bar for Helicopter navigation.

Issue 4:

Lack of unit tests.

Update 2023-04-19

Removed two-state button for next/last route stop. Now old and new buttons are visible on the bottom row.

https://user-images.githubusercontent.com/720808/233161618-6c08d4af-a2a8-4d12-a878-b3d057a402b0.mp4

Added new type of navigation. It could be used as a ruller (maybe rename it to "Ruller"). How to use: select point, start navigation to the point, pick Helicopter icon. <img width="250" alt="iphone-route-2" src="https://user-images.githubusercontent.com/720808/229834815-0c29f835-5d1b-4ac9-b54f-515e87214738.png"> Video: https://user-images.githubusercontent.com/720808/229831475-470db2dd-2e1c-4e77-886b-b05ffd7ff6d3.mp4 There is no navigation along the route. There's no elevation info. No need to download intermediate maps to build a route: <img src="https://user-images.githubusercontent.com/720808/229825836-ca4b33e7-5862-42de-b7f2-b0cb05997a55.jpg" width="300px" alt="Across countries"/> ## Android routing UI changes If you need to build a route with multiple points: Home → School → Coffee → Work. With current UI user needs to select Home as start then select Work as finish point and then select School and Coffee as intermediate points. I added new button which continues route to selected point instead of replaceing finish point. So user can pick Home as start and School as finish then Coffee as new finish and Work as final finish. On UI it looks like dropdown menu on "Route to" button with two options: old one "Route to" button and new "Continue route to". Selected option is saved in preferences. <img src="https://user-images.githubusercontent.com/720808/229827092-622fa364-b3da-490a-8cdc-e8d9bd6a6619.jpg" width="300px"/> ## Known issues: ### Issue 1: Line is drawn below buildings: <img src="https://user-images.githubusercontent.com/720808/229825606-e15081f4-1360-4777-9301-73659e226007.png" width="300px"/> ### Issue 2 [Android]: New dropdown menu icon is shifted: <img src="https://user-images.githubusercontent.com/720808/229826423-c61b0563-8528-4023-b2ec-0024217d803c.jpg" width="300px"/> ### Issue 3 [iOS]: We don't need to show time in bottom bar for Helicopter navigation. ### Issue 4: Lack of unit tests. ## Update 2023-04-19 Removed two-state button for next/last route stop. Now old and new buttons are visible on the bottom row. https://user-images.githubusercontent.com/720808/233161618-6c08d4af-a2a8-4d12-a878-b3d057a402b0.mp4

👍 6

strump reviewed 2023-04-04 15:31:16 +00:00

routing/async_router.cpp

					
				@ -374,2 +374,3 @@

				  bool const needAbsentRegions = (code != RouterResultCode::Cancelled);

				  bool const needAbsentRegions = (code != RouterResultCode::Cancelled &&

				          route->GetRouterId() != "helicopter-router");

strump commented

2023-04-04 15:31:04 +00:00

How to replace string comparison here?

biodranik (Migrated from github.com) reviewed 2023-04-05 16:34:54 +00:00

routing/async_router.cpp

					
				@ -374,2 +374,3 @@

				  bool const needAbsentRegions = (code != RouterResultCode::Cancelled);

				  bool const needAbsentRegions = (code != RouterResultCode::Cancelled &&

				          route->GetRouterId() != "helicopter-router");

biodranik (Migrated from github.com) commented

2023-04-05 16:34:54 +00:00

That line is not called too often, so should be ok. I would also try to reuse a router-id constexpr constant instead of copy-pasting a string. Or maybe add a method like route->IsHelicopterRouter()

biodranik (Migrated from github.com) reviewed 2023-04-05 17:03:11 +00:00

biodranik (Migrated from github.com) left a comment

Good start!

android/jni/app/organicmaps/Framework.cpp

					
				@ -1451,0 +1459,4 @@

				  data.m_title = jni::ToNativeString(env, title);

				  data.m_subTitle = jni::ToNativeString(env, subtitle);

				  data.m_pointType = RouteMarkType::Finish;

				  data.m_intermediateIndex = static_cast<size_t>(intermediateIndex);

biodranik (Migrated from github.com) commented

2023-04-05 16:40:00 +00:00

Почему-то поплыло форматирование здесь и в других местах...

android/src/app/organicmaps/routing/RoutingBottomMenuController.java

biodranik (Migrated from github.com) commented

2023-04-05 16:43:01 +00:00

    if (showTime)
    {

```suggestion if (showTime) { ```

biodranik (Migrated from github.com) commented

2023-04-05 16:43:22 +00:00

      final String dot = "\u00A0• ";

```suggestion final String dot = "\u00A0• "; ```

android/src/app/organicmaps/routing/RoutingBottomMenuController.java

biodranik (Migrated from github.com) commented

2023-04-05 16:42:32 +00:00

В 120 символов не помещается?

android/src/app/organicmaps/routing/RoutingController.java

biodranik (Migrated from github.com) commented

2023-04-05 16:43:57 +00:00

    if (point.sameAs(startPoint))
    {
      if (endPoint == null)
      {

```suggestion if (point.sameAs(startPoint)) { if (endPoint == null) { ```

android/src/app/organicmaps/routing/RoutingController.java

biodranik (Migrated from github.com) commented

2023-04-05 16:44:12 +00:00

nit: use final everywhere.

nit: use `final` everywhere.

android/src/app/organicmaps/routing/RoutingPlanController.java

biodranik (Migrated from github.com) commented

2023-04-05 16:44:59 +00:00

        throw new IllegalArgumentException("unknown router: " + router);

```suggestion throw new IllegalArgumentException("unknown router: " + router); ```

android/src/app/organicmaps/widget/placepage/PlacePageButtons.java

biodranik (Migrated from github.com) commented

2023-04-05 16:45:37 +00:00

      if (pickedButton < 0 || pickedButton>=types.size())

```suggestion if (pickedButton < 0 || pickedButton>=types.size()) ```

biodranik (Migrated from github.com) commented

2023-04-05 16:46:19 +00:00

    for (int i=0; i < types.size(); i++)

```suggestion for (int i=0; i < types.size(); i++) ```

android/src/app/organicmaps/widget/placepage/PlacePageButtons.java

biodranik (Migrated from github.com) commented

2023-04-05 16:48:28 +00:00

Should we separately rename the existing "ROUTE_ADD" to "ROUTE_INSERT" to avoid confusion in the future?

map/routing_manager.cpp

biodranik (Migrated from github.com) commented

2023-04-05 16:50:21 +00:00

Transit?

map/routing_manager.cpp

biodranik (Migrated from github.com) commented

2023-04-05 16:51:18 +00:00

nit: Clearer to make if (type == ...) comparison.

map/routing_manager.cpp

biodranik (Migrated from github.com) commented

2023-04-05 16:51:47 +00:00

          subroute->m_tailFakeDistance = 90.0f; //Assuming that Helicopter line is shorter than 90°. Otherwise line tail would have a gray color.

```suggestion subroute->m_tailFakeDistance = 90.0f; //Assuming that Helicopter line is shorter than 90°. Otherwise line tail would have a gray color. ```

biodranik (Migrated from github.com) commented

2023-04-05 16:52:06 +00:00

Что там за серая линия? Откуда она берётся?

map/routing_manager.cpp

biodranik (Migrated from github.com) commented

2023-04-05 16:52:18 +00:00

  // Finish point is now Intermediate point

```suggestion // Finish point is now Intermediate point ```

biodranik (Migrated from github.com) commented

2023-04-05 16:52:42 +00:00

    if (mark)

```suggestion if (mark) ```

routing/async_router.cpp

biodranik (Migrated from github.com) commented

2023-04-05 16:53:19 +00:00

Для чего это?

routing/helicopter_router.cpp

biodranik (Migrated from github.com) commented

2023-04-05 16:55:23 +00:00

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

  vector<m2::PointD> const & points = checkpoints.GetPoints();

Не нужно создавать лишнюю копию вектора. ```suggestion vector<m2::PointD> const & points = checkpoints.GetPoints(); ```

biodranik (Migrated from github.com) commented

2023-04-05 16:58:59 +00:00

  Segment const segment(kFakeNumMwmId, 0, 0, false);
  for (size_t i = 1; i < points.size(); ++i)
  {
    turns::TurnItem turn;
    turn.m_index = i;
    if (i == points.size() - 1)
      turn.m_pedestrianTurn = turns::PedestrianDirection::ReachedYourDestination;
    geometry::PointWithAltitude const junction(points[i], mockAltitude);
    RouteSegment::RoadNameInfo const roadNameInfo;

    auto routeSegment = RouteSegment(segment, turn, junction, roadNameInfo);
    routeSegments.push_back(move(routeSegment));
  }

```suggestion Segment const segment(kFakeNumMwmId, 0, 0, false); for (size_t i = 1; i < points.size(); ++i) { turns::TurnItem turn; turn.m_index = i; if (i == points.size() - 1) turn.m_pedestrianTurn = turns::PedestrianDirection::ReachedYourDestination; geometry::PointWithAltitude const junction(points[i], mockAltitude); RouteSegment::RoadNameInfo const roadNameInfo; auto routeSegment = RouteSegment(segment, turn, junction, roadNameInfo); routeSegments.push_back(move(routeSegment)); } ```

biodranik (Migrated from github.com) commented

2023-04-05 16:59:47 +00:00

  for(size_t i = 1; i < points.size(); ++i)
  {
    auto subrt = Route::SubrouteAttrs(geometry::PointWithAltitude(points[i - 1], mockAltitude),
                                      geometry::PointWithAltitude(points[i], mockAltitude), i - 1, i);

```suggestion for(size_t i = 1; i < points.size(); ++i) { auto subrt = Route::SubrouteAttrs(geometry::PointWithAltitude(points[i - 1], mockAltitude), geometry::PointWithAltitude(points[i], mockAltitude), i - 1, i); ```

biodranik (Migrated from github.com) commented

2023-04-05 17:00:00 +00:00

indent

biodranik (Migrated from github.com) commented

2023-04-05 17:00:47 +00:00

}  // namespace routing

```suggestion } // namespace routing ```

routing/helicopter_router.cpp

					
				@ -0,0 +46,4 @@

				 */

				RouterResultCode HelicopterRouter::CalculateRoute(Checkpoints const & checkpoints,

				                                             m2::PointD const & startDirection,

biodranik (Migrated from github.com) commented

2023-04-05 16:53:40 +00:00

Выравнивание?

routing/helicopter_router.cpp

					
				@ -0,0 +132,4 @@

				                                                   m2::PointD const & direction, double radius,

				                                                   EdgeProj & proj)

				{

				  //TODO

biodranik (Migrated from github.com) commented

2023-04-05 17:00:16 +00:00

Is it really needed? When is it used?

routing/helicopter_router.hpp

biodranik (Migrated from github.com) commented

2023-04-05 17:01:16 +00:00

  std::string GetName() const override { return {"helicopter-router"}; }

```suggestion std::string GetName() const override { return {"helicopter-router"}; } ```

biodranik (Migrated from github.com) commented

2023-04-05 17:01:46 +00:00

}  // namespace routing

```suggestion } // namespace routing ```

routing/helicopter_router.hpp

					
				@ -0,0 +17,4 @@

				  bool FindClosestProjectionToRoad(m2::PointD const & point, m2::PointD const & direction,

				                                   double radius, EdgeProj & proj) override;

				  // Do we need guides in this router?

biodranik (Migrated from github.com) commented

2023-04-05 17:01:38 +00:00

@vng это о чём?

routing/routing_session.cpp

biodranik (Migrated from github.com) commented

2023-04-05 17:02:25 +00:00

Can we avoid copy-paste?

biodranik (Migrated from github.com) commented

2023-04-05 17:03:00 +00:00

@vng can you please help? What's going on here?

strump reviewed 2023-04-07 19:02:19 +00:00

map/routing_manager.cpp

strump commented

2023-04-07 19:02:19 +00:00

I didn't create new VehicleType 'cause it would require MWM rebuild.

I didn't create new `VehicleType` 'cause it would require MWM rebuild.

biodranik (Migrated from github.com) reviewed 2023-04-08 13:05:44 +00:00

map/routing_manager.cpp

biodranik (Migrated from github.com) commented

2023-04-08 13:05:44 +00:00

@vng can you please advise here? And also review the code?

RedAuburn reviewed 2023-04-08 23:14:57 +00:00

android/src/app/organicmaps/editor/EditorFragment.java

RedAuburn commented

2023-04-08 23:14:56 +00:00

There is no icon_up view in the editor fragment, so this line causes the app to crash when try to edit a POI, or when you try to add a new POI. Is this line needed for something? It works fine with it commented out :)

strump reviewed 2023-04-21 11:21:06 +00:00

map/routing_manager.cpp

strump commented

2023-04-21 11:21:06 +00:00

If I add new VehicleType and update VehicleType::Count from 4 to 5, then in road_access_serialization.hpp:161 OM will try to read one more section from WMW and will fail. Any new VehicleType requires maps regeneration.

If I add new `VehicleType` and update `VehicleType::Count` from 4 to 5, then in [road_access_serialization.hpp:161](https://github.com/organicmaps/organicmaps/blob/master/routing/road_access_serialization.hpp#L161) OM will try to read one more section from WMW and will fail. Any new `VehicleType` requires maps regeneration.

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

No reviewers