Reload source tiles when a filter or layout property is modified #6201
Conversation
|
|
There's a race condition in this implementation: |
jfirebaugh
force-pushed
the
source-reload
branch
from
2c5de5c to
06dc7a1
Compare
|
@jfirebaugh Looking good! The examples with paint property updates work as expected now. I did notice however, that when removing a layer, nothing happens until I interact with the map (I would add a gif, but you wouldn't see my interaction anyway). You can try this out in the runtime style test activity if you like. About the crash, I also don't have access yet (integrating the result in bitrise is underway iirc), but I ran into only one crash intermittently which @cammace reported yesterday: #6193 |
| std::unique_ptr<GeometryTileData> clone() const override { | ||
| return std::make_unique<VectorTileData>(*this); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are we making a copy here? *TileData objects should be designed in a read-only, all-const manner (all of its methods are const), so we should be able to use shared pointers. Given that they don't hold any resources, except for a shared_ptr<const std::string>, they can be destructed in any thread.
|
@jfirebaugh Why are we making a deep copy of the *TileData object? We should be able to use shared pointers for them. |
|
Ugh, I got fooled by the However, |
|
If we want
Lazy-but-caching implementations with interior mutability don't mix well with cross-thread sharing. @springmeyer, is this compatible with the plans you have for vector-tile? |
jfirebaugh
force-pushed
the
source-reload
branch
from
06dc7a1 to
1be5f59
Compare
|
@jfirebaugh @tobrun retrieved the logs for me, it seems there is another issue in play in this testcase: I could reproduce this locally only by running the entire test suite, the individual test case succeeds. |
|
@ivovandongen Unfortunately the stack trace is missing symbols -- is it a release build? |
|
Oh, I suppose #6221 is the cause of the missing symbols. |
Yes. Overall looks compatible. Also, I removed the |
jfirebaugh
force-pushed
the
source-reload
branch
from
1be5f59 to
a88b5d1
Compare
jfirebaugh
force-pushed
the
source-reload
branch
from
a88b5d1 to
607c5b0
Compare
|
@ivovandongen I think the lack of repaint when removing a layer is a separate issue; can you file a ticket? Also can you post the new Android test output? Hopefully it has symbols in the stack trace now. |
Yes. However, all tests pass now for me locally (ran them twice to be sure). I "symbolized" against the current state of your branch, so it might be off a little. |
jfirebaugh
force-pushed
the
source-reload
branch
from
607c5b0 to
194301b
Compare
|
@ivovandongen Changing visibility should work now, thanks for the catch. That stack trace looks nonsensical. Could be a symbol mismatch. I really need to see the full logs from an up-to-date build (#6220) and possibly help interpreting them. |
jfirebaugh
force-pushed
the
source-reload
branch
from
194301b to
a5812b4
Compare
|
The way I'd like to fix the race condition is to change the worker API to: However, this depends on serial processing order of worker messages, so that we can guarantee that So I'm going to work on fixing #1471, which if implemented in the way I have in mind, would solve this problem as well. |
Avoids conversion to GeometryCollection and clipping for features that are not used.
* Renamed {Source,Tile}Observer::onNeedsRepaint to onTileUpdated. Messages should be in terms of what happened to the observed object, not in terms of what the observer needs to do. This also removes a confusing overlap of virtual methods on StyleObserver.
* Added style::Observer::onUpdate(Update). This is also a violation of the above rule, but I'm hopeful that it will disappear when update batching is implemented.
jfirebaugh
force-pushed
the
source-reload
branch
from
a5812b4 to
3c6bfe5
Compare
|
Changing the worker API is turning out to be difficult. In order to unblock other issues, I'm going to merge the changes so far and ticket the race condition and GeoJSON mutation for followups. |
|
iOS build is failing because in the latest rebase I wiped out d77a13e with the deletion of |
jfirebaugh
force-pushed
the
source-reload
branch
from
0029ab2 to
64ad521
Compare
…quiring SDKs to use Map::update This paves the way for updates to filter and layout properties to trigger a source reload, without each SDK having to participate in the implementation.
Until Style::recalculate() is called to check that there are no visible layers using the source, we have to assume there are. Otherwise, Style::isLoaded() can return a false positive.
jfirebaugh
force-pushed
the
source-reload
branch
from
64ad521 to
d6f667a
Compare
|
🎉 |
Fixes #5701
Fixes #6063
Fixes #6116
Fixes #6017
Fixes #6233
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. The same is true for modifications to layer filters.
This PR can be divided into three parts:
Source,GeometryTile, andWorkerTileso that tiles can be reloaded (first 5 commits). This involves ensuring that the worker can hold onto the tile data for reuse, rather than passing ownership back to the main thread. Because the main thread also needs tile data for query purposes, it has to be made copyable. (/cc @kkaefer for review of this portion.)Styleto be able to cause a reload at the appropriate time (next 2 commits). This required modifying our change notification infrastructure so that the style can observe layer mutations directly, rather than being indirectly notified in a way that requires the cooperation of SDK bindings. (/cc @1ec5 @frederoni @ivovandongen for review of this portion.)Remaining work:
Styleshould auto-batch source reloadsNodeMap::SetFiltershould useLayer::accept. Or should we have an intermediate base class forFillLayer,LineLayer,CircleLayer, andSymbolLayer, with pure virtual common methods:Fix race condition:
GeometryTile::redoLayoutassumes that the worker has tile data from a previous call toworker.parseGeometryTile(...), but it also cancels the pending work request, which might be the very request that provides the worker with the tile data.Reload when GeoJSON source data changes.