gravitystorm / openstreetmap-carto

A general-purpose OpenStreetMap mapnik style, in CartoCSS
Other
1.53k stars 817 forks source link

Symbol and label prioritization is not in sync with the starting zoom levels #3880

Open imagico opened 4 years ago

imagico commented 4 years ago

Currently the drawing order of point symbols and labels (layers amenity-points and text-point) but also of other text layers is not in sync with the starting zoom levels as defined by the MSS code in amenity-points.mss. This leads to in some cases symbols and labels disappearing as you zoom because other symbols and labels appear at a later zoom level but with higher priority.

This is related to #234.

The superficial solution would be to manually work through these layers and adjust the drawing order to match the order of starting zoom levels. This is however very maintenance intensive. The alternative would be to define the starting zoom levels in SQL and use that for starting zoom level selection rather than MSS logic.

This would also have the advantage it could easily be adapted to dynamic starting zoom levels based on continuous importance rating and local map scale/latitude.

Neither of these solutions would of course affect the same problem when to comes due to labels and symbols from different layers competing with each other.

Examples:

https://www.openstreetmap.org/node/59646817#map=16/47.9973/7.8364 https://www.openstreetmap.org/node/59646817#map=17/47.99652/7.83686

https://www.openstreetmap.org/node/3913744216#map=18/47.99380/7.85089 https://www.openstreetmap.org/node/3913744216#map=19/47.99384/7.85109

jeisenbe commented 4 years ago

I agree that at high zoom levels, icons that appear at z17 should not disappear at z18 or z19 because another feature begins to render at the higher zoom levels.

For very large features, it sometimes makes sense for the text label to disappear when the feature begins to take up most of the screen. This was implemented for landcover layer features and related text labels in , and also is relevant for airports and some other very large features that are rendered with icons and text.

One issue currently is with amenity=parking which is rendered in the low-priority layer, but starts rendering at z10. Even if this was changed to z14, it might not be a good idea to prioritize parking icons or text labels over other features which appear at z15 or later (hospitals, place_of_worship, supermarkets, etc). So there may be situations like this were strictly following a rule of "first in rendering, first in priority" would not be ideal.

sommerluk commented 4 years ago

The big question is how the MSS code should look like.

Example:

#amenity-points[zoom >= 10][int_startzoom <= 10],
#amenity-points[zoom >= 11][int_startzoom <= 11],
#amenity-points[zoom >= 12][int_startzoom <= 12],
#amenity-points[zoom >= 13][int_startzoom <= 13],
#amenity-points[zoom >= 14][int_startzoom <= 14],
#amenity-points[zoom >= 15][int_startzoom <= 15],
#amenity-points[zoom >= 16][int_startzoom <= 16],
#amenity-points[zoom >= 17][int_startzoom <= 17],
#amenity-points[zoom >= 18][int_startzoom <= 18],
#amenity-points[zoom >= 19][int_startzoom <= 19],
#amenity-points[zoom >= 20] {
  // Here comes all the existing icon/label code
}

This would work. But it would increase the size of the Carto-generated XML file from 3 MB to about 6 MB for the hole style (just a quick test, not an exact measurement). Mainly because zoom filters are different from “normal” data filters like here “int_startzoom”. For each zoom level CartoCSS has to generate an own set of XML rules. So you get many duplicate rules that will never be used (for example for shops on z10, z11, z12, z13, z14, z15, z16) because at these levels shops do not render.

It would be obviously better if we could use the current zoom level to compare against the SQL-generated int_startzoom, but I'm not sure render-time variables like @zoom will be reliable, see also here.

imagico commented 4 years ago

