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

Remove viewport workaround in CarPlay #2134

Merged
merged 6 commits into from Jun 27, 2019
Merged

Remove viewport workaround in CarPlay #2134

merged 6 commits into from Jun 27, 2019

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented May 23, 2019

Now that mapbox/mapbox-gl-native#14664 gained traction, I took the chance to remove the hacky workaround for the viewport in CarPlay and the NavigationMapView.

This PR is blocked by the PR mentioned above, and a new release of the Maps SDK. As well as some cleanup work and testing.

This PR will unblock #1139

Fixes #2134.

cc @mapbox/navigation-ios @astojilj

@frederoni frederoni added - performance CarPlay Bugs, improvements and feature requests on Apple CarPlay blocked Blocked by dependency or unclarity. ⚠️ DO NOT MERGE PR should not be merged! labels May 23, 2019
@astojilj
Copy link
Contributor

@frederoni Thanks. Looks good - simpler, though I'm not familiar with the code to properly review it.
It would be interesting to see how it affects performance - the viewport area should be now 2 times smaller:
from the screenshot looks like it was (1.5 * width + 100pixels) x (1.5 * height + 100pixels), width and height and 100px are in logical coordinates.

100 pixels comes from CollisionIndex viewportPadding that we could also try to tweak.

@1ec5
Copy link
Contributor

1ec5 commented May 23, 2019

Awesome – so glad this is becoming a reality!

I’m not sure why, but the vanishing point is offset slightly too far to the right, or the puck is offset slightly too far to the left. The puck’s arrow and the straightaway along the route should look like they point directly upward, tangent to the top edge of the screen.

@frederoni
Copy link
Contributor Author

frederoni commented May 24, 2019

I’m not sure why, but the vanishing point is offset slightly too far to the right, or the puck is offset slightly too far to the left.

Yea, the userAnchorPoint doesn't exactly match the safeArea’s centerY which results in the viewpoint’s X position being out of sync every minor turn, so there is a small discrepancy that'll need to be addressed before this PR can land. Good pixelhunting 👁

@1ec5
Copy link
Contributor

1ec5 commented Jun 19, 2019

mapbox/mapbox-gl-native#14664 is shipping today as part of map SDK v5.1.0. Since the bug fix isn’t shipping as a semver-major change, the workaround needs to be removed to avoid overcorrecting the vanishing point.

@1ec5
Copy link
Contributor

1ec5 commented Jun 19, 2019

#2143 prevents this SDK from working with v5.1.0 for the time being, but we need to remove that restriction and depend on v5.1.x in short order to avoid blocking various applications from upgrading the map SDK.

@1ec5 1ec5 removed blocked Blocked by dependency or unclarity. blocked: upstream ⚠️ DO NOT MERGE PR should not be merged! labels Jun 20, 2019
@1ec5 1ec5 added this to the v0.35.0 milestone Jun 20, 2019
@1ec5 1ec5 self-assigned this Jun 26, 2019
@@ -337,10 +337,7 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate {
if tracksUserCourse {
let newCamera = camera ?? MGLMapCamera(lookingAtCenter: location.coordinate, altitude: altitude, pitch: 45, heading: location.course)
let function: CAMediaTimingFunction? = animated ? CAMediaTimingFunction(name: .linear) : nil
let point = userAnchorPoint
let padding = UIEdgeInsets(top: point.y, left: point.x, bottom: bounds.height - point.y, right: bounds.width - point.x)
// Omit padding when https://github.com/mapbox/mapbox-gl-native/pull/14081 has landed
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment was a bit misleading: the actual workarounds were the paddings added in showcase(_:padding:animated:) and fit(to:facing:padding:animated:), not the one here. The edge padding here is necessary in order to account for the puck’s uncentered location even within the content frame.

/ref #2145 (comment)

@1ec5
Copy link
Contributor

1ec5 commented Jun 27, 2019

These assertions are failing and crashing on iOS 12 for some odd reason:

XCTAssertEqual(Int(round(durationBetweenDepartAndArrive)), 1041)
XCTAssertEqual(Int(round(durationBetweenDepartAndReroute)), 225)
XCTAssertEqual(Int(round(durationBetweenRerouteAndArrive)), 816)

@1ec5 1ec5 merged commit 20cc7a6 into master Jun 27, 2019
@1ec5 1ec5 deleted the simplify-viewport branch June 27, 2019 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CarPlay Bugs, improvements and feature requests on Apple CarPlay
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants