major rewrite of amenity-points #2597

nebulon42 · 2017-03-25T21:37:14Z

Fixes #234, #1640.

Improvements

more compact code, more DRY
labels are tied to icons with shields, so label without icon is impossible, a second pass of icon only fills up remaining space
harmonization to dy of 11 with a few exceptions (omitting text due to collision with icon cannot happen with shields)
landcover labelling code moved to landcover.mss
island, islet labelling code moved to placenames.mss
icon and label in one query possible, less complex label query
fixes most visible fountain rendering problems (see fountain rendering problems #1934) by using a SVG file instead of two overlapping markers on z17
features that use a POI icon but landcover labelling now use a text configuration similar to other POIs when the icon is shown, before that they use the landcover labelling

Caveats

Shields do not support SVG re-coloring. This requires us to have the SVGs in a specific color. But I think this is not a big deal, because re-coloring without shields is still possible and downstream customization is also still possible and not so difficult when using sed or similar

matthijsmelissen · 2017-03-26T19:18:26Z

I think this goes in the right direction. Let me know when it is ready for review.

HolgerJeromin · 2017-03-27T15:21:00Z

You could try my idea of using vertical-alignment=top (#2248)

nebulon42 · 2017-03-27T20:48:53Z

@HolgerJeromin I'm not sure if this would still be needed with shields as the position of the image does not affect the position of the text, so the icon cannot block the text.

HolgerJeromin · 2017-03-28T06:58:35Z

Yes, an icon cannot block the text, but we can have different distances between the icon bottom and text top. Large text can result in smaller distance than small text heights.

BTW shield-vertical-alignment-keyword differs here from the normal align that it defaults to middle with no influence of dy.

nebulon42 · 2017-03-28T18:24:37Z

Yes of course. It's a bit hidden because there are so many changes, but the reduction of dy for the generic shop is already there: https://github.com/nebulon42/openstreetmap-carto/blob/27bc13cc8f51f42c04abefa48ce08294725fe7f7/amenity-points.mss#L782.

nebulon42 · 2017-05-01T19:41:58Z

@math1985 This is now ready for review.
Actually it would benefit from as much review by different persons as it is able to get.

I have not touched the low-priority layer. And I have a few questions regarding it:

I don't really understand why parking related features are part of the low priority layer as they are part of the regular layers anyway. Was this meant as space-filling with icons? Then it can be removed now.
Are any features in this layer supposed to have text? If yes, there need to be changes to account for that.

pnorman

I focused on reviewing the output, not a line-by-line review, and this looks good.

matthijsmelissen

You're moving the coloring of the icons from the stylesheet to the SVGs. It was once a deliberate choice to do the colorings in the stylesheet, for easy customization. Would this rewrite also be possible without coloring the SVGs directly?

nebulon42 · 2017-05-05T18:40:33Z

Would this rewrite also be possible without coloring the SVGs directly?

No, unfortunately not. The ShieldSymbolizer does apparently not support SVG re-colouring otherwise I would have used it. It is not ideal, but as I wrote above customization is still possible if no shields are used and otherwise by altering the colours in the SVGs directly. This can also be automated by using sed or any other search and replace utility.

For me the improvements of the shield approach outweigh the downside of having the colours in the SVGs.

pnorman · 2017-05-06T00:35:40Z

For me the improvements of the shield approach outweigh the downside of having the colours in the SVGs.

👍

matthijsmelissen

Cool work! I'm still a bit sceptical if this is going to work out or not in the end though.

If I understand correctly, labels will now be given priority over icons. I'm not sure if that's an improvement. In Luxembourg, I have seen cases where, on z17, a single bank label is blocking four restaurant icons.

Some other issues:

Airport labels are not longer italic
Lowzoom airport icons are colored wrong

matthijsmelissen · 2017-05-06T09:50:14Z

amenity-points.mss

-    marker-file: url('symbols/shelter.svg');
+  [feature = 'tourism_alpine_hut'][zoom >= 14],
+  [feature = 'tourism_wilderness_hut'][zoom >= 14],
+  [feature = 'amenity_shelter'][zoom >= 17],


Did you mean to change the zoomlevels for alpie hut, wilderness hut and shelter?

It looks like if I did, but for z13 (alpine hut, wilderness hut) and z16 (shelter) the markers do not have text, so they are in the "markers without text" category.

matthijsmelissen · 2017-05-06T09:51:44Z

amenity-points.mss

+    shield-text-dy: 11;
+    shield-unlock-image: true;
+
+    // second pass for icons without text where there is still space


Nice approach!

matthijsmelissen · 2017-05-06T09:56:08Z

amenity-points.mss

+      shield-fill: @transportation-text;
+      shield-text-dy: 9;
+      a/marker-file: url('symbols/bus_stop.12.svg');
+      a/marker-fill: @transportation-icon;


Do we still need those marker-fill lines after recolouring the icon files?

Strictly speaking we don't but I wanted to keep those as we did have them before. But when I think about it it is redundant and probably should be removed. Should I?

matthijsmelissen · 2017-05-06T09:58:11Z

amenity-points.mss

+      a/marker-file: url('symbols/museum.svg');
+    }
+    [feature = 'man_made_mast'] {
+      shield-file: url('symbols/communications.svg');


Shouldn't this one has a shield-fill?

matthijsmelissen · 2017-05-06T10:00:39Z

amenity-points.mss

+      a/marker-file: url('symbols/windmill.svg');
+    }
+    [feature = 'amenity_hunting_stand'] {
+      shield-file: url('symbols/hunting_stand.svg');


Missing shield-fill? If so, could you please double-check all of them?

matthijsmelissen · 2017-05-06T10:01:53Z

amenity-points.mss

  }
+  [feature  ='amenity_fountain'][zoom >= 17] {


Whitespace typo

matthijsmelissen · 2017-05-06T10:03:48Z

amenity-points.mss

  }
+  [feature  ='amenity_fountain'][zoom >= 17] {


I probably miss something, why can we not handle fountain in the generic way?

The problem was the layering of nozzle and pool. It required marker-allow-overlap to display both layered above each other. The downside to this approach is that it also allows other markers to overlap with them which happened frequently. (see #1934)

matthijsmelissen · 2017-05-06T10:05:48Z

amenity-points.mss

+    }
+  }
+
+  // markers without text


I strongly dislike the duplication here, can we not solve this differently?

I'm not sure how a shield without text behaves and we would still have the text-dy which probably takes up space even if name is empty. I'm also not sure if a shield symbolizer uses more resources so it might be not good to use it unnecessarily.

But I'm open to other ideas.

Another option would be to solve this cartographically by not having POI icons where their label shows up at later zoom level. But I would be against that.

matthijsmelissen · 2017-05-06T11:13:50Z

project.mml

@@ -1646,6 +1646,8 @@ Layer:
                               'laundry', 'dry_cleaning', 'beverages', 'perfumery', 'cosmetics', 'variety_store', 'wine', 'outdoor', 
                               'copyshop', 'sports', 'deli', 'tobacco', 'art', 'tea', 'coffee') THEN shop
                               ELSE 'other' END AS shop,
+            name,


The diff of the changes in this file is a bit hard to read, what do we change exactly in this file?

Is it still necessary to have a separate text layer in addition to the amenity-points layer?

I added the name attribute (and other necessary ones for text rendering) to the amenity-points queries. At the same time I removed the now unnecessary parts of the text layer queries. Those are still needed, because there are other text features (e.g. water related) that also use the queries.

matthijsmelissen · 2017-05-07T18:07:37Z

Another strange issue:

nebulon42 · 2017-05-07T19:02:28Z

I have adressed the points raised:

checked color and added shield-fill
ice cream fix
whitespace fix
airport font fix

Lowzoom airport icons are colored wrong

I did not see this. Could you show an example?

Thanks both for the thorough review.

If I understand correctly, labels will now be given priority over icons. I'm not sure if that's an improvement. In Luxembourg, I have seen cases where, on z17, a single bank label is blocking four restaurant icons.

To be a bit more specific labelled POI or those without rendered label now come first and then the remaining space is filled with icons where possible. But the rules are now more straightforward, before it was rather arbitrary.

z17 is still quite crowded in cities even after your shop rendering change. Another thing I could think of would be to penalize long names somehow so that too long names do not block too many icons. But this was just an idea, I would have to think about how to do that.

imagico · 2017-05-07T19:21:18Z

I have not had time to test this but am i right to assume that this change does not solve the long time problem of symbols frequently vanishing and later re-appearing as you zoom in because symbol priorities are not strictly in order of the starting zoom level?

If this is not the case is this change at least neutral in that regard or does it make this problem worse?

nebulon42 · 2017-05-07T19:52:58Z

It definitely does not solve the problem and I would expect that this change is neutral towards this.
Am I right that this happens mostly at metatile boundaries?

imagico · 2017-05-07T19:57:26Z

No, this has nothing to do with metatiles.

The problem occurs if at a certain zoom level new symbols start to be shown - and are rendered with higher priority than symbols that already show at a lower zoom level.

nebulon42 · 2017-05-07T20:02:52Z

Yes, so I thought sorting the rules by lowest zoom level would help but this was not the case for a instance of this problem. The only places where I found vanishing symbols in my test rendering were metatile boundaries. But since my Kosmtik rendering suffers from cut-off things at these boundaries anyway this might be something else. I thought if the buffer overlap is not sufficient collisions there are more likely.

pnorman · 2017-05-08T08:51:11Z

Vanishing symbols with zoom variations should happen when either there's a change in label collisions interactions, or you have two things which collide and on the one rendered at lower zooms appears later in the query results at higher zooms.

Because I don't think we order the amenity queries by tagging at all, this PR is probably no change to that.

pnorman · 2017-05-10T07:08:19Z

As this has cartography changes, this PR is currently held up by the feature freeze until 4.0.0 has been released.

matthijsmelissen · 2017-05-10T21:05:07Z

@nebulon42 Can you give some example before/after renderings of a densely mapped city in your area? It might help us to determine whether the disappearance of the label-without-icon-problem offsets the lesser amount of shops shown.

nebulon42 · 2017-05-20T17:04:43Z

Previews of Vienna (inner city).

before

z17

z18

after

z17

z18

…s, a second pass of only icons fills up free space, dy was harmonized, restructuring of code to avoid repetition, fixes gravitystorm#234

nebulon42 · 2017-05-25T14:45:22Z

Since we now require carto 0.18 I have updated this to use a feature of carto >= 0.17.2:
https://github.com/gravitystorm/openstreetmap-carto/pull/2597/files#diff-5d9237083037cf1d6d91c329a0ff61adR811

Changes

pnorman

Because this can't be backported to 3.x with the use of none, it can't be merged until at least two MINOR releases have occured.

nebulon42 · 2017-05-25T17:34:13Z

@pnorman Yes, I know. But it can be manually backported without using this feature with minimal effort. I'm willing to do this once merged to master.

matthijsmelissen · 2017-05-25T20:09:58Z

Thanks for the previews!

Unfortunately, in the new rendering quite a lot of information is lost by icons that are blocked by labels and cannot be rendered. This plays especially on z18 (mainly in the second example) but also in z17 (for example the Jewish museum).

The main problem this is solving, labels without icons, seems quite rare. If I'm not mistaken, we have no single instance of this happening in the previews.

I therefore think this PR is not an improvement - even though it's very cool from a technical perspective.

nebulon42 · 2017-05-28T08:32:57Z

There are definitely changes in labelling and some labels block other icons. But I would not use "a lot of information".

One way to tackle this could be to introduce a length limit for labels. When this is exceeded the process is reversed and first icons without label are placed and after that icons with label. But probably this gets to complex and is not worth it.

It remains to be decided if the gain in consistency and reduction of complexity is worth those changes in rendering.

This uses the same technique as gravitystorm/openstreetmap-carto#2597 of a shield symbolizer to place icons+labels, then a marker symbolizer to try just labels. This ensures that there is never text without labels, and textless labels can fill in empty room.

nebulon42 · 2017-07-05T20:22:47Z

Before I'm going through the hassle of rebasing and resolving the conflicts I'd like to sort out the question if it is worth to go on or not. @math1985 I respect your opinion, but I'd like to hear from other maintainers what they think about the problem with icons blocked by labels. Apparently @pnorman used the technique in another project so I'm curious what he thinks about that.

pnorman · 2017-07-07T00:45:42Z

Apparently @pnorman used the technique in another project so I'm curious what he thinks about that.

It works well, and it's the best technique I know of. The limitations I found were

duplication with needing to specify a shield-file and marker-file that are the same and
having to adjust the colour in the SVGs, meaning I couldn't use icons from our symbol source without first modifying them, and any colour changes require changing a bunch of SVGs.

The only overall improvement I'd make is to use a name like fallback instead of a for the marker symbolizer, since it makes it clearer what it's for.

I'd be okay merging it, except I'd have to resolve the conflicts, and handle the backporting to 3.x.

matthijsmelissen · 2017-07-07T01:05:07Z

@pnoeman You're aware of the drawbacks I pointed out right?

matthijsmelissen · 2017-07-07T01:05:30Z

@pnorman You're aware of the drawbacks I pointed out right?

pnorman · 2017-07-07T01:12:35Z

@pnorman You're aware of the drawbacks I pointed out right?

I had missed those.

Unfortunately, in the new rendering quite a lot of information is lost by icons that are blocked by labels and cannot be rendered. This plays especially on z18 (mainly in the second example) but also in z17 (for example the Jewish museum).

I'm just not seeing this when I look at the previews. I see different stuff being rendered, but no overwhelming trends. Perhaps there's some adjustments we can make to the margins to allow labels to be closer?

nebulon42 · 2017-07-31T18:55:58Z

I'm tired of this.

matthijsmelissen · 2017-07-31T19:09:21Z

What are you tired of exactly? If it's about my comments, you have my full blessing to go ahead with this if that's what you think is best for the project.

matthijsmelissen · 2017-07-31T19:10:40Z

Also, is there anything we can help you with?

nebulon42 · 2017-07-31T19:36:34Z

:) It's just that I would have had to resolve the merge conflicts and I saw what has changed and it was just a mess. So I decided not to do it, I do not have the energy to keep this up to date with master.
Better to let it be. I'm not tired of persons, just to be clear.

sommerluk mentioned this pull request Mar 26, 2017

Typography for amenity-points.mss #2600

Closed

nebulon42 mentioned this pull request Mar 29, 2017

Tie POI names to icons #234

Open

sommerluk mentioned this pull request May 1, 2017

Prepare for amenity linespacing control #2624

Closed

nebulon42 force-pushed the poi-shields branch 2 times, most recently from a8fdcac to fdb6d26 Compare May 1, 2017 19:22

nebulon42 changed the title ~~[WIP] major rewrite of amenity-points~~ major rewrite of amenity-points May 1, 2017

pnorman previously approved these changes May 2, 2017

View reviewed changes

matthijsmelissen requested changes May 4, 2017

View reviewed changes

matthijsmelissen requested changes May 6, 2017

View reviewed changes

nebulon42 force-pushed the poi-shields branch 2 times, most recently from f15fcb1 to 92e3014 Compare May 7, 2017 18:53

nebulon42 force-pushed the poi-shields branch 2 times, most recently from 60135b5 to 9f9c332 Compare May 20, 2017 17:00

major rewrite of amenity-points, labels are tied to icons with shield…

94ece39

…s, a second pass of only icons fills up free space, dy was harmonized, restructuring of code to avoid repetition, fixes gravitystorm#234

nebulon42 force-pushed the poi-shields branch from 9f9c332 to 94ece39 Compare May 25, 2017 14:42

pnorman requested changes May 25, 2017

View reviewed changes

pnorman approved these changes Jul 7, 2017

View reviewed changes

nebulon42 closed this Jul 31, 2017

nebulon42 mentioned this pull request Aug 16, 2017

Project governing status #2436

Closed

imagico mentioned this pull request May 8, 2019

Too many icons #3635

Open

jeisenbe mentioned this pull request Oct 8, 2019

Give more priority to museums #3924

Open

major rewrite of amenity-points #2597

major rewrite of amenity-points #2597

Conversation

nebulon42 commented Mar 25, 2017 • edited Loading

matthijsmelissen commented Mar 26, 2017

HolgerJeromin commented Mar 27, 2017

nebulon42 commented Mar 27, 2017

HolgerJeromin commented Mar 28, 2017

nebulon42 commented Mar 28, 2017

nebulon42 commented May 1, 2017

pnorman left a comment

Choose a reason for hiding this comment

matthijsmelissen left a comment

Choose a reason for hiding this comment

nebulon42 commented May 5, 2017

pnorman commented May 6, 2017

matthijsmelissen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthijsmelissen commented May 7, 2017

nebulon42 commented May 7, 2017

imagico commented May 7, 2017

nebulon42 commented May 7, 2017

imagico commented May 7, 2017

nebulon42 commented May 7, 2017

pnorman commented May 8, 2017

pnorman commented May 10, 2017

matthijsmelissen commented May 10, 2017

nebulon42 commented May 20, 2017

nebulon42 commented May 25, 2017

pnorman left a comment

Choose a reason for hiding this comment

nebulon42 commented May 25, 2017

matthijsmelissen commented May 25, 2017

nebulon42 commented May 28, 2017

nebulon42 commented Jul 5, 2017

pnorman commented Jul 7, 2017

matthijsmelissen commented Jul 7, 2017

matthijsmelissen commented Jul 7, 2017

pnorman commented Jul 7, 2017 • edited Loading

nebulon42 commented Jul 31, 2017

matthijsmelissen commented Jul 31, 2017

matthijsmelissen commented Jul 31, 2017

nebulon42 commented Jul 31, 2017

nebulon42 commented Mar 25, 2017 •

edited

Loading

pnorman commented Jul 7, 2017 •

edited

Loading