-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
[WIP] Basic schema enhancements for Felt basemap #37
Conversation
…asic network support
200, | ||
0}; | ||
|
||
for (int i = 0; i < pop_breaks.length; i++) { |
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 isn't working in Java, but works fine in Python. Help!
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 to break
once you find a result in the array that matches, right?
it seems like population_rank
at the top is assigned to 0 or the OSM value. Since there's not really a case where it's nullable I don't think you need to use the Integer
type, just plain int
should be OK.
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.
.setZoomRange(12, 15); | ||
|
||
if( sf.hasTag("intermittent", "yes" ) ) { | ||
feat.setAttr("intermittent", 1); |
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 way 316401077 in israel-and-palestine GeoFabrik:
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.
@bdon up to you if you want this to be a boolean in the MVT or an int
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.
Tilezen uses a boolean so let's converge on that later
} else if (sf.hasTag("shop", "grocery", "supermarket" )) { | ||
kind = sf.getString("shop"); | ||
} else if (sf.hasTag("tourism", "attraction", "camp_site", "hotel" )) { | ||
kind = sf.getString("tourism"); |
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.
if( shield_text != null ) { | ||
if( shield_text.contains("US ") ) { | ||
shield_text = shield_text.replaceAll("US ", ""); | ||
network_val = "US:US"; |
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.
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 the Tilezen way of specifying the network values, which is mostly true to OSM's values.
network_val = "US:US"; | ||
} else if( shield_text.contains("I ") ) { | ||
shield_text = shield_text.replaceAll("I ", ""); | ||
network_val = "US:I"; |
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.
shield_text = shield_text.replaceAll("I ", ""); | ||
network_val = "US:I"; | ||
} else { | ||
network_val = "other"; |
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.
@@ -1187,10 +1231,16 @@ export function labels_layers(source: string, c: Theme): any[] { | |||
"source-layer": "places", | |||
filter: ["==", "pmap:kind", "city"], | |||
layout: { | |||
"symbol-sort-key": ["number", ["get", "pmap: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.
"text-anchor": { | ||
stops: [ | ||
[7, "left"], | ||
[8, "center"], |
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.
sf.hasTag("railway", "station"))) | ||
{ | ||
String kind = "other"; | ||
if (sf.hasTag("aeroway", "aerodrome" )) { |
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.
Almost all aerodromes are ways and relations in OSM
String highway = sourceFeature.getString("highway"); | ||
String shield_text = sourceFeature.getString("ref"); | ||
String network_val = sourceFeature.getString("network"); | ||
shield_text = (shield_text == null ? null : shield_text.split(";")[0].replaceAll("\\s", "")); |
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.
Shields don't look good on very long ref values (like US 101;CA 1
), so we split on the ;
and because sprites generally should be smaller not bigger we also strip the internal whitespace.
String network_val = sourceFeature.getString("network"); | ||
shield_text = (shield_text == null ? null : shield_text.split(";")[0].replaceAll("\\s", "")); | ||
if( shield_text != null ) { | ||
if( shield_text.contains("US ") ) { |
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 cheap way of imputing the network without walking all the relations. The output for tile user is the same, and can be upgraded to rels later without changing the contract.
network_val = "other"; | ||
} | ||
} | ||
Integer shield_text_length = (shield_text == null ? null : shield_text.length()); |
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.
Once we've done all our transforms on the string, we calculate the shield_text_length for later export as ref_length
... This makes styling in MapLibre much easier.
@@ -31,29 +48,81 @@ public void processFeature(SourceFeature sourceFeature, FeatureCollector feature | |||
.setAttrWithMinzoom("bridge", sourceFeature.getString("bridge"), 12) | |||
.setAttrWithMinzoom("tunnel", sourceFeature.getString("tunnel"), 12) | |||
.setAttrWithMinzoom("layer", sourceFeature.getString("layer"), 12) | |||
.setAttrWithMinzoom("oneway", sourceFeature.getString("oneway"), 14) | |||
.setAttr("ref", sourceFeature.getString("ref")); |
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.
We don't always want export the ref
for all zooms... instead we do it by highway types (below). This reduces tile size.
OsmNames.setOsmNames(feat, sourceFeature, 13); | ||
} else if (highway.equals("residential") || highway.equals("service") || highway.equals("unclassified") || | ||
highway.equals("road")) { | ||
highway.equals("road") || highway.equals("raceway")) { |
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 for speedway roads.
|
||
// try first for polygon -> point representations | ||
if (sf.canBePolygon()) { | ||
var poly_label_position = features.pointOnSurface(this.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.
If it's a polygon, then we need to get a point on it's surface for export
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.
TODO: This needs QA/QC
@@ -43,9 +43,18 @@ public void processFeature(SourceFeature sf, FeatureCollector features) { | |||
.setAttr("leisure", sf.getString("leisure")) | |||
.setAttr("water", sf.getString("water")) | |||
.setAttr("waterway", sf.getString("waterway")) | |||
// Add less common attributes only at higher zooms |
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.
These props mostly apply to water lines, but there are exceptions for polygons, too, so are included here for parity.
@@ -0,0 +1,21 @@ | |||
package com.protomaps.basemap.names; |
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.
New class so NE names can be transformed to look like OSM names
String highway = sourceFeature.getString("highway"); | ||
String shield_text = sourceFeature.getString("ref"); | ||
String network_val = sourceFeature.getString("network"); | ||
shield_text = (shield_text == null ? null : shield_text.split(";")[0]); |
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.
We just want the first ref, not list of refs as is often done in OSM on ways.
Better way is to walk the relations and rank order them, but later...
network_val = "other"; | ||
} | ||
} | ||
shield_text = (shield_text == null ? null : shield_text..replaceAll("\\s", "")); |
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.
Shorter refs are better for display in map shields vis-a-vis sprite artwork.
"source-layer": "transit", | ||
filter: [ | ||
"any", | ||
["==", "pmap:kind", "pier"], |
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.
Integer adminLevel = Parse.parseIntOrNull(relation.getString("admin_level")); | ||
Integer disputed = relation.hasTag("boundary", "disputed") ? 1 : 0; |
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.
TODO: This seems to always eval to 0
in error
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.
@nvkelso what relations/area are you testing on? on observation with Overpass Turbo it seems many relations with boundary=disputed
lack the type=boundary
tag, eg https://www.openstreetmap.org/relation/13562322
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.
The Line of Control in Cyprus (r/13562322)is an odd ball – it's admin_level
is 3
for starters.
I was testing around Israel as the build there has a smaller bounding box than India or China and more admin_level
= 2
features.
- Israel - Egypt boundary way with relevant relations: w/95211614
- Israel - Gaza boundary way with relevant relations: w/25057102
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.
It seems to evaluate to 1 on this relation: https://www.openstreetmap.org/relation/1703814
Ran |
|
||
if (highway.equals("motorway")) { | ||
feat.setAttrWithMinzoom("ref", shield_text, 7) | ||
.setAttrWithMinzoom("ref_length", shield_text_length, 7) |
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.
@bdon up to you what you want to call the exported shield_text_length
. In Tilezen it's shield_text_length
but other schemas call it ref_length
.
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.
+1 to matching tilezen
cd compare | ||
npm run serve | ||
``` | ||
|
||
5. Linting to apply code formatting | ||
|
||
```shell |
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.
When adding the new linting section, IntelliJ IDEA CE recommends shell
not sh
so I updated the instances above, too.
@@ -30,6 +30,7 @@ public void processFeature(SourceFeature sf, FeatureCollector features) { | |||
List<OsmReader.RelationMember<AdminRecord>> recs = sf.relationInfo(AdminRecord.class); | |||
if (recs.size() > 0) { | |||
OptionalInt minAdminLevel = recs.stream().mapToInt(r -> r.relation().adminLevel).min(); |
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: This looks like it prevents the City and County of San Francisco from appearing as a locality boundary line, instead it only appears as a county boundary line. An odd case, but this is true for several other "global cities" in the USA and elsewhere, too.
@bdon this captures most of what we talked about Monday.
TODO:
population_rank
isn't working as expected