Add tile template option to raster and vector sources #6316

boundsj · 2016-09-13T05:06:47Z

introduce MGLTileSet
add tileset initializer to MGLVectorSource
test MGLVectorSource in iosapp
add tileset initializer to MGLRasterSource
test MGLRasterSource in iosapp
unit tests
documentation updates

mention-bot · 2016-09-13T05:06:48Z

@boundsj, thanks for your PR! By analyzing this pull request, we identified @1ec5, @incanus and @friedbunny to be potential reviewers.

1ec5 · 2016-09-13T14:22:57Z

platform/darwin/src/MGLRasterSource.h


 @interface MGLRasterSource : MGLSource

 @property (nonatomic, readonly, copy) NSURL *URL;
 @property (nonatomic, readonly, assign) CGFloat tileSize;
+@property (nonatomic, readonly, copy) NS_ARRAY_OF(NSString *) *tileURLTemplates;
+@property (nonatomic, readonly) uint8_t minimumZoomLevel;


These should be NSUInteger-typed. In general, we should prefer Foundation typealiases over the compiler’s built-in numeric primitives in Objective-C code. It’s fine if the whole range of NSUInteger won’t work; if it fails hard, we can clamp the value to whatever range is supported.

1ec5 · 2016-09-14T15:59:12Z

