[core] AddedMap::{set,get}LatLngBounds by brunoabinader · Pull Request #8583 · mapbox/mapbox-gl-native

brunoabinader · 2017-03-30T13:41:56Z

Implements a setter and getter for coordinate bounds in Map API - which can be used e.g. to limit the boundaries in which the user can set geographical coordinates.

Related: #3602.

1ec5

For the most part, this PR looks reasonable, but I have some concerns about the antimeridian edge case (the gift that keeps on giving). I’d also defer to someone on the Android side to speak to whether this addresses the Android SDK’s needs.

#3602 (comment) called for a setMinTilt() and setMaxTilt(), which would imply a setMinPitch() and setMaxPitch() in mbgl::Map. Would that fall within the scope of this PR?

1ec5 · 2017-03-30T16:42:16Z

    LatLngBounds(const CanonicalTileID&);

+    explicit operator bool() const {
+        return (sw.latitude <= ne.latitude) && (sw.longitude <= ne.longitude);


This would be consistent with the GeoJSON specification, but inconsistent with GeoJSONVT’s behavior of allowing a single feature or annotation to span the antimeridian (#6088), correct? If one were to calculate the bounding box of an annotation, would this operator end up evaluating to false? Would it still be possible to fit the bounds to the annotation?

My understanding of LatLngBounds::extend is that it preserves the assertion above. Also, this should be fine as long as the coordinates used are unwrapped.

Nonetheless, we can add a unit test to verify 👍

brunoabinader · 2017-03-30T18:32:29Z

I have some concerns about the antimeridian edge case (the gift that keeps on giving)

Do you have one in particular? From my tests, this shouldn't be an issue as every coordinate passed to the map gets unwrapped e.g. here.

brunoabinader · 2017-03-30T18:33:50Z

#3602 (comment) called for a setMinTilt() and setMaxTilt(), which would imply a setMinPitch() and setMaxPitch() in mbgl::Map. Would that fall within the scope of this PR?

Yep, we can certainly add it too 😃

1ec5 · 2017-03-30T19:46:47Z

I have some concerns about the antimeridian edge case (the gift that keeps on giving)

Do you have one in particular? From my tests, this shouldn't be an issue as every coordinate passed to the map gets unwrapped e.g. here.

I was referring to #8583 (comment). But as long as you’ve considered this possibility, 👍.

kkaefer · 2017-03-31T09:53:13Z

-    LatLng(double lat = 0, double lon = 0, WrapMode mode = Unwrapped)
+    constexpr LatLng(null) : latitude(std::numeric_limits<double>::quiet_NaN()), longitude(latitude) {}
+
+    constexpr LatLng(double lat, double lon, WrapMode mode = Unwrapped)


Why did you change this?

By making LatLngBounds::world() constexpr here, LatLngBounds needed a constexpr ctor, which required its members (LatLng for southwest and northeast bounds) to also have a constexpr ctor. We useLatLngBounds::world() to initialize the default TransformState bounds here.

kkaefer · 2017-03-31T09:53:41Z

 }

-void Transform::setMaxZoom(const double maxZoom) {
+void Transform::setMaxZoom(double maxZoom) {


Why did you remove const? I generally don't add it in the headers, but do add it in the implementation files to avoid modifying the parameter. Those definitions are compatible.

As you mentioned, there is no point in adding this to headers. Also, const-pass-by-value is not a common behavior in our codebase.

For me, it is clear that a non-const pass-by-value is a different object from the one it has been copied from i.e. declaring it const does not guarantee that the original object remains immutable.

Sorry, what I meant to say is that I'm doing this as a convention to avoid accidental changes to the parameter that might get read later on in the function.

Ack - let's add them back on the definition file 👍

kkaefer · 2017-03-31T09:59:51Z


    ScreenCoordinate point = {
-        -latLng.longitude * Bc,
+        -constrained.longitude * Bc,


Reasoning for this change?

That's where we constrain the received LatLng to the specified bounds. I preferred this to be done when inserting because I suppose we set LatLng values much less than we request them from TransformState.

tobrun · 2017-04-03T12:39:56Z

Been testing out and adding binding integration in #8622,
thank you for picking this up @brunoabinader.

brunoabinader · 2017-04-03T15:38:20Z

Removed the constexpr-related changes and added {set,get}{Min,Max}Pitch.

kkaefer · 2017-04-04T08:54:43Z

+        }
+        return LatLng {
+            p.latitude < sw.latitude ? sw.latitude : p.latitude > ne.latitude ? ne.latitude : p.latitude,
+            p.longitude < sw.longitude ? sw.longitude : p.longitude > ne.longitude ? ne.longitude : p.longitude,


Parentheses to clarify operator precedence would make the code mode readable

brunoabinader · 2017-04-11T06:16:23Z

@kkaefer can you please re-review?

brunoabinader added Core The cross-platform C++ core, aka mbgl feature GL JS parity For feature parity with Mapbox GL JS labels Mar 30, 2017

brunoabinader self-assigned this Mar 30, 2017

brunoabinader mentioned this pull request Mar 30, 2017

Limit Viewport #3602

Closed

brunoabinader requested review from 1ec5, jfirebaugh and kkaefer March 30, 2017 14:02

1ec5 reviewed Mar 30, 2017

View reviewed changes

kkaefer reviewed Mar 31, 2017

View reviewed changes

tobrun mentioned this pull request Apr 3, 2017

Limit Viewport #8622

Merged

brunoabinader force-pushed the map-latlngbounds branch from 4b8849b to 893bc78 Compare April 3, 2017 15:37

brunoabinader force-pushed the map-latlngbounds branch from 893bc78 to 93f7a11 Compare April 3, 2017 15:40

kkaefer reviewed Apr 4, 2017

View reviewed changes

brunoabinader force-pushed the map-latlngbounds branch from 93f7a11 to 713974b Compare April 4, 2017 11:51

brunoabinader added 4 commits April 10, 2017 18:02

[core] Move {Map,Transform}::{get,set}{Min,Max}Zoom to bounds pragma

278efe1

[core] Added LatLngBounds::{valid,constrain}

d68b5eb

[core] Added Map::{get,set}LatLngBounds

f885372

[core] Added {set,get}{Min,Max}Pitch

5891f0c

brunoabinader force-pushed the map-latlngbounds branch from 713974b to 5891f0c Compare April 10, 2017 15:02

kkaefer approved these changes Apr 11, 2017

View reviewed changes

brunoabinader merged commit c4fc899 into master Apr 11, 2017

brunoabinader deleted the map-latlngbounds branch April 11, 2017 09:51

kkaefer mentioned this pull request May 30, 2017

add API for arbitrary constraints #236

Closed

friedbunny mentioned this pull request May 30, 2017

Drifting across the international dateline is sticky #8951

Closed

Conversation

brunoabinader commented Mar 30, 2017

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brunoabinader commented Mar 30, 2017

Uh oh!

brunoabinader commented Mar 30, 2017

Uh oh!

1ec5 commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobrun commented Apr 3, 2017

Uh oh!

brunoabinader commented Apr 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brunoabinader commented Apr 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

1ec5 commented Mar 30, 2017 •

edited

Loading