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

2600 offset map center #3523

Closed
wants to merge 6 commits into from

Conversation

lgeromegnace
Copy link

This pull-request is linked to #3497. This is a WIP so please DO NOT MERGE.

Larry Geromegnace added 6 commits December 30, 2015 11:41
…ransform and TransformState classes to handle the update of insets as an state update.

The insets are taken into account in the map center conversion (LatLng => x,y) if no gesture are taking place.
…mpute center according to insets.

Add in NativeMapView header setInsets API.
[ios/android] Add debug view to visualize insets.
[core] Add setPitching API to let TransformState be aware of pitching state.
TransformState : Apply insets on projection matrix only while pitching gesture occurs.
Transform.easeTo : Apply insets only no gesture occurs.
@1ec5 1ec5 added feature ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold iOS Mapbox Maps SDK for iOS Android Mapbox Maps SDK for Android labels Jan 12, 2016
@1ec5
Copy link
Contributor

1ec5 commented Jan 13, 2016

Thank you very much for this work. It’ll help a great deal in coming up with a solution that works on every platform for #2600. One of the challenges as I clean up Transform and TransformState will be to account for frontends that don’t take advantage of Transform’s built-in animation capabilities, instead calling the non-animated methods repeatedly using a platform-provided animation facility. To hand-wave for a moment, it’d be really cool to let UIKit Dynamics or Core Animation drive custom animations this way.

I’m curious what led to the decision to maintain the insets as a field on TransformState. While working on #3497, I was thinking that Transform’s methods are used in too many ways for there to be a good sense of when to automatically apply insets. The gestureInProgress state appears to have been a first step towards making Transform more aware of the caller’s intent, but I’m not sure that it’s even being used the way it was originally intended anymore.

A conservative first step would be to just have the platform-specific code (MGLMapView in the case of iOS) keep track of the view insets and pass them into Transform methods as an option in CameraOptions. MGLMapView has all the context it needs to determine whether to apply the inset as well as to report the shifted center coordinate to client code when appropriate.

I see you’ve thought about tilting too. What’s the expected interaction for a tilted map? mbgl uses a vertical field of view based on the view’s height. Perhaps the axis of tilt would be shifted downward, but I’m wary of introducing an additional degree of freedom within the scope of this feature. (It would be awesome to have that feature, though!)

@1ec5 1ec5 mentioned this pull request Jan 13, 2016
@lgeromegnace
Copy link
Author

Hi @1ec5,

Thanks a lot for your feedback !

Your are right about keeping track of insets in CameraOptions. What led me to add them in TransformState was the fact that in Map there are others API like MoveBy that does not use the CameraOptions and also at some point of my attempts I needed the insets in TransformState. Howover I think it is okay to ignore the first reason I mentioned since these API are not exposed in MapView.

So with insets in CameraOptions I have a map that can apply insets to center since it is not "pitched".

I saw you just finished your refactor in #3497. I'll have a look on it and I think I will plug this solution on it.

For the tilted map and insets I'll see if I can figure it out but as you said maybe this should be addressed in another PR.

@1ec5
Copy link
Contributor

1ec5 commented Jan 15, 2016

@lgeromegnace, some good news and bad news. I see you recently pushed https://github.com/Mappy/mapbox-gl-native/tree/2600-insets. The bad news is that we had an internal project that needed insets right away on OS X, so I had to step on your toes a bit and start implementing insets in https://github.com/mapbox/mapbox-gl-native/tree/1ec5-padding-2600. The good news is that you and I chose almost identical approaches at the core level, and since you’re working on the iOS SDK and I’m working on the OS X SDK, conflicts between our branches should be minimal. I’d be happy to handle reconciling the two branches, if you’d like.

Apologies for any work duplication here. I really like what you’re doing on the iOS side though. 👍 Are you planning to work on shifting the center during user tracking mode also? (That was the original motivation behind #2600.)

@lgeromegnace
Copy link
Author

@1ec5, no need to apologize. On the contrary I'm happy to have you on this field 👍. I like the work you started, moveLatLng in TransformState is very interesting, I missed it from your previous refactor. For the conflicts I assume you will be faster than me to handle them. However if you have any questions feel free to ask.

About shifting the center during user tracking mode : yes we have to apply inset automatically. So we expect to have the user position centered in the bounds defined by the insets.

@tobrun
Copy link
Member

tobrun commented Feb 22, 2016

@lgeromegnace
I believe this has landed for both iOS and Android.
Can we close this?

@lgeromegnace
Copy link
Author

@tobrun you're right. Thanks for the reminder.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold feature iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants