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

Strip long names from boundaries. #1700

Merged
merged 3 commits into from
Nov 6, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions integration-test/1683-strip-names-off-boundary-lines.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
# -*- encoding: utf-8 -*-
from . import FixtureTest


class StripNamesOffBoundaryLinesTest(FixtureTest):

def _setup(self, z, x, y, left_name, right_name):
from tilequeue.tile import coord_to_bounds
from shapely.geometry import LineString
from ModestMaps.Core import Coordinate
import dsl

minx, miny, maxx, maxy = coord_to_bounds(
Coordinate(zoom=z, column=x, row=y))

# move the coordinate points slightly out of the tile, so that we
# don't get borders along the sides of the tile.
w = maxx - minx
h = maxy - miny
minx -= 0.5 * w
miny -= 0.5 * h
maxx += 0.5 * w
maxy += 0.5 * h

self.generate_fixtures(
dsl.way(
1,
LineString([
[minx, miny],
[minx, maxy],
[maxx, maxy],
[minx, miny],
]), {
'boundary': 'administrative',
'admin_level': '2',
'name': left_name,
}
),
dsl.way(
2,
LineString([
[minx, miny],
[maxx, maxy],
[maxx, miny],
[minx, miny],
]), {
'boundary': 'administrative',
'admin_level': '2',
'name': right_name,
}
),
)

def test_do_not_strip_short_names(self):
z, x, y = (8, 128, 128)

self._setup(z, x, y, 'Foo', 'Bar')

self.assert_has_feature(
z, x, y, 'boundaries', {
'kind': 'country',
'name': 'Foo - Bar',
})

def test_strip_long_names(self):
z, x, y = (8, 128, 128)

n = 16
self._setup(z, x, y, 'Foo' * n, 'Bar' * n)

# should have stripped all the names off with such a ridiculously long
# name.
self.assert_no_matching_feature(
z, x, y, 'boundaries', {
'kind': 'country',
'name': None,
})
3 changes: 2 additions & 1 deletion integration-test/976-fractional-pois.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ def test_state_boundary(self):
'https://www.openstreetmap.org/relation/61320',
], clip=self.tile_bbox(9, 150, 192, padding=2))

# NOTE: might not have an ID if it has been merged
Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks for adding this note!

self.assert_has_feature(
9, 150, 192, 'boundaries',
{'min_zoom': 8, 'id': -224951,
{'min_zoom': 8,
'source': 'openstreetmap.org',
'name': 'New Jersey - New York'})

Expand Down
6 changes: 4 additions & 2 deletions integration-test/992-boundaries-min_zoom-and-name.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,15 @@ def test_10m_regions(self):
class BoundariesMinZoomAndNameOsm(FixtureTest):
def test_region_boundary_zug_luzern(self):
# Switzerland region HAS name, OpenStreetMap
# do this at z12, as the boundary between Zug and Luzern is quite
# short, and we want enough space to label.
self.load_fixtures([
'http://www.openstreetmap.org/relation/1686447',
'http://www.openstreetmap.org/relation/1685677',
], clip=self.tile_bbox(8, 133, 89))
], clip=self.tile_bbox(12, 2144, 1438))

self.assert_has_feature(
8, 133, 89, 'boundaries',
Copy link
Member

Choose a reason for hiding this comment

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

I think we should still keep the zoom 8 test but for kind: region only as that proves we are taking the OSM data correctly at zoom 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Fixed in fb5565c.

12, 2144, 1438, 'boundaries',
{'kind': 'region', 'name': 'Zug - Luzern'})

def test_region_boundary_salzburg_tirol(self):
Expand Down
18 changes: 18 additions & 0 deletions queries.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -592,11 +592,29 @@ post_process:
attribute: kind
target_attribute: landuse_kind
cutting_attrs: { sort_key: 'sort_rank', reverse: True }


# turn admin polygons (countries, regions, etc...) into oriented boundaries with left/right names.
- fn: vectordatasource.transform.admin_boundaries
params:
base_layer: boundaries
start_zoom: 8

# try to merge boundaries into as long segments as possible.
- fn: vectordatasource.transform.merge_line_features
params:
source_layer: boundaries
start_zoom: 8
end_zoom: 15

# drop labels on boundaries which are too short to render.
- fn: vectordatasource.transform.drop_names_on_short_boundaries
params:
source_layer: boundaries
start_zoom: 8
end_zoom: 11
factor: 11
Copy link
Member

Choose a reason for hiding this comment

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

Please add comment about factor, what it does, and it's units.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added comment and changed to a more descriptive name in a75fd59.


# drop names on boundary lines (country, region, macroregion) except zoom 7 from Natural Earth
- fn: vectordatasource.transform.drop_properties
params:
Expand Down
87 changes: 87 additions & 0 deletions vectordatasource/transform.py
Original file line number Diff line number Diff line change
Expand Up @@ -1959,6 +1959,93 @@ def admin_boundaries(ctx):
return layer


def _unicode_len(s):
if isinstance(s, str):
return len(s.decode('utf-8'))
elif isinstance(s, unicode):
return len(s)
return None


def _delete_labels_longer_than(max_label, props):
Copy link
Member

Choose a reason for hiding this comment

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

is max_label = max_length, both terms are mentioned here but I think only one is used in practice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well spotted, thanks! I think that was a typo / thinko. Fixed in a75fd59.

"""
Delete entries in the props dict where the key starts with 'name' and the
unicode length of the value is greater than max_length.

If one half of a left/right pair is too long, then the opposite in the pair
is also deleted.
"""

to_delete = set()

for k, v in props.iteritems():
if not k.startswith('name'):
continue

length = _unicode_len(v)
if length is None:
# huh? name isn't a string?
continue

if length <= max_label:
continue

to_delete.add(k)
if k.startswith('name:left:'):
opposite_k = k.replace(':left:', ':right:')
to_delete.add(opposite_k)
elif k.startswith('name:right:'):
opposite_k = k.replace(':right:', ':left:')
to_delete.add(opposite_k)

for k in to_delete:
if k in props:
del props[k]


def drop_names_on_short_boundaries(ctx):
"""
Drop all names on a boundaries which are too small to render the shortest
name.
"""

params = _Params(ctx, 'drop_names_on_short_boundaries')
layer_name = params.required('source_layer')
start_zoom = params.optional('start_zoom', typ=int, default=0)
end_zoom = params.optional('end_zoom', typ=int)
factor = params.optional('factor', typ=(int, float), default=10.0)

layer = _find_layer(ctx.feature_layers, layer_name)
zoom = ctx.nominal_zoom

if zoom < start_zoom or \
(end_zoom is not None and zoom >= end_zoom):
return None

tolerance = factor * tolerance_for_zoom(zoom)
Copy link
Member

@nvkelso nvkelso Nov 3, 2018

Choose a reason for hiding this comment

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

Please add comment for what type of units this results in? Is it "must fit exactly plus the factor" or?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this to meters_per_letter, since that's what it is - Mercator meters per letter / character / grapheme cluster in the string. The factor was roughly how many pixels to expect the width of each letter to be, I've found that 11 is a pretty good value by experimentation, but we should see what the results of a build look like and tweak it.


for shape, props, fid in layer['features']:
geom_type = shape.geom_type

if geom_type in ('LineString', 'MultiLineString'):
label_shape = shape.simplify(tolerance)
if geom_type == 'LineString':
shape_length = label_shape.length
else:
# get the longest section to see if that's labellable - if
# not, then none of the sections could have a label and we
# can drop the names.
shape_length = max(part.length for part in label_shape)

# maximum number of characters we'll be able to print at this
# zoom.
max_label = int(shape_length / tolerance)
Copy link
Member

Choose a reason for hiding this comment

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

should this be called max_length (or even max_chars)?

Please add comment about what units these are in... logical pixels? meters? characters?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed this to max_label_chars, added some comments and made other variable names more descriptive in a75fd59 - hopefully that makes it clear enough?


_delete_labels_longer_than(max_label, props)

return None


def handle_label_placement(ctx):
"""
Converts a geometry label column into a separate feature.
Expand Down