Setting a layout property needs to trigger tile reparse

[jfirebaugh]

When a layout property is modified, all existing tiles for that source need to be reparsed, so that vertex arrays that are calculated based on that property are recalculated.

This will fix #5610 (comment).

Additionally, once data-driven-styling lands, modifying a data-driven paint property also needs to trigger a reparse.

We should port the general strategy for batched updates from gl-js -- see mapbox/mapbox-gl-js#2355 and the current style.js.

cc @ivovandongen @mourner @kkaefer

[kkaefer]

Right now, we're throwing away the parsed representation of a tile after we created all buckets, and when reparsing a tile, we'll have to do a new request (which is fast because it'll likely be served from cache, but still slower because it needs to be decompressed etc.). We should change this behavior and instead have all tiles/buckets hang on to the data they loaded so that we can do faster reparses.

[mkv123]

Is there some workaround on Android to force the reparse after changing runtime styles until this gets fixed?

[incanus]

Adding and removing layers seems to cause reloads, so perhaps a dummy (possibly even unstyled?) layer could help here.

[mkv123]

Unfortunately I can't seem to get things to re-render even by adding a layer. However in trying it I got this simple way to replicate the issue, at least on my Nexus 5X with 6.0.1.

map.getMapAsync(new com.mapbox.mapboxsdk.maps.OnMapReadyCallback() {
            @Override
            public void onMapReady(final MapboxMap mapboxMap) {

                mapboxMap.setCameraPosition(new CameraPosition.Builder().target(new LatLng(60.186132, 24.827881)).zoom(18).build());
                addOnMapChangedListener(new com.mapbox.mapboxsdk.maps.MapView.OnMapChangedListener() {
                    @Override
                    public void onMapChanged(int change) {
                        if(change == DID_FINISH_LOADING_MAP) {
                            FillLayer layer = new FillLayer("force-refresh", "composite");
                            layer.setSourceLayer("landuse");
                            layer.setFilter(Filter.eq("class", "grass"));
                            layer.setProperties(PropertyFactory.fillColor("#ff0000"));
                            layer.setMinZoom(9);
                            layer.setMaxZoom(22);
                            mapboxMap.addLayer(layer);
                        }
                    }
                });

            }
        });

[mkv123]

Behaviour above also confirmed with the Android Demo App's SimpleMapViewActivity after updating to latest sdk snapshot and making the following change:

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_basic_simple_mapview);

        mapView = (MapView) findViewById(R.id.mapView);
        mapView.onCreate(savedInstanceState);

        mapView.getMapAsync(new OnMapReadyCallback() {
            @Override
            public void onMapReady(final MapboxMap mapboxMap) {
                mapboxMap.setCameraPosition(new CameraPosition.Builder().target(new LatLng(60.186132, 24.827881)).zoom(18).build());
                mapView.addOnMapChangedListener(new com.mapbox.mapboxsdk.maps.MapView.OnMapChangedListener() {
                    @Override
                    public void onMapChanged(int change) {
                        if(change == DID_FINISH_LOADING_MAP) {
                            FillLayer layer = new FillLayer("force-refresh", "composite");
                            layer.setSourceLayer("landuse");
                            layer.setFilter(Filter.eq("class", "grass"));
                            layer.setProperties(PropertyFactory.fillColor("#ff0000"));
                            layer.setMinZoom(9);
                            layer.setMaxZoom(22);
                            mapboxMap.addLayer(layer);
                        }
                    }
                });



            }
        });
    }

[incanus]

This is currently a blocker for runtime style API on both mobile platforms (and core technically, as well). Anything involving styling existing layers (and not adding or removing a layer) triggers this bug and doesn't show style updates.

[jfirebaugh]

We should change this behavior and instead have all tiles/buckets hang on to the data they loaded so that we can do faster reparses.

This looks kind of tricky. Currently, the data (std::unique_ptr<GeometryTileData>) is retained after parsing in order to support queryRenderedFeatures. Ownership works like this:

• Ownership is passed into GeometryTile::setData, which passes it on to the worker's parseGeometryTile.
• The worker owns it during parsing/layout.
• When complete, ownership is passed back to the main thread via TileParseResultData, and then is available for use by queryRenderedFeatures.

I'm not seeing a simple way to introduce retention for purposes of re-layouts to this scheme.

• TileWorker can't retain ownership, because then queryRenderedFeatures won't work. For similar reasons, once GeometryTile has ownership, we don't want it to give it up, because then queryRenderedFeatures would behave unpredictably.
• GeometryTile is the current eventual owner, but we need to be able to cancel a pending layout and restart it with the same tile data but different layer data. [Hmm, maybe not. JS does not do this. Instead it waits for the initial layout, then re-layouts.]
• Implementations of GeometryTile may not be (and in fact are not) safe for concurrent use from multiple threads. Let's not try to share ownership.

What does GL JS do? The worker is responsible for making the request. When parsing and layout are complete, it transfers the raw data to the main thread but retains a reference to the parsed representation for subsequent re-layouts. The main thread lazily reparses the raw data for queryRenderedFeatures. I think we'll need to mimic this approach.

[mourner]

@jfirebaugh would it be OK to defer queryRenderedFeatures results until the tile finishes reparsing so that you can transfer ownership there and back again?

[jfirebaugh]

No, queryRenderedFeatures is synchronous.

[mourner]

@jfirebaugh how about just not allowing it while reload is in progress? I could be an OK tradeoff.

[incanus]

Not allowing would manifest pretty weirdly in the mobile SDKs, for one. The querying feature would just not work intermittently if runtime styling was involved, for example.

[jfirebaugh]

I'm going to introduce a layer of indirection, and have GeometryTile work with a GeometryTileDataProvider which can, on demand, produce a fresh std::unique_ptr<const GeometryTileData> copy. GeometryTile will keep one copy, and pass another to the worker.

[jonkan]

Hi, I'm experiencing a bug where updating a layer filter (setting the NSPredicate in iOS) fails to update (or invalidate) a source that's currently not visible, causing it to show with the old filter when re-appering.

Given a MGLVectorSource with minzoom=maxzoom=9 and a MGLLineStyleLayer with minzoom=9, maxzoom=13 and a filter/predicate A.

• Zoom map through zoom 9 -> 13 (layer rendered according to filter A) -> 14 (layer disappear as expected).
• Update the layer filter to filter B.
• Zooming back to 13 the layer still shows according to filter A, as opposed to filter B that was expected.

I found that in the void Style::relayout() tiles are only reloaded if the source is enabled which according to the comment is when "the source is used by any layers visible at the current zoom level". Removing the && source->baseImpl->enabled-part of the if-statement (i.e. reverting part of 9127299 "don't load tiles from sources that aren't used") does solve my issue.

Maybe @kkaefer have some ideas?

[jfirebaugh]

@jonkan Can you please file a new issue?

[jonkan]

Sure! #8505