WIP: Don't replace a fully loaded tile with its parent or child until symbols are loaded, to prevent flicker.upstream/start-collision-two-pass

When a tile isn't replacing another tile (e.g. on pan operations), start rendering non-symbol layers before symbols finish loading.

10 files changed, 188 insertions, 91 deletions

diff --git a/src/mbgl/algorithm/update_renderables.hpp b/src/mbgl/algorithm/update_renderables.hpp
index c583b6b2b6..d691a7def0 100644
--- a/src/mbgl/algorithm/update_renderables.hpp
+++ b/src/mbgl/algorithm/update_renderables.hpp
@@ -1,127 +1,176 @@
 #pragma once
 
+#include <mbgl/tile/tile.hpp>
 #include <mbgl/tile/tile_id.hpp>
 #include <mbgl/tile/tile_necessity.hpp>
 #include <mbgl/util/range.hpp>
 
 #include <unordered_set>
 
+#include <iostream>
+
 namespace mbgl {
 namespace algorithm {
 
+
 template <typename GetTileFn,
           typename CreateTileFn,
           typename RetainTileFn,
-          typename RenderTileFn,
-          typename IdealTileIDs>
-void updateRenderables(GetTileFn getTile,
+          typename RenderTileFn>
+bool findBestTile(GetTileFn getTile,
                        CreateTileFn createTile,
                        RetainTileFn retainTile,
                        RenderTileFn renderTile,
-                       const IdealTileIDs& idealTileIDs,
                        const Range<uint8_t>& zoomRange,
-                       const uint8_t dataTileZoom) {
-    std::unordered_set<UnwrappedTileID> checked;
-    bool covered;
-    int32_t overscaledZ;
+                       const uint8_t dataTileZoom,
+                       const UnwrappedTileID& idealRenderTileID,
+                       const OverscaledTileID& idealDataTileID,
+                       const bool requireFullyRenderable,
+                       std::unordered_set<UnwrappedTileID> checked) {
 
-    // for (all in the set of ideal tiles of the source) {
-    for (const auto& idealRenderTileID : idealTileIDs) {
-        assert(idealRenderTileID.canonical.z >= zoomRange.min);
-        assert(idealRenderTileID.canonical.z <= zoomRange.max);
-        assert(dataTileZoom >= idealRenderTileID.canonical.z);
+    auto tile = getTile(idealDataTileID);
+    if (!tile) {
+       // std::cout << "Create tile: " << idealDataTileID << std::endl;
+        tile = createTile(idealDataTileID);
+        assert(tile);
+    }
 
-        const OverscaledTileID idealDataTileID(dataTileZoom, idealRenderTileID.wrap, idealRenderTileID.canonical);
-        auto tile = getTile(idealDataTileID);
-        if (!tile) {
-            tile = createTile(idealDataTileID);
-            assert(tile);
-        }
+    bool covered = true;
+    std::vector<std::pair<UnwrappedTileID,decltype(tile)>> renderTiles;
+    // if (source has the tile and bucket is loaded) {
+    if (requireFullyRenderable ? tile->isFullyRenderable() : tile->isRenderable()) {
+        retainTile(*tile, TileNecessity::Required);
+        renderTiles.emplace_back(std::make_pair(idealRenderTileID, tile));
+    } else {
+        // We are now attempting to load child and parent tiles.
+        bool parentHasTriedOptional = tile->hasTriedCache();
+        bool parentIsLoaded = tile->isLoaded();
 
-        // if (source has the tile and bucket is loaded) {
-        if (tile->isRenderable()) {
-            retainTile(*tile, TileNecessity::Required);
-            renderTile(idealRenderTileID, *tile);
+        // The tile isn't loaded yet, but retain it anyway because it's an ideal tile.
+        retainTile(*tile, TileNecessity::Required); // TODO: Is it fine to retain this twice for both full and partial render?
+
+        
+        int32_t overscaledZ = dataTileZoom + 1;
+        if (overscaledZ > zoomRange.max) {
+            // We're looking for an overzoomed child tile.
+            const auto childDataTileID = idealDataTileID.scaledTo(overscaledZ);
+            tile = getTile(childDataTileID);
+            if (tile && (requireFullyRenderable ? tile->isFullyRenderable() : tile->isRenderable())) {
+                retainTile(*tile, TileNecessity::Optional);
+                renderTiles.emplace_back(std::make_pair(idealRenderTileID, tile));
+            } else {
+                covered = false;
+            }
         } else {
-            // We are now attempting to load child and parent tiles.
-            bool parentHasTriedOptional = tile->hasTriedCache();
-            bool parentIsLoaded = tile->isLoaded();
-
-            // The tile isn't loaded yet, but retain it anyway because it's an ideal tile.
-            retainTile(*tile, TileNecessity::Required);
-            covered = true;
-            overscaledZ = dataTileZoom + 1;
-            if (overscaledZ > zoomRange.max) {
-                // We're looking for an overzoomed child tile.
-                const auto childDataTileID = idealDataTileID.scaledTo(overscaledZ);
+            // Check all four actual child tiles.
+            for (const auto& childTileID : idealDataTileID.canonical.children()) {
+                const OverscaledTileID childDataTileID(overscaledZ, idealRenderTileID.wrap, childTileID);
                 tile = getTile(childDataTileID);
-                if (tile && tile->isRenderable()) {
+                if (tile && (requireFullyRenderable ? tile->isFullyRenderable() : tile->isRenderable())) {
                     retainTile(*tile, TileNecessity::Optional);
-                    renderTile(idealRenderTileID, *tile);
+                    renderTiles.emplace_back(std::make_pair(childDataTileID.toUnwrapped(), tile));
                 } else {
+                    // At least one child tile doesn't exist, so we are going to look for
+                    // parents as well.
                     covered = false;
                 }
-            } else {
-                // Check all four actual child tiles.
-                for (const auto& childTileID : idealDataTileID.canonical.children()) {
-                    const OverscaledTileID childDataTileID(overscaledZ, idealRenderTileID.wrap, childTileID);
-                    tile = getTile(childDataTileID);
-                    if (tile && tile->isRenderable()) {
-                        retainTile(*tile, TileNecessity::Optional);
-                        renderTile(childDataTileID.toUnwrapped(), *tile);
-                    } else {
-                        // At least one child tile doesn't exist, so we are going to look for
-                        // parents as well.
-                        covered = false;
-                    }
-                }
             }
+        }
 
-            if (!covered) {
-                // We couldn't find child tiles that entirely cover the ideal tile.
-                for (overscaledZ = dataTileZoom - 1; overscaledZ >= zoomRange.min; --overscaledZ) {
-                    const auto parentDataTileID = idealDataTileID.scaledTo(overscaledZ);
-                    const auto parentRenderTileID = parentDataTileID.toUnwrapped();
+        if (!covered) {
+            // We couldn't find child tiles that entirely cover the ideal tile.
+            for (overscaledZ = dataTileZoom - 1; overscaledZ >= zoomRange.min; --overscaledZ) {
+                const auto parentDataTileID = idealDataTileID.scaledTo(overscaledZ);
+                const auto parentRenderTileID = parentDataTileID.toUnwrapped();
 
-                    if (checked.find(parentRenderTileID) != checked.end()) {
-                        // Break parent tile ascent, this route has been checked by another child
-                        // tile before.
-                        break;
+                if (checked.find(parentRenderTileID) != checked.end()) {
+                    // Break parent tile ascent, this route has been checked by another child
+                    // tile before.
+                    break;
+                } else {
+                    checked.emplace(parentRenderTileID);
+                }
+
+                tile = getTile(parentDataTileID);
+                // Don't do cache lookups for parents while we're in the "fully renderable" pass:
+                // Since the cache tiles aren't currently displaying, we don't have to worry about symbol flicker
+                // And we'd rather get a partially rendered tile closer to our ideal zoom level
+                if (!tile && !requireFullyRenderable && (parentHasTriedOptional || parentIsLoaded)) {
+                    tile = createTile(parentDataTileID);
+                }
+
+                if (tile) {
+                    if (!parentIsLoaded) {
+                        // We haven't completed loading the child, so we only do an optional
+                        // (cache) request in an attempt to quickly load data that we can show.
+                        retainTile(*tile, TileNecessity::Optional);
                     } else {
-                        checked.emplace(parentRenderTileID);
+                        // Now that we've checked the child and know for sure that we can't load
+                        // it, we attempt to load the parent from the network.
+                        retainTile(*tile, TileNecessity::Required);
                     }
 
-                    tile = getTile(parentDataTileID);
-                    if (!tile && (parentHasTriedOptional || parentIsLoaded)) {
-                        tile = createTile(parentDataTileID);
-                    }
+                    // Save the current values, since they're the parent of the next iteration
+                    // of the parent tile ascent loop.
+                    parentHasTriedOptional = tile->hasTriedCache();
+                    parentIsLoaded = tile->isLoaded();
 
-                    if (tile) {
-                        if (!parentIsLoaded) {
-                            // We haven't completed loading the child, so we only do an optional
-                            // (cache) request in an attempt to quickly load data that we can show.
-                            retainTile(*tile, TileNecessity::Optional);
-                        } else {
-                            // Now that we've checked the child and know for sure that we can't load
-                            // it, we attempt to load the parent from the network.
-                            retainTile(*tile, TileNecessity::Required);
-                        }
-
-                        // Save the current values, since they're the parent of the next iteration
-                        // of the parent tile ascent loop.
-                        parentHasTriedOptional = tile->hasTriedCache();
-                        parentIsLoaded = tile->isLoaded();
-
-                        if (tile->isRenderable()) {
-                            renderTile(parentRenderTileID, *tile);
-                            // Break parent tile ascent, since we found one.
-                            break;
-                        }
+                    if ((requireFullyRenderable ? tile->isFullyRenderable() : tile->isRenderable())) {
+                        //std::cout << "Rendering parent zoom change: " << parentRenderTileID.canonical.z - idealRenderTileID.canonical.z << std::endl;
+                        renderTiles.emplace_back(std::make_pair(parentRenderTileID, tile));
+                        covered = true;
+                        // Break parent tile ascent, since we found one.
+                        break;
                     }
                 }
             }
         }
     }
+    if (covered || !requireFullyRenderable) {
+        // Only add tiles to render tree if we've covered the ideal tile OR we're on our second pass
+        for (auto tilePair : renderTiles) {
+          //  std::cout << "Rendering tile: " << tilePair.first << std::endl;
+            renderTile(tilePair.first, *(tilePair.second));
+        }
+    } else {
+       // std::cout << "No tiles added to render tree" << std::endl;
+    }
+    return covered;
+}
+
+template <typename GetTileFn,
+          typename CreateTileFn,
+          typename RetainTileFn,
+          typename RenderTileFn,
+          typename IdealTileIDs>
+void updateRenderables(GetTileFn getTile,
+                       CreateTileFn createTile,
+                       RetainTileFn retainTile,
+                       RenderTileFn renderTile,
+                       const IdealTileIDs& idealTileIDs,
+                       const Range<uint8_t>& zoomRange,
+                       const uint8_t dataTileZoom) {
+    std::unordered_set<UnwrappedTileID> checkedPartial;
+    std::unordered_set<UnwrappedTileID> checkedFull;
+
+    //std::cout << "Begin updateRenderables" << std::endl;
+
+    // for (all in the set of ideal tiles of the source) {
+    for (const auto& idealRenderTileID : idealTileIDs) {
+        assert(idealRenderTileID.canonical.z >= zoomRange.min);
+        assert(idealRenderTileID.canonical.z <= zoomRange.max);
+        assert(dataTileZoom >= idealRenderTileID.canonical.z);
+
+        const OverscaledTileID idealDataTileID(dataTileZoom, idealRenderTileID.wrap, idealRenderTileID.canonical);
+        
+        //std::cout << "Ideal tile ID: " << idealDataTileID << std::endl;
+
+        // TODO: Two-pass algorithm isn't necessary for tiles that don't have two stage loading (e.g. raster tiles)
+        // TODO: More efficient to write algorithm as single pass?
+       if (!findBestTile(getTile, createTile, retainTile, renderTile, zoomRange, dataTileZoom, idealRenderTileID, idealDataTileID, true, checkedFull)) {
+            findBestTile(getTile, createTile, retainTile, renderTile, zoomRange, dataTileZoom, idealRenderTileID, idealDataTileID, false, checkedPartial);
+       }
+    }
 }
 
 } // namespace algorithm
diff --git a/src/mbgl/renderer/paint_parameters.cpp b/src/mbgl/renderer/paint_parameters.cpp
index beaa9d34f2..36520806bd 100644
--- a/src/mbgl/renderer/paint_parameters.cpp
+++ b/src/mbgl/renderer/paint_parameters.cpp
@@ -26,7 +26,7 @@ PaintParameters::PaintParameters(gl::Context& context_,
     debugOptions(updateParameters.debugOptions),
     contextMode(contextMode_),
     timePoint(updateParameters.timePoint),
-    symbolFadeChange(updateParameters.mode == MapMode::Still ? 1 : std::chrono::duration<float>(placementCommitTime - updateParameters.timePoint) / Duration(std::chrono::milliseconds(300))), // TODO don't hardcode
+    symbolFadeChange((updateParameters.mode == MapMode::Still) ? 1 : std::chrono::duration<float>(placementCommitTime - updateParameters.timePoint) / Duration(std::chrono::milliseconds(300))), // TODO don't hardcode
     pixelRatio(pixelRatio_),
 #ifndef NDEBUG
     programs((debugOptions & MapDebugOptions::Overdraw) ? staticData_.overdrawPrograms : staticData_.programs)
@@ -67,6 +67,7 @@ gl::DepthMode PaintParameters::depthModeFor3D(gl::DepthMode::Mask mask) const {
 }
 
 gl::StencilMode PaintParameters::stencilModeForClipping(const ClipID& id) const {
+    //return gl::StencilMode::disabled();
     return gl::StencilMode {
         gl::StencilMode::Equal { static_cast<uint32_t>(id.mask.to_ulong()) },
         static_cast<int32_t>(id.reference.to_ulong()),
diff --git a/src/mbgl/renderer/render_layer.hpp b/src/mbgl/renderer/render_layer.hpp
index 79c8bcccf7..4c33cd5435 100644
--- a/src/mbgl/renderer/render_layer.hpp
+++ b/src/mbgl/renderer/render_layer.hpp
@@ -1,6 +1,7 @@
 #pragma once
 
 #include <mbgl/renderer/render_pass.hpp>
+#include <mbgl/renderer/render_tile.hpp>
 #include <mbgl/style/layer_impl.hpp>
 #include <mbgl/style/layer_type.hpp>
 #include <mbgl/tile/geometry_tile_data.hpp>
@@ -8,6 +9,8 @@
 #include <memory>
 #include <string>
 
+#include <iostream>
+
 namespace mbgl {
 
 class Bucket;
@@ -62,6 +65,14 @@ public:
     bool needsRendering(float zoom) const;
 
     virtual void render(PaintParameters&, RenderSource*) = 0;
+    void render(PaintParameters& parameters, RenderSource* source, const std::map<UnwrappedTileID, ClipID>& clipIDs) {
+        for (const RenderTile& tile : renderTiles) {
+            if (clipIDs.find(tile.id) == clipIDs.end()) {
+                std::cout << "Rendering tile without a clipping mask!" << std::endl;
+            }
+        }
+        render(parameters, source);
+    }
 
     // Check wether the given geometry intersects
     // with the feature
diff --git a/src/mbgl/renderer/renderer_impl.cpp b/src/mbgl/renderer/renderer_impl.cpp
index 36d8895085..7f3d4949aa 100644
--- a/src/mbgl/renderer/renderer_impl.cpp
+++ b/src/mbgl/renderer/renderer_impl.cpp
@@ -29,6 +29,8 @@
 #include <mbgl/util/string.hpp>
 #include <mbgl/util/logging.hpp>
 
+#include <iostream>
+
 namespace mbgl {
 
 using namespace style;
@@ -233,12 +235,21 @@ void Renderer::Impl::render(const UpdateParameters& updateParameters) {
 
             filteredLayers.push_back(layer);
         }
+//
+//        for (auto renderTile : renderSources.at(source->id)->getRenderTiles()) {
+//            //std::cout << source->id << " renderTile " << renderTile.get().id << std::endl;
+//        }
 
         renderSources.at(source->id)->update(source,
                                              filteredLayers,
                                              needsRendering,
                                              needsRelayout,
                                              tileParameters);
+        
+//         for (auto renderTile : renderSources.at(source->id)->getRenderTiles()) {
+//            //std::cout << source->id << " renderTile clip " << renderTile.get().clip << std::endl;
+//        }
+
     }
 
     transformState = updateParameters.transformState;
@@ -457,6 +468,7 @@ void Renderer::Impl::render(const UpdateParameters& updateParameters) {
                       0);
     }
 
+    auto clipIDs = parameters.clipIDGenerator.getClipIDs();
     // - CLIPPING MASKS ----------------------------------------------------------------------------
     // Draws the clipping masks to the stencil buffer.
     {
@@ -464,8 +476,14 @@ void Renderer::Impl::render(const UpdateParameters& updateParameters) {
 
         static const style::FillPaintProperties::PossiblyEvaluated properties {};
         static const FillProgram::PaintPropertyBinders paintAttibuteData(properties, 0);
+        
+        
+        //std::cout << "Drawing clipping masks" << std::endl;
+        
 
-        for (const auto& clipID : parameters.clipIDGenerator.getClipIDs()) {
+
+        for (const auto& clipID : clipIDs) {
+           // std::cout << clipID.first << ", " << clipID.second << std::endl;
             parameters.staticData.programs.fill.get(properties).draw(
                 parameters.context,
                 gl::Triangles(),
@@ -556,7 +574,7 @@ void Renderer::Impl::render(const UpdateParameters& updateParameters) {
             parameters.currentLayer = i;
             if (it->layer.hasRenderPass(parameters.pass)) {
                 MBGL_DEBUG_GROUP(parameters.context, it->layer.getID());
-                it->layer.render(parameters, it->source);
+                it->layer.render(parameters, it->source, clipIDs);
             }
         }
     }
diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp
index 9c83864a87..0faf7ae592 100644
--- a/src/mbgl/tile/geometry_tile.cpp
+++ b/src/mbgl/tile/geometry_tile.cpp
@@ -68,6 +68,10 @@ GeometryTile::~GeometryTile() {
     markObsolete();
 }
 
+bool GeometryTile::isFullyRenderable() const {
+    return renderable && !symbolBuckets.empty();
+}
+
 void GeometryTile::cancel() {
     markObsolete();
 }
@@ -125,11 +129,11 @@ void GeometryTile::setShowCollisionBoxes(const bool showCollisionBoxes_) {
 }
 
 void GeometryTile::onLayout(LayoutResult result, const uint64_t resultCorrelationID) {
-    // Don't mark ourselves loaded or renderable until the first successful placement
-    // TODO: Ideally we'd render this tile without symbols as long as this tile wasn't
-    //  replacing a tile at a different zoom that _did_ have symbols.
+    loaded = true;
+    renderable = true;
     (void)resultCorrelationID;
     nonSymbolBuckets = std::move(result.nonSymbolBuckets);
+    symbolBuckets.clear();
     featureIndex = std::move(result.featureIndex);
     data = std::move(result.tileData);
     observer->onTileChanged(*this);
diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp
index 3f4b36984b..43004b7793 100644
--- a/src/mbgl/tile/geometry_tile.hpp
+++ b/src/mbgl/tile/geometry_tile.hpp
@@ -31,6 +31,8 @@ public:
                  const TileParameters&);
 
     ~GeometryTile() override;
+    
+    bool isFullyRenderable() const override;
 
     void setError(std::exception_ptr);
     void setData(std::unique_ptr<const GeometryTileData>);
diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp
index 969b137c1f..542845e81c 100644
--- a/src/mbgl/tile/geometry_tile_worker.cpp
+++ b/src/mbgl/tile/geometry_tile_worker.cpp
@@ -418,6 +418,8 @@ void GeometryTileWorker::attemptPlacement() {
             buckets.emplace(pair.first, bucket);
         }
     }
+    
+    //std::this_thread::sleep_for(Milliseconds(1500));
 
     parent.invoke(&GeometryTile::onPlacement, GeometryTile::PlacementResult {
         std::move(buckets),
diff --git a/src/mbgl/tile/tile.hpp b/src/mbgl/tile/tile.hpp
index 6c163c93bb..0ccf2e72db 100644
--- a/src/mbgl/tile/tile.hpp
+++ b/src/mbgl/tile/tile.hpp
@@ -79,6 +79,10 @@ public:
     bool isRenderable() const {
         return renderable;
     }
+    
+    virtual bool isFullyRenderable() const {
+        return renderable;
+    }
 
     // A tile is "Loaded" when we have received a response from a FileSource, and have attempted to
     // parse the tile (if applicable). Tile implementations should set this to true when a load
diff --git a/src/mbgl/tile/tile_cache.cpp b/src/mbgl/tile/tile_cache.cpp
index 3fafb1259c..ed0ef9ae87 100644
--- a/src/mbgl/tile/tile_cache.cpp
+++ b/src/mbgl/tile/tile_cache.cpp
@@ -52,6 +52,7 @@ std::unique_ptr<Tile> TileCache::get(const OverscaledTileID& key) {
     }
 
     return tile;
+    //return std::unique_ptr<Tile>();
 }
 
 bool TileCache::has(const OverscaledTileID& key) {
diff --git a/test/algorithm/mock.hpp b/test/algorithm/mock.hpp
index b8eb020105..dea2162b04 100644
--- a/test/algorithm/mock.hpp
+++ b/test/algorithm/mock.hpp
@@ -33,12 +33,17 @@ struct MockTileData {
     bool isRenderable() const {
         return renderable;
     }
+    
+    bool isFullyRenderable() const {
+        return fullyRenderable;
+    }
 
     bool isLoaded() const {
         return loaded;
     }
 
     bool renderable = false;
+    bool fullyRenderable = false;
     bool triedOptional = false;
     bool loaded = false;
     const mbgl::OverscaledTileID tileID;