Remove CoordinateBounds

[ljbade]

The CoordinateBounds class is the same as BoundingBox and should be merged.

Also BoundingBox should implement offset from #1948

[ljbade]

However I think I prefer the name CoordinateBounds as is it more clearly states it is in lat/lon units.

BoundingBox sounds more like pixel units.

@tobrun @bleege which name do you prefer?

[tobrun]

Same preference here.

Now we are on this subject:

• should this class automatically detect which coordinate is SW and which is NE?
• is this for the end developer to determine?

If not correctly inserted the following method will not show the correct region:

mapView.setVisibleCoordinateBounds(new CoordinateBounds(mMyLocation, mAddressLatLng), new RectF(100, 0, 100, 0), true);

[ljbade]

@tobrun BoundingBox does have a isValid function. But I can't see a reason for a "negative" bounding box.

We should at the least print a warning or throw an exception.

@incanus on iOS what happens if you accidentally make a backwards CoordinateBounds?

[1ec5]

MGLCoordinateBounds is a simple struct that contains no logic of its own. It’s up to routines that work with MGLCoordinateBoundses to do the right thing. An MGLCoordinateBounds with a southwest corner at (−1, 1) and a northeast corner at (1, −1) would cover almost the entire world by spanning the antemeridian. I’m not sure if either MapKit or the Mapbox iOS SDK handles that situation correctly, but those are the semantics I’d expect.

MGLCoordinateBounds is named after UIView.bounds and related properties in UIKit. It’s actually a bit more awkward to name variables around “a bounds” and “multiple bounds” than around “a box” and “multiple boxes”, but I don’t have a strong opinion either way.

[ljbade]

@1ec5 perhaps a CoordBoundingBox or LatLngBoundingBox?

[bleege]

Perhaps we should have used LatLngBounds as that's what's used in the Google Maps API. 😉

[1ec5]

Perhaps we should have used LatLngBounds as that's what's used in the Google Maps API. 😉

That’s the principle by which we chose the iOS SDK name: follow platform (vendor) conventions.

[ljbade]

@1ec5 @bleege We have the opportunity to rename it to that class since removing CoordinateBounds is a breaking change to setVisibleBounds() anyhow.

[bleege]

We have the opportunity to rename it to that class since removing CoordinateBounds is a breaking change to setVisibleBounds() anyhow.

So that means it's going to be renamed to LatLngBounds then? Also, we'll need to first add Deprecation to both CoordinateBounds and BoundingBox for a release so that we don't break apps.

[ljbade]

@bleege What about setVisibleBounds that currently takes CoordinateBounds? To deprecate we will need to duplicate the JNI code to read from both the old and the new class.

[ljbade]

I notice Google Maps LatLng is immutable. I wonder what the benefit is.

[ljbade]

@1ec5 Recommends making LatLng immutable. I will do the same with LatLngZoom and ProjectedMeters.

Going to take this as an opportunity to refactor JavaDoc the geometry package.

@tobrun This might impact your test cases.

[ljbade]

We also need to fully implement wrap around in LatLngBounds.

From the Google docs:
The bounds conceptually includes all points where:

• the latitude is in the range [northeast.latitude, southwest.latitude];
• the longitude is in the range [southwest.longtitude, northeast.longitude] if southwest.longtitude ≤ northeast.longitude; and
• the longitude is in the range [southwest.longitude, 180) ∪ [-180, northeast.longitude] if southwest.longtitude > northeast.longitude.

@tobrun We are going to have to remove isValid, and also fixup the various methods to take this into account.

We also need to carefully follow Google's definition of getCenter() and including()

[ljbade]

Should also check for any valuable use of annotations we can add.

[ljbade]

@tobrun I have started the initial refactor in 2896-refactor-geom

There is still a lot of TODOs.

Do you mind looking at the implementations of the various calculation functions as they will all need fixes to handle proper longitude wrapping, and to match Google's definitions.

The unit tests will also need fixing up. Also tests should be added for various configs including wrapping longitude, and invalid latitude.

[tobrun]

@ljbade next thing after Sirius for me is writing testcases for CI integration, I will pick this up ;)

[bleege]

Going to take this as an opportunity to refactor JavaDoc the geometry package.

@ljbade Please be VERY careful here. This has the likely hood of being a breaking change. I'd recommend making use of Deprecated so that people have time to adjust.

[ljbade]

Yeah I'm going to deprecate existing two bounding box classes and replace with new LatLngBounds. Old classes will not have any choice changed and continue to work exactly the same.

[tobrun]

Would love to see this land, currently I need to work around this issue by applying this code:

// FIXME cleanup this method if fix for BoundingBox lands, currently not possible to determine SW and NE without BoundingBox
private void createMarkerAndMoveViewport(final MapView mapView, LatLng start, LatLng stop, String stopText) {
    final Marker destination = mapView.addMarker(new MarkerOptions().position(stop).title(stopText));

    List<LatLng> coordinates = new ArrayList<>();
    coordinates.add(start);
    coordinates.add(stop);

    BoundingBox box = BoundingBox.fromLatLngs(coordinates);
    LatLng southWest = new LatLng(box.getLatSouth(), box.getLonWest());
    LatLng northEast = new LatLng(box.getLatNorth(), box.getLonEast());

    Resources resources = mapView.getResources();
    float hMargin = resources.getDimension(R.dimen.marker_horizontal_margin);
    float tMargin = resources.getDimension(R.dimen.marker_top_margin);
    float bMargin = resources.getDimension(R.dimen.marker_bottom_margin);
    mapView.setVisibleCoordinateBounds(new CoordinateBounds(southWest, northEast), new RectF(hMargin, tMargin, hMargin, bMargin), true);

    // Save search
    HistoryItem entry = new HistoryItem();
    entry.key = stopText;
    entry.latitude = stop.getLatitude();
    entry.longitude = stop.getLongitude();
    entry.save();
}

[tobrun]

Going to pick this up in migration from MapView to MapboxMap in #3145

[tobrun]

This has landed in a1aca1b

[bleege]

@tobrun Can you link to the PR too? Makes tracking things easier. Thanks!

[tobrun]

@bleege this was with the first iteration of MapboxMap in #3145, there I needed to migrate our public api from MapView to MapboxMap, since this feature was going to change to CameraUpdateFactory.newLatLngBounds , I just removed it from MapVIew and integrated the latter in #3754.