-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Round 2 enhancements for Felt #47
Conversation
…set pmap:kind_detail on other kind
…operty coallese and extra tags (protomaps#39), show some POIs earlier depending on kind or area, special handling for kind = national_park
… merge buildings at z14; note future height quantization
…oundary and landuse; national_park kind; add kind_detail; special case no name POI zooms; special case other minor kind zooms
Will fix #48. |
|
||
## Versioning the Protomaps Basemap vector tile schema | ||
|
||
Upon our version `1.0.0` release Protomaps Basemap makes the following promises based on the Tilezen schema: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we prefer starting at 1.0.0
for semver? In relation to the hosted API urls:
api.protomaps.com/tiles/v2/
is major version 2 (the live one right now)
so api.protomaps.com/tiles/v2/{z}/{x}/{y}.mvt
is the latest tileset for v2. These are the main stable URLs in the system.
api.protomaps.com/tiles/v2/20230701/{z}/{x}/{y}.mvt
is a timestamped tileset compatible with the major version. it makes no guarantees about the minor version, but the full version string can be queried through metadata. Ideally we don't keep more than a couple months to a year of history, so these URLs should be treated as transient.
How should we address beta releases? api.protomaps.com/tiles/v2/20230701-beta/{z}/{x}/{y}.mvt
? These should reflect the -beta
postfix in the metadata version string. Or should we use a different endpoint entirely?
I'd like to avoid building endpoints like v2/2.3.1
as I think every minor version should supersede all previous minor versions of that major version. If you wanted to find a specific minor release of the official build, we can build an index mapping timestamped tilesets to minor version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is largely a copy-paste job from Tilezen, so there may be some details (some of which you've noted) to iron out still. Thanks for being thorough here...
SemVer is pretty explicate that once you hit 1.0.0 you've made a contract with the end-user with respect to future major-minor-patch versions.
So even though I'm conceptualizing this PR as being for Protomaps version 3.0.0
schema, I think this doc should still talk about 1.0.0
.
Your description of how Protomaps API works is related to the version of the schema
. Your description of /v2/
and /v2/20230701/
make sense to me.
I don't have strong opinions about how you setup the API, though I think it's useful to think about it in SemVer terms:
- Next major version:
/v3/20230701-beta/
or/v3/beta-v3.0.0-pre-20230701/
- Next minor version:
/v2/20230701-beta/
or/v2/beta-v2.5.0-pre-20230701/
- Next patch version:
/v2/20230701-beta/
or/v2/beta-v2.5.1-pre-20230701/
Where 20230701-beta
is an arbitrary build date (and allow for an {a,b,c} variant) with text indicator that it's not production worthy. So the question is where to put metadata about what schema (pre)release or SHA for the code was used, what OSM data went in, what Natural Earth version was used – and in future what Who's On First version (and even a landcover source)...
All the beta releases shouldn't have any user facing contract – the are effectively ephemeral. But both incremental minor released versions and patch released versions and beta versions need to be allowed for under the same "/v2/
" path and depending on developer velocity sometimes they may all be in flight at the same time.
l'll note here other discussion elsewhere in this repo about NPM package is more about the style
, which is somewhat dependent on the schema. Which creates an awkward situation of having 2 different "versions" in the same repo. Hmm.
- **Natural Earth** (used for zooms 0-8 for most everything) updates infrequently (often annually) | ||
- **OpenStreetMap** (used for zooms 9+ for most everything, sometimes earlier) updates frequently (at least daily) | ||
- **OpenStreetMapData** (used for zooms 9+ in the earth and water layers only) updates infrequently (optimistically monthly) | ||
- **Who’s On First** (used for zooms 12+ for places layer) updates frequently (at least daily) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the inclusion of WOF change the attribution requirements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WOF is not yet incorporated here in v3, that's a v4 task (or v3.1 if really ambitious). So this line should be removed or rephrased now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiles/src/main/java/com/protomaps/basemap/feature/CountryNameZooms.java
Outdated
Show resolved
Hide resolved
|
||
if (sf.hasTag("intermittent", "yes")) { | ||
feat.setAttr("intermittent", 1); | ||
feat.setAttr("intermittent", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use booleans to communicate we're not doing arithmetic on them
@@ -19,6 +23,22 @@ public String name() { | |||
return "places"; | |||
} | |||
|
|||
// Evaluates place layer sort ordering of inputs into an integer for the sort-key field. | |||
/* | |||
static int getSortKey(float min_zoom, int population_rank, long population, String name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want this:
// Evaluates place layer sort ordering of inputs into an integer for the sort-key field.
static int getSortKey(OptionalInt min_zoom, OptionalInt population_rank, OptionalLong population, String name) {
return SortKey
// ORDER BY "min_zoom" ASC NULLS LAST,
.orderByDouble(min_zoom.isEmpty() ? 15.0 : min_zoom.getAsInt(), 0.0, 15.0, 20)
// population_rank DESC NULLS LAST,
.thenByInt(population_rank.isEmpty() ? 15 : population_rank.getAsInt(), 0, 15)
// population DESC NULLS LAST,
.thenByLog(population.isEmpty() ? 0 : population.getAsLong(), 1, 1000000000, 100)
// length(name) ASC
.thenByInt(name == null ? 0 : name.length(), 0, 31)
.get();
}
the difference between int
and Integer
is int
is a primitive (non-pointer) and cannot be null, Integer
is a "boxed" object. OptionalInt
is a better choice as it's more explicit but requires calling the isEmpty()
and getAsInt()
methods to check or access it.
@@ -19,6 +23,22 @@ public String name() { | |||
return "places"; | |||
} | |||
|
|||
// Evaluates place layer sort ordering of inputs into an integer for the sort-key field. | |||
/* | |||
static int getSortKey(float min_zoom, int population_rank, long population, String name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, pretty sure levels
needs to be > 1 to be meaningful, the goal here is to define the range of the mapping without explicit knowledge of min-max values. so we can specify a conservative value like 1 billion for population as the maximum.
@@ -19,6 +23,22 @@ public String name() { | |||
return "places"; | |||
} | |||
|
|||
// Evaluates place layer sort ordering of inputs into an integer for the sort-key field. | |||
/* | |||
static int getSortKey(float min_zoom, int population_rank, long population, String name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want getSortKey here to actually work on nulls or can we just use 0
as a null value for population
, population_rank
, etc?
|
||
// we set the sort keys so the label grid can be sorted predictably (bonus: tile features also sorted) | ||
feat.setPointLabelGridPixelSize(12, 128); | ||
//feat.setSortKey(getSortKey("pmap:min_zoom", "pmap:population_rank", "population", "name")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2nd comment: If we are going to need pmap:min_zoom
again then we should extract that out into a local variable instead of retrieving the state that was set in setAttr
.
sf.getString("min_zoom") == null ? theme_min_zoom : (int) Double.parseDouble(sf.getString("min_zoom")), | ||
theme_max_zoom) | ||
.setAttr("pmap:ne_id", sf.getString("ne_id")) | ||
.setAttr("pmap:brk_a3", sf.getString("brk_a3")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this part of tilezen? don't see it in https://tilezen.readthedocs.io/en/latest/layers/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be an oversight in the documentation with the final v1.9 changes? It was added, IIRC, because it allows the cartographer the flexibility to turn off/on certain country boundary lines by ID in the style explicitly as a fail safe for legal compliance without needing a data or code change (which can take time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, but if it overlaps with ne_id
in use cases I'd prefer the one with more semantics (how often are NE IDs reassigned?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ne:brk_a3
is highly stable, while the ne:id
is mostly stable but may vary between scale set (1:10m, 1:50m, 1:110m) and major Natural Earth release versions.
.setZoomRange( | ||
sf.getString("min_zoom") == null ? theme_min_zoom : (int) Double.parseDouble(sf.getString("min_zoom")), | ||
theme_max_zoom) | ||
.setAttr("pmap:ne_id", sf.getString("ne_id")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to embed ne or OSM IDs in the 64-bit MVT id - are we ever going to use this for visualization or joining?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For boundaries and pmap:ne_id
, yes the intent (unlike with other layers) is to provide a fail safe during styling. This shouldn't be standard practice. Are the MVT IDs addressable in a MapLibre style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean about ne_id
being useful for failsafes, though it seems like referring to specific one-off features by name would be more natural. MVT IDs can addressed in the style, but in practice because I'm doing some bit twiddling the number is not going to be useful. I could be convinced to abandon MVT IDs if the storage doesn't inflate by storing all of source, ID, etc separately.
But it's important for us to have the OSM ID if possible to power debugging "where did this feature come from"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if we switch to daylight distribution and add buildings in the IDs all get renumbered and are worthless. (I too find them great for debug.)
.setAttr("pmap:min_admin_level", minAdminLevel.getAsInt()) | ||
.setAttr("pmap:kind", kind) | ||
.setAttr("pmap:kind_detail", kind_detail) | ||
.setAttr("pmap:min_zoom", min_zoom) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So as is we will have a feature with pmap:min_zoom=0
that starts appearing at 6, is this to enable tag continuity with the NE feature from zooms 0-5? Or used as a proxy for label weight/size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug... since names for text labels aren't included the min_zoom here should be omitted in v3.
To your question... Yes, to enable tag continuity with the NE features. Deliberate because to the end user they don't care where the feature came from... but it should have a consistent "min_zoom" based on when the user first saw it, not when it was transitioned to another data source on the server in the "black box".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #79.
@@ -98,6 +196,8 @@ public List<VectorTile.Feature> postProcess(int zoom, List<VectorTile.Feature> i | |||
else if (zoom <= 5) | |||
minArea = 800 / (4096 * 4096) * (256 * 256); | |||
items = Area.filterArea(items, minArea); | |||
|
|||
//return FeatureMerge.mergeNearbyPolygons(items, 3.125, 3.125, 0.5, 0.5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems unlikely this will catch many things, unlike buildings, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every 1-3% counts. I was debating myself if this is useful. If we don't merge landuse then they can be stroked to separate individual "parks"... else it's just one large splotch of green. But most maps will probably not stroke them?
With natural layer it's clear cut to me that "you seen one tree, you've seen them all" haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say leave it commented out for now with a TODO - we should determine empirically if this is useful later
} | ||
} | ||
|
||
@Override | ||
public List<VectorTile.Feature> postProcess(int zoom, List<VectorTile.Feature> items) { | ||
if (zoom < 12) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brandon fixed a bug near here, watch for merge conflicts / pull that in now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've merged that PR.
Something strange is going on where I can't comment on some of your PR comments... Said @bdon:
If a building has any "business" like tags they are all handled in the POIs layer now. If a building doesn't have any relevant POI tags, then this change removes the generic "this building is named this" tag. This is technically a regression. The way Tilezen solves this is to peel the remaining building names off as point labels, stored in the buildings layer. In Protomaps v3 those can't go in the polygon only buildings layer, so you'd either need a building_points layer, or put them as a new kind into the POIs layer. But for v3 I'm proposing to ignore this regression, as v4 has a planned fix for it and it's very edge case. (The same thing is true for address points, actually.)
No, polygons with names will have really odd labeling behavior on tile boundaries. This will be more noticably bad at some zooms over other zooms, and can somewhat be worked around by setting padding around the text &etc. But it's not awesome. |
We can use |
I find Planetiler's area representations to be super confusing. I got it to work for the POIs layer with a lot of trial and error. But yes, this is totally doable (and once we have real "meter" units what's in Tilezen can just be copied over).
Buildings layer is one of the biggest file size drivers. So this is for all types of buildings if they are attached to their neighbors or not. +1 to having brownstones in a test suite.
If it were me I'd recast
Correct, that's my general principal to never include names on polygon features.
My 2¢: If you want revGeo tiles then make revGeo tiles. |
@nvkelso I'm going to merge this now, cleanup and the few remaining tasks we can address in follow up PRs (and gives us an opportunity to flesh out the visual CI suite) |
* show 3 style layers later * disable min_zoom export (optional v4 future feature) * always export min_zoom for label collisions; bug fix for levels (at all zooms) * always export min_zoom for label collisions * hide funky SF university; show small parks earlier; hide tall storage buildings; defer long named POIs to later zooms * always export min_zoom for label collisions; bug fix for levels (at all zooms) * remove reference to Whos On First (since thats not integrated yet) * switch to ISO codes for country, region matching * add Kansas target (USA country label) * show more low zoom places from NE * USA test and more ISO-ing * add planet-xl target; twiddle planet * workaround oddness around iso key name * full list of region data * add wikidata for regions * switch to wikidata for regions, ensure name * download data; use dl data in planet targets * use NE values not TZ; add byWikidata functionality * switch country min zoom to wikidata lookup * also test new wikidata path * fix NE minZoom for region boundaries * apply full setSortKey to NE zooms * bug fix, QA, and expand wikidata faceting * only wikidata (not also wikidataid) * sort bug fixes? simplify, remove iso (for wikidata), lint * exclude min_zoom, show one step more NE * aerodrome earlier, wilderness later, few less POIs in thinning * bug fix sortKey * follow same Region data structure conventions as CountryInfo; bug fix tests * ref name type handling (low zooms) * do not use 110m NE sources (show Hawaii earlier) * do not use 110m NE sources (show Hawaii earlier) * bug fix for zoom 0 content and low zoom country and region labels * bug fix zoom 0 tile places * inclusive of tz_place, add comments * only export disputed if true; stop exporting IDs at low zooms (merging) * avoid exporting empty props * merge landuse at low zooms only * use same pixel size for NE earth and water * stop pruning cities in postprocessing * remove symbol-sort-key as it's redundant with tile ordering; remove rank assignment in places postprocess. * sort key: we want higher priority cities to have lower keys, so invert the poprank and population min/max. * unify labelgrid cells on 64px (4x4) size; add place kind to sortkey * minZoom first, then kindRank as tie breaker * less range; varNames * wider range of kindRank; cleanup getSortKey comments * remove the bad minZoom <> themeMinZoom test and reset * remove outdated test (failed low zoom localities) * setSortKey on any label layer, less cities per grid cell (one more zoom) * negative water layers are also brunnel tunnels with level -1 * push small park areas later (and some other kinds with areas), push node parks later --------- Co-authored-by: Brandon Liu <bdon@bdon.org>
This is a followup to #37 which established basic changes needed for Felt's basemap.
TL;DR for v3:
This should get expanded into a CHANGELOG.md file at the root dir...
Core Tilezen schema properties added:
pmap:kind
is present on every feature now in every layerpmap:kind_detail
is optionally present on some features in some layerspmap:
prefix will be removed (sopmap:kind
will become simplykind
)Core OSM tags for different kinds of places have been augmented, but...
DEPRECATION WARNING
: These OSM tags are marked for deprecation in v4 schema, do not use these for styling. They aren't needed and take up extra file size.kind
, or included inkind_detail
. If there is a gap, please file an issue so it can be addressed.Zoom ranges of most features have been adjusted to remove details (and reduce file size) at early and mid-zooms.
Names have been removed from most features at early and mid-zooms (to also reduce file size)
When features do have names, a
pmap:min_zoom
is added to achieve more predictable label collisions client side.pmap:
prefix will be removed (sopmap:min_zoom
will become simplymin_zoom
)Add Makefile with common build commands for easier development
Improve consistency of internal private variable names in Planetiler profile.
This PR will cover bug fixes and enhancements:
town
,village
aren'tneighbourhood
; addpmap:kind_detail
for original places tag values)population_rank
be prefixed withpmap:
? Yes in v3.national_park
features (versuspark
) since you also have to look atoperator
tag to derive this from OSMpmap:area_debug
propertyarea
boost aheight
boost?attraction
,craft
,historic
,shop
, andtourism
tags.beach
) and landuse (cemetery
,recreation_ground
,winter_sports
,quarry
,park
,forest
,military
) tags.national_park
features (versuspark
) since you also have to look atoperator
tag to derive this from OSMforest
features (versuswood
) since you also have to look atoperator
tag to derive this from OSMnational_park
,protected_area
, andnature_reserve
to landuse layer as they are not natural but cultural.pmap:kind_detail
for values ofservice
tag forother
kind roads (eg forparking_aisle
features)ref_length
toshield_text_length
to more quickly converge towards Tilezen syntaxRoads layer: highways with links are borked, investigateWorks fine in default style, debug Felt style.brunnel
oris_tunnel
/is_bridge
booleans?pmap:level
already does this?Roads layer: Consider turning off the link magic post processing (which would also keep thekind_detail
) as it seems to be borking some highways?pmap:kind
coallesce as in the Landuse layergrass
polygons.pmap:level
(same as roads layer) and intermittent indicatorskind
values for:beach
,zoo
,military
,naval_base
,airfield
leisure
tag are mapped to individual kind values instead of all erroneously topark
.amenity
tag are mapped to individual kind values instead of all erroneously toschool
when nothospital
.highway
INpedestrian
orfootway
are still exported as kind =pedestrian
(as wellman_made
=bridge
still mapping to kind =pedestrian
)pier
polygons.pmap:kind
=water
, with thekind_detail
set toreservoir
?pmap:level
(same as roads layer) and intermittent, alkaline, reservoir indicatorspmap:level
(same as roads layer)height
tag at mid-zooms so more buildings merge in post-processing (to reduce file size)min_height
tag (for 2.5D and 3D visualizations)locality
(city) boundary lines (even if coterminous with parent placetypes)Followup in separate PR?