Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

refactor VectorTile to vend geometry #928

Merged
merged 27 commits into from
Mar 10, 2015
Merged

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Mar 10, 2015

See background in #893 (comment). Basically the following classes are tightly coupled to PBF-specific vector tile geometry routines:

  • TileParser
  • LineBucket
  • FillBucket
  • SymbolBucket

All of these step through individual geometries in vector tiles (example) which ties the use of their structures to PBF binary-format tiles. We want to be able to also have "live tiles" that back annotations, which are structured similarly to vector tiles (multi-layer, pre-simplified, able to be filtered for styling purposes) yet come from sources such as live-adding or GeoJSON parsing.

The parser and buckets should ask for geometries in an intermediate format (probably Tile, which also has 4096 extents) and then do things with them.

@incanus incanus added this to the iOS Beta milestone Feb 27, 2015
@incanus incanus mentioned this pull request Feb 27, 2015
7 tasks
@incanus
Copy link
Contributor Author

incanus commented Feb 27, 2015

Related: #250

@incanus incanus changed the title refector VectorTile to provide geometry refector VectorTile to vend geometry Mar 2, 2015
incanus added a commit that referenced this pull request Mar 3, 2015
@incanus
Copy link
Contributor Author

incanus commented Mar 3, 2015

Making some headway here — Got a GeometryTile and friends (Feature, TagExtractor, etc.) base and VectorTile is now a derived class of it. A little bit of new template, iterator, and inheritance territory for me, so a little slow-going, but just about set.

Now adapting the various buckets to request geometries out of a more abstract interface instead of mucking with pbf directly, then back to the work in #893 which creates a LiveTile (and friends) bridge derived class of GeometryTile providing similar geometry, though not from pbf, out of the GeoJSONVT polygon simplification stuff. Work on that is mostly done and just needs copy-pasted over (some lessons learned while designing it originally).

incanus added a commit that referenced this pull request Mar 4, 2015
@incanus
Copy link
Contributor Author

incanus commented Mar 4, 2015

It builds! </lowbar>

@gregzo
Copy link

gregzo commented Mar 5, 2015

Eagerly following...

@jfirebaugh jfirebaugh mentioned this pull request Mar 6, 2015
5 tasks
@incanus
Copy link
Contributor Author

incanus commented Mar 8, 2015

Ok, setting this down for a moment and getting some other eyes on it. I have been working alone for a while, probing the limits of my C++ knowledge and leveling-up along the way. @kkaefer @jfirebaugh @ljbade could you take a peek? Also cc'ing @artemp @springmeyer @tmpsantos in case you've got any bandwidth to check this out.

Current status is at master...07fd062

My current problem is failure to link because the ++, *, and !+ operator implementations are missing from GeometryFilteredTileLayer. I'll explain more below.

The overall plan for this refactor is to move these currently-base classes down into derived classes:

  • VectorTile
  • VectorTileLayer
  • FilteredVectorTileLayer
  • VectorTileFeature

It places them under abstract interface (pure virtual) base classes:

  • GeometryTile
  • GeometryTileLayer
  • GeometryFilteredTileLayer
  • GeometryTileFeature

One other note: the old Geometry, which was pbf-specific, is now PBFGeometry.

It then migrates TileParser and the buckets (LineBucket, FillBucket, and SymbolBucket) away from dealing with pbf vector tile data directly, and instead understanding Geometry objects, which is a variant-based class for holding GeometryPoint, GeometryLine, and GeometryPolygon objects. Geometry objects can then be put into a GeometryCollection vector, which is analogous to how the current setup iterates through pbf geometry for Geometry::move_to commands in order to string geometries together. Instead of the buckets themselves observing Geometry::move_to commands and dividing up geometries, they receive a GeometryCollection, which is created out of a GeometryTileFeature::nextGeometry(). This pushes all of the manual pbf manipulation inside of vector_tile.cpp.

I ran into a bit of hairyness with the filtered layers and their iterator subclass, mostly because I'm a bit confused about how I might use the derived classes generically while still treating them as an iterator. I got rid of range-based for loops and instead moved to a manual for loop based explicitly on iterator::begin(), iterator::end(), and iterator::operator++(). You can see a good example of this in symbol_bucket.cpp. However, since operators cannot be virtual, I have to declare them in the base interface, but get linking problems because they don't have actual implementations. I'm not sure how to handle such an interface situation with iterators and operators, where typically I have been using std::shared_ptr objects to work with the interfaces.

I had at one point moved away from the iterator model and to the FilteredVectorTileLayer itself vending a nextFeature(), but this got nasty because determining if there was a next feature was tied to iterating next(2) in the layer's pbf and this was upsetting other components which didn't want the pbf to mutate. I thought it better to stick closer to the original implementation with iterator and for loops.


After this is done, I should be able to jump back to #893 to create LiveTile etc. classes that also derive from GeometryTile etc. After that, I've already got the GeoJSONVT work in a format that can create these sorts of tiles, which should get them into our rendering pipeline.

@incanus
Copy link
Contributor Author

incanus commented Mar 10, 2015

@jfirebaugh has 👀 on this and we're working through it.


public:
inline explicit Geometry(pbf& data);
inline explicit PBFGeometry(pbf data);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this was an intentional to avoid mutating the passed in parameter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accident, I think — was going to take a review back through and improve passing by reference where applicable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intentionally changed from a reference to a value, as pbf is intended to be passed by value and at the call site was being manually copied to a temporary. In any case, I'm about to push a commit that inlines PBFGeometry entirely.

Copy link
Contributor

@incanus incanus Mar 10, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kkaefer kkaefer changed the title refector VectorTile to vend geometry refactor VectorTile to vend geometry Mar 10, 2015
@incanus
Copy link
Contributor Author

incanus commented Mar 10, 2015

Much 🙇 to @jfirebaugh; merging now.

incanus added a commit that referenced this pull request Mar 10, 2015
@incanus incanus merged commit cc7371e into master Mar 10, 2015
@incanus incanus deleted the 928-refactor-vend-geometry branch March 10, 2015 19:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants