refs #893 #992: point annotations API #1018
Conversation
|
|
||
| namespace mbgl { | ||
|
|
||
| class LiveTileFeature : public GeometryTileFeature, private util::noncopyable { |
There was a problem hiding this comment.
These classes are derived from util::noncopyable already so you don't need to re-inherit it here.
| auto features_it = annotation->tileFeatures.find(tile_it.first); | ||
| if (features_it != annotation->tileFeatures.end()) { | ||
| auto layer = tile_it.second.second->getLayer(util::ANNOTATIONS_POINTS_LAYER_ID); | ||
| auto liveLayer = std::static_pointer_cast<LiveTileLayer>(layer); |
There was a problem hiding this comment.
Getting this one now.
| public: | ||
| TileParser(const std::string& rawData, | ||
| TileParser(const GeometryTile* geometryTile, |
| @@ -37,6 +37,9 @@ class GlyphAtlas; | |||
| class SpriteAtlas; | |||
| class LineAtlas; | |||
| class Environment; | |||
| class AnnotationManager; | |||
|
|
|||
| typedef std::vector<LatLng> AnnotationSegment; | |||
There was a problem hiding this comment.
This doesn't appear to be used by this header; should move to annotation.hpp.
There was a problem hiding this comment.
The reasoning here is because the shapes API will have it; you pass vectors of AnnotationSegment objects to represent lines in a shape.
| #ifndef MBGL_MAP_ANNOTATIONS | ||
| #define MBGL_MAP_ANNOTATIONS | ||
|
|
||
| #include <mbgl/map/map.hpp> |
There was a problem hiding this comment.
Can this be a forward declaration instead of an include?
There was a problem hiding this comment.
This leads to not seeing AnnotationSegment, and that can't be forward declared because it is used in getPoint(). That method can't be moved inside of annotation.cpp because then you get a redefinition error between the forward declaration of AnnotationSegment and the one brought into annotation.cpp with #include <mbgl/map/map.hpp>. This went really deep and doing it like this seemed the only way to keep things happy.
There was a problem hiding this comment.
The underlying problem is the cyclic dependency between AnnotationManager and Map. It looks like you can break the cycle by removing Map& map from AnnotationManager and passing the max zoom to AnnotationManager::getAnnotationsInBounds and AnnotationManager::addPointAnnotations.
There was a problem hiding this comment.
This is weird from an API point of view — clients like Cocoa would have to query, then pass the max zoom, when it doesn't appear to need to be an argument of querying bounds for annotations (they are independent of zoom and an implementation detail).
There was a problem hiding this comment.
I thought clients would use the API provided by Map?
|
→ #1021 |
This is Phase One of the point annotations API — adding, removing, querying, and setting sprite-based symbology for point annotations.
@jfirebaugh If you have some time would you mind sharing some 👀 — especially since this relates to the
GeometryTilerefactor.Two things I want to do to this next, but input is welcome:
LiveTileParserentirely and defaulting toTileParser. This was created in order to get acreateBucket()and address troubles with thegetLayer()API, but I think that's obsolete now.