Move Transform from MapData to Map

Pass current state to MapContext where needed. Remove mutex from
Transform.

6 files changed, 64 insertions, 113 deletions

diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp
index 01e199f460..cc40ff2ba9 100644
--- a/src/mbgl/map/map.cpp
+++ b/src/mbgl/map/map.cpp
@@ -1,6 +1,8 @@
 #include <mbgl/map/map.hpp>
 #include <mbgl/map/map_context.hpp>
 #include <mbgl/map/view.hpp>
+#include <mbgl/map/transform.hpp>
+#include <mbgl/map/transform_state.hpp>
 #include <mbgl/map/map_data.hpp>
 
 #include <mbgl/util/projection.hpp>
@@ -11,7 +13,8 @@
 namespace mbgl {
 
 Map::Map(View& view, FileSource& fileSource, MapMode mode)
-    : data(std::make_unique<MapData>(view, mode)),
+    : transform(std::make_unique<Transform>(view)),
+      data(std::make_unique<MapData>(mode)),
       context(std::make_unique<util::Thread<MapContext>>(util::ThreadContext{"Map", util::ThreadType::Map, util::ThreadPriority::Regular}, view, fileSource, *data))
 {
     view.initialize(this);
@@ -39,15 +42,21 @@ void Map::resume() {
 }
 
 void Map::renderStill(StillImageCallback callback) {
-    context->invoke(&MapContext::renderStill, callback);
+    context->invoke(&MapContext::renderStill, transform->currentState(), callback);
 }
 
 void Map::renderSync() {
-    context->invokeSync(&MapContext::renderSync);
+    bool rerender = context->invokeSync<bool>(&MapContext::renderSync, transform->currentState());
+
+    if (transform->needsTransition()) {
+        update(Update(transform->updateTransitions(Clock::now())));
+    } else if (rerender) {
+        update();
+    }
 }
 
 void Map::update(Update update_) {
-    context->invoke(&MapContext::triggerUpdate, update_);
+    context->invoke(&MapContext::triggerUpdate, transform->currentState(), update_);
 }
 
 #pragma mark - Style
@@ -71,43 +80,44 @@ std::string Map::getStyleJSON() const {
 #pragma mark - Size
 
 void Map::resize(uint16_t width, uint16_t height, float ratio) {
-    if (data->transform.resize(width, height, ratio, width * ratio, height * ratio)) {
+    if (transform->resize(width, height, ratio, width * ratio, height * ratio)) {
         context->invoke(&MapContext::resize, width, height, ratio);
+        update();
     }
 }
 
 #pragma mark - Transitions
 
 void Map::cancelTransitions() {
-    data->transform.cancelTransitions();
+    transform->cancelTransitions();
     update();
 }
 
 void Map::setGestureInProgress(bool inProgress) {
-    data->transform.setGestureInProgress(inProgress);
+    transform->setGestureInProgress(inProgress);
     update();
 }
 
 #pragma mark - Position
 
 void Map::moveBy(double dx, double dy, Duration duration) {
-    data->transform.moveBy(dx, dy, duration);
+    transform->moveBy(dx, dy, duration);
     update();
 }
 
 void Map::setLatLng(LatLng latLng, Duration duration) {
-    data->transform.setLatLng(latLng, duration);
+    transform->setLatLng(latLng, duration);
     update();
 }
 
 LatLng Map::getLatLng() const {
-    return data->transform.getLatLng();
+    return transform->getLatLng();
 }
 
 void Map::resetPosition() {
-    data->transform.setAngle(0);
-    data->transform.setLatLng(LatLng(0, 0));
-    data->transform.setZoom(0);
+    transform->setAngle(0);
+    transform->setLatLng(LatLng(0, 0));
+    transform->setZoom(0);
     update(Update::Zoom);
 }
 
@@ -115,30 +125,30 @@ void Map::resetPosition() {
 #pragma mark - Scale
 
 void Map::scaleBy(double ds, double cx, double cy, Duration duration) {
-    data->transform.scaleBy(ds, cx, cy, duration);
+    transform->scaleBy(ds, cx, cy, duration);
     update(Update::Zoom);
 }
 
 void Map::setScale(double scale, double cx, double cy, Duration duration) {
-    data->transform.setScale(scale, cx, cy, duration);
+    transform->setScale(scale, cx, cy, duration);
     update(Update::Zoom);
 }
 
 double Map::getScale() const {
-    return data->transform.getScale();
+    return transform->getScale();
 }
 
 void Map::setZoom(double zoom, Duration duration) {
-    data->transform.setZoom(zoom, duration);
+    transform->setZoom(zoom, duration);
     update(Update::Zoom);
 }
 
 double Map::getZoom() const {
-    return data->transform.getZoom();
+    return transform->getZoom();
 }
 
 void Map::setLatLngZoom(LatLng latLng, double zoom, Duration duration) {
-    data->transform.setLatLngZoom(latLng, zoom, duration);
+    transform->setLatLngZoom(latLng, zoom, duration);
     update(Update::Zoom);
 }
 
@@ -186,48 +196,48 @@ void Map::resetZoom() {
 }
 
 double Map::getMinZoom() const {
-    return data->transform.currentState().getMinZoom();
+    return transform->currentState().getMinZoom();
 }
 
 double Map::getMaxZoom() const {
-    return data->transform.currentState().getMaxZoom();
+    return transform->currentState().getMaxZoom();
 }
 
 
 #pragma mark - Size
 
 uint16_t Map::getWidth() const {
-    return data->transform.currentState().getWidth();
+    return transform->currentState().getWidth();
 }
 
 uint16_t Map::getHeight() const {
-    return data->transform.currentState().getHeight();
+    return transform->currentState().getHeight();
 }
 
 
 #pragma mark - Rotation
 
 void Map::rotateBy(double sx, double sy, double ex, double ey, Duration duration) {
-    data->transform.rotateBy(sx, sy, ex, ey, duration);
+    transform->rotateBy(sx, sy, ex, ey, duration);
     update();
 }
 
 void Map::setBearing(double degrees, Duration duration) {
-    data->transform.setAngle(-degrees * M_PI / 180, duration);
+    transform->setAngle(-degrees * M_PI / 180, duration);
     update();
 }
 
 void Map::setBearing(double degrees, double cx, double cy) {
-    data->transform.setAngle(-degrees * M_PI / 180, cx, cy);
+    transform->setAngle(-degrees * M_PI / 180, cx, cy);
     update();
 }
 
 double Map::getBearing() const {
-    return -data->transform.getAngle() / M_PI * 180;
+    return -transform->getAngle() / M_PI * 180;
 }
 
 void Map::resetNorth() {
-    data->transform.setAngle(0, std::chrono::milliseconds(500));
+    transform->setAngle(0, std::chrono::milliseconds(500));
     update();
 }
 
@@ -255,11 +265,11 @@ const LatLng Map::latLngForProjectedMeters(const ProjectedMeters projectedMeters
 }
 
 const vec2<double> Map::pixelForLatLng(const LatLng latLng) const {
-    return data->transform.currentState().pixelForLatLng(latLng);
+    return transform->currentState().pixelForLatLng(latLng);
 }
 
 const LatLng Map::latLngForPixel(const vec2<double> pixel) const {
-    return data->transform.currentState().latLngForPixel(pixel);
+    return transform->currentState().latLngForPixel(pixel);
 }
 
 #pragma mark - Annotations
diff --git a/src/mbgl/map/map_context.cpp b/src/mbgl/map/map_context.cpp
index 97cc8de48b..823bff54a4 100644
--- a/src/mbgl/map/map_context.cpp
+++ b/src/mbgl/map/map_context.cpp
@@ -75,11 +75,12 @@ void MapContext::pause() {
 
 void MapContext::resize(uint16_t width, uint16_t height, float ratio) {
     view.resize(width, height, ratio);
-    triggerUpdate();
 }
 
-void MapContext::triggerUpdate(const Update u) {
+void MapContext::triggerUpdate(const TransformState& state, const Update u) {
+    transformState = state;
     updated |= static_cast<UpdateType>(u);
+
     asyncUpdate->send();
 }
 
@@ -119,7 +120,8 @@ void MapContext::loadStyleJSON(const std::string& json, const std::string& base)
     style->setDefaultTransitionDuration(data.getDefaultTransitionDuration());
     style->setObserver(this);
 
-    triggerUpdate(Update::Zoom);
+    updated |= static_cast<UpdateType>(Update::Zoom);
+    asyncUpdate->send();
 
     auto staleTiles = data.annotationManager.resetStaleTiles();
     if (staleTiles.size()) {
@@ -215,7 +217,8 @@ void MapContext::updateAnnotationTiles(const std::unordered_set<TileID, TileID::
 
     cascadeClasses();
 
-    triggerUpdate(Update::Classes);
+    updated |= static_cast<UpdateType>(Update::Classes);
+    asyncUpdate->send();
 
     data.annotationManager.resetStaleTiles();
 }
@@ -230,9 +233,6 @@ void MapContext::update() {
     const auto now = Clock::now();
     data.setAnimationTime(now);
 
-    updated |= data.transform.updateTransitions(now);
-    transformState = data.transform.currentState();
-
     if (style) {
         if (updated & static_cast<UpdateType>(Update::DefaultTransitionDuration)) {
             style->setDefaultTransitionDuration(data.getDefaultTransitionDuration());
@@ -259,8 +259,8 @@ void MapContext::update() {
             }
         }
 
-        if (mayRender) {
-            render();
+        if (callback) {
+            renderSync(transformState);
         } else {
             view.invalidate();
         }
@@ -269,7 +269,7 @@ void MapContext::update() {
     updated = static_cast<UpdateType>(Update::Nothing);
 }
 
-void MapContext::renderStill(StillImageCallback fn) {
+void MapContext::renderStill(const TransformState& state, StillImageCallback fn) {
     if (!fn) {
         Log::Error(Event::General, "StillImageCallback not set");
         return;
@@ -296,25 +296,23 @@ void MapContext::renderStill(StillImageCallback fn) {
     }
 
     callback = fn;
-    mayRender = true;
-    triggerUpdate(Update::RenderStill);
-}
+    transformState = state;
 
-void MapContext::renderSync() {
-    mayRender = true;
-    render();
-    mayRender = false;
+    updated |= static_cast<UpdateType>(Update::RenderStill);
+    asyncUpdate->send();
 }
 
-void MapContext::render() {
+bool MapContext::renderSync(const TransformState& state) {
     assert(util::ThreadContext::currentlyOn(util::ThreadType::Map));
 
+    transformState = state;
+
     // Cleanup OpenGL objects that we abandoned since the last render call.
     glObjectStore.performCleanup();
 
     if (data.mode == MapMode::Still && (!callback || !data.getFullyLoaded())) {
         // We are either not waiting for a map to be rendered, or we don't have all resources yet.
-        return;
+        return false;
     }
 
     assert(style);
@@ -327,8 +325,6 @@ void MapContext::render() {
     painter->setDebug(data.getDebug());
     painter->render(*style, transformState, data.getAnimationTime());
 
-    mayRender = false;
-
     if (data.mode == MapMode::Still) {
         callback(nullptr, view.readStillImage());
         callback = nullptr;
@@ -336,10 +332,7 @@ void MapContext::render() {
 
     view.swap();
 
-    // Schedule another rerender when we definitely need a next frame.
-    if (data.transform.needsTransition() || style->hasTransitions()) {
-        triggerUpdate();
-    }
+    return style->hasTransitions();
 }
 
 double MapContext::getTopOffsetPixelsForAnnotationSymbol(const std::string& symbol) {
@@ -371,7 +364,7 @@ void MapContext::onLowMemory() {
 
 void MapContext::onTileDataChanged() {
     assert(util::ThreadContext::currentlyOn(util::ThreadType::Map));
-    triggerUpdate();
+    asyncUpdate->send();
 }
 
 void MapContext::onResourceLoadingFailed(std::exception_ptr error) {
diff --git a/src/mbgl/map/map_context.hpp b/src/mbgl/map/map_context.hpp
index 810219e6b0..fedafb7d09 100644
--- a/src/mbgl/map/map_context.hpp
+++ b/src/mbgl/map/map_context.hpp
@@ -38,10 +38,10 @@ public:
     void resize(uint16_t width, uint16_t height, float ratio);
 
     using StillImageCallback = std::function<void(std::exception_ptr, std::unique_ptr<const StillImage>)>;
-    void renderStill(StillImageCallback callback);
-    void renderSync();
 
-    void triggerUpdate(Update = Update::Nothing);
+    void triggerUpdate(const TransformState&, Update = Update::Nothing);
+    void renderStill(const TransformState&, StillImageCallback callback);
+    bool renderSync(const TransformState&);
 
     void setStyleURL(const std::string&);
     void setStyleJSON(const std::string& json, const std::string& base);
@@ -61,8 +61,6 @@ public:
     void onResourceLoadingFailed(std::exception_ptr error) override;
 
 private:
-    void render();
-
     // Style-related updates.
     void cascadeClasses();
 
@@ -87,7 +85,6 @@ private:
     std::string styleURL;
     std::string styleJSON;
 
-    bool mayRender = false;
     StillImageCallback callback;
     size_t sourceCacheSize;
     TransformState transformState;
diff --git a/src/mbgl/map/map_data.hpp b/src/mbgl/map/map_data.hpp
index 2d22a00689..10f33df192 100644
--- a/src/mbgl/map/map_data.hpp
+++ b/src/mbgl/map/map_data.hpp
@@ -11,8 +11,6 @@
 #include <condition_variable>
 
 #include <mbgl/map/mode.hpp>
-#include <mbgl/map/transform.hpp>
-#include <mbgl/map/transform_state.hpp>
 #include <mbgl/map/annotation.hpp>
 
 namespace mbgl {
@@ -21,7 +19,7 @@ class MapData {
     using Lock = std::lock_guard<std::mutex>;
 
 public:
-    inline MapData(View& view, MapMode mode_) : transform(view), mode(mode_) {
+    inline MapData(MapMode mode_) : mode(mode_) {
         setAnimationTime(TimePoint::min());
         setDefaultTransitionDuration(Duration::zero());
     }
@@ -88,7 +86,6 @@ public:
     };
 
 public:
-    Transform transform;
     AnnotationManager annotationManager;
     const MapMode mode;
 
diff --git a/src/mbgl/map/transform.cpp b/src/mbgl/map/transform.cpp
index b7296f6f7e..475589c2ae 100644
--- a/src/mbgl/map/transform.cpp
+++ b/src/mbgl/map/transform.cpp
@@ -35,8 +35,6 @@ Transform::Transform(View &view_)
 
 bool Transform::resize(const uint16_t w, const uint16_t h, const float ratio,
                        const uint16_t fb_w, const uint16_t fb_h) {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     if (state.width != w || state.height != h || state.pixelRatio != ratio ||
         state.framebuffer[0] != fb_w || state.framebuffer[1] != fb_h) {
 
@@ -64,14 +62,10 @@ void Transform::moveBy(const double dx, const double dy, const Duration duration
         return;
     }
 
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     _moveBy(dx, dy, duration);
 }
 
 void Transform::_moveBy(const double dx, const double dy, const Duration duration) {
-    // This is only called internally, so we don't need a lock here.
-
     view.notifyMapChange(duration != Duration::zero() ?
                            MapChangeRegionWillChangeAnimated :
                            MapChangeRegionWillChange);
@@ -111,8 +105,6 @@ void Transform::setLatLng(const LatLng latLng, const Duration duration) {
         return;
     }
 
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     const double m = 1 - 1e-15;
     const double f = std::fmin(std::fmax(std::sin(util::DEG2RAD * latLng.latitude), -m), m);
 
@@ -127,8 +119,6 @@ void Transform::setLatLngZoom(const LatLng latLng, const double zoom, const Dura
         return;
     }
 
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     double new_scale = std::pow(2.0, zoom);
 
     const double s = new_scale * util::tileSize;
@@ -152,8 +142,6 @@ void Transform::scaleBy(const double ds, const double cx, const double cy, const
         return;
     }
 
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     // clamp scale to min/max values
     double new_scale = state.scale * ds;
     if (new_scale < state.min_scale) {
@@ -171,8 +159,6 @@ void Transform::setScale(const double scale, const double cx, const double cy,
         return;
     }
 
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     _setScale(scale, cx, cy, duration);
 }
 
@@ -181,26 +167,18 @@ void Transform::setZoom(const double zoom, const Duration duration) {
         return;
     }
 
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     _setScale(std::pow(2.0, zoom), -1, -1, duration);
 }
 
 double Transform::getZoom() const {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     return state.getZoom();
 }
 
 double Transform::getScale() const {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     return state.scale;
 }
 
 void Transform::_setScale(double new_scale, double cx, double cy, const Duration duration) {
-    // This is only called internally, so we don't need a lock here.
-
     // Ensure that we don't zoom in further than the maximum allowed.
     if (new_scale < state.min_scale) {
         new_scale = state.min_scale;
@@ -233,8 +211,6 @@ void Transform::_setScale(double new_scale, double cx, double cy, const Duration
 
 void Transform::_setScaleXY(const double new_scale, const double xn, const double yn,
                             const Duration duration) {
-    // This is only called internally, so we don't need a lock here.
-
     view.notifyMapChange(duration != Duration::zero() ?
                            MapChangeRegionWillChangeAnimated :
                            MapChangeRegionWillChange);
@@ -289,8 +265,6 @@ void Transform::rotateBy(const double start_x, const double start_y, const doubl
         return;
     }
 
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     double center_x = static_cast<double>(state.width) / 2.0, center_y = static_cast<double>(state.height) / 2.0;
 
     const double begin_center_x = start_x - center_x;
@@ -323,8 +297,6 @@ void Transform::setAngle(const double new_angle, const Duration duration) {
         return;
     }
 
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     _setAngle(new_angle, duration);
 }
 
@@ -333,8 +305,6 @@ void Transform::setAngle(const double new_angle, const double cx, const double c
         return;
     }
 
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     double dx = 0, dy = 0;
 
     if (cx >= 0 && cy >= 0) {
@@ -351,8 +321,6 @@ void Transform::setAngle(const double new_angle, const double cx, const double c
 }
 
 void Transform::_setAngle(double new_angle, const Duration duration) {
-    // This is only called internally, so we don't need a lock here.
-
     view.notifyMapChange(duration != Duration::zero() ?
                            MapChangeRegionWillChangeAnimated :
                            MapChangeRegionWillChange);
@@ -383,8 +351,6 @@ void Transform::_setAngle(double new_angle, const Duration duration) {
 }
 
 double Transform::getAngle() const {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     return state.angle;
 }
 
@@ -419,18 +385,14 @@ void Transform::startTransition(std::function<Update(double)> frame,
 }
 
 bool Transform::needsTransition() const {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
     return !!transitionFrameFn;
 }
 
 UpdateType Transform::updateTransitions(const TimePoint now) {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
     return static_cast<UpdateType>(transitionFrameFn ? transitionFrameFn(now) : Update::Nothing);
 }
 
 void Transform::cancelTransitions() {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     if (transitionFinishFn) {
         transitionFinishFn();
     }
@@ -440,15 +402,11 @@ void Transform::cancelTransitions() {
 }
 
 void Transform::setGestureInProgress(bool inProgress) {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     state.gestureInProgress = inProgress;
 }
 
 #pragma mark - Transform state
 
 const TransformState Transform::currentState() const {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     return state;
 }
diff --git a/src/mbgl/map/transform.hpp b/src/mbgl/map/transform.hpp
index a92b4abaf8..678f3ffc11 100644
--- a/src/mbgl/map/transform.hpp
+++ b/src/mbgl/map/transform.hpp
@@ -9,7 +9,7 @@
 
 #include <cstdint>
 #include <cmath>
-#include <mutex>
+#include <functional>
 
 namespace mbgl {
 
@@ -55,8 +55,6 @@ public:
     const TransformState currentState() const;
 
 private:
-    // Functions prefixed with underscores will *not* perform any locks. It is the caller's
-    // responsibility to lock this object.
     void _moveBy(double dx, double dy, Duration = Duration::zero());
     void _setScale(double scale, double cx, double cy, Duration = Duration::zero());
     void _setScaleXY(double new_scale, double xn, double yn, Duration = Duration::zero());
@@ -64,8 +62,6 @@ private:
 
     View &view;
 
-    mutable std::recursive_mutex mtx;
-
     TransformState state;
 
     void startTransition(std::function<Update(double)> frame,