Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Fix rotation edge cases and compass overspinning #1829

Merged
merged 4 commits into from
Jul 6, 2015
Merged

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Jul 2, 2015

This PR addresses some edge cases around bearing, obsoleting #1558:

  • mbgl::util::wrap() is now min-inclusive, max-exclusive. It is the responsibility of the caller to handle the case in which min is returned. Previously, wrap(360, 0, 360) would return 360 but wrap(720, 0, 360) would return 0. This is how mapbox-gl-js behaves, but I believe the max-exclusive behavior is more predictable.
  • -[MGLMapView direction] now returns a value in the range [0, 360) instead of [0, 360].
  • When pressing the compass to reset the direction to due north, the compass’ animation now begins at the current rotation. Previously, it began at due north and spun 360° every time, due to an overzealous response to change notifications. (ref fix start/finish rendering map delegate callbacks #1431, Always drive rendering from the main thread #1624, 6532438)
  • For consistency, during drift rotation, changes to the compass’ rotation are driven by mbgl animation rather than view-based animation.

/cc @mourner @friedbunny @kkaefer

@1ec5 1ec5 added bug iOS Mapbox Maps SDK for iOS labels Jul 2, 2015
@1ec5 1ec5 added this to the iOS Beta 3 milestone Jul 2, 2015
@1ec5 1ec5 self-assigned this Jul 2, 2015
@jfirebaugh
Copy link
Contributor

👍

Want to port the wrap change to gl-js too?

@friedbunny
Copy link
Contributor

It's still possible to trigger #1558 by doing a sharp-rotate across 180º, but only going right-to-left.

@friedbunny
Copy link
Contributor

Also, when tapping to reset to north, the compass does a strange little jaunt in the opposite direction before correcting to north.

@1ec5
Copy link
Contributor Author

1ec5 commented Jul 2, 2015

Want to port the wrap change to gl-js too?

Sure, once I’m reasonably confident about this change. wrap() has more callers in gl-js, but I think I understand the intent of each one.

It's still possible to trigger #1558 by doing a sharp-rotate across 180º, but only going right-to-left.

Does 2ccb4e94f4936b6be1ffefb6c6ef0de48a6a1664 do the trick?

Also, when tapping to reset to north, the compass does a strange little jaunt in the opposite direction before correcting to north.

Happens in master too. Turn on Slow Animations (⌘T) in the Simulator to see it.

@1ec5
Copy link
Contributor Author

1ec5 commented Jul 3, 2015

4936f8637e12967a3046a7716aa3c598e966b968 fixes the compass overspinning.

@1ec5 1ec5 changed the title Normalize bearing edge cases Fix rotation edge cases and compass overspinning Jul 3, 2015
@1ec5
Copy link
Contributor Author

1ec5 commented Jul 3, 2015

As I pointed out in #1582 (comment), we need to be careful about updating Cocoa-side state in response to model/mbgl changes. Something like 4936f8637e12967a3046a7716aa3c598e966b968 will be required for #1582 as well.

@friedbunny
Copy link
Contributor

Does 2ccb4e9 do the trick?

Nope, though the behavior has changed. Sharply rotating over 180º from either direction will get the compass stuck on the screen upon tapping, but doing it again the reverse direction will unstick it. 😁

I'll fiddle around with this over the long weekend.

@1ec5
Copy link
Contributor Author

1ec5 commented Jul 6, 2015

Look good to you, @friedbunny?

@friedbunny
Copy link
Contributor

giphy

1ec5 added 4 commits July 6, 2015 13:07
Fixed a bug in which -[MGLMapView direction] could return 360 instead of 0. Switched to mbgl::util::wrap() for clarity. -updateCompass now relies on -direction to avoid code duplication.
If (wrap] is desired, it is the responsibility of the caller to handle the case in which min is returned.
Rely on mbgl to drive compass rotation for simplicity. Eliminated fall-through from mbgl::MapChangeRegionIsChanging to mbgl::MapChangeRegionDidChange, because is-changing should not cause did-change to be fired.
Wrap the angle on each interpolated step instead of when normalizing.
@1ec5 1ec5 force-pushed the 1ec5-compass-wrap branch from db47eb1 to 08acb65 Compare July 6, 2015 20:09
@1ec5 1ec5 merged commit 08acb65 into master Jul 6, 2015
@1ec5 1ec5 removed the in progress label Jul 6, 2015
@1ec5 1ec5 mentioned this pull request Jul 6, 2015
@1ec5 1ec5 deleted the 1ec5-compass-wrap branch July 6, 2015 20:41
@1ec5
Copy link
Contributor Author

1ec5 commented Jul 6, 2015

Want to port the wrap change to gl-js too?

mapbox/mapbox-gl-js#1363

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants