PredictiveCacheController loads volatile sources

zeac commented 3 years ago

Android API: API 30 Mapbox Navigation SDK version: 2.0.0-beta.22

Steps to trigger behavior

Set MapView style with traffic enabled.
Enable PredictiveCacheController
Build and set a route to MapboxNavigation

Expected behavior

Only non-volatile tiles are loaded by PredictiveCacheController.

Actual behavior

Offline tilepacks for traffic-v2 are requested.

korshaknn commented 3 years ago

@zeac hey! how do you set a map instance to the cache controller?

zeac commented 3 years ago

@zeac hey! how do you set a map instance to the cache controller?

Yes. I reproduced this with qa-test-app/MapboxRouteLineActivity by changing the style to Style.TRAFFIC_NIGHT. After that i see requests like https://api.mapbox.com/tilepack/v1/mapbox.mapbox-traffic-v1/11/327/717/11-14.mtp in the sniffer.

korshaknn commented 3 years ago

@zeac you can filter what sources you need to cache

fun createMapControllers(
        map: MapboxMap,
        sourceIdsToCache: List<String> = emptyList()
    )

check how do you call createMapControllers() in MapboxRouteLineActivity. Do you provide any list? if not, as docs say, all available sources will be cached.

/**

Create cache controllers for a map instance.
Call when a new map instance is available.
@param map an instance of [MapboxMap]
@param sourceIdsToCache a list of sources to cache.
Source id's should look like "mapbox://mapbox.satellite", "mapbox://mapbox.mapbox-terrain-v2".
The system only supports source hosted on Mapbox Services which URL starts with "mapbox://".
If no ids are passed all available style sources will be cached. */

korshaknn commented 3 years ago

@zeac so, if you expect

Only non-volatile tiles are loaded by PredictiveCacheController.

you need provide them

createMapControllers(map, listOf("non-volatile tiles"))

zeac commented 3 years ago

Oh, i expected that only eligible sources will be cached by default. Can we have such logic in PredictiveCacheController? I would prefer to keep list of sources in the style only.

zeac commented 3 years ago

@zeac what do you mean by eligible? we have 2 options:

cache all style sources (default behaviour)

provide a list of sources to cache

All mapbox-hosted, vector or raster, non-volatile style sources.

korshaknn commented 3 years ago

@zeac right now there are 2 options

cache all sources (default)
cache only passed sources

should we filter somehow sources with dynamic data (like traffic) not to cache them? or we should cache? cc @LukasPaczos @Guardiola31337

Guardiola31337 commented 3 years ago

@kkaefer what do you think? ☝️

kkaefer commented 3 years ago

I think we should filter volatile sources. Volatile sources are usually valid only for a few minutes, so predictively caching traffic for a location that is a few kilometers or more away (like predictive cache does) isn't sensible.

korshaknn commented 3 years ago

how we can filter them? do we now exactly how they can be named?

traffic what else?

@Guardiola31337 @kkaefer

zeac commented 3 years ago

I was told that getStyleSourceProperties() contains volatile key for such sources, but i didn't find it.

tobrun commented 3 years ago

I got a style json from @korshaknn for a style used for a test. None of them are using the optional volatile style-spec configuration. That @zeac isn't able to find a volatile key is expected behavior.

Either there is a disconnect in using the term volatile (eg. tiles that have a short expiration date) or we need to update the style definition to include volatile=true.

korshaknn commented 3 years ago

I think we should filter volatile sources. Volatile sources are usually valid only for a few minutes, so predictively caching traffic for a location that is a few kilometers or more away (like predictive cache does) isn't sensible.

@kkaefer should it be done by default by CommonSDK (right now or as a future improvement)? or NavSDK should filter volatile sources and create cache controllers for non-volatile only?

cc @LukasPaczos

pozdnyakov commented 3 years ago

or NavSDK should filter volatile sources and create cache controllers for non-volatile only?

That should be done in TilesetDescriptor implementation - the volatile sources/tilesets shall be filtered out. One option could be re-using the tileset descriptors created by the Maps Offline API.

korshaknn commented 3 years ago

@pozdnyakov have some questions

is there a way to get tileset descriptors created by the Maps Offline API? at the moment we get TileStore from maps with
```
val tileStore = map.getResourceOptions().tileStore
```
and pass the tileStore to NavigationNative to create cache controllers
should we pass tilesetDescriptors + tileStore to NavigationNative side? so they will create cache controllers for valid sources only?

at the moment NavNative has the next constructor:

public native PredictiveCacheController createPredictiveCacheController(
    @NonNull com.mapbox.common.TileStore tileStore, 
    @NonNull PredictiveCacheControllerOptions cacheOptions, 
    @NonNull PredictiveLocationTrackerOptions locationTrackerOptions);

pozdnyakov commented 3 years ago

is there a way to get tileset descriptors created by the Maps Offline API?

Could you fetch them from the Maps OfflineManager (smth. like

val tilesetDescriptor = offlineManager.createTilesetDescriptor(
  TilesetDescriptorOptions.Builder()
    .styleURI(Style.OUTDOORS)
    .minZoom(0)
    .maxZoom(16)
    .build()
)

) and then pass the descriptors along with the tileStore to the PredictiveCacheController ?

korshaknn commented 3 years ago

@pozdnyakov OfflineManager is built with ResourceOptions, I guess it should be built on customer side, not SDK. We can add OfflineManager to PredictiveCacheController constructor. and change NN API to pass tilesetDescriptor

btw, do we need to create a separate cache controller for each style source? can it be done by NN/Common internally, if we provide tileStore + tilesetDescriptors?

cc @LukasPaczos @Guardiola31337 @mskurydin

Guardiola31337 commented 3 years ago

Hey @kkaefer and @mskurydin could you help with above questions? 🙏

mskurydin commented 3 years ago

It's worth considering the creation of the predictive cache using a tileset descriptor directly. We will check how complicated it is (there was some WIP on this previously).

kkaefer commented 3 years ago

Yeah, I think we should switch over to Tileset Descriptors so that the SDK doesn't have to worry about filtering these and can leverage GL Native/Maps SDK to create these.

korshaknn commented 3 years ago

@kkaefer at the moment on SDK side we iterate over style sources to get tileVariants. And we create PredictiveCacheController for each tileVariant using NN API. If we pass tilesetDescriptor to NN, there will be no need to create controllers for each tileVariant, right?

kkaefer commented 3 years ago

Yep, that's right.

DmitryAk commented 3 years ago

is there a way to get tileset descriptors created by the Maps Offline API?

Could you fetch them from the Maps OfflineManager (smth. like
val tilesetDescriptor = offlineManager.createTilesetDescriptor(
  TilesetDescriptorOptions.Builder()
    .styleURI(Style.OUTDOORS)
    .minZoom(0)
    .maxZoom(16)
    .build()
)
) and then pass the descriptors along with the tileStore to the PredictiveCacheController ?

@pozdnyakov Is it possible to filter-out volatile sources/tilesets in another way (without re-using the tiles descriptors created by the Maps Offline API)? Maybe we can find another solution?

TilesetDescriptor instance can be in unresolved state - so in general case NavNative cannot create completed PredictiveCacheController by TilesetDescriptor - we must wait until it is resolved to know the data domain (Maps or Navigation). This requires asynchronous CreatePredictiveCache introducing, which can be not very convenient to use. Also currently one PredictiveCacheController operates on one dataset, one version, in one data domain (Maps or Navigation). But a TilesetDescriptor can be resolved with several datasets/versions from different data domains. So the only more-or-less simple way is to introduce a new method to asynchronously create a list of PredictiveCacheControllers by TilesetDescriptor. Otherwise it is required to redesign PredictiveCacheController. I’m not sure this is the best option. WDYT?

pozdnyakov commented 3 years ago

@pozdnyakov Is it possible to filter-out volatile sources/tilesets in another way (without re-using the tiles descriptors created by the Maps Offline API)?

Otherwise, you need to repeat the functionality of Maps Offline API which does not look like an optimal way and it can also get inconsistent.

This requires asynchronous CreatePredictiveCache introducing, which can be not very convenient to use. Also currently one PredictiveCacheController operates on one dataset, one version, in one data domain (Maps or Navigation).

@DmitryAk could you guide me to the code so that I could give a better advice on it?

mapbox / mapbox-navigation-android