-
Notifications
You must be signed in to change notification settings - Fork 319
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
Update Default Zoom Level #655
Conversation
956d208
to
4f9b468
Compare
@@ -45,13 +45,11 @@ dependencies { | |||
// Mapbox Android Services | |||
api (dependenciesList.mapboxServices) { | |||
transitive = true | |||
exclude module: 'mapbox-java-geojson' | |||
exclude module: 'mapbox-java-core' | |||
exclude module: 'mapbox-java-services' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to re-exclude these modules to prevent duplicate imports. mapboxServices
is the 2.2.9
version of our services library, which we are using for just telemetry at the moment. mapboxSdkServices
is the 3.0.0
of the library and includes the modules above. Sorry for the confusion on this as we transition these libraries!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops- okay, they are now re-excluded. Thanks for clarifying!
@sarahlensing Thanks for the work on this - this will make it a lot easier to drive a dynamic camera along a route. Just a minor comment regarding re-excluding those dependencies. @ericrwolfe Bumping the zoom by one and keeping the same target distance pushes the puck further into the middle of the map: The way we calculate the target distance based on the Doubling target distance seemed to work pretty well, testing on a 4 different devices with varying screen densities and sizes. For the sake of the zoom bump, this looks like something we can do for now until we figure out a better way to address the target distance. |
4f9b468
to
41fea84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sarahlensing this looks good to go on our end. If you want to write those tests prior to merging, please feel free :)
@ericrwolfe I'll address the target distance adjustments in a new PR (most likely the one in regard to a more dynamic camera)
41fea84
to
1bb595e
Compare
@sarahlensing I'm going to go ahead and merge this, as we are looking to build off of it for the rest of the dynamic camera work. I cut a ticket to track the test integration #678 |
This PR lays the foundation for providing an easy way to customize the camera via a new abstract
Camera
class which can be set withMapboxNavigation#setCameraEngine
. It moves the camera positioning logic fromNavigationCamera
into a new classSimpleCamera
. By default,MapboxNavigation
now uses an instance ofSimpleCamera
to determine how the camera should be position while routing.SimpleCamera
's default zoom values have also been updated so that they are consistent with iOS.Further work can use the properties passed in
RouteInformation
to determine whether the user is starting the route, in the middle of it, or at their destination.Closes #602