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

Simplify MGLSource and subclasses #7377

Merged
merged 14 commits into from
Dec 14, 2016
Merged

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Dec 12, 2016

This PR simplifies MGLSource and subclasses, removing parallel code paths and parallel state and making it possible to avoid relying on backpointers as we add getters for more properties in the future.

MGLRasterSource and MGLVectorSource now share a common abstract superclass, MGLTileSource. MGLTileSet has been removed. MGLTileSource is modeled after the common parts of mbgl::style::RasterSource and mbgl::style::VectorSource. It has initializers that incorporate the parameters of MGLTileSet’s initializers, but it lacks getters for everything but the attribution string, reflecting what’s available in mbgl. MGLTileSet’s properties have been converted into options that can be passed into MGLTileSource’s initializers in a dictionary, similar to the options for MGLShapeSource. MGLTileSource retains an attributionInfos property that is now computed based on the underlying mbgl object and represented as an array of MGLAttributionInfo objects. MGLAttributionInfo is now public for this reason.

MGLShape has a new class initializer and instance method that deserialize GeoJSON data into a shape and serialize the shape into GeoJSON data, respectively. The new initializer handles parsing errors gracefully via an NSError out parameter, which causes the bridged method in Swift to be marked with the throws keyword.

In an effort to reduce parallel state and parallel code paths in MGLShape, this class no longer has an initializer that accepts GeoJSON data or a property that returns GeoJSON data. The URL property’s getter, meanwhile, is computed based on the underlying mbgl object. Developers working with GeoJSON data are now expected to go through the new MGLShape initializer to get an MGLShape representation. Alternatively, if performance is a concern, a local file URL can be passed into the other MGLShapeSource initializer, which has been left intact.

This refactoring exposed a latent bug in the source and style layer classes. The covariant rawSource property was incorrectly implemented by both MGLSource and its concrete subclasses. The idea was that the subclasses would merely constrain the property’s type, but they ended up causing shadow ivars to get autosynthesized, so that a subclass could set _rawSource directly (rather than going through the rawSource setter) but could then get a different value out of the rawSource getter.

These changes mean some existing example code that uses the runtime styling API will need to be modified. We should therefore land these changes before the next beta, which we expect to be the final beta. I don’t think the changes will be a showstopper, but we do need to keep an eye out for any leaps of logic that may not be obvious to developers with fresh eyes. In particular, we may want to consider adding a convenience initializer (but not a convenience property) to MGLShapeSource that takes an array of features (and turns it into an MGLShapeCollectionFeature under the hood), since that’s one of the most common use cases for MGLShapeSource. I attempted to bridge this gap in documentation, but I’m concerned that this initializer’s absence may reinforce a misconception that each shape needs its own source or style layer.

A summary of the significant changes:

Other changes:

  • Audit public and private MGL*Source headers for nullability
  • Mark -[MGLSource init] as unavailable and audit designated initializers on concrete MGLSource subclasses
  • Make MGLTileSet public in the macOS SDK
  • Fix covariant MGL*Source.rawSource properties to prevent shadowing
  • Fix covariant MGL*StyleLayer.rawLayer properties to prevent shadowing
  • Reduce the number of MGLShape conversion methods
  • Add missing headers to ios.xcodeproj and macos.xcodeproj

Before this PR can land:

/cc @frederoni @incanus @ericrwolfe

@1ec5 1ec5 added crash iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor runtime styling labels Dec 12, 2016
@1ec5 1ec5 added this to the ios-v3.4.0 milestone Dec 12, 2016
@1ec5 1ec5 self-assigned this Dec 12, 2016
@1ec5 1ec5 requested a review from boundsj December 12, 2016 01:03
@mention-bot
Copy link

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

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 12, 2016

