Map snapshotter additions #10163

ivovandongen · 2017-10-09T15:10:52Z

Some additions to the map snapshotter api

More flexible re-use:
- Core - add accessors to map snapshotter
- iOS: Fixes Reuse MapSnapshotter #10053
- Android: Fixes iOS/macOS reusable snapshotter #10038
Convenient snapshot wrappers Fixes Convenient snapshot wrappers #9916
- Core - wrap transform state for SDK to use
- iOS - add wrapper
- Android - add wrapper
Expose attribution (Snapshots should include attribution #10114)
- Core - add access to attribution

jfirebaugh · 2017-10-09T19:02:48Z

src/mbgl/style/style_impl.cpp

+    , fileSource(other.fileSource)
+    , spriteLoader(std::make_unique<SpriteLoader>(pixelRatio))
+    , light(std::make_unique<Light>(*other.light))
+    , observer(&nullObserver) {


What about all the other members? images, sources, layers, etc.?

Copying the collections is going to be tricky. The wrapper classes should be copied, because they have an observer reference, while the immutable instances (and the immutable vector) should be shared.

Ideally, Style::Impl is itself held in an Immutable, and we move the wrappers, observers, Scheduler, FileSource, AsyncRequest, and SpriteLoader to Style. Then MapSnapshotter can accept an Immutable<Style::Impl> and doesn't have to handle making copies of anything, while the dirty work of copying all the wrappers and setting new observers can be centralized in Style(const Style&).

1ec5 · 2017-10-09T23:30:25Z

platform/darwin/src/MGLMapSnapshotter.h

@@ -101,7 +120,7 @@ typedef void (^MGLMapSnapshotCompletionHandler)(NSImage* _Nullable snapshot, NSE
 options.zoomLevel = 10

 var snapshotter = MGLMapSnapshotter(options: options)
- snapshotter.start { (image, error) in
+ snapshotter.start { (snapshot, error) in


Add this example to MGLDocumentationExampleTests.swift, per these instructions for adding a code example. Otherwise, the next time someone generates runtime styling code, this block may get overwritten.

Was missed when the sample was added apparently. Added it now and updated the code.

1ec5 · 2017-10-09T23:31:35Z

platform/darwin/src/MGLMapSnapshotter.h

+/**
+ URL of the map style to snapshot.
+ */
+@property (nonatomic) NSURL* styleURL;


Nit: space before asterisk. (Sorry. 😬)

1ec5 · 2017-10-09T23:34:03Z

platform/darwin/src/MGLMapSnapshotter.h

@@ -139,6 +158,37 @@ MGL_EXPORT
 - (void)cancel;

 /**
+ The zoom level.
+
+ The default zoom level is 0. This overwrites the camera zoom level if set.


For consistency, let’s match the language used in MGLMapSnapshotOptions documentation:

mapbox-gl-native/platform/darwin/src/MGLMapSnapshotter.h

Lines 32 to 35 in d913f4b

The zoom level.

The default zoom level is 0. If this property is non-zero and the camera property

is non-nil, the camera’s altitude is ignored in favor of this property’s value.

1ec5 · 2017-10-09T23:36:42Z

platform/darwin/src/MGLMapSnapshotter.mm

+    mbgl::ScreenCoordinate sc = _pointForFn(MGLLatLngFromLocationCoordinate2D(coordinate));
+    return NSMakePoint(sc.x * self.scale, sc.y * self.scale);
+}
+#endif


FYI, on macOS, NSPoint is a typedef for CGPoint and NSMakePoint() calls CGPointMake(), so conditional compilation isn’t strictly necessary in the implementation.

1ec5 · 2017-10-09T23:37:58Z

platform/darwin/src/MGLMapSnapshotter.mm

+- (NSURL*)styleURL;
+{
+    NSString *styleURLString = @(_mbglMapSnapshotter->getStyleURL().c_str());
+    return styleURLString && styleURLString.length > 0 ? [NSURL URLWithString:styleURLString] : nil;


FYI, styleURLString.length ? … : nil is acceptable shorthand.

1ec5 · 2017-10-09T23:39:06Z

platform/darwin/src/MGLMapSnapshotter.mm

+- (NSURL*)styleURL;
+{
+    NSString *styleURLString = @(_mbglMapSnapshotter->getStyleURL().c_str());
+    return styleURLString && styleURLString.length > 0 ? [NSURL URLWithString:styleURLString] : nil;


Can styleURL be nil? If so, then the property should be declared nullable in the interface. Otherwise, we can assert here that the style is non-nil.

ivovandongen · 2017-10-26T12:14:41Z

As discussed, Runtime Style support is going to need more effort to separate Style from Map on iOS (and Android in the next major release) #9914. Dropping from this PR.

ivovandongen · 2017-10-27T17:21:50Z

@fabian-guerra @tobrun I finished up the wrappers and added the attributions to the callback so they are ready to use in the SDKs. Should be good to go.

tobrun · 2017-10-27T21:33:24Z

Played around with the Android code and is looking 👍

1ec5

These are good changes – thank you. On the iOS/macOS front, just one documentation request, a historical note, and a proposed improvement to the provided example code.

1ec5 · 2017-10-31T00:36:28Z

platform/darwin/src/MGLMapSnapshotter.h

+/**
+ A camera representing the viewport visible in the snapshot.
+
+ This overwrites the coordinate bounds if set.


Similarly to #10163 (comment), we should try to be consistent with the corresponding properties of MGLMapSnapshotOptions:

mapbox-gl-native/platform/darwin/src/MGLMapSnapshotter.h

Lines 40 to 44 in d913f4b

A camera representing the viewport visible in the snapshot.

If this property is non-nil and the `coordinateBounds` property is set to a non-empty

coordinate bounds, the camera’s center coordinate and altitude are ignored in favor

of the `coordinateBounds` property.

mapbox-gl-native/platform/darwin/src/MGLMapSnapshotter.h

Lines 49 to 52 in d913f4b

The cooordinate rectangle that encompasses the bounds to capture.

If this property is non-empty and the camera property is non-nil, the camera’s

center coordinate and altitude are ignored in favor of this property’s value.

etc.

1ec5 · 2017-10-31T00:39:29Z

platform/darwin/src/MGLMapSnapshotter.mm

@@ -186,4 +214,81 @@ - (void)cancel;
    _mbglMapSnapshotter.reset();
 }

+- (NSURL*)styleURL;


The trailing semicolon here is technically incorrect, but GCC and Clang are lenient about it by default because it used to be very common a decade or so ago. (If I remember correctly, older versions of Interface Builder would generate boilerplate code for you that included the semicolon.) Just an FYI; I thought you might find it interesting. The most substantive issue is the space before the asterisk. 😛

1ec5 · 2017-10-31T00:45:23Z

platform/darwin/test/MGLDocumentationExampleTests.swift

+        //#-example-code
+        let camera = MGLMapCamera()
+        camera.centerCoordinate = CLLocationCoordinate2D(latitude: 37.7184, longitude: -122.4365)
+        camera.pitch = 20


The preferred way to create a camera is via one of the other initializers, either MGLMapCamera(lookingAtCenter:fromEyeCoordinate:eyeAltitude:) or MGLMapCamera(lookingAtCenter:fromDistance:pitch:heading:). Following MapKit’s lead, these initializers encourage developers to make it clear as to whether the camera is based on a focused point on the map or an actor somewhere in space.

(You can specify whatever distance you want, since zoomLevel will override it below. #5583 would allow us to eliminate this awkward situation.)

Just moved over the existing documentation, but I will adapt.

tobrun · 2017-10-31T01:06:14Z

@ivovandongen noticing the following when implementing attribution on android:

I'm getting back the following String:

<a href="https://www.mapbox.com/about/maps/" target="_blank">&copy; Mapbox</a> <a href="http://www.openstreetmap.org/about/" target="_blank">&copy; OpenStreetMap</a> <a class="mapbox-improve-map" href="https://www.mapbox.com/map-feedback/" target="_blank">Improve this map</a>

Would it be possible to exclude the improve this map link?

Additionally the attribution was setup as String[], but these seem to be placed in a single string:

Also there is an empty string that shouldn't be there.

1ec5 · 2017-10-31T01:09:18Z

platform/default/mbgl/map/map_snapshotter.cpp

+        }};
+
+        // Collect all source attributions
+        std::vector<std::string> attributions;


By the way, we decided in #6465, we decided that mbgl wouldn’t be responsible for collecting each source’s attribution strings. I take it this is merely a stopgap until we have full runtime styling support for snapshots?

The situation is slightly different here as the Map/Style/Sources live on another thread. This means we can't call them directly from the main thread and copying them over just to give access to the attribution strings seems like a waste. That's why I opted to pass them along with the snapshot result.

1ec5 · 2017-10-31T01:11:32Z

Additionally the attribution was setup as String[], but these seem to be placed in a single string

Most likely, there are two sources, and one has multiple links in its attribution HTML, while another has an empty string as its attribution HTML. It should be the job of platform-specific code to parse links out of the HTML and detect map feedback links, as it does in the usual case for interactive map views.

tobrun · 2017-10-31T02:00:53Z

Thank you for adding the context, will adapt the binding code accordingly

…m latlng for snapshot - Wraps the TransformState for the snapshot so that the snapshotter itself is free to be re-used or destroyed

…o the pointFor functionality

ivovandongen added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Oct 9, 2017

ivovandongen self-assigned this Oct 9, 2017

ivovandongen force-pushed the ivd-mapsnapshotter-extensions branch 2 times, most recently from 23b471e to d93b9b5 Compare October 9, 2017 16:59

jfirebaugh reviewed Oct 9, 2017

View reviewed changes

1ec5 added the runtime styling label Oct 9, 2017

1ec5 reviewed Oct 9, 2017

View reviewed changes

tobrun mentioned this pull request Oct 13, 2017

[android] [WIP] Create snapshot wrapper #10048

Closed

ivovandongen force-pushed the ivd-mapsnapshotter-extensions branch 4 times, most recently from 180c242 to 4a89384 Compare October 27, 2017 17:18

This was referenced Oct 27, 2017

Make MapSnapshot logo optional #10310

Merged

Resize logo based on snapshot size #10312

Merged

1ec5 approved these changes Oct 31, 2017

View reviewed changes

1ec5 reviewed Oct 31, 2017

View reviewed changes

ivovandongen added 5 commits October 31, 2017 10:00

[core] map snapshotter - add mutators

286d91b

[android] map snapshotter - add mutators

3b5676c

[darwin] map snapshotter - snapshotter mutability

3020c73

[default] headless frontend - allow access to the transform state

311ca6e

[default] map snapshotter - add function to get screen coordinate fro…

8a58ef8

…m latlng for snapshot - Wraps the TransformState for the snapshot so that the snapshotter itself is free to be re-used or destroyed

ivovandongen added 5 commits October 31, 2017 10:05

[android] map snapshotter - wrap snapshot in native peer for access t…

8cd4fcb

…o the pointFor functionality

[darwin][ios][macos] map snapshotter - add MGLMapSnapshot wrapper

8c1b6bd

[core] map snapshotter - add attributions to snapshotter callback

d6518fb

[darwin] map snapshotter - expose attributions

d65676d

[android] map snapshotter - expose attributions

1c4b6a7

ivovandongen force-pushed the ivd-mapsnapshotter-extensions branch from 4a89384 to 1c4b6a7 Compare October 31, 2017 08:06

jfirebaugh approved these changes Oct 31, 2017

View reviewed changes

ivovandongen merged commit e0a3052 into master Oct 31, 2017

fabian-guerra mentioned this pull request Nov 1, 2017

Cherry Pick 10163 #10342

Merged

1ec5 mentioned this pull request Nov 3, 2017

Snapshots should include attribution #10114

Closed

tobrun mentioned this pull request Nov 3, 2017

Release Android v5.2.0-beta.4 #10379

Closed

21 tasks

This was referenced Nov 14, 2017

Release Android v5.2.0-beta.5 #10463

Closed

Release Android v5.2.0 #10486

Closed

frederoni mentioned this pull request Feb 4, 2018

Add support for annotations when snapshotting #11113

Closed

jfirebaugh deleted the ivd-mapsnapshotter-extensions branch July 27, 2018 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map snapshotter additions #10163

Map snapshotter additions #10163

ivovandongen commented Oct 9, 2017 •

edited

Loading

jfirebaugh Oct 9, 2017

1ec5 Oct 9, 2017

ivovandongen Oct 27, 2017

1ec5 Oct 9, 2017

1ec5 Oct 9, 2017

1ec5 Oct 9, 2017

1ec5 Oct 9, 2017

1ec5 Oct 9, 2017

ivovandongen commented Oct 26, 2017

ivovandongen commented Oct 27, 2017

tobrun commented Oct 27, 2017

1ec5 left a comment

1ec5 Oct 31, 2017

1ec5 Oct 31, 2017

1ec5 Oct 31, 2017

ivovandongen Oct 31, 2017

tobrun commented Oct 31, 2017

1ec5 Oct 31, 2017

ivovandongen Oct 31, 2017

1ec5 commented Oct 31, 2017

tobrun commented Oct 31, 2017

	The zoom level.

	The default zoom level is 0. If this property is non-zero and the camera property
	is non-nil, the camera’s altitude is ignored in favor of this property’s value.

	A camera representing the viewport visible in the snapshot.

	If this property is non-nil and the `coordinateBounds` property is set to a non-empty
	coordinate bounds, the camera’s center coordinate and altitude are ignored in favor
	of the `coordinateBounds` property.

	The cooordinate rectangle that encompasses the bounds to capture.

	If this property is non-empty and the camera property is non-nil, the camera’s
	center coordinate and altitude are ignored in favor of this property’s value.

Map snapshotter additions #10163

Map snapshotter additions #10163

Conversation

ivovandongen commented Oct 9, 2017 • edited Loading

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

ivovandongen commented Oct 26, 2017

ivovandongen commented Oct 27, 2017

tobrun commented Oct 27, 2017

1ec5 left a 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

tobrun commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Oct 31, 2017

tobrun commented Oct 31, 2017

ivovandongen commented Oct 9, 2017 •

edited

Loading