Tune placenames typography

[sommerluk]

Tune placenames typography

Resolves #2378

• In general, in this style the quotient text-wrap-width ∕ text-size is increasing on higher zoom levels. There were however some exceptions. This was the reason why the line wrap of “Freiburg im Breisgau” was two lines on lower zoom level, on line on middle zoom level and two lines on higher zoom level again. This PR calculates consequently the quotient (≙ line wrap in em, em is the current font size) and makes sure that it never decreases.

• The font size of state labels is now at least 10. To get this, rendering if moved one zoom level upward.

• Avoid steps bigger than 1 in the font size between two zoom levels.

• The line spacing is adjusted. Its value ranges from −0.15 em for line-wrap-width ≈ 3 em up to −0.05 em for line-wrap-width ≈ 5 em. For tall asian scripts, −0.15 em means that some glyphs might slightly touch, but stay legible. For latin scripts I don’t think anything will ever touch. And at −0.05 em there should be almost nothing that will ever touch.

• Labels get a margin of 7 em to prevent rendering too close to other labels to avoid confusion.

I suppose that rendering problems like this
are related to the margin. Is this something that can be solved by different mapnik settings for treating of tile boundaries? Or is this a mapnik bug?

[nebulon42]

Travis CI fails because this requires Mapnik 3. Not something that has to be changed.

[nebulon42]

Some good finetuning, thanks! Here a preview of my low zoom extract on z6:

Definitely an improvement.

Some remarks:

• I'm not sure I like the shift in zoom level for state labels. Does rendering states without labels make sense? At least I think we should then drop state abbreviations and start with the full names right away.
• Originially, I chose less margin (https://github.com/gravitystorm/openstreetmap-carto/pull/2374/files). May your proposed margin be too much?

Regarding cut labels: This is something I see frequently with Kosmtik, but have never found out what really causes it (ref kosmtik/kosmtik#57)

[sommerluk]

For the margins, I’ve played with various values, so that (at higher zoom levels) no line of any neighbour label is never closer to a line of a multiline label. In the current PR this is always 0.7 em. We could also think about make this value bigger for labels with wider line spacing and smaller for labels for smaller line spacing. What margin would you preferr?

I’m also happy dropping state abbreviations (does not exist in the whole world anyway, and also these who are existing only some of them are shown currently depending of the size of the state at z4)

[pnorman]

Manually ran the travis commands on the result of a merge, and it passes.

[matthijsmelissen]

Thanks, looking good.

I would also prefer dropping state abbreviations altogether.

[matthijsmelissen]

When viewing this in kosmtik, everything works fine. However, when running carto manually, I get error messages:

$ kosmtik/node_modules/carto/bin/carto ~/osm/project.mml > ~/osm/osm.xml
Error: placenames.mss:29:6 Unrecognized rule: text-margin
placenames.mss:29:6 Unrecognized rule: text-margin
placenames.mss:23:6 Unrecognized rule: text-margin
placenames.mss:17:6 Unrecognized rule: text-margin
placenames.mss:12:4 Unrecognized rule: text-margin
placenames.mss:101:6 Unrecognized rule: shield-margin
placenames.mss:101:6 Unrecognized rule: shield-margin
placenames.mss:83:4 Unrecognized rule: shield-margin
<snip>
    at Object.env.error (kosmtik/node_modules/carto/lib/carto/parser.js:241:55)
    at Rule.tree.Rule.toXML (kosmtik/node_modules/carto/lib/carto/tree/rule.js:56:20)
    at Definition.tree.Definition.symbolizersToXML (kosmtik/node_modules/carto/lib/carto/tree/definition.js:139:46)
    at Definition.tree.Definition.toXML (kosmtik/node_modules/carto/lib/carto/tree/definition.js:203:33)
    at kosmtik/node_modules/carto/lib/carto/tree/style.js:31:27
    at Array.map (native)
    at Object.tree.StyleXML (kosmtik/node_modules/carto/lib/carto/tree/style.js:30:29)
    at Renderer.render (kosmtik/node_modules/carto/lib/carto/renderer.js:138:43)
    at compileMML (kosmtik/node_modules/carto/bin/carto:87:31)
    at Object.<anonymous> (kosmtik/node_modules/carto/bin/carto:178:9)

Any idea what might cause this? The carto version seems fine:

$ kosmtik/node_modules/carto/bin/carto -v
carto 0.16.3 (Carto map stylesheet compiler)

[matthijsmelissen]

Figured it out already: one should manually add -a "3.0.0" when using Mapnik 3 functions.

[nebulon42]

@math1985 Default is currently 2.3.0 but this should be latest Mapnik API version or at least 3.0.0 now (ref mapbox/carto#454)

[matthijsmelissen]

And the reason travis fails here is that this PR is based against a version of the .travis.yml that does not yet have the -a "3.0.0".

[pnorman]

And the reason travis fails here is that this PR is based against a version of the .travis.yml that does not yet have the -a "3.0.0".

It's right to be failing because it doesn't work with the carto defaults and we don't document a need to specify the mapnik-reference target. The fact that would pass with the current .travis.yml is a bug.

[kocio-pl]

Is this rule testing issue a blocking problem or just a glitch that we can safely ignore if we want to merge this code?

@sommerluk I also think we should drop state abbreviations.

[pnorman]

Is this rule testing issue a blocking problem or just a glitch that we can safely ignore if we want to merge this code?

It's a blocker because this code won't work with the instructions in INSTALL.md

[sommerluk]

Rebased. Travis check passes now. As far as I see the current INSTALL.md requieres carto 0.16 and mapnik 3, so I don’t see a problem here.

[sommerluk]

Do not use state abbreviations – now no state abbreviations are used.

[sommerluk]

I would leave the margin as-is for the moment…

[pnorman]

Rebased. Travis check passes now. As far as I see the current INSTALL.md requieres carto 0.16 and mapnik 3, so I don’t see a problem here.

It requires Mapnik 3 for rendering, but says nothing about mapnik-reference. Travis is wrong here, and we need to either fix it or the documentation.

[sommerluk]

@pnorman Is this something that you could fix? (I do not know anything about Travis.)

[pnorman]

@pnorman Is this something that you could fix? (I do not know anything about Travis.)

My plan was to change the documentation

[kocio-pl]

All the obstacles are out and new release is done, so it can join the master now.

Thanks, @sommerluk!

[pnorman]

This PR included a bunch of bad git stuff so should have been merged differently (e.g. squashed, rebased, cherry-picked), but we're stuck with it now :(

[kocio-pl]

Sorry, too much general sorting things up made me overlook how many commits it consists of - I've just seen that it changes one file...

Are you sure we can't revert them and do the things in a proper way?

[pnorman]

Are you sure we can't revert them and do the things in a proper way?

Reverting undoes the changes, it doesn't remove them from git. You should never rewrite history on a public branch.

[sommerluk]

I suppose this happened when I rebased this PR. Actually there are only three or four commits directly related to this PR. The rest must be the history rebase. I’m sorry.

[nebulon42]

While this was not pretty I don't really see any anomalies in the log (I see them here on Github, but not with git log). I suspect that git is smart enough to detect that these were old commits that were already present in the master branch.

The only commit I don't find a PR for is 5ba3ce4. But I may have missed it.

[pnorman]

I suspect that git is smart enough to detect that these were old commits that were already present in the master branch.

These aren't old commits. 9e30b56 is a new commit in this PR while f07b249 is the original GPG-signed commit.