My idea was actually not to have any zoom level based selection in MSS code but to select per zoom level in SQL using the Z(!scale_denominator!) (https://github.com/imagico/osm-carto-alternative-colors/blob/master/z.sql). This way the MSS code would only contain selection of symbol/label types in a flat fashion but no zoom level logic.

sommerluk commented 4 years ago

Okay, got it. But wouldn't this be a problem with vector tiles? If I understand correctly vector tiles will be produced only up to z14 (or z15?) and everything above is rendered from z14 tiles. (I havno experience with vector tiles.)

jeisenbe commented 4 years ago

@imagico - do you have an example of how this would work? It doesn't seem to have been implemented in https://github.com/imagico/osm-carto-alternative-colors yet?

imagico commented 4 years ago

@sommerluk - i don't see a fundamental problem here - as @jeisenbe illustrated you could have a kind of startzoom attribute assigned to the features, which can either be an integer or even a fractional zoom level, and could use that for rendering decisions. It is just in our current setup where the data is queried from the database separately for every zoom it is much easier and more efficient to query only the features needed for every zoom level.

There are different ways to implement this - you can either write this directly in SQL in project.mml or you could have a yaml file with the different POI types and their starting zoom levels and either auto-generate an SQL function from that which matches these with a large CASE statement (similar to https://github.com/imagico/osm-carto-alternative-colors/blob/master/line-widths-generated.sql) or generate a separate postgres table with these once similar to indexes.sql.

jeisenbe commented 4 years ago

See #3635 which is related to this issue.

jeisenbe commented 4 years ago

I'm trying to figure out how to implement this:

My idea was actually not to have any zoom level based selection in MSS code but to select per zoom level in SQL using the Z(!scale_denominator!) (https://github.com/imagico/osm-carto-alternative-colors/blob/master/z.sql). This way the MSS code would only contain selection of symbol/label types in a flat fashion but no zoom level logic.

you can either write this directly in SQL in project.mml or you could have a yaml file...

I think it would be best to start with SQL in project.mml directly, since it will be clearer how things work and we can make changes. Is there an example of this somewhere?

Perhaps we could start by testing with amenity-low-priority first, since this would be very simple?

imagico commented 4 years ago

You would do something like

SELECT
  ...
  FROM
  (SELECT
    ...
    CASE
      WHEN highway = 'mini_roundabout' THEN 17
      WHEN railway = 'level_crossing' THEN 14
      WHEN railway = 'crossing' THEN 15
      WHEN amenity = 'bench' THEN 19
      WHEN amenity = 'waste_basket' THEN 19
      WHEN amenity = 'waste_disposal' THEN 19
      WHEN man_made = 'cross' THEN 16
      WHEN historic = 'wayside_cross' THEN 16
      WHEN historic = 'wayside_shrine' THEN 17
      WHEN barrier IN ('bollard', 'gate', 'lift_gate', 'swing_gate', 'block', 'log', 'cattle_grid', 'stile', 'motorcycle_barrier', 'cycle_barrier', 'full-height_turnstile', 'turnstile', 'kissing_gate') THEN 17
    END AS start_zoom
    FROM
      ...
  ) AS _
  WHERE start_zoom >= z(!scale_denominator!)
    AND ...
  ORDER BY start_zoom ASC
jeisenbe commented 4 years ago

How do I add z() to PostGIS?

"In addition you need to install the z() function in PostGIS which is supplied in z.sql"

I've added z.sql to my repository but get the error:

Postgis Plugin: ERROR: function z(numeric) does not exist ... HINT: No function matches the given name and argument types. You might need to add explicit type casts

imagico commented 4 years ago

You need to run z.sql in postgis on the osm database to install the function.

jeisenbe commented 4 years ago

Explain it to me step-by-step, please: the “For Dummies” version.

I am using the docker container set-up.

paulgillard commented 4 years ago

@jeisenbe I believe it's a case of using the 'psql' command in a terminal window to run the statement contained in z.sql on the Postgres installation itself. Something like:

psql -d <insert-database-name> -c 'create or replace function z (numeric) returns integer language sql immutable returns null on null input as $func$ select case when $1 > 600000000 or $1 = 0 then null else cast (round(log(2,559082264.028/$1)) as integer) end; $func$;'

imagico commented 4 years ago

Also related to #1026 and #964.

The building-text layer is also part of this problem because it starts at z14 but has lower priority than the POI symbols - many of which only start later. This leads to for example for a hotel the name appearing first as a building name (if large enough) and then being replaced by a hotel label as you zoom in.

eehpcm commented 4 years ago

From my viewpoint, this can occasionally be a feature not a bug.

At z=18 the cafe's icon and name are visible. At z=19 the cafe's name has gone, but now you can see that there is an outdoor seating area.

imagico commented 4 years ago

Symbols vanishing as you zoom in can be a deliberate and useful choice but never the vanishing and reappearance later - which is a common effect with the non-consistent prioritization. Also typically you would - even if you choose to have a symbol vanish - continue rendering the feature in question in a different form - using something other than a symbol - maybe a label only - or a geometry using a drawing signature classifying it as what it is.

Anyway - this issue is about the prioritization being non-consistent and arbitrary. To deliberately have symbols vanish under certain conditions would likewise depend on first having a consistent prioritization to then deliberately adjust it as such.

imagico commented 2 years ago

By the way - i found an impressive example demonstrating this problem:

https://www.openstreetmap.org/#map=19/48.00981/7.90126

imagico commented 1 year ago

4734 showed additional problems when dynamic line labeling is involved. Case example shown was https://www.openstreetmap.org/way/510921128. Any change to address this problem will need to be checked against such cases.

imagico commented 1 year ago

I have now demonstrated and discussed an example of how this issue can be addressed on

https://imagico.de/blog/en/competing-priorities-symbols-and-labels-in-rule-based-map-rendering/

This deals with a lot of additional things and not only the prioritization issue, but it should give some hints and ideas to anyone who wants to work on this here.

imagico commented 7 months ago

To clarify - because that seems to be occasionally misunderstood:

This issue is of high importance to fix, and it should be done in a way that scales well in light of the 250+ different symbols we render. Manually syncing the starting zoom levels defined in MSS with an equally hand written prioritization logic in SQL is not a viable way to do that IMO (see #4836).

That does not mean we need to wait with fixing any of the concrete prioritization problems we have until this is addressed. We have other related issues like #4631 and #4676 that can be fixed independently of this (though there would of course be a benefit of addressing them together) and introducing an ability to explicitly define order of prioritization for specific feature types would be possible as well in that context.

My impression is that solving this issue is right now hampered by two things:

What i can offer in that regard is that i look at any proposal how to address this and assess the suitability and if i would be in support of such an approach here.

See also #4901.

dch0ph commented 6 months ago

I've been experimenting with an approach summarised in this gist.

It started as an experiment to see whether text-dy could be automatically adjusted depending on whether a POI was rendered with a symbol or a marker. But once you've introduced a table for POI information, then you might as well see how the prioritisation issue might be addressed.

I'm only testing this on a personal style, hence the rendering of craft=* as a testbed.

As already stated, overhauling all of amenity-points.mss will be a huge job, and is likely to (should?) lead to some differences in behaviour. Realistically target a new major version?

imagico commented 6 months ago

I am not quite sure if i understand where your sketch is meant to go.

You seem to re-frame the feature column into a coarser classification, which requires you to have two big CASE statements in the SQL code in mml - containing the translation from tag to POIDefaults.feature_class and from POIDefaults.feature_class to POITable.feature_class.

You have some styling data (starting zoom levels, SVG file names) in SQL code while other styling data (text parameters, but also way_pixels thresholds - which change the starting zoom levels) in MSS.

Given the complexity of the layer you need to take care of avoiding combinatorial explosion in the CartoCSS to Mapnik translation. This is why we work with a feature column and not with key and value. Andy has explained this in detail in his early OSM-Carto talks (like https://www.youtube.com/watch?v=RgSye4IbnD8).

In principle the idea of creating database tables and filling them with styling data that is necessary on the SQL level (for priority ordering) is a viable approach. But i would not recommend trying to design these tables in a way that designers are meant to directly edit SQL code for filling these tables. That is fairly awkward in terms of style writing ergonomics and it requires you to compromise between optimizing for human editing and optimizing for efficiency in use. The more recommendable approach is to have a script fill the table from styling data provided in a YAML file.

The other big principal question you would need to decide is if you want to have the MSS code for the amenity-points layer manually maintained or script generated as well. There are pros and cons for both.

But as you say - amenity-points.mss is a rather big and messy file (with various errors i might add - so yes, a major rewrite will realistically involve differences in styling logic in some cases) so whatever approach we would take here would need to be evaluated for scalability and maintainability. More generally speaking: IMO a solution for this should be in particular designed with the usability for the map designer in mind.

All of this is just my personal perspective on this though - other maintainers might have a different view on this. In light of this my recommendation would be not to invest too much work in writing production code at this stage but work on an overall concept and see if that can find consensus support from the maintainers.

Realistically target a new major version?

Technically a new major version is needed only with a change of database schema or the software versions required. But this is not really a question we need to look at at this stage.

dch0ph commented 6 months ago

You seem to re-frame the feature column into a coarser classification, which requires you to have two big CASE statements in the SQL code in mml - containing the translation from tag to POIDefaults.feature_class and from POIDefaults.feature_class to POITable.feature_class.

Not quite. The big primary CASE which selects the features for rendering is retained (with a bit of rationalisation in order to group related items together). The second CASE is limited to pulling out the relevant primary key:

            CASE
              WHEN features.feature = 'craft' THEN craft
              WHEN features.feature = 'office' THEN office
              WHEN features.feature = 'naturalarea' THEN "natural"
            END = POITable.feature_value

and so should be quite limited. It would be great to get rid of this, but I haven't seen a way to retain both feature and value as keys and feature_value as a combined key from the "primary CASE".

You have some styling data (starting zoom levels, SVG file names) in SQL code while other styling data (text parameters, but also way_pixels thresholds - which change the starting zoom levels) in MSS.

All the starting zoom information is in the Postgres table(s), which it is necessary for the correct prioritisation. The SVG pass through doesn't need to be included, but it seems a nice way to simplify the MSS and mapnik XML if you're going to the trouble of creating a POI table. It seems you don't need a separate filter for every symbol, which would remove a lot of repetition from amenity-points.mss! That aspect could be removed, but (without changing the current logic) you need to be able to set a zoom level for some key_value combinations, e.g. natural=scree has different start zoom (9) to most other "natural areas".

Introducing a POI database is always going to split where information is placed. I guess this is why there are different opinions?

Given the complexity of the layer you need to take care of avoiding combinatorial explosion in the CartoCSS to Mapnik translation. This is why we work with a feature column and not with key and value. Andy has explained this in detail in his early OSM-Carto talks (like https://www.youtube.com/watch?v=RgSye4IbnD8).

I've looked into this, and the number of mapnik rules is not increased; you still still have one rule for a given styling:

    <Rule>
      <MaxScaleDenominator>750000</MaxScaleDenominator>
      <Filter><![CDATA[([feature] = 'naturalarea') and ([natural] = 'grassland') and ([way_pixels] > 30000)]]></Filter>
      <TextSymbolizer fill="#4d7c20" fontset-name="fontset-2" halo-fill="rgba(255, 255, 255, 0.75)" halo-radius="1.1" line-spacing="-1.95" size="13" wrap-width="39"><![CDATA[[name]]]></TextSymbolizer>
    </Rule>

i.e. [featureclass = 'X'][value = 'Y'] doesn't seem to create more rules than [feature = 'X_Y']. The problems seem to come if you have >1 qualifier within the overall "headline selection", with qualifiers like [surface = 'grass'] and ['designation' = 'public_footpath'] creating all possible combinations.

But i would not recommend trying to design these tables in a way that designers are meant to directly edit SQL code for filling these tables. [...] The more recommendable approach is to have a script fill the table from styling data provided in a YAML file.

Agreed. This is just testing ideas. It would be perfect sense to generate the Postgres tables in a similar way to indexes.sql.

The other big principal question you would need to decide is if you want to have the MSS code for the amenity-points layer manually maintained or script generated as well. There are pros and cons for both.

I think creating a POI database and simplifying / rationalising the MSS would be a first step. It would be easier to judge the usefulness of auto-generating amenity-points.mss once it was reduced in size. Frankly it's too much of a mess as things stand to understand which option would work best.

so whatever approach we would take here would need to be evaluated for scalability and maintainability. More generally speaking: IMO a solution for this should be in particular designed with the usability for the map designer in mind.

Absolutely. The current model is only suitable for the "map tinkerer" adding the odd POI. It's very hard to design when it's so difficult to get an overview. Realistically some kind of POI database is needed to bring some order.

In light of this my recommendation would be not to invest too much work in writing production code at this stage but work on an overall concept and see if that can find consensus support from the maintainers.

Indeed. This is "production code" in the sense that I'm using it on an experimental branch, but it was posted as a gist for comment / sharing rather than rushing to a PR on the parent project.

Technically a new major version is needed only with a change of database schema or the software versions required. But this is not really a question we need to look at at this stage.

I suspect that as a PR on the master branch, there would be extreme nervousness about a wholesale rewrite of amenity points. A bleeding edge branch with maintenance on 5.8.X until folks were happy with the direction of travel might provide a smoother route.

imagico commented 6 months ago

All the starting zoom information is in the Postgres table(s),

No, according to your sketch it is not - as i have already indicated. With the polygon size dependent labeling of landcover and water polygons the starting zoom level depends on the size - large lakes get labeled earlier than small ones. And there are other POI types where the starting zoom level depends on secondary tags. You could flatten those out into different feature types - but then you quite definitely would need to script generate the code for that because of the large number of feature types this generates.

For determining the starting zoom levels based on way_pixels thresholds you'd need something like

https://github.com/imagico/osm-carto-alternative-colors/blob/d39d061ec793d79c5abe602287fadb79b237b615/sql/map_functions.sql#L52-L59

Absolutely. The current model is only suitable for the "map tinkerer" adding the odd POI.

To be clear: The current structure of amenity-points rendering is fairly designer friendly in principle (in terms of the complexity of the work needed to make changes). The main problems are that

I suspect that as a PR on the master branch, there would be extreme nervousness about a wholesale rewrite of amenity points. A bleeding edge branch with maintenance on 5.8.X until folks were happy with the direction of travel might provide a smoother route.

The route i see forward here is the following

The decision on if to maintain a separate long term branch would be decided in the second step. Since this would be a fairly well compartmentalized project affecting only two layers of the style i see less need for that than for other bigger changes in the past.

I would not worry too much about the bulk work of converting the logic of amenity-points.mss in a different form while maintaining functional equivalence. This is somewhat tedious work but if a suitable framework has been selected to implement this logic that is ultimately not a big deal.

dch0ph commented 6 months ago

With the polygon size dependent labeling of landcover and water polygons the starting zoom level depends on the size - large lakes get labeled earlier than small ones.

 [feature = 'natural_water'],
  [feature = 'waterway_dock'] {
    [zoom >= 5][way_pixels > 3000][way_pixels <= 768000],
    [zoom >= 17][way_pixels <= 768000] {
      text-name: "[name]";
      text-size: 10;
[...]

As far as I could tell, the starting zoom usually just depends on a single way_pixel cutoff? [Here also with a max cutoff, which could apply more generally?]. Most of the size dependent logic is styling of the text size.

And there are other POI types where the starting zoom level depends on secondary tags.

From a quick scan, height and telescope:diameter are secondary tags used as "size qualifiers". Personally I find it counter-intuitive for man_made=mast to start anywhere between Z14 and Z18 depending on a tag that is only present on ~20% of masts. [Playing with the starting zoom (rather than changing symbol) seems a poor way to indicate height, as well as creating a mapnik XML combinatorial explosion.] If keeping this logic, you could split into "majormast" and "minormast", starting at Z14 and Z16/17, and handle the remaining zoom start levels division in MSS (as below).

memorial andcastle_type are the other ones I've found. I'll think about whether logic for a secondary tag could be easily added, but with something like:

 [feature = 'historic_castle'][castle_type != 'stately'][castle_type != 'manor'][zoom >= 15],
  [feature = 'historic_castle'][castle_type = 'stately'][zoom >= 16],
  [feature = 'historic_castle'][castle_type = 'manor'][zoom >= 16],

you could again set the start_zoom to 15, and sort out the rest in MSS:

 [feature = 'historic_castle'][castle_type != 'stately'][castle_type != 'manor'],
  [feature = 'historic_castle'][castle_type = 'stately'][zoom >= 16],
  [feature = 'historic_castle'][castle_type = 'manor'][zoom >= 16],

Different castle types wouldn't be quite prioritised correctly against other Z16 POIs, but I doubt that anybody would notice!

So you don't need to reproduce all of the zoom logic in SQL, although it might ultimately be desirable not to have zoom logic in multiple places.

imagico commented 6 months ago

As far as I could tell, the starting zoom usually just depends on a single way_pixel cutoff?

For the landcover and water polygon labels that is correct. way_pixels of course depends on the zoom level (through scale_denominator) - so a way_pixels cutoff indirectly defines a starting zoom level (as a function of way_area - according to the formula i linked to).

Different castle types wouldn't be quite prioritised correctly against other Z16 POIs, but I doubt that anybody would notice!

Well - this issue calls for prioritizing by actual starting zoom level, not by some other manually defined importance scoring. I also don't see how introducing an additional, less distinctly defined scoring would make things any easier.

dch0ph commented 6 months ago

so a way_pixels cutoff indirectly defines a starting zoom level

So as far as I can tell, the only parameters determining starting zoom level are area in the form of way_pixels or tagging (including secondary tags). Indeed, it's hard to see what would not be covered by these two!

I can see a way to do this more neatly. I'll test out a version 2.

P.S. It was interesting to see from Andy Allan's presentation what OSMCarto looked like in the early days. It's certainly looking a lot better!

dch0ph commented 6 months ago

Version 2 here.

This is much cleaner. The first version tried to group things to reduce the number of database rows. But this is not relevant given that the plan would be to generate the POI database table via a YAML config file.

This version also demonstrates how zoom levels affected by secondary tags could be handled, as well as handling (or not) of catch alls.

imagico commented 6 months ago

As mentioned before i don't really see the reasoning behind the complex data structures you generate. All that is needed to solve this issue is:

The difficulty with that is not the need to develop come complex data structures and algorithms, it is to design this in a designer-friendly way that scales well with style complexity.

If you want to go beyond that (like by script generating/parameterizing MSS code) that is fine. But do not underestimate the challenges involved in that.

Independent of that - your sketch so far still fails to actually determine the starting zoom level for features where this depends on way_area. For you the starting zoom level for natural=wood labels for example is either z8 or z17 (and is not constant but varies with the zoom level). That is not correct. I already mentioned the formula you would need to use here.

dch0ph commented 6 months ago

your sketch so far still fails to actually determine the starting zoom level for features where this depends on way_area. For you the starting zoom level for natural=wood labels for example is either z8 or z17 (and is not constant but varies with the zoom level). That is not correct`

The starting zoom level function for selection works as present:

[feature = 'natural_wood'] {
    [zoom >= 8][way_pixels > 3000][is_building = 'no'],
    [zoom >= 17] {

i.e. the feature will be selected and shown once the way_pixels exceeds the given threshold. I agree that once it has reached this threshold it is given a fixed start_zoom / priority of 8, but I wasn't aware of issues where the prioritisation of large land cover labels was necessary. I've been focussed on symbol prioritisation at high zoom (e.g. shelter vs. bus stop).

I agree that it would be more elegant to also get rid of way_pixels, not least because you could imagine calculating the start zoom for each POI once and storing this value.

But given the amount of cold water dropped on these efforts so far, I'm not motivated to continue to work on this problem.

imagico commented 6 months ago

your sketch so far still fails to actually determine the starting zoom level for features where this depends on way_area. For you the starting zoom level for natural=wood labels for example is either z8 or z17 (and is not constant but varies with the zoom level). That is not correct`

The starting zoom level function for selection works as present:

[feature = 'natural_wood'] {
    [zoom >= 8][way_pixels > 3000][is_building = 'no'],
    [zoom >= 17] {

i.e. the feature will be selected and shown once the way_pixels exceeds the given threshold.

There might have been a misunderstanding here due to my formulation of this issue. When i am talking about the starting zoom level that refers to the first zoom level at which a symbol/label actually occurs.

But given the amount of cold water dropped on these efforts so far, I'm not motivated to continue to work on this problem.

The reason why i provided critical comments early on regarding your ideas was to avoid you investing a lot of work into something only to later run into issues that cannot be solved using the approach taken. That is also the reason for the procedure suggested above in https://github.com/gravitystorm/openstreetmap-carto/issues/3880#issuecomment-1945784857.

But again: This is only based on my perspective on this matters (which is based on my own work on this problem mentioned in https://github.com/gravitystorm/openstreetmap-carto/issues/3880#issuecomment-1560964205 - and therefore certainly somewhat limited). Other maintainers might well have a different view of this and might be able to guide you in a way you find more motivating. And i am perfectly fine if that leads to solving this issue or otherwise improving the symbol/label prioritization in a way that is different from what i have in mind here.

dch0ph commented 6 months ago

Yes, certainly other input is needed on direction here.

Personally I think the only scalable and maintainable solutions involve "intermediate" tables, such as a table of POI starting priorities. From the map designer view, the detailed implementation shouldn't matter (and can be iterated) as it would sit behind say a YAML configuration interface.