The test failures are due to a test I’ve added of the new serialization routine that ensures MGLFeatures are serialized as GeoJSON features, not geometries. This test will fail until #7339 lands. A few merge conflicts will arise from #7339 landing, but they shouldn’t be a problem.

} catch (std::runtime_error &err) {
if (outError) {
*outError = [NSError errorWithDomain:MGLErrorDomain code:MGLErrorCodeUnknown userInfo:@{
NSLocalizedFailureReasonErrorKey: [NSString stringWithFormat:@"%s", err.what()],
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Could be simplified as @(err.what()) because it's UTF-8 encoded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to make this change before merging. Followed up in d34356c.

*/
@interface MGLSource : NSObject

#pragma mark Initializing a Source

- (instancetype)init __attribute__((unavailable("Use -initWithIdentifier: instead.")));
Copy link
Contributor

@frederoni frederoni Dec 12, 2016

Choose a reason for hiding this comment

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

Isn't it possible use explanation with NS_UNAVAILABLE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it isn't. 😕

@boundsj
Copy link
Contributor

boundsj commented Dec 12, 2016

#7339 (with MGL -> mbgl conversion) has been merged

@boundsj
Copy link
Contributor

boundsj commented Dec 12, 2016

For clarity, I updated the original description as follows:

In an effort to reduce parallel state and parallel code paths in MGLShape MGLShapeSource

Copy link
Contributor

@boundsj boundsj left a comment

Choose a reason for hiding this comment

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

I think I noticed an issue with the raster source tile size default but, otherwise, this looks great to me!

}
return self;
- (instancetype)initWithIdentifier:(NSString *)identifier configurationURL:(NSURL *)configurationURL {
return [self initWithIdentifier:identifier configurationURL:configurationURL tileSize:0];
Copy link
Contributor

Choose a reason for hiding this comment

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

We say "The default value for this option is 512 points on Retina displays and 256 points otherwise." in the API documentation but I don't see where we actually set the value. If you change the iosapp example to use this initializer in https://github.com/mapbox/mapbox-gl-native/blob/1ec5-tile-unset-7360/platform/ios/app/MBXViewController.m#L755 the raster layer is not visible because of the value 0 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I meant to follow up on this. The style specification says tileSize is optional, defaulting to 512. However, via this code path:


url = util::mapbox::canonicalizeTileURL(url, type, tileSize);

result += tileSize == util::tileSize ? "@2x" : "{ratio}";

a tileSize of 512 should cause @2x to be hardcoded into the tile URL template regardless of the screen resolution, whereas any other value (including 0) would cause {ratio} to be used. I want the {ratio} behavior by default, as that optimizes for the screen resolution.

Copy link
Contributor Author

@1ec5 1ec5 Dec 13, 2016

Choose a reason for hiding this comment

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

the raster layer is not visible because of the value 0 here

This demo explicitly applies an opacity function to the layer so that it’s invisible at low zoom levels but becomes visible as you zoom in. The initializer appears to work fine for me.

@jfirebaugh, is it intentional that, by default, the current code on master and the release branch loads Retina raster tiles on non-Retina displays like iPad 2 (due to the default tile size of 512 pixels)? It seems like the more sensible default would be a value (any value) other than 512, so that the tile resolution would depend on the screen resolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Continued in #7377 (comment).

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 13, 2016

6a1abf5905dffdee1cb62af482fcb44b3b54aaa4 further streamlines MGLShape and subclasses by replacing -mbglFeature and -mbglFeatureCollection with -geoJSONObject. #7339 is no longer recognizable as such, but the fix is still in there thanks to class inheritance and variants.

@boundsj
Copy link
Contributor

boundsj commented Dec 13, 2016

6a1abf5 further streamlines MGLShape and subclasses by replacing -mbglFeature and -mbglFeatureCollection with -geoJSONObject

👍

square measuring 512 <em>pixels</em> on each side. If this option is set to a
value other than 256 or 512, the behavior is undefined.

The default value for this option is 256. This option is only applicable to
Copy link
Contributor Author

@1ec5 1ec5 Dec 13, 2016

Choose a reason for hiding this comment

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

This was a difficult option to document. I arrived at this verbose description of the tileSize option based on discussions with @jfirebaugh and @tmcw. For simplicity’s sake, I described only two valid values, 256 and 512, leaving all other values undefined.

MGLRasterTileSource applies a default value of 256 for mapbox: tile URLs and 512 for any other tile URL. This differs from the style specification and GL JS, which default to 512 regardless of origin. I believe MGLRasterTileSource’s behavior is justified, because the MGLRasterTileSource default and the underlying option in mbgl are applied only to mapbox: tile URLs, and mapbox: tile URLs are always normalized into Maps API v4 URLs. We do expect v4 URLs to be used with a 256 tileSize.

Apparently we intend to revise the Maps API in the future to serve 512-point tiles (measuring 1 square kibipixel on a Retina display). A default value of 512 would make sense for requests to that API. However, I’d imagine that we’d map something other than mapbox://<mapid> to Maps API v5, in which case MGLRasterTileSource would automatically apply the standard 512 default, or we’d drop support for v4 tiles, which would be a backwards incompatible change anyways.

Meanwhile, at the moment, I believe a default other than 256 for mapbox: URLs would only lead to bugs in application code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Other than the two corrections noted above, I agree with what you say here and with implementing a default of 256 for mapbox: URLs.

This option applies only to tiles hosted by Mapbox and accessed through version
4 of the
<a href="https://www.mapbox.com/api-documentation/#maps">Mapbox Maps API</a>,
such as through `mapbox://<mapid>` URLs. This option has no effect on tiles
Copy link
Contributor

@jfirebaugh jfirebaugh Dec 13, 2016

Choose a reason for hiding this comment

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

On the contrary, this option is relevant and does have an effect for third-party tile servers. In fact, most sources which use a third-party tile service will want to specify a value of 256; if a tile service provides 256 pixel tiles and the source uses a value of 512, tiles will appear at twice their intended size and appear blurry or pixelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I missed the existence of TileSourceImpl::getTileSize(), so I thought tileSize was only used for the call to canonicalizeTileURL(). It make sense that coveringZoomLevel() would depend on this option too. I’ll update the documentation.

Given that third-party tile servers are affected, I can see now why a default value of 512 would be the forward-looking choice. I think it’d still be desirable to special case mapbox: URLs to default to 256, since it’s a known quantity, although now I can see the argument for treating it like any other tile source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose this means 256 and 512 aren’t the only valid values, either, although 512 is still a magic value with respect to mapbox: URLs.

256, the source requests 1× images on a standard-resolution display and 2×
images on a Retina display, and each image is scaled to fit in a square
measuring 256 points on each side. When the value is 512, the source requests
2× images regardless of the screen resolution, and each image is displayed in a
Copy link
Contributor

Choose a reason for hiding this comment

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

This particular behavior is specific to sources using a mapbox:// URL. For third party tile sources, there is no connection between this option and the behavior of the {ratio} placeholder in template URLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The special 512 behavior for mapbox: URLs doesn’t fit into the coherent (and erroneous) model I originally thought it did. Explaining it will only make the option more confusing. I think we can omit this paragraph. mapbox: URLs will have a sensible default behavior, so it’s less likely that someone will run into this edge case.

MGLRasterSource and MGLVectorSource now share a common abstract superclass, MGLTileSource. MGLTileSet has been removed. MGLTileSource is modeled after mbgl::style::RasterSource and mbgl::style::VectorSource. It has initializers that incorporate the parameters of MGLTileSet’s initializers, but it lacks getters for everything but the attribution string. MGLTileSet’s properties have been converted into options that can be passed into MGLTileSource’s initializers in a dictionary.

Properly implement rawSource as a covariant property so that it doesn’t end up getting autosynthesized as a shadow ivar. This prevents concrete subclasses of MGLSource from setting _rawSource directly but getting a different value out of self.rawSource.

Marked -[MGLSource init] as unavailable and ensured that concrete subclasses of MGLSource have the right set of initializers marked as designated initializers.

Documentation comments for each concrete source class identify the corresponding source type in the style specification. Clarified the purpose of MGLTileSetScheme, now known as MGLTileCoordinateSystem.
Sticking to a default value of 256 for mapbox: URLs, but other URLs get the standard 512 value.
rawSource is never nil, so there’s no need for a -commonInit method. Extracted -geoJSONOptions from MGLShapeSource into a standalone function for easier testing.
Realphabetized headers in groups. Added headers missing from one project or the other.
Added a class initializer and instance method to MGLShape that deserialize and serialize the shape as GeoJSON data, respectively. The new initializer handles parsing errors gracefully.

Removed methods specific to GeoJSON data from MGLShapeSource, in an effort to reduce parallel state. Developers are now expected to go through the new MGLShape initializer to get an MGLShape representation. Alternatively, a local file URL can be passed into the other MGLShapeSource initializer.
Every MGLShape now knows its most specific mbgl::GeoJSON representation.
mbgl::GeoJSON, which is a variant, allows a single GeoJSON representation method to traffic in whatever type is needed for a particular shape class. This change removes some hidden private protocols, which are a bug waiting to happen.
Properly implement rawLayer as a covariant property so that it doesn’t end up getting autosynthesized as a shadow ivar. This prevents concrete subclasses of MGLStyleLayer from setting _rawLayer directly but getting a different value out of self.rawLayer.
Made MGLAttributionInfo public. Replaced MGLTileSource’s attribution property with an attributionInfos property set to an array of MGLAttributionInfo objects. Added an MGLTileSourceOption for specifying an array of MGLAttributionInfo objects instead of an HTML string (either is acceptable when creating an MGLTileSource).
@1ec5 1ec5 merged commit ef71597 into release-ios-v3.4.0 Dec 14, 2016
@1ec5 1ec5 deleted the 1ec5-tile-unset-7360 branch December 14, 2016 01:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crash iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor runtime styling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants