Add LatLngforScreenCoordinate to snapshotter API #12221

tobrun · 2018-06-26T16:11:35Z

fabian-guerra · 2018-06-27T22:18:27Z

platform/ios/CHANGELOG.md

@@ -2,6 +2,13 @@

 Mapbox welcomes participation and contributions from everyone. Please read [CONTRIBUTING.md](../../CONTRIBUTING.md) to get started.

+## Future


We leave the word master for this case.

fabian-guerra · 2018-06-27T22:18:41Z

platform/macos/CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog for Mapbox Maps SDK for macOS

+## Future


Same as above.

fabian-guerra · 2018-06-27T22:20:51Z

platform/ios/CHANGELOG.md

+
+### Other changes 
+
+* Added `-[MGLSnapshot coordinateForPoint:]` that returns a map coordinate for a specified snapshot image point. ([#12221](https://github.com/mapbox/mapbox-gl-native/pull/12221))


I think you meant -[MGLMapSnapshot coordinateForPoint:]

Great catch - thanks!

fabian-guerra · 2018-06-27T22:21:01Z

platform/macos/CHANGELOG.md

+
+### Other changes
+
+* Added `-[MGLSnapshot coordinateForPoint:]` that returns a map coordinate for a specified snapshot image point. ([#12221](https://github.com/mapbox/mapbox-gl-native/pull/12221))


Same as above.

1ec5 · 2018-06-27T22:44:59Z

platform/darwin/src/MGLMapSnapshotter.h

@@ -85,6 +85,11 @@ MGL_EXPORT
 */
 - (CGPoint)pointForCoordinate:(CLLocationCoordinate2D)coordinate;

+/**
+ Converts the specified image point to a map coordinate


Nit: missing period.

1ec5 · 2018-06-27T22:46:10Z

platform/darwin/src/MGLMapSnapshotter.mm

+
+- (CLLocationCoordinate2D)coordinateForPoint:(NSPoint)point
+{
+    mbgl::LatLng latLng = _latLngForFn(mbgl::ScreenCoordinate(point.x, point.y));


Does this code account for the fact that the screen origin on macOS is at the bottom-left, by contrast with iOS where it’s at the top-left?

No! Superb catch - and I think this will be an existing bug for -[MGLMapSnapshot pointForCoordinate:] on macos.

1ec5 · 2018-06-27T22:48:14Z

platform/darwin/src/MGLMapSnapshotter.mm

+                                                                                          mbgl::PremultipliedImage image,
+                                                                                          mbgl::MapSnapshotter::Attributions attributions,
+                                                                                          mbgl::MapSnapshotter::PointForFn pointForFn,
+                                                                                          mbgl::MapSnapshotter::LatLngForFn latLngForFn) {


Nit: This indentation is a bit extreme. It’s technically correct (per Xcode), but I think it would be reasponable to dedent a bit, such as by indenting both parameters to std::make_unique by only two tabs.

LukasPaczos

Looks good on the Android side 🚀

fabian-guerra

👍🏼

julianrex · 2018-06-29T18:17:57Z

Rebased.

friedbunny

Few questions/nits:

The submodule at platform/ios/vendor/mapbox-events-ios doesn’t need to be bumped.
Are updates to the mapbox-gl-js submodule intentional?
If we intend to merge @julianrex’s commits, they should be prefixed with [ios] and/or [macos] (and perhaps squashed).

tobrun · 2018-07-05T15:15:14Z

@friedbunny thanks for catching those! just pushed changes addressing comments.

…ed test.

tobrun added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels Jun 26, 2018

tobrun added this to the android-v6.3.0 milestone Jun 26, 2018

tobrun self-assigned this Jun 26, 2018

tobrun requested a review from LukasPaczos June 26, 2018 16:11

tobrun added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Jun 27, 2018

julianrex mentioned this pull request Jun 27, 2018

Feature request for Snapshotter#latLngForPixel #11365

Closed

julianrex requested a review from fabian-guerra June 27, 2018 20:05

julianrex self-assigned this Jun 27, 2018

fabian-guerra suggested changes Jun 27, 2018

View reviewed changes

1ec5 reviewed Jun 27, 2018

View reviewed changes

LukasPaczos approved these changes Jun 28, 2018

View reviewed changes

1ec5 approved these changes Jun 29, 2018

View reviewed changes

fabian-guerra approved these changes Jun 29, 2018

View reviewed changes

julianrex force-pushed the tvn-add-latlngforpixel-snapshot branch from 99452ed to acef960 Compare June 29, 2018 18:16

julianrex removed their assignment Jul 2, 2018

tobrun force-pushed the tvn-add-latlngforpixel-snapshot branch from acef960 to 846249a Compare July 5, 2018 12:54

friedbunny reviewed Jul 5, 2018

View reviewed changes

tobrun force-pushed the tvn-add-latlngforpixel-snapshot branch from 846249a to e333843 Compare July 5, 2018 15:15

tobrun and others added 3 commits July 5, 2018 17:59

[android] - add LatLng for pixel conversion feature to MapSnapshotter

51762da

[core] - add LatLng for screencoordinate to mapsnapshotter

7242c09

[ios] [macos] Added -[MGLSnapshot coordinateForPoint:] and associat…

2311c7e

…ed test.

tobrun force-pushed the tvn-add-latlngforpixel-snapshot branch from e333843 to 2311c7e Compare July 5, 2018 16:01

tobrun merged commit 56ab331 into master Jul 5, 2018

tobrun deleted the tvn-add-latlngforpixel-snapshot branch July 5, 2018 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LatLngforScreenCoordinate to snapshotter API #12221

Add LatLngforScreenCoordinate to snapshotter API #12221

tobrun commented Jun 26, 2018

fabian-guerra Jun 27, 2018

fabian-guerra Jun 27, 2018

fabian-guerra Jun 27, 2018

julianrex Jun 28, 2018

fabian-guerra Jun 27, 2018

1ec5 Jun 27, 2018

1ec5 Jun 27, 2018

julianrex Jun 28, 2018

1ec5 Jun 27, 2018

LukasPaczos left a comment

fabian-guerra left a comment

julianrex commented Jun 29, 2018

friedbunny left a comment •

edited

Loading

tobrun commented Jul 5, 2018

		@@ -2,6 +2,13 @@

		Mapbox welcomes participation and contributions from everyone. Please read [CONTRIBUTING.md](../../CONTRIBUTING.md) to get started.

		## Future

		@@ -1,5 +1,11 @@
		# Changelog for Mapbox Maps SDK for macOS

		## Future


		### Other changes

		* Added `-[MGLSnapshot coordinateForPoint:]` that returns a map coordinate for a specified snapshot image point. ([#12221](https://github.com/mapbox/mapbox-gl-native/pull/12221))

Add LatLngforScreenCoordinate to snapshotter API #12221

Add LatLngforScreenCoordinate to snapshotter API #12221

Conversation

tobrun commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos left a comment

Choose a reason for hiding this comment

fabian-guerra left a comment

Choose a reason for hiding this comment

julianrex commented Jun 29, 2018

friedbunny left a comment • edited Loading

Choose a reason for hiding this comment

tobrun commented Jul 5, 2018

friedbunny left a comment •

edited

Loading