diff options
22 files changed, 228 insertions, 172 deletions
diff --git a/android/java/.gitignore b/android/java/.gitignore index 5f00b1092f..8664418107 100644 --- a/android/java/.gitignore +++ b/android/java/.gitignore @@ -18,3 +18,6 @@ local.properties # Token file MapboxGLAndroidSDKTestApp/src/main/res/raw/token.txt + +# Twitter Fabric / Crashlytics +fabric.properties diff --git a/android/java/MapboxGLAndroidSDKTestApp/build.gradle b/android/java/MapboxGLAndroidSDKTestApp/build.gradle index 46c95c21ec..d407629b2d 100644 --- a/android/java/MapboxGLAndroidSDKTestApp/build.gradle +++ b/android/java/MapboxGLAndroidSDKTestApp/build.gradle @@ -1,15 +1,23 @@ buildscript { repositories { mavenCentral() + maven { url 'https://maven.fabric.io/public' } } dependencies { classpath 'com.android.tools.build:gradle:1.2.3' classpath 'com.jakewharton.sdkmanager:gradle-plugin:0.12.0' + classpath 'io.fabric.tools:gradle:1.+' } } apply plugin: 'android-sdk-manager' apply plugin: 'com.android.application' +apply plugin: 'io.fabric' + +repositories { + maven { url 'https://maven.fabric.io/public' } +} + apply plugin: 'checkstyle' task accessToken { @@ -78,6 +86,9 @@ dependencies { compile 'com.android.support:support-v4:22.1.1' compile 'com.android.support:appcompat-v7:22.1.1' compile 'com.mapzen.android:lost:1.0.0' + compile('com.crashlytics.sdk.android:crashlytics:2.4.0@aar') { + transitive = true; + } } checkstyle { diff --git a/android/java/MapboxGLAndroidSDKTestApp/src/main/AndroidManifest.xml b/android/java/MapboxGLAndroidSDKTestApp/src/main/AndroidManifest.xml index 901a63f572..fa21e4a121 100644 --- a/android/java/MapboxGLAndroidSDKTestApp/src/main/AndroidManifest.xml +++ b/android/java/MapboxGLAndroidSDKTestApp/src/main/AndroidManifest.xml @@ -20,6 +20,10 @@ <category android:name="android.intent.category.LAUNCHER" /> </intent-filter> </activity> + <meta-data + android:name="io.fabric.ApiKey" + android:value="9724157045ff7d083492c6d9ae03e60e8609d461" /> </application> + <uses-permission android:name="android.permission.INTERNET" /> </manifest> diff --git a/android/java/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxgl/testapp/MainActivity.java b/android/java/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxgl/testapp/MainActivity.java index bb9a8e7518..87f076670c 100644 --- a/android/java/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxgl/testapp/MainActivity.java +++ b/android/java/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxgl/testapp/MainActivity.java @@ -24,6 +24,7 @@ import android.widget.ImageView; import android.widget.Spinner; import android.widget.TextView; +import com.crashlytics.android.Crashlytics; import com.mapbox.mapboxgl.annotations.Marker; import com.mapbox.mapboxgl.annotations.MarkerOptions; import com.mapbox.mapboxgl.annotations.Polygon; @@ -37,6 +38,7 @@ import com.mapzen.android.lost.api.LocationRequest; import com.mapzen.android.lost.api.LocationServices; import com.mapzen.android.lost.api.LostApiClient; +import io.fabric.sdk.android.Fabric; import org.json.JSONException; import java.io.IOException; @@ -100,6 +102,7 @@ public class MainActivity extends ActionBarActivity { @Override protected void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); + Fabric.with(this, new Crashlytics()); if (savedInstanceState != null) { mIsGpsOn = savedInstanceState.getBoolean(STATE_IS_GPS_ON, false); diff --git a/include/mbgl/map/map.hpp b/include/mbgl/map/map.hpp index 82aef65b65..6e24d177a9 100644 --- a/include/mbgl/map/map.hpp +++ b/include/mbgl/map/map.hpp @@ -84,6 +84,10 @@ public: void setDefaultTransitionDuration(const Duration& = Duration::zero()); Duration getDefaultTransitionDuration() const; + + void setDefaultTransitionDelay(const Duration& = Duration::zero()); + Duration getDefaultTransitionDelay() const; + void setStyleURL(const std::string& url); void setStyleJSON(const std::string& json, const std::string& base = ""); std::string getStyleURL() const; diff --git a/include/mbgl/map/update.hpp b/include/mbgl/map/update.hpp index 40cc1bc2bc..d2d6133bc2 100644 --- a/include/mbgl/map/update.hpp +++ b/include/mbgl/map/update.hpp @@ -10,7 +10,7 @@ using UpdateType = uint32_t; enum class Update : UpdateType { Nothing = 0, Dimensions = 1 << 1, - DefaultTransitionDuration = 1 << 2, + DefaultTransition = 1 << 2, Classes = 1 << 3, Zoom = 1 << 4, RenderStill = 1 << 5, diff --git a/platform/default/glfw_view.cpp b/platform/default/glfw_view.cpp index 6d8c208dc4..e83a832728 100644 --- a/platform/default/glfw_view.cpp +++ b/platform/default/glfw_view.cpp @@ -335,7 +335,8 @@ void GLFWView::run() { glfwWaitEvents(); const bool dirty = !clean.test_and_set(); if (dirty) { - map->renderSync(); + const bool needsRerender = map->renderSync(); + map->nudgeTransitions(needsRerender); } } } diff --git a/platform/ios/MGLMapView.mm b/platform/ios/MGLMapView.mm index e807d21c6d..0b8baeff24 100644 --- a/platform/ios/MGLMapView.mm +++ b/platform/ios/MGLMapView.mm @@ -1409,11 +1409,12 @@ std::chrono::steady_clock::duration secondsAsDuration(float duration) - (void)setDebugActive:(BOOL)debugActive { _mbglMap->setDebug(debugActive); + _mbglMap->setCollisionDebug(debugActive); } - (BOOL)isDebugActive { - return _mbglMap->getDebug(); + return (_mbglMap->getDebug() || _mbglMap->getCollisionDebug()); } - (void)resetNorth diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index d983a9c518..5b96b83199 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -422,13 +422,22 @@ std::vector<std::string> Map::getClasses() const { void Map::setDefaultTransitionDuration(const Duration& duration) { data->setDefaultTransitionDuration(duration); - update(Update::DefaultTransitionDuration); + update(Update::DefaultTransition); } Duration Map::getDefaultTransitionDuration() const { return data->getDefaultTransitionDuration(); } +void Map::setDefaultTransitionDelay(const Duration& delay) { + data->setDefaultTransitionDelay(delay); + update(Update::DefaultTransition); +} + +Duration Map::getDefaultTransitionDelay() const { + return data->getDefaultTransitionDelay(); +} + void Map::setSourceTileCacheSize(size_t size) { context->invoke(&MapContext::setSourceTileCacheSize, size); } diff --git a/src/mbgl/map/map_context.cpp b/src/mbgl/map/map_context.cpp index 61614dd0e4..1a03c9330c 100644 --- a/src/mbgl/map/map_context.cpp +++ b/src/mbgl/map/map_context.cpp @@ -136,7 +136,7 @@ void MapContext::loadStyleJSON(const std::string& json, const std::string& base) // force style cascade, causing all pending transitions to complete. style->cascade(); - updated |= static_cast<UpdateType>(Update::DefaultTransitionDuration); + updated |= static_cast<UpdateType>(Update::DefaultTransition); updated |= static_cast<UpdateType>(Update::Classes); updated |= static_cast<UpdateType>(Update::Zoom); asyncUpdate->send(); @@ -343,7 +343,7 @@ MapContext::RenderResult MapContext::renderSync(const TransformState& state, con return RenderResult { style->isLoaded(), - style->hasTransitions() + style->hasTransitions() || painter->needsAnimation() }; } diff --git a/src/mbgl/renderer/frame_history.cpp b/src/mbgl/renderer/frame_history.cpp index cb36a1d834..73336c029f 100644 --- a/src/mbgl/renderer/frame_history.cpp +++ b/src/mbgl/renderer/frame_history.cpp @@ -23,11 +23,11 @@ bool FrameHistory::needsAnimation(const Duration& duration) const { // If we have a value that is older than duration and whose z value is the // same as the most current z value, and if all values inbetween have the // same z value, we don't need animation, otherwise we probably do. - const FrameSnapshot &pivot = history.back(); + const FrameSnapshot& pivot = history.back(); int i = -1; while ((int)history.size() > i + 1 && history[i + 1].now + duration < pivot.now) { - i++; + ++i; } if (i < 0) { @@ -38,7 +38,7 @@ bool FrameHistory::needsAnimation(const Duration& duration) const { // Make sure that all subsequent snapshots have the same zoom as the last // pivot element. - for (; (int)history.size() > i; i++) { + for (; (int)history.size() > i; ++i) { if (history[i].z != pivot.z) { return true; } diff --git a/src/mbgl/style/applied_class_properties.cpp b/src/mbgl/style/applied_class_properties.cpp index 6df075b202..9563250ba7 100644 --- a/src/mbgl/style/applied_class_properties.cpp +++ b/src/mbgl/style/applied_class_properties.cpp @@ -2,28 +2,28 @@ namespace mbgl { -AppliedClassProperty::AppliedClassProperty(ClassID class_id, const TimePoint& begin_, const TimePoint& end_, const PropertyValue &value_) +AppliedClassPropertyValue::AppliedClassPropertyValue(ClassID class_id, const TimePoint& begin_, const TimePoint& end_, const PropertyValue &value_) : name(class_id), begin(begin_), end(end_), value(value_) {} // Returns the ID of the most recent -ClassID AppliedClassProperties::mostRecent() const { +ClassID AppliedClassPropertyValues::mostRecent() const { return propertyValues.empty() ? ClassID::Fallback : propertyValues.back().name; } -void AppliedClassProperties::add(ClassID class_id, const TimePoint& begin, const TimePoint& end, const PropertyValue &value) { +void AppliedClassPropertyValues::add(ClassID class_id, const TimePoint& begin, const TimePoint& end, const PropertyValue &value) { propertyValues.emplace_back(class_id, begin, end, value); } -bool AppliedClassProperties::hasTransitions() const { +bool AppliedClassPropertyValues::hasTransitions() const { return propertyValues.size() > 1; } // Erase all items in the property list that are before a completed transition. // Then, if the only remaining property is a Fallback value, remove it too. -void AppliedClassProperties::cleanup(const TimePoint& now) { +void AppliedClassPropertyValues::cleanup(const TimePoint& now) { // Iterate backwards, but without using the rbegin/rend interface since we need forward // iterators to use .erase(). for (auto it = propertyValues.end(), begin = propertyValues.begin(); it != begin;) { @@ -45,7 +45,7 @@ void AppliedClassProperties::cleanup(const TimePoint& now) { } } -bool AppliedClassProperties::empty() const { +bool AppliedClassPropertyValues::empty() const { return propertyValues.empty(); } diff --git a/src/mbgl/style/applied_class_properties.hpp b/src/mbgl/style/applied_class_properties.hpp index fbba966143..a33e2bd62b 100644 --- a/src/mbgl/style/applied_class_properties.hpp +++ b/src/mbgl/style/applied_class_properties.hpp @@ -9,9 +9,9 @@ namespace mbgl { -class AppliedClassProperty { +class AppliedClassPropertyValue { public: - AppliedClassProperty(ClassID class_id, const TimePoint& begin, const TimePoint& end, const PropertyValue &value); + AppliedClassPropertyValue(ClassID class_id, const TimePoint& begin, const TimePoint& end, const PropertyValue &value); public: const ClassID name; @@ -21,9 +21,9 @@ public: }; -class AppliedClassProperties { +class AppliedClassPropertyValues { public: - std::list<AppliedClassProperty> propertyValues; + std::list<AppliedClassPropertyValue> propertyValues; public: // Returns the ID of the most recent diff --git a/src/mbgl/style/style_layer.cpp b/src/mbgl/style/style_layer.cpp index ad96c38c3c..5fc6af793c 100644 --- a/src/mbgl/style/style_layer.cpp +++ b/src/mbgl/style/style_layer.cpp @@ -54,7 +54,7 @@ void StyleLayer::setClasses(const std::vector<std::string> &class_names, const T continue; } - AppliedClassProperties &appliedProperties = property_pair.second; + AppliedClassPropertyValues &appliedProperties = property_pair.second; // Make sure that we don't do double transitions to the fallback value. if (appliedProperties.mostRecent() != ClassID::Fallback) { // This property key hasn't been set by a previous class, so we need to add a transition @@ -93,7 +93,7 @@ void StyleLayer::applyClassProperties(const ClassID class_id, // If the most recent transition is not the one with the highest priority, create // a transition. - AppliedClassProperties &appliedProperties = appliedStyle[key]; + AppliedClassPropertyValues &appliedProperties = appliedStyle[key]; if (appliedProperties.mostRecent() != class_id) { const PropertyTransition &transition = class_properties.getTransition(key, defaultTransition); @@ -137,7 +137,7 @@ template <typename T> void StyleLayer::applyStyleProperty(PropertyKey key, T &target, const float z, const TimePoint& now, const ZoomHistory &zoomHistory) { auto it = appliedStyle.find(key); if (it != appliedStyle.end()) { - AppliedClassProperties &applied = it->second; + AppliedClassPropertyValues &applied = it->second; // Iterate through all properties that we need to apply in order. const PropertyEvaluator<T> evaluator(z, zoomHistory); for (auto& property : applied.propertyValues) { @@ -155,7 +155,7 @@ template <typename T> void StyleLayer::applyTransitionedStyleProperty(PropertyKey key, T &target, const float z, const TimePoint& now, const ZoomHistory &zoomHistory) { auto it = appliedStyle.find(key); if (it != appliedStyle.end()) { - AppliedClassProperties &applied = it->second; + AppliedClassPropertyValues &applied = it->second; // Iterate through all properties that we need to apply in order. const PropertyEvaluator<T> evaluator(z, zoomHistory); for (auto& property : applied.propertyValues) { @@ -273,10 +273,10 @@ bool StyleLayer::hasTransitions() const { void StyleLayer::cleanupAppliedStyleProperties(const TimePoint& now) { for (auto it = appliedStyle.begin(); it != appliedStyle.end();) { - AppliedClassProperties& appliedPropertyValues = it->second; - appliedPropertyValues.cleanup(now); + AppliedClassPropertyValues& values = it->second; + values.cleanup(now); // If the current properties object is empty, remove it from the map entirely. - appliedPropertyValues.empty() ? appliedStyle.erase(it++) : ++it; + values.empty() ? appliedStyle.erase(it++) : ++it; } } diff --git a/src/mbgl/style/style_layer.hpp b/src/mbgl/style/style_layer.hpp index 97bd479b25..56b8cb8f19 100644 --- a/src/mbgl/style/style_layer.hpp +++ b/src/mbgl/style/style_layer.hpp @@ -78,7 +78,7 @@ public: private: // For every property, stores a list of applied property values, with // optional transition times. - std::map<PropertyKey, AppliedClassProperties> appliedStyle; + std::map<PropertyKey, AppliedClassPropertyValues> appliedStyle; public: // Stores the evaluated, and cascaded styling information, specific to this diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp index 24993ed72d..ba92ce6b4e 100644 --- a/src/mbgl/text/glyph_pbf.cpp +++ b/src/mbgl/text/glyph_pbf.cpp @@ -1,77 +1,32 @@ #include <mbgl/text/glyph_pbf.hpp> -#include <mbgl/text/font_stack.hpp> #include <mbgl/storage/file_source.hpp> #include <mbgl/storage/resource.hpp> #include <mbgl/storage/response.hpp> - +#include <mbgl/text/font_stack.hpp> +#include <mbgl/text/glyph_store.hpp> +#include <mbgl/util/exception.hpp> #include <mbgl/util/pbf.hpp> #include <mbgl/util/string.hpp> -#include <mbgl/util/thread.hpp> +#include <mbgl/util/thread_context.hpp> #include <mbgl/util/token.hpp> #include <mbgl/util/url.hpp> #include <sstream> -namespace mbgl { - -GlyphPBF::GlyphPBF(const std::string& glyphURL, - const std::string& fontStack, - GlyphRange glyphRange, - const GlyphLoadedCallback& successCallback, - const GlyphLoadingFailedCallback& failureCallback) - : parsed(false) { - // Load the glyph set URL - url = util::replaceTokens(glyphURL, [&](const std::string &name) -> std::string { - if (name == "fontstack") return util::percentEncode(fontStack); - if (name == "range") return util::toString(glyphRange.first) + "-" + util::toString(glyphRange.second); - return ""; - }); +namespace { - // The prepare call jumps back to the main thread. - FileSource* fs = util::ThreadContext::getFileSource(); - req = fs->request({ Resource::Kind::Glyphs, url }, util::RunLoop::getLoop(), [&, successCallback, failureCallback](const Response &res) { - req = nullptr; - - if (res.status != Response::Successful) { - std::stringstream message; - message << "Failed to load [" << url << "]: " << res.message; - failureCallback(message.str()); - } else { - // Transfer the data to the GlyphSet and signal its availability. - // Once it is available, the caller will need to call parse() to actually - // parse the data we received. We are not doing this here since this callback is being - // called from another (unknown) thread. - data = res.data; - successCallback(this); - } - }); -} - -GlyphPBF::~GlyphPBF() { - if (req) { - util::ThreadContext::getFileSource()->cancel(req); - } -} - -void GlyphPBF::parse(FontStack &stack) { - if (!data.size()) { - // If there is no data, this means we either haven't received any data, or - // we have already parsed the data. - return; - } - - // Parse the glyph PBF - pbf glyphs_pbf(reinterpret_cast<const uint8_t *>(data.data()), data.size()); +void parseGlyphPBF(mbgl::FontStack& stack, const std::string& data) { + mbgl::pbf glyphs_pbf(reinterpret_cast<const uint8_t *>(data.data()), data.size()); while (glyphs_pbf.next()) { if (glyphs_pbf.tag == 1) { // stacks - pbf fontstack_pbf = glyphs_pbf.message(); + mbgl::pbf fontstack_pbf = glyphs_pbf.message(); while (fontstack_pbf.next()) { if (fontstack_pbf.tag == 3) { // glyphs - pbf glyph_pbf = fontstack_pbf.message(); + mbgl::pbf glyph_pbf = fontstack_pbf.message(); - SDFGlyph glyph; + mbgl::SDFGlyph glyph; while (glyph_pbf.next()) { if (glyph_pbf.tag == 1) { // id @@ -102,14 +57,84 @@ void GlyphPBF::parse(FontStack &stack) { glyphs_pbf.skip(); } } +} + +} + +namespace mbgl { + +GlyphPBF::GlyphPBF(GlyphStore* store, + const std::string& fontStack, + const GlyphRange& glyphRange) + : parsed(false) { + // Load the glyph set URL + std::string url = util::replaceTokens(store->getURL(), [&](const std::string &name) -> std::string { + if (name == "fontstack") return util::percentEncode(fontStack); + if (name == "range") return util::toString(glyphRange.first) + "-" + util::toString(glyphRange.second); + return ""; + }); + + auto requestCallback = [this, store, fontStack, url](const Response &res) { + req = nullptr; + + if (res.status != Response::Successful) { + std::stringstream message; + message << "Failed to load [" << url << "]: " << res.message; + emitGlyphPBFLoadingFailed(message.str()); + } else { + data = res.data; + parse(store, fontStack, url); + } + }; + + FileSource* fs = util::ThreadContext::getFileSource(); + req = fs->request({ Resource::Kind::Glyphs, url }, util::RunLoop::getLoop(), requestCallback); +} + +GlyphPBF::~GlyphPBF() { + if (req) { + util::ThreadContext::getFileSource()->cancel(req); + } +} + +void GlyphPBF::parse(GlyphStore* store, const std::string& fontStack, const std::string& url) { + if (data.empty()) { + // If there is no data, this means we either haven't + // received any data. + return; + } - data.clear(); + try { + parseGlyphPBF(**store->getFontStack(fontStack), std::move(data)); + } catch (const std::exception& ex) { + std::stringstream message; + message << "Failed to parse [" << url << "]: " << ex.what(); + emitGlyphPBFLoadingFailed(message.str()); + return; + } parsed = true; + + emitGlyphPBFLoaded(); +} + +void GlyphPBF::setObserver(Observer* observer_) { + observer = observer_; } -bool GlyphPBF::isParsed() const { - return parsed; +void GlyphPBF::emitGlyphPBFLoaded() { + if (observer) { + observer->onGlyphPBFLoaded(); + } +} + +void GlyphPBF::emitGlyphPBFLoadingFailed(const std::string& message) { + if (!observer) { + return; + } + + auto error = std::make_exception_ptr(util::GlyphRangeLoadingException(message)); + observer->onGlyphPBFLoadingFailed(error); } } diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp index cc083e7f10..2aa2134d16 100644 --- a/src/mbgl/text/glyph_pbf.hpp +++ b/src/mbgl/text/glyph_pbf.hpp @@ -2,48 +2,53 @@ #define MBGL_TEXT_GLYPH_PBF #include <mbgl/text/glyph.hpp> +#include <mbgl/util/noncopyable.hpp> -#include <functional> #include <atomic> +#include <functional> #include <string> namespace mbgl { +class GlyphStore; class FontStack; class Request; -class GlyphPBF { +class GlyphPBF : private util::noncopyable { public: - using GlyphLoadedCallback = std::function<void(GlyphPBF*)>; - using GlyphLoadingFailedCallback = std::function<void(const std::string&)>; + class Observer { + public: + virtual ~Observer() = default; + + virtual void onGlyphPBFLoaded() = 0; + virtual void onGlyphPBFLoadingFailed(std::exception_ptr error) = 0; + }; - GlyphPBF(const std::string &glyphURL, - const std::string &fontStack, - GlyphRange glyphRange, - const GlyphLoadedCallback& successCallback, - const GlyphLoadingFailedCallback& failureCallback); - ~GlyphPBF(); + GlyphPBF(GlyphStore* store, + const std::string& fontStack, + const GlyphRange& glyphRange); + virtual ~GlyphPBF(); - void parse(FontStack &stack); - bool isParsed() const; + bool isParsed() const { + return parsed; + }; - std::string getURL() const { - return url; - } + void setObserver(Observer* observer); private: - GlyphPBF(const GlyphPBF &) = delete; - GlyphPBF(GlyphPBF &&) = delete; - GlyphPBF &operator=(const GlyphPBF &) = delete; - GlyphPBF &operator=(GlyphPBF &&) = delete; + void emitGlyphPBFLoaded(); + void emitGlyphPBFLoadingFailed(const std::string& message); + + void parse(GlyphStore* store, const std::string& fontStack, const std::string& url); std::string data; - std::string url; std::atomic<bool> parsed; Request* req = nullptr; + + Observer* observer = nullptr; }; -} // end namespace mbgl +} #endif diff --git a/src/mbgl/text/glyph_store.cpp b/src/mbgl/text/glyph_store.cpp index 17c1524a88..eb770fbf6e 100644 --- a/src/mbgl/text/glyph_store.cpp +++ b/src/mbgl/text/glyph_store.cpp @@ -1,38 +1,13 @@ #include <mbgl/text/glyph_store.hpp> -#include <mbgl/text/font_stack.hpp> #include <mbgl/text/glyph_pbf.hpp> -#include <mbgl/util/exception.hpp> #include <mbgl/util/thread_context.hpp> namespace mbgl { -GlyphStore::GlyphStore() { -} - -GlyphStore::~GlyphStore() { -} - void GlyphStore::requestGlyphRange(const std::string& fontStackName, const GlyphRange& range) { assert(util::ThreadContext::currentlyOn(util::ThreadType::Map)); - auto successCallback = [this, fontStackName](GlyphPBF* glyph) { - try { - { - auto fontStack = createFontStack(fontStackName); - glyph->parse(**fontStack); - } - emitGlyphRangeLoaded(); - } catch (const std::exception&) { - std::string message = "Failed to parse [" + glyph->getURL() + "]"; - emitGlyphRangeLoadingFailed(message); - } - }; - - auto failureCallback = [this](const std::string& message) { - emitGlyphRangeLoadingFailed(message); - }; - std::lock_guard<std::mutex> lock(rangesMutex); auto& rangeSets = ranges[fontStackName]; @@ -41,8 +16,10 @@ void GlyphStore::requestGlyphRange(const std::string& fontStackName, const Glyph return; } - rangeSets.emplace(range, std::make_unique<GlyphPBF>(glyphURL, fontStackName, range, - successCallback, failureCallback)); + auto glyphPBF = std::make_unique<GlyphPBF>(this, fontStackName, range); + glyphPBF->setObserver(this); + + rangeSets.emplace(range, std::move(glyphPBF)); } @@ -55,12 +32,13 @@ bool GlyphStore::hasGlyphRanges(const std::string& fontStackName, const std::set const auto& rangeSets = ranges[fontStackName]; bool hasRanges = true; - for (const auto& range : glyphRanges) { const auto& rangeSetsIt = rangeSets.find(range); if (rangeSetsIt == rangeSets.end()) { - // Post the request to the Map thread. + // Push the request to the MapThread, so we can easly cancel + // if it is still pending when we destroy this object. workQueue.push(std::bind(&GlyphStore::requestGlyphRange, this, fontStackName, range)); + hasRanges = false; continue; } @@ -73,7 +51,7 @@ bool GlyphStore::hasGlyphRanges(const std::string& fontStackName, const std::set return hasRanges; } -util::exclusive<FontStack> GlyphStore::createFontStack(const std::string& fontStack) { +util::exclusive<FontStack> GlyphStore::getFontStack(const std::string& fontStack) { auto lock = std::make_unique<std::lock_guard<std::mutex>>(stacksMutex); auto it = stacks.find(fontStack); @@ -81,37 +59,25 @@ util::exclusive<FontStack> GlyphStore::createFontStack(const std::string& fontSt it = stacks.emplace(fontStack, std::make_unique<FontStack>()).first; } + // FIXME: We lock all FontStacks, but what we should + // really do is lock only the one we are returning. return { it->second.get(), std::move(lock) }; } -util::exclusive<FontStack> GlyphStore::getFontStack(const std::string &fontStack) { - auto lock = std::make_unique<std::lock_guard<std::mutex>>(stacksMutex); - - const auto& it = stacks.find(fontStack); - if (it == stacks.end()) { - return { nullptr, nullptr }; - } - - return { it->second.get(), std::move(lock) }; -} - -void GlyphStore::setObserver(Observer* observer_) { - observer = observer_; -} - -void GlyphStore::emitGlyphRangeLoaded() { +void GlyphStore::onGlyphPBFLoaded() { if (observer) { observer->onGlyphRangeLoaded(); } } -void GlyphStore::emitGlyphRangeLoadingFailed(const std::string& message) { - if (!observer) { - return; +void GlyphStore::onGlyphPBFLoadingFailed(std::exception_ptr error) { + if (observer) { + observer->onGlyphRangeLoadingFailed(error); } +} - auto error = std::make_exception_ptr(util::GlyphRangeLoadingException(message)); - observer->onGlyphRangeLoadingFailed(error); +void GlyphStore::setObserver(Observer* observer_) { + observer = observer_; } } diff --git a/src/mbgl/text/glyph_store.hpp b/src/mbgl/text/glyph_store.hpp index 5d0f498c71..f8de6cbaf5 100644 --- a/src/mbgl/text/glyph_store.hpp +++ b/src/mbgl/text/glyph_store.hpp @@ -1,8 +1,11 @@ #ifndef MBGL_TEXT_GLYPH_STORE #define MBGL_TEXT_GLYPH_STORE +#include <mbgl/text/font_stack.hpp> #include <mbgl/text/glyph.hpp> +#include <mbgl/text/glyph_pbf.hpp> #include <mbgl/util/exclusive.hpp> +#include <mbgl/util/noncopyable.hpp> #include <mbgl/util/run_loop.hpp> #include <mbgl/util/work_queue.hpp> @@ -13,11 +16,10 @@ namespace mbgl { -class FontStack; -class GlyphPBF; - -// Manages GlyphRange PBF loading. -class GlyphStore { +// The GlyphStore manages the loading and storage of Glyphs +// and creation of FontStack objects. The GlyphStore lives +// on the MapThread but can be queried from any thread. +class GlyphStore : public GlyphPBF::Observer, private util::noncopyable { public: class Observer { public: @@ -27,24 +29,33 @@ public: virtual void onGlyphRangeLoadingFailed(std::exception_ptr error) = 0; }; - GlyphStore(); - ~GlyphStore(); + GlyphStore() = default; + virtual ~GlyphStore() = default; util::exclusive<FontStack> getFontStack(const std::string& fontStack); + // Returns true if the set of GlyphRanges are available and parsed or false + // if they are not. For the missing ranges, a request on the FileSource is + // made and when the glyph if finally parsed, it gets added to the respective + // FontStack and a signal is emitted to notify the observers. This method + // can be called from any thread. bool hasGlyphRanges(const std::string& fontStackName, const std::set<GlyphRange>& glyphRanges); void setURL(const std::string &url) { glyphURL = url; } + std::string getURL() const { + return glyphURL; + } + + // GlyphPBF::Observer implementation. + void onGlyphPBFLoaded() override; + void onGlyphPBFLoadingFailed(std::exception_ptr error) override; + void setObserver(Observer* observer); private: - void emitGlyphRangeLoaded(); - void emitGlyphRangeLoadingFailed(const std::string& message); - - util::exclusive<FontStack> createFontStack(const std::string &fontStack); void requestGlyphRange(const std::string& fontStackName, const GlyphRange& range); std::string glyphURL; diff --git a/src/mbgl/util/work_queue.hpp b/src/mbgl/util/work_queue.hpp index dcec5668b9..55b687a468 100644 --- a/src/mbgl/util/work_queue.hpp +++ b/src/mbgl/util/work_queue.hpp @@ -14,11 +14,18 @@ namespace util { class RunLoop; +// The WorkQueue will manage a queue of closures +// and it will make sure they get executed on the +// thread that created the WorkQueue. All pending +// works are canceled when the queue gets destructed. class WorkQueue : private util::noncopyable { public: WorkQueue(); - virtual ~WorkQueue(); + ~WorkQueue(); + // Push a closure to the queue. It is advised to + // only push tasks calling functions on the object + // that owns the queue to avoid use after free errors. void push(std::function<void()>&&); private: diff --git a/test/style/glyph_store.cpp b/test/style/glyph_store.cpp index fb4355b98b..fe614e8c60 100644 --- a/test/style/glyph_store.cpp +++ b/test/style/glyph_store.cpp @@ -138,7 +138,8 @@ TEST_F(GlyphStoreTest, LoadingFail) { ASSERT_TRUE(error != nullptr); auto fontStack = store->getFontStack(params.stack); - ASSERT_EQ(*fontStack, nullptr); + ASSERT_TRUE(fontStack->getMetrics().empty()); + ASSERT_TRUE(fontStack->getSDFs().empty()); for (const auto& range : params.ranges) { ASSERT_FALSE(store->hasGlyphRanges(params.stack, {range})); @@ -171,6 +172,10 @@ TEST_F(GlyphStoreTest, LoadingCorrupted) { ASSERT_TRUE(error != nullptr); + auto fontStack = store->getFontStack(params.stack); + ASSERT_TRUE(fontStack->getMetrics().empty()); + ASSERT_TRUE(fontStack->getSDFs().empty()); + for (const auto& range : params.ranges) { ASSERT_FALSE(store->hasGlyphRanges(params.stack, {range})); } @@ -215,7 +220,8 @@ TEST_F(GlyphStoreTest, InvalidURL) { ASSERT_TRUE(error != nullptr); auto fontStack = store->getFontStack(params.stack); - ASSERT_EQ(*fontStack, nullptr); + ASSERT_TRUE(fontStack->getMetrics().empty()); + ASSERT_TRUE(fontStack->getSDFs().empty()); stopTest(); }; diff --git a/test/style/resource_loading.cpp b/test/style/resource_loading.cpp index 025b9b0271..b4fdbc7c5a 100644 --- a/test/style/resource_loading.cpp +++ b/test/style/resource_loading.cpp @@ -164,4 +164,4 @@ INSTANTIATE_TEST_CASE_P(Style, ResourceLoading, std::make_pair("sprite.png", "Could not parse sprite image"), std::make_pair("raster.png", "Failed to parse \\[17\\/6553(4|5|6|7)\\/6553(4|5|6|7)\\]\\: error parsing raster image"), std::make_pair("vector.pbf", "Failed to parse \\[1(5|6)\\/1638(3|4)\\/1638(3|4)\\]\\: pbf unknown field type exception"), - std::make_pair("glyphs.pbf", "Failed to parse \\[test\\/fixtures\\/resources\\/glyphs.pbf\\]"))); + std::make_pair("glyphs.pbf", "Failed to parse \\[test\\/fixtures\\/resources\\/glyphs.pbf\\]: pbf unknown field type exception"))); |