Computed (on-demand) shape source by JesseCrocker · Pull Request #6940 · mapbox/mapbox-gl-native

JesseCrocker · 2016-11-07T14:18:33Z

Implements new API proposed in #6861 for a custom vector source, that queries a class supplied by the client application for the features to display on a tile.

This defines a new class in core, mbgl::style::CustomVectorSource, and a corresponding new source class for iOS/MacOS MGLCustomVectorSourceDataSource. There is also an new interface defined i obj-c, MGLCustomVectorSourceDataSource that defines a single method - (void)getTileForZoom:(NSInteger)zoom x:(NSInteger)x y:(NSInteger)y callback:( void (^)(NSArray<id <MGLFeature>>*) )callback

I see 4 uses for this:

Data that is generated dynamically. A trivial example of this would be a lat/lon grid, that data is very simple, but a grid with a resolution of 0.01 degrees contains 216000 lines.
Data that is too large to fit in memory. If data has a bounded size, it can be used as a GeoJSON sources, but if the data can be arbitrarily large converting it to GeoJSON may not be possible, for example shapefiles.
Data that is fetched from the network, but not as Mapbox Vector Tiles. With this API you could implement an OSM editor, or a WFS client.
Data that already exists in a format with some sort of spatial index, where querying data for a single tile is faster than fetching all of the data. For example, a set of POI data stored in an SQLite database.

My use case for this is displaying GPS traces, which is #2 and #4 from the above list. I develop an application where users frequently have hundreds of GPS traces, but occasionally have tens of thousands, and some of them have hundreds of thousands of points per trace. The data can also be modified, either by the user, or by a background sync process, in either case the data will need to be redisplayed.

mention-bot · 2016-11-07T14:18:35Z

@JesseCrocker, thanks for your PR! By analyzing the history of the files in this pull request, we identified @boundsj, @1ec5 and @frederoni to be potential reviewers.

1ec5

Just some cursory feedback for starters.

1ec5 · 2016-11-07T16:48:52Z

Rename "zoom" to "zoomLevel" for consistency with other SDK APIs. Should x and y be unsigned? Rename "callback" to "completionHandler" for consistency.

1ec5 · 2016-11-07T16:50:04Z

Rename to -setNeedsUpdateAtZoomLevel:x:y:.

1ec5 · 2016-11-07T16:57:31Z

This is an unfortunate name for a class. Maybe someone else has an idea for what to call it. Also, it seems like it should declare some initializers and properties related to the options declared above.

1ec5 · 2016-11-07T16:58:55Z

Indent by four spaces.

1ec5 · 2016-11-10T10:06:52Z

Use four spaces per tab stop.

1ec5 · 2016-11-10T10:11:26Z

Fill in this documentation and rename callback. Point out that completionHandler must be called. Incidentally, is it absolutely necessary to make this API asynchronous? A synchronous data source method would be far less error-prone.

This doesn't need to be async, since it's called on it's own NSOperationQueue. But it definitely would not work to have be it be synchronous on the main queue. Should i change this to be synchronous?

1ec5 · 2016-11-10T10:11:43Z

s/Usefull/Useful/

1ec5 · 2016-11-10T10:16:55Z

Rename to -reloadData for consistency with Cocoa classes like UITableView.

1ec5 · 2016-11-10T10:20:05Z

It isn’t usually obvious to the developer what to pass in as x and y. Would it be possible to rework this method to take an MGLCoordinateBounds, à la -[MKOverlayRenderer setNeedsDisplayInMapRect:zoomScale:]?

1ec5 · 2016-11-10T10:27:56Z

Revert this change. The code signing issue was fixed in #6948.

1ec5 · 2016-11-10T10:28:19Z

Revert this change too.

1ec5 · 2016-11-10T10:28:48Z

Also import MGLGeoJSONSourceBase.h.

1ec5 · 2016-11-10T10:32:08Z

On second thought, maybe this should be called -reloadTileInCoordinateBounds:zoomLevel:], for consistency with -reloadData.

1ec5 · 2016-11-10T10:36:06Z

