-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
1f4f494
to
3245ab3
Compare
2415987
to
56150de
Compare
Using the Actor model to enable asynchronous communication to the To request data
To set data
|
@asheemmamoowala Looking good! The only thing I'm wondering is wether we can skip going back over the main thread to set the tile data through That said, I'm not sure how much overhead orchestrating the tile data setters on the main thread has to begin with. |
@ivovandongen - I wondered about that as well . Given that each tile request results in its own |
@ivovandongen - having worked on your suggestion, I remembered setting it up through |
b414351
to
fa81ebc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small formatting issues
auto tuple = std::make_tuple(tileID.overscaledZ, tileID.wrap, callbackRef); | ||
tileCallbackMap.insert({ tileID.canonical, std::vector<OverscaledIDFunctionTuple>(1, tuple) }); | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line break
tileCallbackMap.insert({ tileID.canonical, std::vector<OverscaledIDFunctionTuple>(1, tuple) }); | ||
} | ||
else { | ||
for(auto iter = tileCallbacks->second.begin(); iter != tileCallbacks->second.end(); iter++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after for
} | ||
else { | ||
for(auto iter = tileCallbacks->second.begin(); iter != tileCallbacks->second.end(); iter++) { | ||
if(std::get<0>(*iter) == tileID.overscaledZ && std::get<1>(*iter) == tileID.wrap ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after if
} | ||
|
||
void CustomTileLoader::cancelTile(const OverscaledTileID& tileID) { | ||
if(tileCallbackMap.find(tileID.canonical) != tileCallbackMap.end()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after if
void CustomTileLoader::removeTile(const OverscaledTileID& tileID) { | ||
auto tileCallbacks = tileCallbackMap.find(tileID.canonical); | ||
if (tileCallbacks == tileCallbackMap.end()) return; | ||
for(auto iter = tileCallbacks->second.begin(); iter != tileCallbacks->second.end(); iter++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after for
auto tileCallbacks = tileCallbackMap.find(tileID.canonical); | ||
if (tileCallbacks == tileCallbackMap.end()) return; | ||
for(auto iter = tileCallbacks->second.begin(); iter != tileCallbacks->second.end(); iter++) { | ||
if(std::get<0>(*iter) == tileID.overscaledZ && std::get<1>(*iter) == tileID.wrap ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after if
auto iter = tileCallbackMap.find(tileID); | ||
if (iter == tileCallbackMap.end()) return; | ||
dataCache[tileID] = std::make_unique<mapbox::geojson::geojson>(std::move(data)); | ||
for(auto tuple : iter->second) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after for
src/mbgl/tile/custom_tile.cpp
Outdated
necessity = newNecessity; | ||
if (necessity == Necessity::Required) { | ||
loader.invoke(&style::CustomTileLoader::fetchTile, id, actor.self()); | ||
} else if(!isRenderable()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after if
@@ -111,7 +45,7 @@ extern MGL_EXPORT const MGLShapeSourceOption MGLShapeSourceOptionSimplificationT | |||
``` | |||
*/ | |||
MGL_EXPORT | |||
@interface MGLShapeSource : MGLSource | |||
@interface MGLShapeSource : MGLAbstractShapeSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is scheduled to land in iOS SDK v4.0.0, which allows backwards-incompatible changes. That means we can revisit the taxonomy proposed in #6940 (comment). Specifically, we have the opportunity to choose a name for MGLAbstractShapeSource that doesn’t contain the word “abstract” and rename the MGLShapeSourceOption
s to align with whatever class they’re used with. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a single MGLShapeSource
with a data source that provides either tiled or non-tiled data. It would combine the existing MGLShapeSource
and MGLProgrammaticShapeSource
.
I don't have a good idea of what the peer Source class for a GeoJSON source would be in this scenario - maybe it makes room for an actual MGLGeoJSONSource
, but I suspect that was removed earlier for some very valid argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea – would we combine GeoJSONSource
and CustomSource
into a single class to facilitate this simplification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may be a future state, where GeoJSONSource
is rebuilt on top of CustomVectorSource
, but its not in the current plan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that means MGLShapeSource would need to hold a variant<GeoJSONSource, CustomSource>
? More reason to rename CustomSource
to something more descriptive…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Landed on using CustomGeometrySource
in core and android to match other uses of custom.
platform/macos/CHANGELOG.md
Outdated
* Deprecated `+[MGLStyle trafficDayStyleURL]` and `+[MGLStyle trafficNightStyleURL]` with no replacement method. To use the Traffic Day and Traffic Night styles going forward, we recommend that you use the underlying URL. ([#9918](https://github.com/mapbox/mapbox-gl-native/pull/9918)) | ||
* Fixed an issue where stale (but still valid) map data could be ignored in offline mode. ([#10012](https://github.com/mapbox/mapbox-gl-native/pull/10012)) | ||
|
||
## 0.5.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a bad merge.
a626b32
to
3ea2155
Compare
Alll planned work for this PR is complete. Here's a recap of what's changed: Core
iOS, macOS
Android
Node, QtN/A Synchronization
Tail work
Future Work
|
3ea2155
to
8c58a98
Compare
As in #6940 (comment) and #7485 (comment), I continue to fear that calling the callback on a background queue will be a trap for Objective-C and Swift developers, who are accustomed to doing anything UI-related on the main queue. A developer’s first instinct will be to make their view controller class the data source and have the MGLComputedShapeSourceDataSource method implements refer to instance variables, which will lead to concurrency trouble. That said, I understand the performance considerations that led to this approach. Hopefully the presence of NSOperationQueue will point more savvy developers toward correctly isolating the data source method implementations into a separate class, but the asynchronicity does significantly diminish this feature’s attractiveness as part of the solution for #6181. |
I agree that calling the callback on a background queue could be confusing to some developers, though i think this will end up being a feature that beginning developers probably wont find themselves needing, and the main thread checker in xcode9 makes the kinds of errors that it could lead to far more obvious. I think if this is changed to calling the callback on the main thread then the callback needs to be passed a callback, not return data, otherwise this feature would be not usable for many uses, including anything that has to fetch data from the network. |
A callback on the main thread would be less confusing and result in fewer synchronization errors. But we would lose the advantage of canceling stale tile requests provided by
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asheemmamoowala I focussed mostly on the Android bindings this time around. Few nit picky things aside, my main concern is the ownership/life-cylces of the core CustomGeometrySource
, c++ peer and Java peer.
Right now, the c++ peer owns both the core source and maintains a global ref to the java peer, where I think the core class should own the c++ peer. This way, when the core source is destroyed, the c++ peer can be destroyed, releasing the java object. Right now, it seems that the peer classes are never cleaned up.
|
||
import static com.mapbox.mapboxsdk.style.layers.PropertyFactory.lineColor; | ||
|
||
public class GridSourceActivity extends AppCompatActivity implements OnMapReadyCallback { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little hint of JavaDoc would be nice here
@Override | ||
public void onMapReady(@NonNull final MapboxMap map) { | ||
mapboxMap = map; | ||
new Handler(getMainLooper()).post(new Runnable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this posted to the main looper? Shouldn't be necessary.
std::lock_guard<std::mutex> lock(dataCacheMutex); | ||
dataCache.erase(tileID); | ||
} | ||
dataCache.erase(tileID); | ||
} | ||
|
||
void CustomTileLoader::invalidateRegion(const LatLngBounds& bounds, Range<uint8_t> ) { | ||
for(auto idtuple= tileCallbackMap.begin(); idtuple != tileCallbackMap.end(); idtuple++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after for
/** | ||
* Constructs a LatLngBounds from a Tile identifier. | ||
*/ | ||
public static LatLngBounds from(int z, int x, int y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also document the parameters for public methods please.
* | ||
* @param id the source id | ||
*/ | ||
public CustomGeometrySource(String id, GeometryTileProvider provider_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing GeometryTileProvider
JavaDoc
import com.mapbox.mapboxsdk.geometry.LatLngBounds; | ||
import com.mapbox.services.commons.geojson.FeatureCollection; | ||
|
||
public interface GeometryTileProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document public interfaces
* @param id the source id | ||
*/ | ||
public CustomGeometrySource(String id, GeometryTileProvider provider_) { | ||
executor = new ThreadPoolExecutor(2, 2, 60, TimeUnit.SECONDS, new ArrayBlockingQueue(80)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing generic classification on ArrayBlockingQueue
: <Runnable>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we ensure proper shutdown of the executor when the source is removed btw? At the moment, the core source might be removed while there are still tasks in the queue, these keep executing until finalisation of the Java object.
@@ -0,0 +1,122 @@ | |||
#include "custom_geometry_source.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation of this file is kinda off.
options, | ||
std::bind(&CustomGeometrySource::fetchTile, this, std::placeholders::_1), | ||
std::bind(&CustomGeometrySource::cancelTile, this, std::placeholders::_1))) ), | ||
javaPeer(_obj.NewGlobalRef(env)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is going to work properly as the Java/c++ peers will never be cleaned up.
As far as I can tell, the java object is created, which creates the c++ peer. The c++ peer holds a global reference to the java peer, ensuring it is not GC'd. Now, the only way to clean up both would be to destroy the c++ peer, which would release the global reference on the Java peer, making it eligible for GC. This in turn would however try to call the destructor of the c++ peer as that is how it has been registered through jni::RegisterNativePeer
below.
For this to work, the life-cycle of the the c++ peer should be coupled to the core CustomGeometrySource
. When it is destroyed, the c++ peer should be destroyed, releasing the global reference, making the java peer eligible for GC.
@@ -0,0 +1,46 @@ | |||
#pragma once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a left-over after refactoring right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - this is definitely a mistake.
*/ | ||
public CustomGeometrySource(String id, GeometryTileProvider provider_) { | ||
executor = new ThreadPoolExecutor(2, 2, 60, TimeUnit.SECONDS, new ArrayBlockingQueue(80)); | ||
cancelledTileRequests = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using a HashMap safe here. Seems like it is getting modified by multiple threads, unless there is always only one WorkerThread. setTileData is not marked as a WorkerThread.
Since the class is holding an AtomicBoolean it looks like it is intended to be used by multiple threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this @mkrussel77. HashMap
will not work here and will need to use a thread-safe option like ConcurrentHashMap
9227548
to
557f232
Compare
6ec1ec3
to
7d28ab7
Compare
2f48e85
to
5d7fe7f
Compare
…ce in the C++ peer.
5d7fe7f
to
1fb74c2
Compare
I'm researching mapping APIs for offline maptile capability. This feature would be great to have. |
this sdk version can load .mbtiles from local storage/sdcard ? |
This PR alone doesn’t implement MBTiles support. Built-in MBTiles support is being tracked in #6862. |
Follow up to #8473 rebased on master and updated for Async Render and other changes.
TODO:
LatLngBounds
or by tilegeojson-vt
without tile splitting