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

vector_layers #42

Merged
merged 5 commits into from
Jul 30, 2018
Merged

vector_layers #42

merged 5 commits into from
Jul 30, 2018

Conversation

mapsam
Copy link
Contributor

@mapsam mapsam commented May 8, 2018

This documents vector_layers as a required key in TileJSON (for sets of vector tiles). Per discussion in #14 this aims to "describe" the layers and their possible attributes while stopping short of describing geometry.

The language is adapted from the vector_layers section in the 1.3 version of the mbtiles-spec, which is the first place to describe this data structure. The key differences between this and the MBTiles specification description is the tilejson specification does not limit the values in key-value pairs of the fields attributes to be Number, String, or Boolean and simply suggests this as a space to describe the key with a string. Per discussion with @GretaCB and @ericfischer, this may warrant an update to the MBTiles specification to be less strict.

Note: we'll be opening a ticket to document possibly renaming id to name in version 4.0, as suggested by @pnorman in #14 (comment)

refs: #35

3.0/README.md Outdated

These keys are used to describe the situation where different sets of vector layers appear in different zoom levels of the same tileset, for example in a case where a "minor roads" layer is only present at high zoom levels.

Implementations MUST treat unknown key-value pairs as if they weren't present. However, implementations MUST expose unknown key/values in their API so that API users can optionally handle these keys.
Copy link
Contributor Author

@mapsam mapsam May 8, 2018

Choose a reason for hiding this comment

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

@GretaCB I've noticed some implementations (mapbox streets tilejson for example) include other keys here. I'm thinking we can document that this is acceptable with the same language from the Structure section. Duplicating the text feels a little redundant, so I'm wondering if we could update the Structure introduction to say something like:

Implementations MUST treat unknown keys as if they weren't present. This is true at any depth of the JSON. However, implementations MUST expose unknown key/values in their API so that API users can optionally handle these keys.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this section is needed at all, given the text elsewhere in the spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mapsam what if we link to the Structure section to avoid needing to update text in both places, but also keep a reference in both places to avoid it being overlooked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me @GretaCB - will work an update!

3.0/README.md Outdated
@@ -154,6 +154,55 @@ An array of tile endpoints. {z}, {x} and {y}, if present, are replaced with the
```

## 3.15 `vector_layers`

REQUIRED if describing vector tiles.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're saying this is required, but how do we effectively cover non vector-tile cases that cannot be described in this format?

@mapsam
Copy link
Contributor Author

mapsam commented May 8, 2018

I opened a ticket in the mbtiles specification repo to suggest relaxing the requirements for String, Number, or Boolean, which we can think about once the tilejson spec 3.0 has been released: mapbox/mbtiles-spec#53

@mapsam mapsam mentioned this pull request May 8, 2018
41 tasks
Copy link
Contributor

@pnorman pnorman left a comment

Choose a reason for hiding this comment

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

The language identifying the required items should mirror that of the rest of the spec. I think this will be easier to do with subsections since the structure will then be the same too.

3.0/README.md Outdated
@@ -154,6 +154,55 @@ An array of tile endpoints. {z}, {x} and {y}, if present, are replaced with the
```

## 3.15 `vector_layers`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add subsections, defining each of the sub-items.

3.0/README.md Outdated
A JSON object whose value is an array of JSON objects. Each of those JSON objects describes one layer of vector tile data, and MUST contain the following key-value pairs:

* `id` (string): The layer ID, which is referred to as the `name` of the layer in the [Mapbox Vector Tile spec](https://github.com/mapbox/vector-tile-spec/tree/master/2.1#41-layers).
* `fields` (object): A JSON object whose keys and values are the names and descriptions of attributes available in this layer. Each value (description) MUST be a string. The values MAY describe the underlying data type, such as `String`, `Number`, or `Boolean`. Attributes whose type varies between features MAY be listed as `"String"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really clear from this that the values are free-form text, and no special meaning should be given to an attempt to describe the underlying data type, since "Number" could be a description or a type, and different tile formats might have different allowable types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I think we should remove the concept of Number String Boolean entirely and just say this is string value that should describe the property. The explicit options are leftover from the mbtiles-spec, which is likely going to update once we ship v3.

3.0/README.md Outdated

A JSON object whose value is an array of JSON objects. Each of those JSON objects describes one layer of vector tile data, and MUST contain the following key-value pairs:

* `id` (string): The layer ID, which is referred to as the `name` of the layer in the [Mapbox Vector Tile spec](https://github.com/mapbox/vector-tile-spec/tree/master/2.1#41-layers).
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me what the reference to the MVT spec is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per discussion in #14 it is to help connect what this value refers to according to the vector tile layer concept. The MVT spec is used as an example. If we remove this reference entirely, it could be confusing since it’s nice to point to how vector tile specifications refer to layer names. I think we can remove the reference in v4 once we rename the property to name.

Basically, if we're considering matching the id field to name to match the MVT specification, then the reference link to the MVT specification feels relevant.

3.0/README.md Outdated

These keys are used to describe the situation where different sets of vector layers appear in different zoom levels of the same tileset, for example in a case where a "minor roads" layer is only present at high zoom levels.

Implementations MUST treat unknown key-value pairs as if they weren't present. However, implementations MUST expose unknown key/values in their API so that API users can optionally handle these keys.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this section is needed at all, given the text elsewhere in the spec.

3.0/README.md Outdated

A JSON object whose value is an array of JSON objects. Each of those JSON objects describes one layer of vector tile data, and MUST contain the following key-value pairs:

* `id` (string): The layer ID, which is referred to as the `name` of the layer in the [Mapbox Vector Tile spec](https://github.com/mapbox/vector-tile-spec/tree/master/2.1#41-layers).
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be explicit that there is no requirement the id is unique, but some formats may prohibit overlapping id and zoom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, the enforcement of this is vague.

cc @flippmoke @GretaCB

3.0/README.md Outdated
@@ -154,6 +154,55 @@ An array of tile endpoints. {z}, {x} and {y}, if present, are replaced with the
```

## 3.15 `vector_layers`

REQUIRED. (if describing vector tiles)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mentioned in #42 (comment) - if the tilejson spec isn't describing vector tiles, how is this field handled?

Perhaps it shouldn't be REQUIRED?

3.0/README.md Outdated
}
```


Copy link
Contributor

Choose a reason for hiding this comment

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

Stray newline

3.0/README.md Outdated
@@ -116,7 +116,7 @@ A name describing the tileset. The name can contain any legal character. Impleme

## 3.11 `scheme`

OPTIONAL. Default: "xyz".
OPTIONAL. Default: "xyz".
Copy link
Contributor

Choose a reason for hiding this comment

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

This whitespace change should go in a different commit because it's unrelated.

3.0/README.md Outdated
@@ -141,7 +141,7 @@ A semver.org style version number. Describes the version of the TileJSON spec th

## 3.14 `tiles`

REQUIRED.
REQUIRED.
Copy link
Contributor

Choose a reason for hiding this comment

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

This whitespace change should go in a different commit because it's unrelated.

@pnorman
Copy link
Contributor

pnorman commented May 17, 2018

Note: we'll be opening a ticket to document possibly renaming id to name in version 4.0, as suggested by @pnorman in #14 (comment)

If we want to call it name, let's do it now, rather than require a semver major change to do it.

@mapsam
Copy link
Contributor Author

mapsam commented May 18, 2018

Thanks for the comments @pnorman! A couple more responses inline here:

If we want to call it name, let's do it now, rather than require a semver major change to do it.

The goal of 3.x is to solidify the current common practice and reduce the need for architectural changes for implementations and developers. It’s not perfect, but it is what it is. 4.x can introduce downstream architectural changes and will be a more appropriate space for this.

The language identifying the required items should mirror that of the rest of the spec. I think this will be easier to do with subsections since the structure will then be the same too.

Great idea - definitely agree. I'll rework this section to have appropriate subsections.

@mapsam
Copy link
Contributor Author

mapsam commented May 30, 2018

Subsections have been added and a note about how v4 can rename id to name.

@GretaCB ready for another review here!

@GretaCB
Copy link
Contributor

GretaCB commented Jul 30, 2018

Merging 👍

@GretaCB GretaCB merged commit 3be314d into 3.0 Jul 30, 2018
@GretaCB GretaCB deleted the 3.0-vector_layers branch July 30, 2018 19:56
mapsam added a commit that referenced this pull request Aug 18, 2021
* 3.0 skeleton

* name, tilejson, version, description

* bounds first draft

* MAY assume

* update version major example

* maxzoom, minzoom, scheme (#38)

* zooms, scheme, adds default value info, and adds minzoom MUST value

* tiles (#40)

* Add tiles field, and add clarification around extensions used

* Add center field

* data section

* copy over grids

* legend

* template

* fix line break

* Per pnorman's suggestions

* full semver versioning

* Update directory name based on valid semver versioning

* Forgot to delete the old dir

* RFC 8259 is the current JSON spec (#47)

RFC 8259 has obsoleted RFC 7159, which obsoleted RFC 4267.

Only significant difference from RFC 4267 is that it disallows UTF-16 and UTF-32 encodings for JSON and mandates UTF-8 only.

I've also switched the IETF links to the HTML versions and changed "WGS:84" to "WGS 84".

* add fillzoom

* quick updates from @springmeyer

* clarify data field usage

* vector_layers (#42)

* Expand on empty fields object, provide example, and add subsections

* 3.0 notes and updates (#52)

* Add note on grid interactivity

* Ensure integers are integers in the examples

* Consistify use of TileJSON and consistify fillzoom json example with the spec description

* Ensure tile endpoints are absolute urls

* Use set of tiles instead of tileset

* keys and values

* update section order & put required first

* add extra note about bounds of one dimension

* define implementation

* add caching, schema.json

* add osm.json example

* add examples link

* fix link

* add data types, some review updates

* final edits

* update CHANGELOG'

* update README and changelog

Co-authored-by: Carol Hansen <carol@mapbox.com>
Co-authored-by: Sean Gillies <sean.gillies@gmail.com>
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