Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Electronic horizon #2834

Merged
merged 44 commits into from
Mar 16, 2021
Merged

Electronic horizon #2834

merged 44 commits into from
Mar 16, 2021

Conversation

chezzdev
Copy link
Contributor

@chezzdev chezzdev commented Feb 25, 2021

Description

This PR targets resolving EHorizon v3 integration.

Implementation

In the scope of the full integration, we would like to wrap all the generated NavigationNative APIs with Swift classes as well as provide some helper functions like mpp().

The initial implementation provides access to the main entry objects (GraphAccessor, RoadObjectsStore) via generated bindings through RouteController and PassiveLocationDataSource as well as adds the ability to subscribe to EHorizon update events.

@chezzdev chezzdev marked this pull request as draft February 25, 2021 21:12
@1ec5
Copy link
Contributor

1ec5 commented Feb 25, 2021

This PR is currently based on the bamx23/fix-objc-import branch, which should be unnecessary once the fixes in #2829 are in place.

@1ec5 1ec5 added this to the v2.0.0 (Public Preview) milestone Feb 25, 2021
@1ec5 1ec5 added feature New feature request. topic: location labels Feb 25, 2021
@1ec5 1ec5 mentioned this pull request Mar 2, 2021
@1ec5 1ec5 changed the base branch from bamx23/fix-objc-import to release-v2.0 March 4, 2021 02:32
@truburt truburt modified the milestones: v2.0.0 (Public Preview) (iOS), v2.0.0 (Public Preview) Mar 5, 2021
Comment on lines 6 to 8
Offset from the start of edge (0 - 1) pointing to the beginning of road object on this edge
will be 0 for all edges in the line-like road object expect the very first one
in the case of point-like object percentAlongBegin == percentAlongEnd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this is just the copy-pasted description from RoadObjectEdgeLocation, but I've read it several times and still didn't understand what it has to do with line-like road objects. I believe there are few typos in there :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just a typo: expect = except.

import Foundation
import MapboxNavigationNative

public struct RoadName {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use this struct outside EH context? There are some manipulations with road labeling so maybe we could refactor that later and benefit from this struct in multiple places?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: #2834 (comment). It might even make sense for MapboxDirections to consolidate various RouteStep properties into a public Road type that we could reuse here and elsewhere in MapboxCoreNavigation.

@1ec5

This comment has been minimized.

@1ec5 1ec5 self-assigned this Mar 9, 2021
@1ec5 1ec5 force-pushed the pad-ehorizon branch 4 times, most recently from 4079f0f to c6566c3 Compare March 10, 2021 01:14
@1ec5 1ec5 marked this pull request as ready for review March 10, 2021 01:17
@1ec5 1ec5 changed the title EHorizon v3 Electronic horizon Mar 10, 2021
@1ec5
Copy link
Contributor

1ec5 commented Mar 10, 2021

More to do before this PR is in a minimally landable state:

Some of the review feedback above would also be important to address as tail work immediately after landing:

@1ec5
Copy link
Contributor

1ec5 commented Mar 10, 2021

If you need to install this branch before it lands:

  • For CocoaPods, use the podspecs and Podfile snippet in this gist.
  • For SPM, use the following dependency:
    .package(name: "MapboxNavigation", url: "https://github.com/mapbox/mapbox-navigation-ios.git", .branch("pad-ehorizon"))

1ec5 and others added 20 commits March 15, 2021 15:46
Spelled out the “electronic” in “electronic horizon”. Nested several e-horizon types under other types for less repetitiveness. Removed “electronic horizon” from the name of several types to align with a separate naming convention for things related to the road graph.
Electronic horizon updates come from an internal singleton, so exposing them through delegates of RouteController and PassiveLocationDataSource could result in redundancy or contention. Instead, funnel the updates through the Navigator singleton and post notifications. Flattened some notification-specific classes and structs into notification user info dictionaries.

Consolidated creation of RouteObjectsStore wrapper object.
The name and code are mutually exclusive; an edge identified by both a name and a code would have two RoadNames in its names array.
Renamed ElectronicHorizon.Edge.functionalRoadClass to mapboxStreetsRoadClass for consistency with Intersection.mapboxStreetsRoadClass. Mapbox Streets makes slightly different distinctions than OpenStreetMap’s raw highway tags (which aren’t necessarily strictly functional anyways).
The Navigator singleton is already theoretically responsible for tearing down the e-horizon observer, though in practice that will never happen because it’s a singleton. Regardless, it would be inappropriate for RouteController or PassiveLocationDataSource to tear it down because that’s Navigator’s responsibility.
The result type type merely distinguishes an initial revision from an update. A Boolean-typed key is more explicit. As additional states become necessary, we can update the user info dictionary as necessary.
@1ec5
Copy link
Contributor

1ec5 commented Mar 16, 2021

@SiarheiFedartsou it’d be great if you or someone else familiar with the navigator’s inner workings could comment on these questions at some point:

I’m going to merge this PR in the meantime. There are a number of opportunities to continue refining the API anyways:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request. topic: location
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants