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

Added text-repeat-distance for highway names #3318

Merged

Conversation

Penegal
Copy link
Contributor

@Penegal Penegal commented Jul 23, 2018

This PR adds text-repeat-distance for highways, which currently unnecessarily clutter the map on dense areas. It uses a smaller value of 50 for highway=unclassified and lower-importance highways, and 200 for higher-importance highways. This also achieves the split of #3295 in smaller PR as requested by @kocio-pl, and consequently closes #3111.

Before/after examples:
66666216877235_old_highway
66666216877235_new_highway
https://www.openstreetmap.org/#map=17/48.66397/6.17061

68452214932439_old_highway
68452214932439_new_highway
https://www.openstreetmap.org/#map=17/48.68110/6.14494

70195095380471_old_highway
70195095380471_new_highway
https://www.openstreetmap.org/#map=17/48.69825/6.18554

197890463702336_old_highway
197890463702336_new_highway
https://www.openstreetmap.org/#map=16/48.1898/6.4266

Edit: added links to test locations.

@Penegal
Copy link
Contributor Author

Penegal commented Jul 23, 2018

@kocio-pl: this is the last version I did for #3295; it comprises the value of 50 you asked for residential/unclassified roads, and the decrease of text-repeat-distance to 200 for more important highways.

These examples make me think that the current value of 200 is still too many repetitions of the name for important highways, as they change name or direction much less often than residential/unclassified ones; IMHO, 200 still displays them much more than is necessary for following such highways on the map. As you recommended 200, what do you think about this suggested increase?

@kocio-pl
Copy link
Collaborator

You need to fix the roads.mss conflict now.

@kocio-pl
Copy link
Collaborator

I did not ask for any change for residential roads - they look worse (incomplete) with this code:

Example place

Before
nqzfysln

7salmij4

@Penegal
Copy link
Contributor Author

Penegal commented Jul 29, 2018

Ok, I'll fix the residential roads with the conflict; what about the 200 value for more important roads?

@Penegal
Copy link
Contributor Author

Penegal commented Jul 29, 2018

Reverted the highway=residential code to the previous version.

@kocio-pl
Copy link
Collaborator

I will review this. What value would you propose for major roads?

I think every road type change needs separate consideration, at least one test rendering, because you never know and roads are important element of the map.

@Penegal
Copy link
Contributor Author

Penegal commented Jul 30, 2018

I'll do some tests to find a correct value. At first glance, I would say 600 as I remember I tested this value, but, since then, you pointed some elements to care about that I did not care about back then, so I'll see what the tests suggest.

BTW, good remark about testing the rendering of each road type; I would probably not have tested that.

@kocio-pl
Copy link
Collaborator

Great!

Good testing pattern is trying not only the value you think should be right, but also something "too low" and "too high". This way I was able to detect "switch" behavior in the previous code (every global value had exactly the same effect), it also hints if bigger or smaller value might be better than first guess.

@Penegal
Copy link
Contributor Author

Penegal commented Aug 1, 2018

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 1, 2018

Thanks, it will take me some time to review and test and I'm not sure if this PR should be maybe cut into separate PRs to make discussion more focused and easier to follow. What do you think about it?

@Penegal
Copy link
Contributor Author

Penegal commented Aug 1, 2018

I currently use one PR because the current code uses only 2 different values for text-repeat-distance; if the number of different values increases, or if there are still numerous issues, I'd fragment it, but I don't think it would be relevant if there are only one or two issues left. Are you OK with that?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 1, 2018

OK, we might try testing case by case in this PR.

@Penegal
Copy link
Contributor Author

Penegal commented Aug 27, 2018

@kocio-pl: were you able to test this?

@kocio-pl
Copy link
Collaborator

I plan to test it soon, however piece by piece, not all at once.

@matthijsmelissen
Copy link
Collaborator

@kocio-pl What is the status of this PR? If I read it correctly, it is still in your hands?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 7, 2018

Yes, it's just tedious to check and there was explosion of other PRs.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 17, 2018

I think these repeat distance values (500 for major and 100 for minor roads) are way too big, I would go with something 10x smaller. It's enough to avoid 2 carriage roads having the same labels in the same place, but nothing more. For smaller ones I don't like to make any special changes, they are basically OK for me.

Example of major roads - 2-carriage and multiple carriage way (extreme case)

current code

26apn9kz

distance 500

ee0caeys

distance 50

tfb9gqcu

Example of minor roads - multiple carriage way (extreme case)

current code

dddxxavs

distance 100

t2qup65n

distance 10

w95i6d59

@kocio-pl
Copy link
Collaborator

Example of minor roads - 2 carriage way

current code

xaolnfix

distance 100

w_cawz85

distance 10

tet5jrm1

@kocio-pl
Copy link
Collaborator

Example of a more typical major (2 carriage) and minor (1 carriage, small segments) roads

current code

b2xc5uwh

distances: major 500 + minor 100

dcmhbyiv

distances: major 50 + minor 10

bs0novnf

@kocio-pl
Copy link
Collaborator

@Penegal Could you update this PR? I would make some more tests to check if nothing is broken and if it will pass, I would merge it soon.

roads.mss Outdated
@@ -2984,9 +2994,10 @@ tertiary is rendered from z10 and is not included in osm_planet_roads. */
text-clip: false;
text-placement: line;
text-halo-radius: @standard-halo-radius;
[highway = 'living_street'] { text-halo-fill: @living-street-fill; }
[highway = 'living_street'] { text-halo-fill: @living-street-fill; text-repeat-distance: @major-highway-text-repeat-distance;}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that text-repeat-distance for living street should be in separate line, so the brackets should create a small inner block.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you fix this remaining issue? Than it would ready to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kocio-pl : done.

@Penegal
Copy link
Contributor Author

Penegal commented Nov 6, 2018

@kocio-pl : the code should now be as you asked; warn me if there are some problems left.

@kocio-pl kocio-pl merged commit 1419ef8 into gravitystorm:master Nov 6, 2018
@kocio-pl
Copy link
Collaborator

kocio-pl commented Nov 6, 2018

It looks good now, thanks a lot! I hope this will make street experience more pleasant - at least I'm happy with the output I tested.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Nov 9, 2018

Related to #497.

@geostonemarten
Copy link

Is it possible to ignore the label creation for footway if it contain footway=sidewalk because there is also the same name on major highway.
image

https://www.openstreetmap.org/#map=17/50.52778/1.58588

@SomeoneElseOSM
Copy link
Contributor

Is it possible to ignore the label creation for footway if it contain footway=sidewalk because there is also the same name on major highway.

That sounds like a question unrelated to this pull request, but something that achieves a similar effect is technically possible. Example from a related style that uses similar software components: https://github.com/SomeoneElseOSM/SomeoneElse-style/blob/master/style.lua#L974 .

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
5 participants