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

Unwrap LatLngBounds for the shortest path when requesting camera #11759

Merged
merged 1 commit into from
Apr 24, 2018

Conversation

LukasPaczos
Copy link
Contributor

Closes #11733.
Refs. #11758.

@LukasPaczos LukasPaczos added the Android Mapbox Maps SDK for Android label Apr 23, 2018
@LukasPaczos LukasPaczos added this to the android-v6.1.0 milestone Apr 23, 2018
@LukasPaczos LukasPaczos requested review from tobrun and osana April 23, 2018 14:26
@LukasPaczos LukasPaczos force-pushed the 11733-bounds-crossing-idl branch from 534d1a2 to 85a8cf7 Compare April 23, 2018 14:26
@@ -28,7 +28,7 @@
/**
* Construct a new LatLngBounds based on its corners, given in NESW
* order.
*
* <p>
Copy link
Member

Choose a reason for hiding this comment

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

is this javadoc intended?

@@ -278,12 +300,12 @@ static LatLngBounds fromLatLngs(final List<? extends ILatLng> latLngs) {

/**
* Constructs a LatLngBounds from doubles representing a LatLng pair.
*
* <p>
Copy link
Member

Choose a reason for hiding this comment

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

same as above

* This values of latNorth and latSouth should be in the range of [-90, 90],
* see {@link GeometryConstants#MIN_LATITUDE} and {@link GeometryConstants#MAX_LATITUDE},
* otherwise IllegalArgumentException will be thrown.
* latNorth should be greater or equal latSouth, otherwise IllegalArgumentException will be thrown.
*
* <p>
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@LukasPaczos LukasPaczos force-pushed the 11733-bounds-crossing-idl branch from 85a8cf7 to e9771fb Compare April 24, 2018 08:35
@LukasPaczos LukasPaczos force-pushed the 11733-bounds-crossing-idl branch from e9771fb to ded1d89 Compare April 24, 2018 08:46
@LukasPaczos LukasPaczos changed the base branch from release-boba to master April 24, 2018 09:02
@LukasPaczos LukasPaczos merged commit eb39c80 into master Apr 24, 2018
@LukasPaczos LukasPaczos deleted the 11733-bounds-crossing-idl branch April 24, 2018 09:31
@osana
Copy link
Contributor

osana commented Apr 24, 2018

I think it might be cleaner to have two versions of LatLngBounds:

LatLongBoundsUnwrapped :
east >= west (always) , longitude can be any value
that is what maps-gl-native apis expect

LatLongBound (wrapped):
longitude is between -180 and 180, east < west when crossing IDL
that is what Android users expect

At the moment we try to mix the two concepts together

cc @tobrun, @LukasPaczos

@LukasPaczos
Copy link
Contributor Author

To make the LatLngBounds class consistent, I'd rather aim for the LatLngBounds#Builder and LatLngBounds#from to return wrapped values (as the Android users expect) and we can unwrap them with LatLngBounds#unwrapBounds util method introduced in this PR before we push it to the other APIs. This also lets user unwrap them if that's needed for any internal operations. I feel like introducing another object is too much here.
What do you think @osana?

@osana
Copy link
Contributor

osana commented Apr 25, 2018

I can live with it but here is what bothers me most:
unwrapBound() being a public method (I understand that it is that way to use if from MapboxMap)

That means that even though we are saying that our LatLngBounds do not wrap and all values will be stored as wrapped,

we then allow to do:

LatLngBounds latLngBounds = LatLngBounds.from(10, -170, -10, 170).unwrappedBounds();

Now we have latLngBounds.getNorthEast().getLongitude() return 190

(that contradicts our intent to have longitude be between -180, 180). Now this instance of LatLngBounds could be passed around and the notion that bounds were just unwrapped would be lost. Of course, you can get longitude() and compare agains 180..

You can see that more testing will need to be added so that two unwrapped bounds and unwrapped & wrapped bounds produce correct union(), span(), center() etc.

@LukasPaczos
Copy link
Contributor Author

We can document the LatLngBounds#unwrapBounds method as internal only and add additional javadoc notes that LatLngBounds operates on wrapped values only, or even move it to the Utils class so that it's not reachable from the LatLngBounds class itself. Do you think that would be enough?

@osana
Copy link
Contributor

osana commented Apr 26, 2018

Adding docs is always good.
Making union() and intersects() work with both wrapped and unwrapped state bounds would be good.

There are several more APIs in gl-native that use LatLngBounds like Snapshotter.
We need to check what happens when we pass bounds going over antimeridian.

LukasPaczos added a commit that referenced this pull request May 1, 2018
tobrun pushed a commit that referenced this pull request May 2, 2018
@tobrun tobrun mentioned this pull request May 4, 2018
24 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants