Arbitrary offline region shapes #11447

ivovandongen · 2018-03-14T14:33:37Z

Adds the ability to specify offline regions shapes as any GeoJson geometry.

Depends on:

TODO:

zugaldia · 2018-03-16T19:39:43Z

Replaces OfflineTilePyramidRegionDefinition with OfflineRegionDefinition as it covers all shapes.

Would it be possible to keep both around in order not to break the public API (for now)?

ivovandongen · 2018-04-11T08:47:59Z

@zugaldia I've included the changes to the offline tool in this pr:

  ./build/macos/Debug/mbgl-offline {OPTIONS}

    Mapbox GL offline tool

  OPTIONS:

      -h, --help                        Display this help menu
      -t[key], --token=[key]            Mapbox access token
      -s[URL], --style=[URL]            Map stylesheet
      -o[file], --output=[file]         Output database file name
      LatLng bounds:
        --north=[degrees]                 North latitude
        --west=[degrees]                  West longitude
        --south=[degrees]                 South latitude
        --east=[degrees]                  East longitude
      GeoJson geometry:
        --geojson=[file]                  GeoJSON file containing the region
                                          geometry
      --minZoom=[number]                Min zoom level
      --maxZoom=[number]                Max zoom level
      --pixelRatio=[number]             Pixel ratio

asheemmamoowala · 2018-04-26T17:20:41Z

platform/default/mbgl/storage/offline_download.cpp

+    for (uint8_t z = clampedZoomRange.min; z <= clampedZoomRange.max; z++) {
+        result += definition.match(
+                [&](const OfflineTilePyramidRegionDefinition& reg){ return util::tileCount(reg.bounds, z); },
+                [&](const OfflineGeometryRegionDefinition& reg){ return util::tileCount(reg.geometry, z); }


The mbgl::util::tileCount(Geometry<>...) method will block, especially when called in a tight loop like this. #9675 was filed in the past for this same problem with large LatLngBounds

@asheemmamoowala Correct. However, this is identical to the previous implementation.

I'm all for speeding this up. But maybe in a separate issue?

That seems fine. I profiled these changes using mbgl-offline and don't see the tile count or tile cover operations showing up as significant blips on the main thread.

1ec5

In addition to the comments below, please:

Remove the note about MGLTilePyramidOfflineRegion from MGLOfflineRegion’s documentation comment, since it’s no longer an only child.
Add a blurb to the iOS and macOS changelogs noting the new class as a more granular alternative to MGLTilePyramidOfflineRegion.

Thanks!

1ec5 · 2018-06-28T00:44:46Z

platform/darwin/src/MGLGeometryOfflineRegion.h

+ range of zoom levels.
+ */
+MGL_EXPORT
+@interface MGLGeometryOfflineRegion : NSObject <MGLOfflineRegion, NSSecureCoding, NSCopying>


On iOS and macOS, we say “shape” instead of “geometry”.

1ec5 · 2018-06-28T00:47:03Z

platform/darwin/src/MGLGeometryOfflineRegion.h

+ The geometry bounds for the geographic region covered by the downloaded
+ tiles.
+ */
+@property (nonatomic, readonly) MGLShape *geometry;


This property would be named shape rather than geometry.

MGLPointAnnotation is a subclass of MGLShape that corresponds to a Point in GeoJSON. If the region needs to have dimension, we should require that the shape conform to the MGLOverlay protocol (MGLShape <MGLOverlay> *), though that would still allow for MGLPolyline. If the region needs to be two-dimensional, then we’d need to introduce either a runtime class check or a new protocol like “MGLSpaceFilling”.

1ec5 · 2018-06-28T00:48:44Z

platform/darwin/src/MGLGeometryOfflineRegion.h

+ The URL may be a full HTTP or HTTPS URL or a Mapbox URL indicating the style’s
+ map ID (`mapbox://styles/{user}/{style}`).
+ */
+@property (nonatomic, readonly) NSURL *styleURL;


Let’s promote styleURL to the MGLOfflineRegion protocol, since it’s shared in common between the two conforming classes and we don’t expect to introduce a new region class that doesn’t depend on a style URL. I could go either way about promoting minimumZoomLevel and maximumZoomLevel.

1ec5 · 2018-06-28T00:52:38Z

platform/darwin/src/MGLGeometryOfflineRegion.mm

+    [NSException raise:@"Method unavailable"
+                format:
+     @"-[MGLGeometryOfflineRegion init] is unavailable. "
+     @"Use -initWithStyleURL:bounds:fromZoomLevel:toZoomLevel: instead."];


Replace “bounds” with “shape”.

1ec5 · 2018-06-28T00:57:53Z

platform/darwin/src/MGLGeometryOfflineRegion.h

+
+/**
+ An offline region defined by a style URL, geographic coordinate bounds, and
+ range of zoom levels.


Replace “coordinate bounds” with “shape”.

1ec5 · 2018-06-28T00:58:48Z

platform/darwin/src/MGLGeometryOfflineRegion.h

+
+/**
+ An offline region defined by a style URL, geographic coordinate bounds, and
+ range of zoom levels.


Are there any benefits to using an MGLTilePyramidOfflineRegion instead of an MGLShapeOfflineRegion going forward, such as performance? If so, we can mention those benefits here and in MGLTilePyramidOfflineRegion’s documentation comment.

MGLTilePyramidOfflineRegion would perform better at computing the number of tile resources needed, and computing their actual IDs. But that performance advantage over MGLShapeOfflineRegion should be marginal when including the download as well.

More importantly, switching from using the bounding box to an actual shape would reduce the number of resources required, the storage and download cost. It would also help with the tile download ceiling for many customers interested in many small offline regions.

In that case, let’s mention the new class’s benefits in the documentation comments for both classes. The old class is being left in place for backwards compatibility reasons, but we don’t have to say that just yet since we aren’t deprecating it.

1ec5 · 2018-07-03T07:26:25Z

#12285 tracks adding a UI to macosapp for selecting a non-pyramidal offline region for download.

zugaldia · 2018-08-14T15:21:47Z

Tagging @julianrex to help review the last round of iOS changes. Otherwise it seems we're in good shape to get this merged.

1ec5 · 2018-08-14T16:52:32Z

By the way, most of the changes requested in #11447 (review) are renames, so that review should be straightforward to address.

julianrex

Looks good from the iOS/macOS side - we should address @1ec5's rename and protocol recommendations .

julianrex · 2018-08-14T18:02:31Z

platform/darwin/src/MGLGeometryOfflineRegion.mm

+    NSURL *_styleURL;
+}
+
+@synthesize styleURL = _styleURL;


You should be able to replace the @synthesize and the instance var declaration with a single @property

Wouldn't accept just the property, so left this for now.

👍

This ought to work, so I'd be interested in why it might not be for you:

@interface MGLShapeOfflineRegion () <MGLOfflineRegion_Private, MGLShapeOfflineRegion_Private> @property (nonatomic, copy) NSURL *styleURL; @end

It probably has to do with the fact that the property’s getter is now declared in a protocol, MGLOfflineRegion, rather than a concrete class, which would prevent autosynthesis. If that’s the case, then explicit synthesis is fine.

julianrex · 2018-08-14T18:19:07Z

platform/darwin/test/MGLOfflineStorageTests.mm

+    }];
+    [pack requestProgress];
+    [self waitForExpectationsWithTimeout:1 handler:nil];
+}


Very minor: we should probably have pack = nil; at the end here, since it's a __block variable.

julianrex · 2018-08-14T18:20:51Z

platform/darwin/test/MGLOfflineStorageTests.mm

+    }];
+    [self waitForExpectationsWithTimeout:2 handler:nil];
+
+    XCTAssertEqual([MGLOfflineStorage sharedOfflineStorage].packs.count, countOfPacks + 1, @"Added pack should have been added to the canonical collection of packs owned by the shared offline storage object. This assertion can fail if this test is run before -testAAALoadPacks.");


We may have to revisit this for Xcode 10 - IIRC tests can be run in a random order (though I believe this needs to be enabled)

1ec5 · 2018-08-20T17:43:38Z

platform/darwin/src/MGLOfflinePack.mm

@@ -78,7 +87,17 @@ - (void)dealloc {

    const mbgl::OfflineRegionDefinition &regionDefinition = _mbglOfflineRegion->getDefinition();
    NSAssert([MGLTilePyramidOfflineRegion conformsToProtocol:@protocol(MGLOfflineRegion_Private)], @"MGLTilePyramidOfflineRegion should conform to MGLOfflineRegion_Private.");
-    return [(id <MGLOfflineRegion_Private>)[MGLTilePyramidOfflineRegion alloc] initWithOfflineRegionDefinition:regionDefinition];
+    NSAssert([MGLShapeOfflineRegion conformsToProtocol:@protocol(MGLOfflineRegion_Private)], @"MGLGeometryOfflineRegion should conform to MGLOfflineRegion_Private.");


Replace all instances of MGLGeometryOfflineRegion in this PR with MGLShapeOfflineRegion.

1ec5 · 2018-08-20T17:46:49Z

platform/darwin/src/MGLShapeOfflineRegion.mm

+    [NSException raise:@"Method unavailable"
+                format:
+     @"-[MGLShapeOfflineRegion init] is unavailable. "
+     @"Use -initWithStyleURL:bounds:fromZoomLevel:toZoomLevel: instead."];


Replace bounds: with shape:.

1ec5 · 2018-08-20T17:48:46Z

platform/darwin/src/MGLTilePyramidOfflineRegion.h

@@ -13,17 +13,6 @@ NS_ASSUME_NONNULL_BEGIN
 MGL_EXPORT
 @interface MGLTilePyramidOfflineRegion : NSObject <MGLOfflineRegion, NSSecureCoding, NSCopying>


To minimize the resources required by an irregularly shaped offline region, use the MGLShapeOfflineRegion class instead.

/ref #11447 (comment)

1ec5 · 2018-08-20T17:50:43Z

platform/darwin/src/MGLShapeOfflineRegion.h

+/**
+ An offline region defined by a style URL, geographic shape, and
+ range of zoom levels.
+ */


This class requires fewer resources than MGLTilePyramidOfflineRegion for irregularly shaped regions.

/ref #11447 (comment)

julianrex

I think we just need to address @1ec5's comments, and we're good from the iOS/macOS side.

ivovandongen added iOS Mapbox Maps SDK for iOS Android Mapbox Maps SDK for Android ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold Core The cross-platform C++ core, aka mbgl labels Mar 14, 2018

ivovandongen self-assigned this Mar 14, 2018

ivovandongen force-pushed the ivd-arbitrary-offline-region-shapes branch from d36f496 to 479b788 Compare March 14, 2018 16:29

asheemmamoowala force-pushed the tile-cover-geometry branch from 9f6ae2c to aafac15 Compare March 15, 2018 01:31

asheemmamoowala force-pushed the tile-cover-geometry branch from aafac15 to 852963f Compare March 22, 2018 07:51

ivovandongen force-pushed the ivd-arbitrary-offline-region-shapes branch from 479b788 to e6eaabe Compare March 22, 2018 14:31

1ec5 mentioned this pull request Mar 22, 2018

Add method for calculating number of tiles in tile pyramid #11504

Open

ivovandongen force-pushed the ivd-arbitrary-offline-region-shapes branch from e6eaabe to 4495625 Compare March 23, 2018 14:41

asheemmamoowala force-pushed the tile-cover-geometry branch 2 times, most recently from fb53043 to 4bda046 Compare March 23, 2018 16:48

ivovandongen force-pushed the ivd-arbitrary-offline-region-shapes branch 2 times, most recently from 1bdf9a1 to 89d094b Compare March 26, 2018 18:44

1ec5 mentioned this pull request Mar 27, 2018

Support an OfflineRegionDefinition defined by an arbitrary shape #4106

Closed

ivovandongen force-pushed the ivd-arbitrary-offline-region-shapes branch 6 times, most recently from a2af3c3 to 2d44c1b Compare April 11, 2018 08:33

ivovandongen force-pushed the ivd-arbitrary-offline-region-shapes branch from 2d44c1b to 35d6140 Compare April 25, 2018 14:24

ivovandongen mentioned this pull request Apr 25, 2018

[core] Implement TileCover for polygonal regions #11267

Merged

3 tasks

asheemmamoowala reviewed Apr 26, 2018

View reviewed changes

ivovandongen force-pushed the ivd-arbitrary-offline-region-shapes branch from 35d6140 to ac556fe Compare April 27, 2018 07:56

ivovandongen changed the base branch from tile-cover-geometry to master April 27, 2018 07:57

ivovandongen added the macOS Mapbox Maps SDK for macOS label Apr 27, 2018

asheemmamoowala approved these changes Jun 27, 2018

View reviewed changes

1ec5 reviewed Jun 28, 2018

View reviewed changes

1ec5 mentioned this pull request Jul 3, 2018

Expose more options for downloading offline packs in macosapp #12285

Closed

asheemmamoowala mentioned this pull request Aug 3, 2018

Offline maps event mapbox/mapbox-events-android#193

Merged

zugaldia requested a review from julianrex August 14, 2018 15:20

julianrex reviewed Aug 14, 2018

View reviewed changes

ivovandongen force-pushed the ivd-arbitrary-offline-region-shapes branch 3 times, most recently from 084b3a2 to 27ce802 Compare August 20, 2018 16:58

1ec5 reviewed Aug 20, 2018

View reviewed changes

ivovandongen added 4 commits August 20, 2018 21:28

[core] offline region definition - add support for arbitrary geometries

e8b76aa

[android] arbitrary offline region geometries

1a9326f

[darwin] arbitrary offline region geometries

1a4cc2c

[offline] Add option to pass geojson input file

f0a4dcb

ivovandongen force-pushed the ivd-arbitrary-offline-region-shapes branch from 27ce802 to f0a4dcb Compare August 20, 2018 18:28

julianrex approved these changes Aug 20, 2018

View reviewed changes

ivovandongen merged commit fcb6804 into master Aug 20, 2018

ivovandongen deleted the ivd-arbitrary-offline-region-shapes branch August 20, 2018 19:49

1ec5 mentioned this pull request Aug 20, 2018

Show how to download a non-rectangular offline pack mapbox/ios-sdk-examples#206

Open

tobrun added this to the android-v6.5.0 milestone Aug 21, 2018

This was referenced Aug 21, 2018

Consolidate OfflineRegionDefintions #12685

Closed

Update MapView OfflineRegionDefintion initialize #12686

Merged

Optimize for OfflineGeometryRegionDefinition mapbox/mapbox-plugins-android#621

Closed

LukasPaczos mentioned this pull request Aug 24, 2018

Keep OfflineGeometryRegionDefinition fields and constructor #12734

Merged

1ec5 mentioned this pull request Aug 25, 2018

Performance issues with hundreds of offline packs #6651

Closed

zugaldia mentioned this pull request Aug 28, 2018

[ios] Add offline event #12751

Closed

asheemmamoowala mentioned this pull request Aug 28, 2018

[ios] Added an optional tile list parameter to MGLTilePyramidOfflineRegion #8012

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arbitrary offline region shapes #11447

Arbitrary offline region shapes #11447

ivovandongen commented Mar 14, 2018 •

edited

Loading

zugaldia commented Mar 16, 2018

ivovandongen commented Apr 11, 2018

asheemmamoowala Apr 26, 2018

ivovandongen Apr 27, 2018

asheemmamoowala Apr 30, 2018

1ec5 left a comment

1ec5 Jun 28, 2018

1ec5 Jun 28, 2018

1ec5 Jun 28, 2018

1ec5 Jun 28, 2018

1ec5 Jun 28, 2018

1ec5 Jun 28, 2018

asheemmamoowala Jun 28, 2018

1ec5 Jun 28, 2018

1ec5 commented Jul 3, 2018

zugaldia commented Aug 14, 2018

1ec5 commented Aug 14, 2018

julianrex left a comment

julianrex Aug 14, 2018

ivovandongen Aug 20, 2018

julianrex Aug 20, 2018

1ec5 Aug 20, 2018 •

edited

Loading

julianrex Aug 14, 2018

julianrex Aug 14, 2018

1ec5 Aug 20, 2018

1ec5 Aug 20, 2018

1ec5 Aug 20, 2018

1ec5 Aug 20, 2018

julianrex left a comment

		@@ -13,17 +13,6 @@ NS_ASSUME_NONNULL_BEGIN
		MGL_EXPORT
		@interface MGLTilePyramidOfflineRegion : NSObject <MGLOfflineRegion, NSSecureCoding, NSCopying>

Arbitrary offline region shapes #11447

Arbitrary offline region shapes #11447

Conversation

ivovandongen commented Mar 14, 2018 • edited Loading

zugaldia commented Mar 16, 2018

ivovandongen commented Apr 11, 2018

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

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

1ec5 commented Jul 3, 2018

zugaldia commented Aug 14, 2018

1ec5 commented Aug 14, 2018

julianrex 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

1ec5 Aug 20, 2018 • 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

julianrex left a comment

Choose a reason for hiding this comment

ivovandongen commented Mar 14, 2018 •

edited

Loading

1ec5 Aug 20, 2018 •

edited

Loading