Properties that refer to data sources should be weak to avoid a retain cycle, should not copy, and should be id rather than NSObject. Is there a reason it’s read-only? It’s unusual for a data source to be used on a custom queue.

1ec5

I’m excited to see this PR come together. Hope you don’t mind my continued waffling on API naming. Trying to get this right so we don’t confuse people who are familiar with Cocoa APIs.

Don’t forget to update the iOS and macOS changelogs and add any new classes to the jazzy configuration files so they show up at the right place in the table of contents.

So far I’ve only looked at the Objective-C side of these changes. @kkaefer, could you have a look at the C++ side? To kick things off, how about some tests? 😄

1ec5 · 2016-11-12T18:10:15Z

Now that this is a simple getter, it should be called -featuresInTileAtX:y:zoomLevel:, without the “get”.

I think it would be just as generally useful to have a -featuresInCoordinateBounds:zoomLevel:, per #6940 (comment), to accommodate those migrating from MKOverlayRenderer. If you think it’s worth keeping a tile coordinate–based API around, perhaps both methods could be optional methods on this protocol. MGLCustomVectorSource would end up calling one or the other, depending on which one is implemented.

1ec5 · 2016-11-12T18:17:57Z

In Cocoa, data sources aren’t normally required; when absent, the class that normally uses the data source assumes no data is available. By following that model here, it’ll be possible to set up a custom vector source before setting up its data source, which may be handy if the data source takes some time to set up. This would entail removing the dataSource: parameter from this method and making the dataSource property nullable.

1ec5 · 2016-11-12T18:21:43Z

When filling in the documentation, make sure to mention that this method is always called on the operation queue in the requestQueue property.

JesseCrocker · 2016-11-14T16:45:39Z

@1ec5 Thanks for working with me on this, I know it's added work for you that wasn't on the roadmap.
So far I'm still at less than half the time I estimated this was going to take to implement. For a codebase of this size, it's been really easy to get started on.

One issue i'm running into is when I run make iproj the mbgl target in the generated workspace doesn't include the new files i've added to core. Do I need to put those somewhere?

JesseCrocker · 2016-11-14T18:11:25Z

@1ec5 to Implement featuresInCoordinateBounds:zoomLevel: It needs to find the bounds for a tile, it looks like mbgl::LatLngBounds already has a constructor to create a LatLngBounds from a CanonicalTileID, but mbgl/tile/tile_id.hpp can't be included in any of the obj-c++ classes, because it includes boost/functional/hash.hpp, and boost isn't in the search path for the iOS targets. Is it OK to add boost to the header search path for the iOS targets?

1ec5 · 2016-11-14T23:43:02Z

One issue i'm running into is when I run make iproj the mbgl target in the generated workspace doesn't include the new files i've added to core. Do I need to put those somewhere?

Yes, after adding the files to core, run ./scripts/generate-core-files.sh.

1ec5 · 2016-11-14T23:50:35Z

Is it OK to add boost to the header search path for the iOS targets?

Yes, I think it would be OK, although we’ll want to watch the SDK for any significant size increase as a result. To add Boost, add it as a Mason package like we do in core. Be sure to update the macOS configuration too.

1ec5 · 2016-11-22T23:47:42Z

The parts of tile_id.hpp that depend on Boost are the three template specializations of hash at the bottom. Maybe they could be split into a separate header; that would enable the SDK to include tile_id.hpp without including Boost.

1ec5 · 2016-11-22T23:58:32Z

As I’ve mentioned before, we need better names for MGLCustomVectorSource and MGLGeoJSONSourceBase, especially the latter. Meanwhile, #7160 proposes renaming MGLGeoJSONSource to MGLFeatureSource, since most use cases for MGLGeoJSONSource don’t even involve actual GeoJSON source code.

So, two proposals:

