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

Throttle map rendering when device unplugged #370

Closed
ericrwolfe opened this issue Jul 12, 2017 · 11 comments · Fixed by #402
Closed

Throttle map rendering when device unplugged #370

ericrwolfe opened this issue Jul 12, 2017 · 11 comments · Fixed by #402
Assignees
Milestone

Comments

@ericrwolfe
Copy link
Contributor

Right now, rendering 60fps animations between location updates is taxing on the device + battery. When the device is unplugged, we should not animate between location updates.

cc @bsudekum @1ec5

@1ec5
Copy link
Contributor

1ec5 commented Jul 19, 2017

This requires #352, because the course tracking animation built into MGLMapView can’t be disabled.

@ericrwolfe
Copy link
Contributor Author

This also depends on a fix for the general battery monitoring crash: #371

@ericrwolfe ericrwolfe modified the milestones: v0.6.0-2, v0.6.0-1 Jul 21, 2017
@ericrwolfe
Copy link
Contributor Author

@1ec5 Is this something we could achieve in a first pass without a complete overhaul of course tracking, for example with a category on MGLMapView?

@ericrwolfe ericrwolfe removed this from the v0.6.0-2 milestone Jul 27, 2017
@1ec5
Copy link
Contributor

1ec5 commented Jul 27, 2017

Unfortunately not without changes to the map SDK proper or a risky hack. The CADisplayLink frame rate is determined by a constant. We could swizzle -[MGLMapView didUpdateLocationIncrementallyAnimated:], but I’m not a fan of that approach.

@ericrwolfe
Copy link
Contributor Author

Yeah, overriding -[MGLMapView didUpdateLocationIncrementallyAnimated:] is along the lines of what I was thinking. Could we override with a category or in the NavigationMapView subclass without needing to swizzle and break instances outside of the navigation UI?

@1ec5
Copy link
Contributor

1ec5 commented Jul 28, 2017

I agree that swizzling would be a bad idea for the reason that it would break non-NavigationMapView instances of MGLMapView. A category method override would be nondeterministic, right? Overriding in NavigationMapView would be safer, although -_setCenterCoordinate:edgePadding:zoomLevel:direction:duration:animationTimingFunction:completionHandler:, -edgePaddingForFollowing, and -directionByFollowingWithCourse are also internal, so they would need to be forward-declared as well.

@frederoni
Copy link
Contributor

frederoni commented Jul 28, 2017

This is being implemented as part of #402 but it's blocked upstream by some camera refinements.

@1ec5
Copy link
Contributor

1ec5 commented Jul 28, 2017

By the way, if we need a tightly-scoped fix for a patch release, we can override -_setCenterCoordinate:edgePadding:zoomLevel:direction:duration:animationTimingFunction:completionHandler: in a separate PR while #402 bakes.

@frederoni
Copy link
Contributor

Unfortunately, _setCenterCoordinate:edgePadding:zoomLevel:direction:duration:animationTimingFunction:completionHandler: doesn't take pitch into account so when the device is plugged in and we're using animations, setCenter or pitch will cancel the previous transition.

@ericrwolfe
Copy link
Contributor Author

I think we should continue to consider throttling when the device is unplugged, though from tests anything that involves a significant change in course (e.g. turns) looks pretty bad without animations.

Could we disable animations if the device is unplugged and the change in course/heading is small (i.e. <10-15°), otherwise animate between location updates?

@ericrwolfe
Copy link
Contributor Author

Reopening as a follow on to #402.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants