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

Increased text and shield distances on waterways, railways and roads #3295

Closed
wants to merge 4 commits into from

Conversation

Penegal
Copy link
Contributor

@Penegal Penegal commented Jul 6, 2018

This PR fixes #3111: it limits redundant shields and stream labels by using shield-repeat-distance and text-repeat-distance. The *-min-distance versions of these attributes were not used, because they didn't change rendering, even with very large values (i.e. about 1000); besides, this PR drops shield-min-distance, because Mapnik complained about using this deprecated option minimum-distance with new options margin and repeat-distance.

I wandered around to see if the new version does not remove too much labels and shields, but there are still one or two at each view I took, so I assume these values are OK, but I can test on other places if requested, as long as the Geofabrik dump is not too big.

  • Before
    image
    image
  • After
    image
    image

@Adamant36
Copy link
Contributor

Adamant36 commented Jul 6, 2018

I don't know if it would work for shields or not, but with text there is also text-min-path-length so labels can skip smaller path segments. It might be worth trying on them both and seeing if you can find a good balance between that and text-repeat-distance. If you don't, the distance will probably still be a problem where lines are split up a lot.

@Penegal
Copy link
Contributor Author

Penegal commented Jul 6, 2018

@Adamant36: I think we shouldn't assume much on the underlying data, because there can always be cases where the assumption is false. For instance, relations or destination[:*] or *:lanes tags can lead to legitimately making very short ways, so we should have a style accounting for that.

@Adamant36
Copy link
Contributor

Adamant36 commented Jul 6, 2018

I wasn't assuming anything. I Iooked at the code and noticed it wasnt in there and thought you might want to test it out. Its your prerogative though. Its not like someone cant create another issue later if need be. I'm not sure how relations etc would affect it, but thats what testing is for. You should just write off trying something because theres other things you think might cause it to not work because you never know. I just spent 3 days tweaking around with getting golf course rendering to work and even if it was a massive hassle I learned a lot in the proccess. So it wasnt a total waste.

Also, I did do a pull request on a similar issue and know from that one that the more testing in various situations with different values the better. It looks like you only tested it in one place for each thing. You could try a few more with different conditions. It up to you what you want to try out though.

@Penegal
Copy link
Contributor Author

Penegal commented Jul 9, 2018

I don't know other cases where this correction would be effective, as it applies only to small related and connected ways; it's quite specific, so I have to search for other locations to test it, but, if you know some, I'd be happy to test this code on it, as long as the Geofabrik dump is not too heavy.

@Penegal
Copy link
Contributor Author

Penegal commented Jul 9, 2018

Some other test cases (before/after):

image
image

image
image

image
image

image
image

@Penegal
Copy link
Contributor Author

Penegal commented Jul 9, 2018

@Adamant36: sorry for the abruptness of my first answer, I was tired and did not have a cool head. I also see, from my last examples, that the modification I did for shields and streams can also be applied to some more map elements; I'll try that and post new examples soon.

@Adamant36
Copy link
Contributor

No worries. It happens. This was one of the issues I had on my list to deal with eventually if know one else did and I was looking forward to seeing how text-repeat-distance would work out. So thanks for giving it a try. It seems to have helped some.

@Penegal
Copy link
Contributor Author

Penegal commented Jul 9, 2018

I updated the code; now it changes display of highway shields and highway, railway and waterway texts by, for the record, removing some duplicated labels. I used a value of 600 for the *repeat-distance because, AFAIK:

  1. the value is in pixels, and
  2. screens below 800*600 are unlikely, so a value of 600 should still display at least a label at any view, on any reasonable screen resolution.

I chose this value because it removes some duplicates while not removing too much, but it can be changed if the rendering is still too cluttered, or, conversely, if it hides too much labels.

Before/after comparisons below:
image
image

image
image

image
image

image
image

image
image

@Penegal Penegal changed the title Increased text and shield distances on streams and roads Increased text and shield distances on waterways, railways and roads Jul 9, 2018
@kocio-pl
Copy link
Collaborator

kocio-pl commented Jul 10, 2018

Works good also in my test case:

Before
nzddjb_2

After
mxczfgfi

@kocio-pl
Copy link
Collaborator

Looks to me like resolving also #1956.

@kocio-pl
Copy link
Collaborator

Another case - unlike shields, I would be less radical with labels, could you test some smaller values?:

Before
78c9qvcj

After
wdjto8hr

@kocio-pl
Copy link
Collaborator

kocio-pl commented Jul 11, 2018

My test proved that any value of text distance (0, 5, 10, 100) gives the same result as 600, so it basically "turns off" repeating instead of adjusting it. Please look closer at this code and find out what is the problem.

I can probably just adjust the shields so we have less of them, but still it's better to have some control over the distance. Example with only shields distance used:

Before
g0elaq43

After
xgcjhxcr

@matthijsmelissen
Copy link
Collaborator

Anything useful in #2189?

@kocio-pl
Copy link
Collaborator

Only shield distance used - I would say 400 works better than 600, since roads are not always long and some repetitiveness is good for readability:

Cemetery example
Before
2jv1kksc

400
rs878env

600
qi3duqta

Motorway example
Before
n9xjql_o

400
2hwie1jb

600
rdl45sov

@kocio-pl
Copy link
Collaborator

London Borough example - 400 shield distance gives ~4 more street names (+7/-3):

Before
leczvc5s

After
rtd10fty

@kocio-pl
Copy link
Collaborator

kocio-pl commented Jul 11, 2018

On the other hand City of London on z13 is just completely covered with shields when shield distance is 400, so this code needs deeper insight:

Before
f1caxypb

After
dpqa6jmj

@Penegal
Copy link
Contributor Author

Penegal commented Jul 12, 2018

@matthijsmelissen: I don't understand what you mean about #2189, as this is not about places; could you explain?

@kocio-pl: Well, this puzzles me, because, AFAIK and according to the Mapnik doc, the changes I did on shields, limited to shield-min-distance and shield-repeat-distance, only apply to shields with the same content; it should not change the number of shields displayed when they all differ in content. Do you have an idea where that could come from?

@matthijsmelissen
Copy link
Collaborator

matthijsmelissen commented Jul 12, 2018

@matthijsmelissen: I don't understand what you mean about #2189, as this is not about places; could you explain?

This issue provides more information about *-min-distance.

@Penegal
Copy link
Contributor Author

Penegal commented Jul 12, 2018

@matthijsmelissen: Ok, then. I would say it does not give relevant additional information, as the current problem is about shields number, but, once this problem will have been solved, then #2189 could become relevant. That being said, the change in the meaning of text-min-distance is uncommon and worth noting.

@kocio-pl
Copy link
Collaborator

I don't really know, you should probably talk with Mapnik developers about that and test different cases. @talaj, could you bring some light on this problem?

@talaj
Copy link

talaj commented Jul 12, 2018

@kocio-pl Are you referring to the #3295 (comment), that there is many more shields after this pull request applied?

Btw, I've marked shield-min-distance deprecated in mapnik/mapnik-reference#148.

@kocio-pl
Copy link
Collaborator

Yes, it's interesting for me. Looks like this simple function has some strange effects.

The other interesting thing is #3295 (comment) - why text distance values don't work.

@ghost
Copy link

ghost commented Jul 13, 2018

In some cases, information may be lost to the user. Because there are areas that are no longer seen with the current code, I can not imagine with the new code...

@talaj
Copy link

talaj commented Jul 13, 2018

@kocio-pl I think I now see what is going on. What min-distance means depends on whether it's text or shield symbolizer and whether the placement option is set to line or not. It's a kludge. Here is relevant part of code in Mapnik.

Particularly shield-min-distance has always the same meaning as shield-margin. Thus it resolves collision between all objects, not only those with the same text.

text-min-distance with text-placement: line is the same as text-repeat-distance.

shield-repeat-distance, asked by @kocio-pl
@Penegal
Copy link
Contributor Author

Penegal commented Jul 13, 2018

I just tested with shield-margin=40, even on the Great London, and it seems to solve the problem; here is a sample at z13:
image

Thank you for the solution, @talaj; maybe this should be explicited in the Mapnik doc (http://cartocss.readthedocs.io/en/latest/mapnik_api.html). Or I missed it?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Jul 23, 2018

Closed by #3315, the rest is yet to come.

@talaj
Copy link

talaj commented Jul 23, 2018

@kocio-pl, I've tried to make a diff of resulting Mapnik XML with different values of variable @text-repeat-distance. I've observed that variable with the same name is defined also in water.mss, but value of that variable is overridden by valule from roads.mss. This means that variable scope is global across mss files in the project.

Except of this, variable substitution was working as expected.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Jul 23, 2018

Thanks, I will look at this problem later on, when more specific PRs will be ready.

@Penegal
Copy link
Contributor Author

Penegal commented Jul 23, 2018

@kocio-pl: the 4 PR you recommended have now been created, as #3315, #3316, #3317 and #3318.

@kocio-pl
Copy link
Collaborator

Great, thanks!

One more please for you: could you add links to all the testing locations?

@Penegal Penegal deleted the lower_text-spacing branch July 23, 2018 15:27
@Penegal
Copy link
Contributor Author

Penegal commented Jul 23, 2018

Er… Is it possible to do that from the export URL? My history only contains them, and not the tiles viewer ones.

@kocio-pl
Copy link
Collaborator

Oh, I don't mean that exact URL - just a link to the area including the interesting objects (like city, road or river) in the view box.

@Penegal
Copy link
Contributor Author

Penegal commented Jul 23, 2018

OK, then I'll give that for the PRs tomorrow.

@Penegal
Copy link
Contributor Author

Penegal commented Jul 24, 2018

@kocio-pl: done.

@kocio-pl
Copy link
Collaborator

Thanks!

@antifa-ev
Copy link

@Penegal What's the improvement here? As far I can see it generally leads to fewer repetitions of the street name on the street. Ain't this bad because the reader has to raster a bigger area with his eyes?

@dieterdreist
Copy link

dieterdreist commented Aug 1, 2018 via email

@Adamant36
Copy link
Contributor

@dieterdreist Is there even a way to test it on mobile before implementing it first? I don't think you can run a Docker environment on Android or IOS can you?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 1, 2018

I don't agree about what's needed on small screen.

Generic style is not meant for roads. Yes, we have them and they are important to be shown, even with classes, but shields are not needed in cities - they block names and create clutter there, so they should be less frequent. Also outside cities, they are just too crowded. Shields are very strong elements and they look like routing - and generic map is not a car map, we don't need to be routed - there are routing services for that (including OSRM or GraphHopper). Current changes look OK for me - see #3315.

Names are more delicate visually. I think we need more nuanced solution - different values for different type. For example local (residential) road labels are OK and should be not touched. They need to be dense, because these roads are short and should not be blank too much. But bigger (especially dual-carriage) roads are cluttered with names and are long enough to contain labels elsewhere. See #3318.

Mobile use is special. I would be happy if we could cover different uses, like for color-blinded or mobile, but they need separate solutions, because we have problem even for typical, desktop use and it's hard enough. And typical mobile use is not looking at OSM.org map, there are map apps like OSMAnd, Maps.me and many others. They are the proper solution for mobile in my opinion. They even have better routing interface included.

@SomeoneElseOSM
Copy link
Contributor

Is there even a way to test it on mobile before implementing it first? I don't think you can run a Docker environment on Android or IOS can you?

Well you can point a mobile browser (or Android emulator) at a machine showing that style. Even with Docker, you can use an ssh connection to a machine to access a port on an internal VM locally, and point an Android web browser on the same LAN at that.

@SomeoneElseOSM
Copy link
Contributor

for small screens (phones) it is important that the shields are often repeated

That would be true if phone displays had screens that had fewer pixels on them than desktop PCs, but I don't believe that's actually true. A relatively cheap Android phone will be full HD, but a cheap laptop will still be 1366x768. Also it ignores the way that tiles are displayed - by default with Osmdroid and Leaflet on the same phone you'll get very different scaling and the same map style will look very different.

@dieterdreist
Copy link

dieterdreist commented Aug 2, 2018 via email

@dieterdreist
Copy link

dieterdreist commented Aug 2, 2018 via email

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 3, 2018

Just to illustrate why shields are problem even outside the cities - German style shows them stronger and there are two versions of this style to compare currently (click to see the full scale):

Before

screenshot_2018-08-03 openstreetmap tileserver fraunhofer iosb

After

screenshot_2018-08-03 openstreetmap tileserver fraunhofer iosb 1

@bongobongo
Copy link

Great work!
Has anyone described how to implement this feature in Mapnik 2.2, so it will work when using e.g. generate_tiles.py?
If someone know which file(s) to change, and what code to add/change, to make this work in Mapnik 2.2, please let me know.

@SomeoneElseOSM
Copy link
Contributor

@bongobongo If you're not using OSM Carto, then you may be better off asking somewhere else (perhaps https://gis.stackexchange.com ) where the question is more likely to be seen by people running raw Mapnik.

Rather you than me trying to manually edit a mapnik stylesheet manually, though :)

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.

Don't render duplicate label/shield for connected ways with the same computed style
9 participants