Custom vector source

[kkaefer]

This is a squashed version of #6940, with changes separated by functional units rather than development history. This makes it easier to review the soundness of this pull request.

Missing:

• Android implementation
• Rendering unit tests

Needs discussion:

• The callback is implemented in a synchronous manner. This has a few implications: When you call source->setTileData(...) directly in the callback, the Tile object hasn't been added to the list of tiles yet, so setTileData does nothing.
• We are currently iterating through all tiles in the source when adding/reloading tiles. While the number of tiles in the std::map is probably low, we might be able to do better than linear search, e.g. by using std::equal_range and finding all OverzoomedTileIDs that contain the CanonicalTileID we are looking for)
• Why is there clustering code when adding geometries? I think it should be removed and we should use the generated geometry as is. It is up to the implementor to ensure the right level of detail.
• We also need to invalidate tiles in the Source object's cache of abandoned tiles when setTileData is called.
• Why is reloadRegion() required?
• Instead of forcing the implementor to hold on to a pointer of the CustomVectorSource object, we could provide a callback function that becomes a no-op if the Source object goes away. This makes for a safer API.
• I think we should rename CustomVectorSource to CustomGeometrySource to be in line with the naming of GeometryTile and GeometryTileWorker.

/cc @JesseCrocker @1ec5 @jonkan @frederoni @ivovandongen @incanus

[mention-bot]

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

[jfirebaugh]

@kkaefer What are the implications of this feature addition on the difficulty of implementing asynchronous rendering? For instance, does it introduce synchronous tile loading callbacks that would make it difficult to move the ownership of tile data to a background thread?

[1ec5]

Why is there clustering code when adding geometries? I think it should be removed and we should use the generated geometry as is. It is up to the implementor to ensure the right level of detail.

Yes, this makes sense. Since mbgl::style::VectorSource doesn’t support clustering, clustering really is specific to (declarative) GeoJSON sources. It would make sense to move MGLShapeSourceOption back to MGLShapeSource.h and limit them to MGLShapeSource.

Instead of forcing the implementor to hold on to a pointer of the CustomVectorSource object, we could provide a callback function that becomes a no-op if the Source object goes away. This makes for a safer API.

The current design is influenced by the data source pattern in Objective-C and Swift. An arbitrary object (commonly but not necessarily the view controller) conforms to a data source protocol, registers itself as the data source, and implements those callback functions. The Objective-C data source object can have its own state, and the developer is able to assume that the object persists for the lifetime of the source. This is similar to how MGLOpenGLStyleLayer works, even as it wraps mbgl’s CustomLayer class.

The SDK generally wraps each mbgl::style::Source object in an Objective-C object on demand. Unfortunately, this means there’s a different Objective-C object every time the application attempts to access the source. For most source types, this is fine because the presence of a source in a style is more often than not determined by mbgl. mbgl is sort of the server, and the SDK is the dumb client. But with custom vector sources, as with the custom layers mentioned above, the roles are reversed: the SDK is in charge of managing the application state associated with the custom source, and mbgl is only in charge of accessing that state.

In #7375 (comment), we discussed having the SDK hang onto every source that gets added to the style. Even if we don’t take such a drastic step, I do think we should have the SDK manage the source object, just as the SDK manages the layer object for custom layers. If the source goes away behind the SDK’s back, I don’t see how the situation would be any different for custom sources than it would be for GeoJSON sources.

I think we should rename CustomVectorSource to CustomGeometrySource to be in line with the naming of GeometryTile and GeometryTileWorker.

This would parallel the renaming I pushed for on the SDK side. (In the iOS and macOS SDKs, “shape” is used where mbgl would use “geometry”.) I think we should also consider moving away from the term “custom”, which has a tendency to get overloaded. At the SDK level, this feature is now represented by MGLComputedShapeSource, because the defining characteristic of a custom source (like a custom layer) is that it’s programmatic, computed on-demand. I hope we don’t end up having to rename runtime styling to “custom styling” for consistency. 😛

[kkaefer]

What are the implications of this feature addition on the difficulty of implementing asynchronous rendering? For instance, does it introduce synchronous tile loading callbacks that would make it difficult to move the ownership of tile data to a background thread?

The current implementation calls the provided callback on the main thread. If we'd move rendering a secondary thread, we'd still retain the sources and tile management/loading on the main thread. The callback doesn't accept a callback; instead users can call the source object with tile data at any point (from the main thread). Users can either generate geometry synchronously, or use a custom or platform specific way to synthesize the geometry. The iOS/macOS bindings are using a NSOperation queue to process the requests in a background thread. Android bindings would do something similar. However, we could refactor this so that threading is managed by Mapbox GL, and to introduce a safe way to supply synthesized geometry that does not rely on maintaining a weak reference to the Source object.

I do think we should have the SDK manage the source object, just as the SDK manages the layer object for custom layers.

That's not how ownership of sources works in Core. The SDK needs to relinquish ownership to the Map object. How would you implement this?

[1ec5]

That's not how ownership of sources works in Core. The SDK needs to relinquish ownership to the Map object. How would you implement this?

I was referring to management in a conceptual sense, not memory ownership. I think this would work just as MGLOpenGLStyleLayer does today: MGLOpenGLStyleLayer does relinquish ownership of the mbgl::style::CustomLayer object to mbgl but keeps a non-owning pointer to it; MGLStyle strongly owns MGLOpenGLStyleLayer. MGLOpenGLStyleLayer's methods get called by lambdas that get passed into mbgl::style::CustomLayer.

[jfirebaugh]

If we'd move rendering a secondary thread, we'd still retain the sources and tile management/loading on the main thread.

I'm anticipating that we'll want to move tile management/loading to the render thread. Discussion in #8476.

[JesseCrocker]

From a user perspective, having clustering support come from the framework is very handy. If clustering was removed, i would just end up implementing the same thing in my application.

[1ec5]

From a user perspective, having clustering support come from the framework is very handy. If clustering was removed, i would just end up implementing the same thing in my application.

To clarify, there are no plans to remove clustering support from GeoJSON sources (shape sources). The question is whether to extend the support for custom vector sources (computed shape sources). In #8473 (comment), the naming confused me a bit. The source type being added here is actually closer to shape sources than it is to vector sources. That’s an argument for keeping the clustering support, but either way, it’s an option, not something that would be forced on users of this source type.

[JesseCrocker]

This branch is now significantly out of date with master, and will not cleanly merge. An updated version that will cleanly merge into master is at https://github.com/trailbehind/mapbox-gl-native/tree/custom-vector-source-merge-master-7-7-2017

[jfirebaugh]

→ #9983.