search

mapbox / vector-tile-spec

Mapbox Vector Tile specification
https://www.mapbox.com/vector-tiles/specification/
Other
892 stars 210 forks source link

Version field in spec v2 #43

Closed joto closed 8 years ago

joto commented 8 years ago

The spec contains this sentence: "The version field MUST be the first field within the layer." I am not sure what this means in the context of a Protobuf encoded layer. Generally the Protobuf-libraries do not allow you to define the order of fields in a message. Some low-level libraries (such as Protozero) allow this, but generally this is not the case. See the last section "Field Order" in https://developers.google.com/protocol-buffers/docs/encoding for details. This looks to me like the version field will be written last in the Layer message because it has the highest field number.

flippmoke commented 8 years ago

@joto great catch. I know that we are using protozero internally. I wanted to make version the field order 1, but I am concerned as this would make v2 tiles not compatible with v1 decoders. Perhaps we should remove this requirement!

/cc @jfirebaugh

jfirebaugh commented 8 years ago

The "Field Order" section is self-contradictory in at least two different ways, so I assume that there is no normative requirement that fields be in numbered order. But if widely used protobuf writing libraries do not allow control over the order, than that's a valid concern.

If we can't mandate that the version field must be first, how should we recommend that consumers deal with version compatibility issues?

joto commented 8 years ago

I am not sure we can retrofit the version into the old format in a sensible way. As it stands now the version 2 format seems to not really be a new format version, but more of a clarification of the version 1 format. So maybe it doesn't need a new version number and we can live without it for this release? (And then start with proper versioning in the next version together with other changes which will make it not be backwards-compatible anway.)

The other question is whether we really want the version number on the Layer. Shouldn't it be for the whole vector tile? If we do that we can put it there and give it field number 1. For some reason Layer has field number 3, so we should be okay.

flippmoke commented 8 years ago

@joto the changes around polygons (winding order etc) will break decoders that require v2 tiles when they use a v1 tile, however, a v1 decoder should still be able to handle v2 tiles.

The other question is whether we really want the version number on the Layer. Shouldn't it be for the whole vector tile? If we do that we can put it there and give it field number 1. For some reason Layer has field number 3, so we should be okay.

I think in the end that might be a better approach, but trying to work within what we are given currently.

joto commented 8 years ago

Thinking some more about this I realized that we might be going the wrong way with the version thing. Protobuf already has a versioning built in: We can leave the Layer as it is and add a LayerV2 message with a new field number. Or maybe it should be a VectorLayerV2 which would mean that there is a natural place for raster layers in a RasterLayerV1 message or so. If and when we change the format of the Layer, we can keep inventing new Layer versions.

This approach would break backwards compatibility which would be okay if we are actually changing the format, because then we have to break it anway. But in this case (if I understand it correctly) we do not really have a new version, we only have a tighter definition of the old version. So the wire format didn't change, everything that's in the data is still interpreted the same way. We just want to make sure the data that is put into the wire format adhears to some stricter rules. This isn't a new version, this is "just" a promise by the writer that some additional restrictions are kept. Now the question is: How does this affect the decoder and do we have to tell it? What is the decoder going to do different for v1 data vs. v2 data? Is it doing more work for v1 data, for instance fixing polygons, or something like that? Or going to ignore the whole layer? Because if it is not doing anything special, we don't need to mark the new version in any way.

And another thought: Maybe a version field isn't the right solution but some kind of "capability list". So the vector tile or the layer could have optional (string) fields that add information like "PolygonsHaveCorrectWindingOrder".

flippmoke commented 8 years ago

@joto I actually think that isn't a bad idea at all. I will have to think about it further. The big concern I have about this is that it makes writing a decoder much more difficult possibly.

flippmoke commented 8 years ago

I updated the MUST to a SHOULD, I think this closes this ticket out if it does not please let me know.