We should probably add an MGLTileSet class that exposes all the options mbgl::style::TileSet does and accept that in the MGLRasterSource initializer instead of the URL templates. Otherwise, it wouldn’t be possible to set any of the other TileJSON properties such as data or attribution (which will become important once #5999 lands).

boundsj · 2016-09-14T18:34:46Z

We should probably add an MGLTileSet class

This work has started in c0fb939

1ec5 · 2016-09-14T18:57:38Z

platform/darwin/src/MGLTileSet.mm

+
+@implementation MGLTileSet
+{
+    BOOL _minimumZoomLevelIsSet;


If the minimum and maximum zoom levels are optional, then do they need to be represented as NSNumbers publicly? Otherwise, if you use the -initWithTileURLTemplates: initializer, these properties end up being 0.

This reverts a previous change to vector / raster sources that added initializers to those classes so they could take certain tileset properties and create their own mbgl::Tileset objects. Now, a new MGLTileSet class that wraps all available core Tileset options can be created and passed into the sources. The sources pass that tileset object along to core.

This also adds a temporary example to iosapp to show raster sources working (actually vector on top of raster from 3rd party sources using tile URL templates directly)

This changes the type of the minimum and maximum zoom level from a BOOL to an NSNumber. With this commit, a client that does not set the values will still see them represented as nil in the underlying object even though those nil values will translate to non nil defaults in core (0 for minimum and 22 for maximum). In addition, this adds tests for MGLTileSet and an initial attempt at documentation.

boundsj · 2016-09-21T23:42:37Z

With the latest changes here, you can now mash up vector and raster sources created with tile sets:

    // Use raster tile URL templates directly (i.e. mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png)
    // This is Stamen's terrain
    MGLTileSet *rasterTileSet = [[MGLTileSet alloc] initWithTileURLTemplates:@[@"http://a.tile.stamen.com/terrain-background/{z}/{x}/{y}.jpg"]];
    MGLRasterSource *rasterSource = [[MGLRasterSource alloc] initWithSourceIdentifier:@"raster-source" tileSize:256 tileSet:rasterTileSet];
    [self.mapView.style addSource:rasterSource];

    MGLRasterStyleLayer *rasterLayer = [[MGLRasterStyleLayer alloc] initWithLayerIdentifier:@"raster-layer" source:rasterSource];
    [self.mapView.style addLayer:rasterLayer];

    // Use vector tile URL templates directly. This is Mapzen's vector tile service
    MGLTileSet *vectorTileSet = [[MGLTileSet alloc] initWithTileURLTemplates:@[@"https://vector.mapzen.com/osm/all/{z}/{x}/{y}.mvt?api_key=akey"]];
    MGLVectorSource *vectorSource = [[MGLVectorSource alloc] initWithSourceIdentifier:@"mapzen" tileSet:vectorTileSet];
    [self.mapView.style addSource:vectorSource];

    MGLFillStyleLayer *buildingsFillLayer = [[MGLFillStyleLayer alloc] initWithLayerIdentifier:@"filllayer" source:vectorSource sourceLayer:@"buildings"];
    buildingsFillLayer.fillColor = [UIColor magentaColor];
    [self.mapView.style addLayer:buildingsFillLayer];

Not exactly "Streets"

The current implementation does not attempt to reflect the mbgl object's default minimum and maximum zoom values if the Objective-C object's properties are nil. However, documentation was added that states that the defaults will be used if the Objective-C tile set object is used to initialize a source and the min/max values are nil.

cc @1ec5

1ec5 · 2016-09-22T00:19:13Z

platform/darwin/src/MGLTileSet.h

+@interface MGLTileSet : NSObject
+
+/**
+ A REQUIRED `NSArray` of `NSString` objects that represent the tile templates.


Optionality should be indicated through nullability qualifiers; shouting isn't needed here.

1ec5 · 2016-09-22T00:20:35Z

platform/darwin/src/MGLTileSet.h

+ the maximum zoom level at which to display tiles.
+ @return The initialized tile set object.
+ */
+- (instancetype)initWithTileURLTemplates:(NS_ARRAY_OF(NSString *) *)tileURLTemplates minimumZoomLevel:(NSNumber *)minimumZoomLevel maximumZoomLevel:(NSNumber *)maximumZoomLevel;


The zoom level parameters should either be nullable or, better yet, primitive typed.

1ec5 · 2016-09-22T00:27:40Z

platform/ios/test/MGLTileSetTests.mm

+    // has the correct URL templates
+    XCTAssertEqual(mbglTileset.tiles[0], "tile.1");
+    XCTAssertEqual(mbglTileset.tiles[1], "tile.2");
+    XCTAssertEqual(mbglTileset.tiles[2], "tile.3");


Also assert on the tile set count.

1ec5 · 2016-09-22T00:28:15Z

platform/ios/test/MGLTileSetTests.mm

+    XCTAssertEqual([tileSet mbglTileset].zoomRange.max, 2);
+
+    // when the tile set has an attribution
+    tileSet.attribution = @"my tileset";


Also test that Unicode characters are preserved.

1ec5 · 2016-09-22T00:29:36Z

platform/ios/test/MGLTileSetTests.mm

+
+    // when the tile set has min and/or max zoom level set
+    tileSet.minimumZoomLevel = @(1);
+    tileSet.maximumZoomLevel = @(2);


If the developer sets a maximum zoom level less than the minimum, what happens? Would it be possible to test for an Objective-C exception being thrown (ideal)?

0fc9cbc throws Objective-C exceptions if values are crossed initially or later on in setters. This avoids a more shocking C++ exception that happens later when the doomed threshold is crossed when the map is zoomed.

Alternatively, we could ignore invalid values and log a warning avoiding an exception altogether.

This updates the initializer to take an integer value for the zoom level. Logic is also added to throw an exception early if the initial values are invalid or if they are set to invalid values later in property setters.

1ec5

Almost there.

1ec5 · 2016-09-22T23:33:42Z

platform/darwin/src/MGLTileSet.h

- of 0 to 22. The default value is 0 and the source will use the default value
+ An `NSNumber` object containing an integer; specifies the minimum zoom level at 
+ which the source will display tiles. The value should be in the range of 0 to 
+ 22. The default value is 0 and the source will use the default value
 if `minimumZoomLevel` is nil;


This sentence and two more below end in semicolons instead of periods.

1ec5 · 2016-09-22T23:35:49Z

platform/darwin/src/MGLTileSet.h

 */
 @property (nonatomic, nullable) NSNumber *maximumZoomLevel;

 /**
- An OPTIONAL `NSString` object; Contains an attribution to be displayed
- when the map is shown to a user. The default value is nil;
+ An `NSString` object; Contains an attribution to be displayed when the map is 


Nit: replace the semicolon with “that” and lowercase “contains” (plus one more below).

1ec5 · 2016-09-22T23:43:50Z

platform/ios/test/MGLTileSetTests.mm

@@ -43,7 +44,7 @@ - (void)testMBGLTileSet {
    XCTAssertEqual([tileSet mbglTileset].zoomRange.max, 2);

    // when the tile set has an attribution
-    tileSet.attribution = @"my tileset";
+    tileSet.attribution = @"my tileset ©";


© is a decent test, since it falls outside of ASCII. In #5999, I used ©️, which pairs © with a variation selector for an emoji look. But something outside the Basic Multilingual Plane, for example in the Miscellaneous Symbols and Pictographs block, would be even better for detecting mojibake.

Thanks! I think 0e31030 should probably wrap this up

1ec5

🎈

boundsj added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold runtime styling labels Sep 13, 2016

boundsj added this to the ios-v3.4.0 milestone Sep 13, 2016

boundsj self-assigned this Sep 13, 2016

1ec5 reviewed Sep 13, 2016
View reviewed changes

1ec5 added the release blocker Blocks the next final release label Sep 14, 2016

boundsj force-pushed the boundsj-support-tile-sets branch from 9d33392 to c0fb939 Compare September 14, 2016 18:32

1ec5 reviewed Sep 14, 2016

View reviewed changes

boundsj added 5 commits September 21, 2016 12:59

[ios, macos] Add tile template option to vector source

3085d5c

[ios, macos] Add tile template option to raster source

d7596d3

[ios, macos] Standardize URLs in vector and raster sources

9d99d1f

This also adds a temporary example to iosapp to show raster sources working (actually vector on top of raster from 3rd party sources using tile URL templates directly)

boundsj force-pushed the boundsj-support-tile-sets branch from c0fb939 to 9bf02ec Compare September 21, 2016 23:20

boundsj mentioned this pull request Sep 21, 2016

MGLSource and subclasses need documentation #5960

Closed

[ios, macos] Forward declar MGLTileSet in vector source

415508a

1ec5 suggested changes Sep 22, 2016

View reviewed changes

boundsj added 2 commits September 22, 2016 15:33

[ios, macos] Refactor minimum and maximum zoom level in tile set

0fc9cbc

This updates the initializer to take an integer value for the zoom level. Logic is also added to throw an exception early if the initial values are invalid or if they are set to invalid values later in property setters.

[ios, macos] Use integer to represent tile size in raster source

f7a4427

boundsj changed the title ~~[WIP] Add tile template option to vector source~~ Add tile template option to vector source Sep 22, 2016

boundsj added the ✓ ready for review label Sep 22, 2016

boundsj removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Sep 22, 2016

boundsj mentioned this pull request Sep 22, 2016

fix #6379: beef up runtime styling manual tests #6433

Merged

[ios] Remove temporary example

255ca68

1ec5 suggested changes Sep 22, 2016

View reviewed changes

[ios, macos] Fix tile set doc and enhance unicode test for tile set

0e31030

1ec5 approved these changes Sep 23, 2016

View reviewed changes

boundsj changed the title ~~Add tile template option to vector source~~ Add tile template option to rasater and vector sources Sep 23, 2016

boundsj changed the title ~~Add tile template option to rasater and vector sources~~ Add tile template option to raster and vector sources Sep 23, 2016

boundsj merged commit 3ddb8df into master Sep 23, 2016

boundsj deleted the boundsj-support-tile-sets branch September 23, 2016 00:30

1ec5 removed the ✓ ready for review label Oct 5, 2016

boundsj mentioned this pull request Oct 5, 2016

Cannot introspect source or layer obtained from style #6584

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tile template option to raster and vector sources #6316

Add tile template option to raster and vector sources #6316

boundsj commented Sep 13, 2016 •

edited

Loading

mention-bot commented Sep 13, 2016

1ec5 Sep 13, 2016 •

edited

Loading

1ec5 commented Sep 14, 2016

boundsj commented Sep 14, 2016

1ec5 Sep 14, 2016

boundsj commented Sep 21, 2016

1ec5 Sep 22, 2016

1ec5 Sep 22, 2016

1ec5 Sep 22, 2016

1ec5 Sep 22, 2016

1ec5 Sep 22, 2016

boundsj Sep 22, 2016

1ec5 left a comment

1ec5 Sep 22, 2016 •

edited

Loading

1ec5 Sep 22, 2016

1ec5 Sep 22, 2016 •

edited

Loading

boundsj Sep 23, 2016

1ec5 left a comment

Add tile template option to raster and vector sources #6316

Add tile template option to raster and vector sources #6316

Conversation

boundsj commented Sep 13, 2016 • edited Loading

mention-bot commented Sep 13, 2016

1ec5 Sep 13, 2016 • edited Loading

Choose a reason for hiding this comment

1ec5 commented Sep 14, 2016

boundsj commented Sep 14, 2016

Choose a reason for hiding this comment

boundsj commented Sep 21, 2016

Not exactly "Streets"

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

1ec5 left a comment

Choose a reason for hiding this comment

1ec5 Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

boundsj commented Sep 13, 2016 •

edited

Loading

1ec5 Sep 13, 2016 •

edited

Loading

1ec5 Sep 22, 2016 •

edited

Loading

1ec5 Sep 22, 2016 •

edited

Loading