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

Don't wrap LatLngBounds #13540

Merged
merged 3 commits into from
Dec 11, 2018
Merged

Don't wrap LatLngBounds #13540

merged 3 commits into from
Dec 11, 2018

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Dec 11, 2018

Closes #13186

ezgif com-video-to-gif 45

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Dec 11, 2018
@tobrun tobrun added this to the android-v7.0.0-iowaska milestone Dec 11, 2018
@tobrun tobrun self-assigned this Dec 11, 2018
@tobrun tobrun force-pushed the tvn-dont-wrap-bounds branch 3 times, most recently from d034330 to bae2dfe Compare December 11, 2018 14:20

if (latNorth < latSouth) {
throw new IllegalArgumentException("latNorth cannot be less than latSouth");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to remove this check?
I think we should add a similar check for lonEast being always >= lonWest

Copy link
Contributor

Choose a reason for hiding this comment

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

Also LatLngBounds union, intersect, getCenter, getLongitudeSpan, fromLatLngs can be simplified a lot as lonEast >= lonWest

Copy link
Contributor

Choose a reason for hiding this comment

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

GeometryConstants for longitude need to be changed. Do we allow longitude to infinity ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like what you mean @osana has already been done in #13419. Could you finish up that PR and possibly port @tobrun's example while removing the wrap object? This would give us the desired effect, is that right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another place that should be adjusted (I would think) is the Projection.getVisibleRegion() as it can go over dateline and be rotated (on top of that)

Copy link
Contributor

Choose a reason for hiding this comment

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

@LukasPaczos I absolutely agree that #13419 and this one compliment one another. I can move over @tobrun's example or have both PRs merged - either way is fine by me

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we need to remove this check?
my bad, this was re-added.

I think we should add a similar check for lonEast being always >= lonWest
sure, added in follow up commit

Also LatLngBounds union, intersect, getCenter, getLongitudeSpan, fromLatLngs can be simplified a lot as lonEast >= lonWest

Simplified, so not a requirement for this PR?

GeometryConstants for longitude need to be changed. Do we allow longitude to infinity ?

I went with Double.MAX_VALUE in a follow up commit, not seeing that infinity will unlock any use-cases.

@osana osana changed the base branch from master to osana-lat-lng-donotwrap December 11, 2018 19:29
@osana osana merged this pull request into osana-lat-lng-donotwrap Dec 11, 2018
osana pushed a commit that referenced this pull request Dec 12, 2018
* [android] - don't wrap LatLngBounds

* Update GeometryConstants.java

* Update NativeMapTest
@tobrun tobrun deleted the tvn-dont-wrap-bounds branch June 25, 2019 11:58
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