MGLGeoJSONSourceBase → MGLFeatureSource
MGLGeoJSONSource stays the same
MGLCustomVectorSource → MGLComputedSource
MGLCustomVectorSourceDataSource → MGLComputedSourceDataSource
MGLGeoJSONSourceBase → MGLFeatureSource
MGLGeoJSONSource → MGLCollectionSource (given rename MGLGeoJSONSource to MGLFeatureSource #7160 and Allow initializing MGLGeoJSONSource from a shape #7145)
MGLCustomVectorSource → MGLComputedSource
MGLCustomVectorSourceDataSource → MGLComputedSourceDataSource

Any other ideas?

/cc @incanus

…orSource

JesseCrocker · 2017-03-01T23:59:41Z

I've added a couple of tests for this, both on the obj-c side and the c++ side, but i dont think they are really testing anything useful. Any guidance as to how to test this, especially the core part of it, would be greatly appreciated.

JesseCrocker · 2017-03-03T01:08:27Z

After doing some more testing with this, I turned up memory corruption when a source was removed, and a tile fetch was in progress. My last commit should fix that.

1ec5

Other than the one nit, my feedback on the SDK side has all been addressed.

1ec5 · 2017-03-03T11:53:23Z

+@implementation MGLComputedShapeSourceTests
+
+- (void)testInitializer {
+  MGLComputedShapeSource *source = [[MGLComputedShapeSource alloc] initWithIdentifier:@"id"


Nit: indent by four spaces for consistency.

1ec5 · 2017-03-03T11:56:46Z

+ Do not create instances of this class directly, and do not create your own
+ subclasses of this class. Instead, create instances of `MGLShapeSource` or 
+ `MGLComputedShapeSource`.
+ */


By the way, this is totally optional, but we have a mechanism for inserting a Swift code example into a documentation comment via unit tests. Feel free to use MGLShapeSource’s documentation as a model.

JesseCrocker · 2017-03-06T14:15:21Z

Im still working on fixing a use-after-free issue with this.
How to trigger the problem:
Add an MGLComputedShapeSource to the style, with a dataSource that takes some time to return data.
Call [mapView setStyleUrl:] to change the map style

If any calls to the data source were in progress when the style was changed, they will end up calling setTileData on a mbgl::style::CustomVectorSource that has been deallocated.

Address sanitizer picks up the use after free, but without it turned on this manifests as all sort of random crashes from memory corruption.

I've got a good test case for this at trailbehind@1f5f51e , It calls cycleStyles:on a timer, and adds a MGLComputedShapeSource to the style in mapView:didFinishLoadingStyle:

I see 2 different ways to look at this problem:

dealloc doesn't get called on MGLComputedShapeSource if there are any fetch operations in progress, because the operation has a reference to the MGLComputedShapeSource. If dealloc did get called, the MGLComputedShapeSourceFetchOperation would get canceled, and self.source.rawSource->setTileData would not get called.
MGLComputedShapeSource has no way to know when its mbgl::style::CustomVectorSource has been dealloced, because removeFromMapView: does not get called, and MGLComputedShapeSource only has a raw pointer to the mbgl::style::CustomVectorSource

Fixing based on 2 seems like the better fix, but it seems like a big change that would be beyond the scope of this PR.
2 ways i would think it could be fixed based on 2:

Call removeFromMapView: when a style is dealloced.
Switch to use shared_ptr instead of unique_ptr for sources

So im guessing fixing based on 1 is the way to go, but im having trouble getting it to work right.

There's also a third option, let this crash, and add a note to the documentation that the source must be removed from the style before the style is removed from the map.

@1ec5 Do you have any suggestion for how to approach this?

JesseCrocker · 2017-03-06T17:04:47Z

I've got the crash mentioned above fixed by removing the reference to the MGLComputedShapeSource from the MGLComputedShapeSourceFetchOperation

kkaefer

Thank you for your pull request! This is definitely something that we want to add, but it's very hard to review this pull request since it's scattered over so many commits. We generally require all commits to only contain self-contained changes, so before merging this, we'll either have to squash this into one commit (or preferably) isolate self-contained changes in this PR and commit them individually.

I realize that you're more familiar with the iOS/macOS side of things, but we also need Android and ideally Qt bindings before adding this change, as well as a lot more changes that also test rendering of generated tiles.

kkaefer · 2017-03-02T01:35:19Z

+}
+
+void CustomVectorSource::Impl::setTileData(uint8_t z, uint32_t x, uint32_t y, const mapbox::geojson::geojson& geoJSON) {
+    double scale = util::EXTENT / util::tileSize;


constexpr

kkaefer · 2017-03-02T01:35:43Z

+void CustomVectorSource::Impl::setTileData(uint8_t z, uint32_t x, uint32_t y, const mapbox::geojson::geojson& geoJSON) {
+    double scale = util::EXTENT / util::tileSize;
+
+    if(geoJSON.is<FeatureCollection>() && geoJSON.get<FeatureCollection>().size() == 0) {


.size() == 0 => !.empty()

kkaefer · 2017-03-02T01:36:22Z

+    if(geoJSON.is<FeatureCollection>() && geoJSON.get<FeatureCollection>().size() == 0) {
+        for (auto const &item : tiles) {
+            GeoJSONTile* tile = static_cast<GeoJSONTile*>(item.second.get());
+            if(tile->id.canonical.z == z && tile->id.canonical.x == x && tile->id.canonical.y == y) {


Instead of passing z/x/y separately, can we use CanonicalTileID and then compare the objects directly?

kkaefer · 2017-03-06T18:04:29Z

+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wunused-parameter"
+    const auto callback = ^void(uint8_t z, uint32_t x, uint32_t y) {};


Can we instead use C++11 lambdas rather than Apple-Clang specific syntax? You can also remove the z/x/y names to avoid the pragmas. An even better approach would be to pass a null pointer as the callback and check whether callback is invokable before calling it.

kkaefer · 2017-03-06T18:05:23Z

+};
+
+
+} // namespace std


Why was this moved to a separate file? While not wrong, it doesn't look like it's necessary to include in this PR.

It was split into a separate file so that tile_id.hpp could be included in objective-c++ files in the iOS/macOS side of things without having to include boost.

#6940 (comment) 👍

kkaefer · 2017-03-06T18:06:56Z

+        MGLPolylineFeature *feature = [MGLPolylineFeature polylineWithCoordinates:coords count:2];
+        feature.attributes = @{@"value": @(x)};
+        [features addObject:feature];
+    }


Is this entire function something that should be in the core code rather than in a binding-specific code?

You mean as an implementation of #119? I don’t think it satisfies the requirements for that feature, namely that the graticule would have to be visible before the style even starts to load. But it does make for an interesting demo. (This file is part of macosapp; it isn’t part of any SDK.)

kkaefer · 2017-03-06T18:09:09Z

 struct GeoJSONOptions {
    // GeoJSON-VT options
+    uint8_t minzoom = 0;
+


kkaefer · 2017-03-06T18:11:59Z

+    CustomVectorSource(std::string id, GeoJSONOptions options, std::function<void(uint8_t, uint32_t, uint32_t)> fetchTile);
+
+    void setTileData(uint8_t, uint32_t, uint32_t, const mapbox::geojson::geojson&);
+    void updateTile(uint8_t, uint32_t, uint32_t);


What is the difference between setTileData and updateTile? If they're distinct, can we add some documentation?

JesseCrocker · 2017-03-07T16:10:55Z

@kkaefer Thanks for reviewing this. I think i've addressed all of your comments. Sorry about the large number of commits, i had to do some iteration to make this work, and then there were a bunch of commits renaming things based on review feedback.

Im well versed in Android development and capable of doing the Android bindings for this, but probably wont have time to get to it for a couple months.

1ec5 · 2017-03-20T22:00:37Z

#8473 is a squashed, reorganized version of this PR.

ychescale9 · 2017-10-17T06:53:00Z

Wondering if there's any progress (or interests from other users) on this?
We are currently looking at using MBTiles in Android (offline) and evaluating different options.
Hence we would like to know if adding support for MBTiles source is on the roadmap. Thanks!

jfirebaugh · 2017-10-24T20:10:45Z

→ #8473.

1ec5 suggested changes Nov 7, 2016

View reviewed changes

1ec5 mentioned this pull request Nov 10, 2016

Refactor source interface to better accommodate non-tiled sources mapbox/mapbox-gl-js#3186

Open

1ec5 suggested changes Nov 10, 2016

View reviewed changes

JesseCrocker force-pushed the feature/custom-vector-source branch from a614fe3 to 8cba5ea Compare November 10, 2016 12:46

1ec5 added Core The cross-platform C++ core, aka mbgl feature iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling labels Nov 12, 2016

1ec5 suggested changes Nov 12, 2016

View reviewed changes

incanus mentioned this pull request Nov 22, 2016

rename MGLGeoJSONSource to MGLFeatureSource #7160

Closed

JesseCrocker force-pushed the feature/custom-vector-source branch from 603f2f5 to f159953 Compare November 29, 2016 21:21

Jesse Crocker added 11 commits December 8, 2016 10:30

Sketching out CustomVectorSource interface

403ac49

Sketch out interface

5d74f08

Add to Mapbox.h header

579ef87

Move tilesize into options

095281e

obj-c class is getting called for tiles

1656672

Get rid of setSourceLoaded non-sense

d2c62ef

It works!

908a7e2

Inherit Options from GeoJSONOptions

30c9cb9

Create a common base class between MGLGeoJSONSource and MGLCustomVect…

aa5b27e

…orSource

Dont use hardcoded default tile size

b6015b8

Fix indentation

6986db5

Jesse Crocker added 7 commits March 1, 2017 11:52

Fix duplicated options after merging

cbe2815

Fix merge error

b31841d

fix make test failing

13ef94e

Fix failing test

ab55326

Test for MGLComputedShapeSource, but all it tests is the initializer

3fcb46f

First test for CustomVectorSource

f01e1f2

A couple more tests

f75b34d

Fix memory corruption when removing a ComputedShapeSource

c73f6de

1ec5 approved these changes Mar 3, 2017

View reviewed changes

Change indent in MGLComputedShapeSourceTests to 4 spaces

0be7292

1ec5 changed the title ~~Add Custom Vector Source class for iOS/Mac OS~~ Computed (on-demand) shape source Mar 4, 2017

Update layer removal to be consistent with mapbox#7962

86ab288

Fix crash when source is dealloced

4bb3a9c

kkaefer suggested changes Mar 6, 2017

View reviewed changes

Address review comments

883f72e

incanus mentioned this pull request Mar 17, 2017

MBTiles source #6862

Closed

kkaefer mentioned this pull request Mar 20, 2017

Custom vector source #8473

Closed

9 tasks

kkaefer mentioned this pull request May 30, 2017

add support for geodesic polylines (and thus great circles) #1726

Closed

jfirebaugh closed this Oct 24, 2017

1ec5 mentioned this pull request Oct 26, 2017

Custom Vector Source (n+1) #9983

Merged

7 tasks

1ec5 mentioned this pull request Feb 28, 2018

Exposing MGLGeometry_Private.h C++ globals #9819

Closed

1ec5 mentioned this pull request Aug 21, 2018

MGLComputedShapeSource/CustomGeometrySource.java shouldn't create thread pools #11895

Open

Conversation

JesseCrocker commented Nov 7, 2016

Uh oh!

mention-bot commented Nov 7, 2016

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JesseCrocker commented Nov 14, 2016

Uh oh!

JesseCrocker commented Nov 14, 2016

Uh oh!

1ec5 commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1ec5 commented Nov 14, 2016

Uh oh!

1ec5 commented Nov 22, 2016

Uh oh!

1ec5 commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JesseCrocker commented Mar 1, 2017

Uh oh!

JesseCrocker commented Mar 3, 2017

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

1ec5 Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JesseCrocker commented Mar 6, 2017

Uh oh!

JesseCrocker commented Mar 6, 2017

Uh oh!

kkaefer left a comment

Choose a reason for hiding this comment

1ec5 commented Nov 14, 2016 •

edited

Loading

1ec5 commented Nov 22, 2016 •

edited

Loading

1ec5 Mar 3, 2017 •

edited

Loading

1ec5 Mar 7, 2017 •

edited

Loading