Following V2 Specification
flippmoke opened this issue · 22 comments
I took a quick glance over your encoder code and I am concerned about if the code actually makes v2 compliant vector tiles. My suggestion would be to simply tag as v1 tiles until further notice. Don't feel rushed IMO to get up to v2 until you feel you have a solid implementation.
We also are working to put together better documentation around the specification, so please see the living document at http://mapbox.github.io/vector-tile-spec/
Layer Message:
https://github.com/mapbox/vector-tile-spec/tree/master/2.1#41-layers
Name Field:
A layer MUST contain a name field. A Vector Tile MUST NOT contain two or more layers whose name values are byte-for-byte identical. Prior to appending a layer to an existing Vector Tile, an encoder MUST check the existing name fields in order to prevent duplication.
- Looking here if layer name is an empty string does it actually encode a layer name (as this is required)
- You do not seem to check for any duplicate layer names from what I can tell.
Extent Field:
A layer MUST contain an extent that describes the width and height of the tile in integer coordinates. The geometries within the Vector Tile MAY extend past the bounds of the tile's area as defined by the extent.
- While the specification doesn't specify it, I would suggest checking that the extent is greater then 0.
Not Required But Good to Do:
A Vector Tile SHOULD contain at least one layer. A layer SHOULD contain at least one feature.
Feature Message:
https://github.com/mapbox/vector-tile-spec/tree/master/2.1#42-features
Geometry Type Field:
A feature MUST contain a type field as described in the Geometry Types section.
Just glancing at your code I wasn't positive that this is enforced.
Geometry Encoding:
https://github.com/mapbox/vector-tile-spec/tree/master/2.1#43-geometry-encoding
LineTo Command:
https://github.com/mapbox/vector-tile-spec/tree/master/2.1#4332-lineto-command
https://github.com/mapbox/vector-tile-spec/tree/master/2.1#4332-lineto-command
I think your code currently makes it possible for there to be a lineto with no movement.
Line-string Geometry:
https://github.com/mapbox/vector-tile-spec/tree/master/2.1#4343-linestring-geometry-type
The geometry command sequence for a linestring geometry MUST consist of one or more repetitions of the following sequence:
A MoveTo command with a command count of 1
A LineTo command with a command count greater than 0
I believe it is currently possible to make linestring type geometries that consist of only a moveto.
Polygon Geometry:
https://github.com/mapbox/vector-tile-spec/tree/master/2.1#4344-polygon-geometry-type
Each ExteriorRing and InteriorRing MUST consist of the following sequence:
A MoveTo command with a command count of 1
A LineTo command with a command count greater than 1
A ClosePath command
I believe that this is not currently enforced in your code as you could have a polygon type with less then 3 points.
An exterior ring is DEFINED as a linear ring having a positive area as calculated by applying the surveyor's formula to the vertices of the polygon in tile coordinates. In the tile coordinate system (with the Y axis positive down and X axis positive to the right) this makes the exterior ring's winding order appear clockwise.
An interior ring is DEFINED as a linear ring having a negative area as calculated by applying the surveyor's formula to the vertices of the polygon in tile coordinates. In the tile coordinate system (with the Y axis positive down and X axis positive to the right) this makes the interior ring's winding order appear counterclockwise.
I think that this issue might be a good read. I am worried that your code fails specifically in the coordinate transform area. It is important to check winding order after coordinate transform into integers due to the possibility of inverting the winding order! I am pretty sure that your code does not do this currently, this can greatly break multipolygons/polygons when they are decoded.
Linear rings MUST be geometric objects that have no anomalous geometric points, such as self-intersection or self-tangency.
It is a short note currently, but this might be the most difficult line so far for us as we are writing our encoder. We have spent a great deal of time working on problems around ensuring the validity of polygons according to the OGC specification. While the specification does not strictly state the OGC specification, I believe we might shift to that eventually. http://postgis.net/docs/using_postgis_dbmanagement.html#OGC_Validity
In general this is one of the most important things to consider. There are many problems with maintaining validity of polygons AFTER there is any transformation done. This includes rounding and transforming into the vector tile coordinates!
@rmarianski seems like we should be saying 1 here for now, no: https://github.com/mapzen/mapbox-vector-tile/blob/773dbfdc38e75ab08f572f6a1c5fc321007dc252/mapbox_vector_tile/encoder.py#L72 ?
@yohanboniface we've updated to encoding version 1 for now. It may be a couple of weeks before this is reflected in the service though.
Thanks @rmarianski :)
Can you tag a new release also? :)
@yohanboniface 0.3.0 tagged and uploaded to pypi.
Thanks! :)
Updating to version 2 would reduce confusion (and compatibility issues). The examples in the README imply this produces V2 tiles, but as mentioned above the encoder labels layers as v1.
We think we're much more v2 MVT spec compliment in the recent v1.0.0 release of this tilezen/mapbox-vector-tiles library for geometry topology (doesn't address any other v2 topics in this issue).
Hello,
I'd like to sum up the remarks from @flippmoke to follow progress :
- No duplicate nor empty layer name
- Check extent is greater than 0
- A Vector Tile SHOULD contain at least one layer with at least one feature
- A feature MUST have a type
- A lineTo command can't have no movement
- LineString geometry can't be only a moveto command
- A polygon should not have less than 3 points
- Ensure winding order AFTER rounding coordinates
I've checked some issues as resolves according to the current code...
Hey, any update on this issue?
We haven't prioritized this on the Mapzen side, but if you're interested we'd accept a PR (or two)!
A Vector Tile SHOULD contain at least one layer with at least one feature
This one is a little odd and should just be a warning (spec says optional) – Tilezen vector tiles certainly include 0 feature layers sometimes but keep the layer to be consistent with surrounding tiles which will probably have content in that layer.
@nvkelso in general it is a good idea to not have empty layers if you can do it, just saves some bandwidth, storage and processing.
@nvkelso in general it is a good idea to not have empty layers if you can do it, just saves some bandwidth, storage and processing.
What's the recommendation for a tile with no features in it?
It feels like there might be a spec issue there
@pnorman a tile with no features in should have no layers and therefore is an empty buffer, therefore, it is best to not even have a tile in that location. The lack of existence of a tile specifies that there is nothing there.
@pnorman a tile with no features in should have no layers and therefore is an empty buffer, therefore, it is best to not even have a tile in that location. The lack of existence of a tile specifies that there is nothing there.
What would you return over HTTP if someone requests a tile with no features? I realize that's outside MVT spec scope which doesn't specify the higher-level stuff, but that stuff drives the MVT requirements.
204 No Content is probably the correct response
Are web browsers able to trap and deal with 204 separately from a 404 in javascript? That would allow the server to make the distinction between a tile that exists, but is empty, and a tile that does not exist (no such coordinate or wrong format, etc...)
I think it's largely a theoretical question, since all our base tiles have either water or land in them, but it's certainly possible if one requests a pois layer tile in the middle of the ocean. Currently, I think xonacatl will return a 200 OK non-empty response for that.
@nvkelso in general it is a good idea to not have empty layers if you can do it, just saves some bandwidth, storage and processing.
@nvkelso I agree here, but would an optimization like this need to be made only for 2.0 tiles? Not knowing the specifics on the client side, I can see it being possible for some client code to break.
Because it's a SHOULD, not a MUST, returning empty tiles is valid behavior, so any client that breaks is in error.
@nvkelso I agree here, but would an optimization like this need to be made only for 2.0 tiles? Not knowing the specifics on the client side, I can see it being possible for some client code to break.
Yes, I agree it would be a 2.0 breaking change for the Mapzen vector service – this library would need to support both behaviors as a config.
we expect that the remaining issues have been in addressed in #125
anything still outstanding?
yup, we were discussing that list in this review comment. It looks good from my end, just wanted to give others a chance to look it over before I declared victory in case I missed something :)