Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes non-default (4096) layer extent #24

Merged
merged 6 commits into from
Dec 1, 2015
Merged

Conversation

jwass
Copy link
Contributor

@jwass jwass commented Nov 25, 2015

This uses the extent field specified in each layer when decoding.

Prior to this, the extent was an input to the tile decoder. However a layer's extent isn't known until you decode the tile to find out what it is. Also it's possible that different layers may have different extents, which couldn't be handled.

This reads each layer's extent and uses that when decoding geometries. It removes the extents member of the decoder but keeps it in the constructor. I didn't want to remove it because it would be a breaking API change.

Although the extent is optional in the proto file, if it's not specified the protocol buffer library will emit it as the default value (4096). https://developers.google.com/protocol-buffers/docs/reference/python-generated.

The encoder should be modified similarly to allow layers to specify an optional extent but I wanted to get thoughts on this first.

Use the extent specified in the layer, rather than defining it at
the tile level.
Ignore long lines in test code
@jwass
Copy link
Contributor Author

jwass commented Nov 28, 2015

Looking this over again - a problem is that there's no straightforward way to include a layer's extent with the output of decoding because each layer is just an array of the features. Seems like a new interface is needed for this.

@rmarianski
Copy link
Member

Thanks for this. I'm 👍 on these changes, and for breaking changes on the decoding interface to simplify, like changing the constructor. For the results, instead of a list of features, we could return back an object, eg:

{'water': {'features': [], 'extent': 4096}}

If you would like to update the encoder to add support for custom extents, that would be great too!

Change the output so that a tile is formatted as a dict with keys
'extent', 'version', 'features'. 'features' is an array as before.
Update some braces and brackets. Be consistent with formatting of field descriptions.
@jwass
Copy link
Contributor Author

jwass commented Nov 30, 2015

@rmarianski Thanks. Completely agree on output format which also more faithfully corresponds to the description in the proto file.

Code is now updated so that the decoder no longer accepts the extent input. The decoder output is as described above also with the version field at the layer level. Also updated tests and docs.

Redefining the encoder is a bit more involved. Since the decoder has a getMessage() I'd expect the encoder to have a toMessage() (or similar) that would take a dictionary in the same format and output the binary vector tile data - so it can be round-tripped through both. I don't have the use or time for the encoder right now, would Mapzen be able to handle that?

@rmarianski
Copy link
Member

Thanks! The pr looks good to me 👍 If it's ready to go in I'll merge.

I created a ticket to track the work on adding per layer extents to the encoder so it doesn't get lost: #25

@jwass
Copy link
Contributor Author

jwass commented Dec 1, 2015

Good to go. I can squash if you prefer.

rmarianski added a commit that referenced this pull request Dec 1, 2015
Fixes non-default (4096) layer extent
@rmarianski rmarianski merged commit 3404a4d into tilezen:master Dec 1, 2015
@rmarianski
Copy link
Member

I think it's fine the way it is. Thanks again!

@jwass jwass deleted the extent_fix branch December 2, 2015 02:52
@jwass jwass mentioned this pull request Dec 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants