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

Remove way_area filtering for low zoom water and simplify code #4060

Merged
merged 1 commit into from
Mar 19, 2020

Conversation

jeisenbe
Copy link
Collaborator

@jeisenbe jeisenbe commented Mar 7, 2020

Partial Fix for #3273
Revert of #2952

Changes proposed in this pull request:

  • Remove >1 pixel way_area filtering for low zoom water
  • Clean up code to simplify rendering of water areas. Now waterway=dock and landuse=basin will be the same as the other features.

There is still a 1 pixel limit in the SQL select which is probably necessary for z0 to z6 at least, but I would like to consider changing this in the future. See related discussion in #1604 and #754, however. Since the SQL code currently allows rendering down to 1 pixel without significant performance issues, this change should be a good first step.

Filtering out some water areas at lower zoom, as done currently since v4.6.0, shows fewer lakes and rivers at low zoom level, without significantly improving performance. The 32 pixel limit at z2 to z7 causes moderately large lakes to appears suddently, and parts of rivers to appear at different zoom levels.

While solving this issue entirely will require changing the limit in SQL from 1 pixel back to 0.1 or 0.01 pixel (which might cause performance problems), most of the visible problems can be fixed by this PR.

Simplyfing the code for water areas will make it much easier for me to add a rendering for salt=yes and intermittent + salt water lakes - I started that PR but realized this is needed first. Now (water) dock areas and basins are treated exactly the same as the other water area features in the water.mss file

Test rendering with links to the example places:

Alaska, Yukon delta
Before z6
yukon-delta-z6-before
After z6
yukon-delta-z6-after-1-pixel-limit

Before z2
alaska-z2-before
After z2
Uploading alaska-z2-after-1-pixel-limit.png…

Before z2 - exported at 3x (e.g. retina tiles)
alaska-z2-export3x-before
After z2 at 3x
alaska-z2-export3x-after-1-pixel-limit

Magadan Obast, eastern Siberia
https://www.openstreetmap.org/#map=7/62.472/152.644
Before
water-magadan_obast-z5-before
water-magadan_oblast-before-7:62 472:152 644

After
water-magadan_obast-z5-after
water-magadan_oblast-z7-after

South Australia

before z7
water-area-limit-before-7:-31 934:138 274
after z7
water-1-pixel-area-limit-after-7:-31 934:138 274

Clean up code to simplify rendering of water areas. Now waterway=dock and landuse=basin will be the same as the other features.
@jeisenbe jeisenbe added the water label Mar 7, 2020
@kocio-pl
Copy link
Collaborator

kocio-pl commented Mar 8, 2020

I think for riverbeds it is a good change, since I found no way to render just waterways starting with big rivers on lower zoom levels and gradually going with smaller ones. Unfortunately river areas are not perfect in this regard, but at least it gives some continuity of water flow.

At the same time rendering other small water areas just makes visual noise (it renders as pale blue smugdes, hard to recognize at first sight) which should be avoided.

@pnorman pnorman requested a review from imagico March 8, 2020 16:02
@jeisenbe
Copy link
Collaborator Author

I found no way to render just waterways starting with big rivers on lower zoom levels and gradually going with smaller ones.

That would require preprocessing the waterway=river ways using stream order, for example. I believe @imagico showed a demonstration of this - but it's different than just showing the actual, visible area of very wide rivers at low zoom level.

We cannot treat river water areas differently than lakes or other water areas now, since at high zoom level rivers and lakes are rendered identically.

At the same time rendering other small water areas just makes visual noise (it renders as pale blue smugdes, hard to recognize at first sight) which should be avoided.

This PR only shows water areas that are at least one pixel in size. We also show sub-pixel areas of ocean water, outside of the coastline. See for example the coastline of Norway or Chile, or areas of mangroves around New Guinea at low zoom level.

Since we no longer use simplification on the coastline at low zoom level, but rather show the full visible detail, for good feedback to mappers, why should we only show inland water bodies which are mapped in pieces larger than 32 square pixels, or even 4 square pixels?

The current code makes lakes appear suddently, 2 to 3 zoom levels after they could already be visible as a couple of pixels.

Consider that we do not filter out any landcover (like woods) which are larger than 0.01 pixel at z5, so why should water areas need to be larger than 32 pixels at z5 to be shown?

If we want to simplify all landcover and all water bodies at low zoom level, that could be considered, but it would need to be done the same way for oceans, lakes, rivers, woods and all other things that render at z0 to z8.

Until someone develops a way to do that and achieves consensus among the contributors that it would be a good idea, we should first merge this PR to make the rendering of inland water bodies consistent with that of oceans and landcover, right now.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Mar 10, 2020

If you are wondering, "Why shouldn't we encourage mappers to make huge multipolygons which include the entire area of a river": these are easy to break and hard to maintain.

For example: https://www.openstreetmap.org/relation/9969273#map=6/2.680/-61.611

Resulted in "flooding" much of South America last year:

Report of flooding on HDM style:
https://osmus.slack.com/?redir=%2Farchives%2FC029HV951%2Fp1582148821441200

(via https://www.openstreetmap.org/user/migurski/diary/392416)

Mapping sections of a river as closed ways, or small multipolgyons, is much less likely to cause errors, and makes it much easier to download a small section of Openstreetmap data (like just one town).

@pnorman
Copy link
Collaborator

pnorman commented Mar 10, 2020

We cannot treat river water areas differently than lakes or other water areas now, since at high zoom level rivers and lakes are rendered identically.

Yes we can. There's no technical reason why we couldn't filter waterway=riverbank polygons one way and natural=water a different way.

If we want to simplify all landcover and all water bodies at low zoom level, that could be considered, but it would need to be done the same way for oceans, lakes, rivers, woods and all other things that render at z0 to z8.

We don't need to simplify everything the same way, or tackle it all at once.

I'd rather more aggressively filter out small features, but haven't been pushing for it.

If you are wondering, "Why shouldn't we encourage mappers to make huge multipolygons which include the entire area of a river": these are easy to break and hard to maintain.

For example: https://www.openstreetmap.org/relation/9969273#map=6/2.680/-61.611

Resulted in "flooding" much of South America last year:

I don't encourage giant multi-polygons, but that example was caused by the HOT tiles using dated versions of software that try to form old-style multipolygons, not by it being huge.

@jeisenbe
Copy link
Collaborator Author

I should have shown the changes in an area with lots of landcover mapping too, sorry:

z5 Latvia

Current
z5-latvia-master-water-filtering

After (1 pixel limit for water)
z5-latvia-after-water-1-pixel

z6 Latvia

Current (32 pixel limit for water, 0.01 for landcover)
z6-latvia-master-filtered-32pixels

After (1 pixel for water, 0.01 for landcover)
z6-export-1x-1pixel-filtering

Comparison (standard resolution, zoomed-in)
z6-latvia-water-filter-comparison

z7 Latvia

Current
z7-latvia-master-water-filtering

After
z7-latvia-after-water-1-pixel

@jeisenbe
Copy link
Collaborator Author

Yes we can. There's no technical reason why we couldn't filter waterway=riverbank polygons one way and natural=water a different way.

Sorry, I meant "we shouldn't treat river water areas differently than lakes or other water areas now, since at high zoom level rivers and lakes are rendered identically".

If we only show a subtle difference on low zoom level, then mappers might choose to remove water=river from natural=water (or add it inappropriately) to get a certain rendering.

For example, if we render waterway=riverbank and water=river at 0.1 pixels, but lakes at 32 pixels, we will have situations where a river still has gaps because it goes into a narrow reservoir: the river area will be above the limit, but the narrow reservior will be too small to render.

A mapper might try to "fix" this by changing the reservoir to water=river or adding waterway=riverbank.

If we had a different color for river areas, then at least there would be feedback about the change so other mappers might fix it.

I'd rather more aggressively filter out small features, but haven't been pushing for it.

See #4067 (comment) for how this causes problems when done for just one feature (water areas currently), and also see #4067 (comment) for what problems it would cause if done for landcover too.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Mar 10, 2020

that example was caused by the HOT tiles using dated versions of software that try to form old-style multipolygons

You are right, I'm sorry I didn't read the linked article before posting.

A more relevant example (from the same diary entry) is this Euphrates river multipolygon:

https://www.openstreetmap.org/node/2488415849#map=13/34.1019/42.3841

Euphrates-multipolygon-broken

I recall a similar thing happened with the Columbia river several years ago, so when I first started using Opencyclemap the river area in Portland was not rendered for a couple months, until Opencyclemap finally re-rendered the tiles.

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

I think this is an improvement and would not cause any new problems as far as i see.

However i am a bit concerned that this will widely be considered as a 'good enough' way to deal with low zoom rendering that prevents us from working for a more comprehensive solution, also w.r.t. landcover rendering. The current rendering is obviously and ostentatiously wrong. With this change it would still be wrong in the sense of being biased and distorting for a purely technical convenience reason and no cartographic purpose for this. But the difference will be much more subtle and while right now it is obvious to everyone why the rendering is wrong it won't be any more with this change applied.

I think what i want to say here is that with the 1.0 pixel size threshold this style would fail to meet its purpose to be an exemplar stylesheet for rendering OSM data. The recommendation to other styles would need to be: Don't use this in your project.

@imagico
Copy link
Collaborator

imagico commented Mar 10, 2020

Sorry, I meant "we shouldn't treat river water areas differently than lakes or other water areas now, since at high zoom level rivers and lakes are rendered identically".

Exactly - see also #3273 (comment).

The fact that the faultiness of way_area filtering for waterbodies is most obvious for riverbank polygons does not mean it is cartographically useful or desirable for other waterbodies.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Mar 10, 2020

i am a bit concerned that this will widely be considered as a 'good enough' way to deal with low zoom rendering

That is certainly not my intention.

I opened #4067 because I would also like to change the filter back to 0.01 pixels, which will at least be no worse than the current landcover rendering.

This PR is a first step, since it will not have a significant impact on server performance (all these areas are already selected in the current SQL query), while going from 1 pixel to 0.01 pixel in SQL might need performance testing.

I also was planning to add a salt=yes water rendering, and it will be much easier after this PR, so this one is going first.

In the medium-term, there is no substitute to some sort of preprocessed data, whether it's a raster file for landcover and water areas, or preprocessed shapefiles, or another way of rendering the fill colors which does not rely on Mapnik at scales where there are many polygons per pixel, as mentioned in #4067 - and let's discuss further there.

@kocio-pl
Copy link
Collaborator

I see no good solution for rivers, at all. All of them are bad in some way:

  • filtering breaks continuity, which is the most important aspect of waterways (ref to Remove way_area filtering for low zoom water and simplify code #4060 (comment): if rivers would be tagged the same as big lakes, which people don't have the problem tagging as one object, which is suggested by general guidelines, that would be much better; but that's something for tagging list)
  • not filtering will show smaller rivers as pale line smudges
  • preprocessed data are not a silver bullet, since this makes us rendering relying more on 3rd party work, which will make arbitrary decisions and updating this data it can stale for whatever reasons (Kosmtik is a clear warning here)

I'm inclined toward dropping filtering, to stick closer to the main river feature, which is continuity.

Other small water areas just make noise, which can be easily avoided, so I opt for keeping it filtered. This is a different case, even if both are about water, since even long lakes are never simplified as lines.

@jeisenbe
Copy link
Collaborator Author

This will wait until v5.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants