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

Start showing water areas from z0 #2873

Merged
merged 1 commit into from
Oct 13, 2017
Merged

Conversation

kocio-pl
Copy link
Collaborator

Resolves #1604.
Closes #2864.

Test shows that changing pixel query factor from 0.01 to 1 probably does not make visual difference (or the change is subtle), but improves performance a lot and we can reasonably render all the water areas from z0. For details see discussion in #1604.

Only water layer:
no-subpixel-select-biglakes-z6

@matthijsmelissen
Copy link
Collaborator

Test shows that changing pixel query factor from 0.01 to 1 probably does not make visual difference (or the change is subtle),

As the demo server hasn’t rerendered all tiles, we cant be too sure about that.

@kocio-pl
Copy link
Collaborator Author

I have not claimed that there's no doubt, but the fact that I have not yet found visible glitches and maybe one place which hides smallest lakes is quite reassuring. That being said it's good to check as much as it's possible here:

http://bl.ocks.org/math1985/raw/af7a602c222dbf1ff1a2c0d84ed755b7/#3.00/18.42/1.85

@rrzefox
Copy link

rrzefox commented Sep 30, 2017

These are really two changes:
1.) changing the pixel query factor
2.) start rendering water from Z0 instead of Z6.
2 will not work well without 1 because of memory requirements.

It should be mentioned that the sideeffect of change 2 will be massively increased rendering times for (some) tiles on Zoomlevels 0 to 5, e.g. on Z0 from 0.5 to 100+ seconds. However, there are only few tiles in these zoomlevels, and they are usually never rerendered on the fly.

So to phrase the question in a different way: Do we want a Z0 tile that renders very fast but hides away huge, distinctive water areas like the border lakes between .us and .ca, or do we want to spend a few minutes of rendering time to have a better looking, closer to reality and more easily recognizable map?

If someone is really worried about this, it might be possible to drop them in Z0 to Z2, with the justification that it's a really abstract map at these zoom levels. However, IMHO starting from Z3, where the country labels start to show up, you have to show them - else the fact they're missing really hits you in the face. And it's more consistent to show them from Z0.

@kocio-pl
Copy link
Collaborator Author

Just some additional notes:

  • with z6+ we will gain some performance/memory because of 1)
  • La Plata mouth is specific case when even on z1 it's visible that something is broken, so in general I would be against dropping it; we have something similar with Archangielsk on z2+, but there's no border, so there's no such visual problem

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Oct 3, 2017

Example of some rivers rendered as water areas, as discussed in a separate PR (see #2874 (comment)):

http://bl.ocks.org/math1985/raw/af7a602c222dbf1ff1a2c0d84ed755b7/#5.00/65.044/-143.550

They seem to be continuous to me, but it doesn't look great, because some segments are wider and some other are thinner (near 1 px, I guess). That's because we don't use proper generalization of waterway lines on low zoom as rivers classification is currently lacking in OSM (2 parallel proposals exist to fix that problem). For me it's acceptable and enough to drop subpixel accuracy for water - more low zoom rework is needed anyway.

This river:

http://bl.ocks.org/math1985/raw/af7a602c222dbf1ff1a2c0d84ed755b7/#6.00/-2.481/-58.100

does not look continuous on z3 (there are just some parts visible), but that's expected as there's no relation including all the segments:

https://www.openstreetmap.org/relation/32983#map=6/-2.175/-54.130

@matthijsmelissen
Copy link
Collaborator

I suspect this has nothing to do with the subpixel problem, I think even with water-blocks of 0,01 pixel these rivers would be so narrow that they wouldn't (noticeably) render.

Both rendering examples look acceptable to me.

@kocio-pl kocio-pl mentioned this pull request Oct 3, 2017
@daganzdaanda
Copy link

Great to see this, thank you all!

Found an error, probably #1465, the long-known mapnik bug with labels not ending up inside the polygon?
http://bl.ocks.org/math1985/raw/af7a602c222dbf1ff1a2c0d84ed755b7/#4.00/56.40/32.30
Look for the Caspian Sea just west of Kiev ;-)

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Oct 5, 2017

Yes, that was reported already in #1604 (comment).

Any additional tests, comments or reviews? I wonder how much time or actions are needed to make sure and decide (and hopefully merge this code).

@daganzdaanda
Copy link

Yeah, I read that later... Lake Victoria's label is also funny.

Back on topic, I have looked at a lot of different areas worldwide and it is a big improvement for the low zooms everywhere. I mean, just look at it! Lakes are most obvious, but also the big rivers (Congo) and reservoirs really define their landscapes.

@kocio-pl
Copy link
Collaborator Author

We may wait another 2 weeks with decision, but I'm not sure what are we waiting for? From the formal point of view one positive review would be enough - and with all the comments and my tests it looks like it's the only thing that is really needed.

@matthijsmelissen
Copy link
Collaborator

matthijsmelissen commented Oct 13, 2017

Sorry for not reviewing it earlier. It's clear this should go through.

I'm a bit worried still about the performance of the text-poly-low-zoom layer - it doesn't have any minimum pixel requirements in the SQL definition. Did we test this? Even any rough results that this wouldn't lead to significant problems would be fine.

@kocio-pl
Copy link
Collaborator Author

I'm not sure how should we test performance.

Real life testing (#2874 (comment)) with this patch included does not indicate big problems, as we know it already.

There's a separate ticket for performance testing, but no conclusions yet: #1287

@matthijsmelissen
Copy link
Collaborator

Ok, I wasn't sure if text-poly-low-zoom was already included.

Let's go ahead then!

@kocio-pl kocio-pl merged commit fddd481 into gravitystorm:master Oct 13, 2017
@kocio-pl kocio-pl deleted the big-lakes branch October 13, 2017 22:02
@kocio-pl
Copy link
Collaborator Author

You can read about other possible solutions of this problem (this one looks like a mild version of 2):

http://blog.imagico.de/parting-the-waters/

@itscz-org
Copy link

2.) start rendering water from Z0 instead of Z6.

Thank you guys, that just cost me 2 days to figure out why the hell the shapefiles aren't used for my lower zooms that worked properly on another server with an older version. That is why. I downgraded before this commit now.

@kocio-pl
Copy link
Collaborator Author

Good that you have found where the problem is and you were able to work around it.

But what is the problem? This change just added waters directly from OSM database, in addition to the shapefiles. Did you want just shapefiles without any waters on top or I misunderstood you?

@itscz-org
Copy link

itscz-org commented Jan 24, 2018

Before the change:
renderd[2489]: DEBUG: DONE TILE default 0 0-0 0-0 in **0.155 seconds**

After:
renderd[26584]: DEBUG: DONE TILE default 0 0-0 0-0 in **370.529 seconds**

I didn't miss any waters. Fast caostlines and country shapes worked just perfectly for me and is so much faster.

@dieterdreist
Copy link

dieterdreist commented Jan 24, 2018 via email

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Jan 24, 2018

On OSMF servers the update of z0-z12 is made every few weeks and re-rendering all of them takes just few hours with the current version, because they don't change a lot that often and there's no need to do this more often. Most of the time takes rendering z13-z19, because there are much more of them (z13 alone has more tiles than all z0-z12 combined - and so on) and the changes on higher zoom levels are more visible.

Of course if you try to have the most fresh low zoom and don't care for inland waters there, that's the best you can do. However I'm happy with current situation, so it looks we just have different needs.

@itscz-org
Copy link

I'm fine with rare rendering of z0, my users are also starting from z4, what messed me up is that i use z0 for setup tests and need 2 days to figure out why in comparison this is so slow.

I also compared the 2 results and could not see any difference in z0, so this totally makes no sense to me. Why not start with waters in z2++ ?

@kocio-pl
Copy link
Collaborator Author

We didn't try to make artificial thresholds and the problem with Uruguay/Argentina border, described in #2580, is visible from z1 (it's broken currently again, we plan to repair it).

@itscz-org
Copy link

What about updated shapefiles?

@kocio-pl
Copy link
Collaborator Author

I don't understand this question - what do you mean exactly?

Shapefiles are produced from coastlines, but all the waters we have added on low zoom in this PR are inland, so it's a different kind. La Plata mouth looks like just a part of the ocean, but it's tagged as a river area - and probably that's the truth (sweet water vs salt water).

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

Successfully merging this pull request may close these issues.

6 participants