[core] Extract SymbolLayout from SymbolBucket

[jfirebaugh]

I still need to debug this, but I want to get @ansis and @kkaefer 👀 on it as soon as possible.

This PR splits SymbolBucket in two. The new class SymbolLayout lives on the worker thread only and contains the persistent data needed for repeated placement. SymbolBucket contains the data generated during placement, and is transferred to the main thread for rendering.

This eliminates the risky sharing of GeometryTile::buckets between the main thread and worker thread during TileWorker::redoPlacement. SymbolBucket::swapRenderData is no longer necessary.

While here, rationalize the names of states a SymbolLayout may be in:

• Pending: Waiting for the necessary glyphs or icons to be available.
• Prepared: The potential positions of text and icons have been determined.
• Placed: The final positions have been determined, taking into account prior layers.

The related SymbolLayout methods reflect this terminology, which fixes #2884.

In TileWorker, all SymbolLayouts are now stored in a single vector. Each SymbolLayout knows what state it is in, and TileWorker can more straightforwardly determine how much progress it can make toward a final result. I think the advancement of a tile through the stages of processing is clearer now, making further progress towards #2851.

[mention-bot]

@jfirebaugh, thanks for your PR! By analyzing this pull request, we identified @ansis, @yhahn and @tmpsantos to be potential reviewers.

[jfirebaugh]

I'd like to merge this today, so I can continue progress on fixing #6263. Reviews appreciated!

[jfirebaugh]

@ansis @kkaefer Please review.

[kkaefer]

Moving most of the code + changes in one commit makes this really hard to review.

[kkaefer]

The new class SymbolLayout lives on the worker thread only and contains the persistent data needed for repeated placement. SymbolBucket contains the data generated during placement, and is transferred to the main thread for rendering.

Really liking this separation. While reviewing, I didn't spot any logic errors. I think this is good to go.

[kkaefer]

Here's a suggestion for reducing the number of `SymbolBuckets that are gratuitously created, then discarded:

diff --git a/src/mbgl/tile/tile_worker.cpp b/src/mbgl/tile/tile_worker.cpp
index f079847..53f6acb 100644
--- a/src/mbgl/tile/tile_worker.cpp
+++ b/src/mbgl/tile/tile_worker.cpp
@@ -164,9 +164,9 @@ TilePlacementResult TileWorker::redoPlacement(const PlacementConfig& config) {

     for (auto& symbolLayout : symbolLayouts) {
         symbolLayout->state = SymbolLayout::Placed;
-        std::unique_ptr<Bucket> bucket = symbolLayout->place(*result.collisionTile);
-        if (bucket->hasData() || symbolLayout->hasSymbolInstances()) {
-            result.buckets.emplace(symbolLayout->bucketName, std::move(bucket));
+        if (symbolLayout->hasSymbolInstances()) {
+            result.buckets.emplace(symbolLayout->bucketName,
+                                   symbolLayout->place(*result.collisionTile));
         }
     }

[jfirebaugh]

Sorry for the poor diff. Here is a diff between symbol_bucket.cpp before and symbol_layout.cpp after, so you can see what changed in the moved code:

• Updated #include lists
• Changed constructor arguments
• Constructor now runs the code that was parseFeatures
• Changed some variables to camel-case
• needsDependencies ⇢ canPrepare (and negated the sense of the return value)
• addFeatures ⇢ prepare
• placeFeatures ⇢ place, and it returns a bucket
• addToDebugBuffers gets passed SymbolBucket& rather than updating a member variable

[kkaefer]

We could move hasSymbolInstances() to this function.

[kkaefer]

Great!

[mourner]

@jfirebaugh nice! Does it make sense to split the two in GL JS as well?