mapbox-vector-tile icon indicating copy to clipboard operation
mapbox-vector-tile copied to clipboard

Following V2 Specification

Open flippmoke opened this issue 8 years ago • 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!

flippmoke avatar Mar 22 '16 13:03 flippmoke

@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 avatar Apr 14 '16 20:04 yohanboniface

@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.

rmarianski avatar Apr 15 '16 15:04 rmarianski

Thanks @rmarianski :)

Can you tag a new release also? :)

yohanboniface avatar Apr 15 '16 15:04 yohanboniface

@yohanboniface 0.3.0 tagged and uploaded to pypi.

rmarianski avatar Apr 15 '16 18:04 rmarianski

Thanks! :)

yohanboniface avatar Apr 15 '16 18:04 yohanboniface

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.

fosskers avatar Jun 14 '16 17:06 fosskers

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).

nvkelso avatar Oct 27 '16 21:10 nvkelso

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
  • [x] A feature MUST have a type
  • [ ] A lineTo command can't have no movement
  • [x] LineString geometry can't be only a moveto command
  • [x] A polygon should not have less than 3 points
  • [x] Ensure winding order AFTER rounding coordinates

I've checked some issues as resolves according to the current code...

lexman avatar Nov 07 '16 08:11 lexman

Hey, any update on this issue?

ptpt avatar Nov 02 '17 14:11 ptpt

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 avatar Nov 02 '17 18:11 nvkelso

@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.

flippmoke avatar Nov 02 '17 19:11 flippmoke

@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 avatar Nov 02 '17 19:11 pnorman

@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.

flippmoke avatar Nov 02 '17 19:11 flippmoke

@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.

pnorman avatar Nov 02 '17 19:11 pnorman

204 No Content is probably the correct response

flippmoke avatar Nov 02 '17 20:11 flippmoke

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.

zerebubuth avatar Nov 03 '17 10:11 zerebubuth

@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.

rmarianski avatar Nov 03 '17 16:11 rmarianski

Because it's a SHOULD, not a MUST, returning empty tiles is valid behavior, so any client that breaks is in error.

pnorman avatar Nov 03 '17 16:11 pnorman

@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.

nvkelso avatar Nov 03 '17 23:11 nvkelso