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

Do not render small water areas on low zoom #2952

Merged

Conversation

matthijsmelissen
Copy link
Collaborator

@matthijsmelissen matthijsmelissen commented Nov 20, 2017

On zoom levels 0-7, don't render small water areas (smaller than 32px).

On zoom levels 0-8, don't render small water areas (smaller than 32px).
@matthijsmelissen
Copy link
Collaborator Author

@rrzefox would it be possible to test this on your server?

@kocio-pl
Copy link
Collaborator

Could you also publish some before/after renderings? Probably the places that you gave as example of problems with small water areas would be good.

@matthijsmelissen
Copy link
Collaborator Author

@kocio-pl asked in #2925:

what filter do we want to use and up to which zoom level?

It currently bothers me both on zoom levels 6 and 7, and perhaps 8 and 9. So i think we definitely need measures for the zoom levels up to 7 (on levels 0-5 the small water areas are not very useful to me either).

I eyeballed 32px as a cut-off, let's see how it looks like when we have a rendering demo.

@matthijsmelissen
Copy link
Collaborator Author

Could you also publish some before/after renderings?

I don't have enough data in my database at the moment for realistic example renderings at these zoom levels.

@rrzefox
Copy link

rrzefox commented Nov 20, 2017

@rrzefox would it be possible to test this on your server?

Done, and Z0-8 have already been rerendered.

@matthijsmelissen
Copy link
Collaborator Author

matthijsmelissen commented Nov 20, 2017

@kocio-pl
Copy link
Collaborator

Unfortunately it reintroduces #2580 on z0-z2.

@matthijsmelissen
Copy link
Collaborator Author

This looks like an improvement to me.

@kocio-pl
Copy link
Collaborator

On z0-z1 we don't see any of the biggest lakes (except Caspian, of course). On z2 Ontario, Erie and Onega are still not visible.

@kocio-pl
Copy link
Collaborator

I also think that filtering produces nice effects, so it's worth applying. On the other hand La Plata and big lakes are special, so it would be good to preserve them. The simplest solution would be to start filtering from z3 instead of z0. What do you think about it?

@matthijsmelissen
Copy link
Collaborator Author

Yes, I agree.

polygon-gamma: 0.6;
[landuse = 'basin']::landuse {
[zoom >= 7][way_pixels >= 32],
[zoom >= 8] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

On zoom levels 0-8, don't render small water areas (smaller than 32px).

So it should be:

[zoom >= 9] {

water.mss Outdated
}
}

[natural = 'water']::natural,
[landuse = 'reservoir']::landuse,
[waterway = 'riverbank']::waterway {
[zoom >= 0] {
[zoom >= 0][way_pixels >= 32],
[zoom >= 8] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same as above: [zoom >= 9] {

@rrzefox
Copy link

rrzefox commented Nov 24, 2017

So some screenshot for later reference:
Z1 old:
z1-old
Z1 new:
z1-new
The big lakes between .us and .ca disappeared.
Z3 old:
z3-old
Z3 new:
z3-new
On Z3, all the mini-waters in Canada are gone. The same happens in northern europe (no screenshot).
In .nl, the Ijsselmeer disappears, it appears on Z4.

I have now deployed a slightly different version of this PR on the "demo server", it uses this less aggressive filtering:

  [natural = 'water']::natural,
  [landuse = 'reservoir']::landuse,
  [waterway = 'riverbank']::waterway {
    [zoom >= 0][way_pixels >= 4],
    [zoom >= 1][way_pixels >= 16],
    [zoom >= 2][way_pixels >= 32],
    [zoom >= 8] {

@kocio-pl
Copy link
Collaborator

Thanks! I was also thinking about progressive filtering and with these values it seems to be the best of both worlds, indeed. I would merge this version.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 3, 2017

@matthijsmelissen Do you plan to embrace the @rrzefox proposition? If it's just lack of time, I can fine tune the code and merge it soon.

@matthijsmelissen
Copy link
Collaborator Author

matthijsmelissen commented Dec 3, 2017

Generally I prefer steps of factor 4, as for every zoom level, lines get twice as large and areas four times as large. So for example 4-16-64 or 2-8-32. However, 4-16-32 might work as well (it means on z2 some additional water areas pop up, which might make sense).

Aside from the exact numbers, I'm happy with @rrzefox's approach. I was planning to get back at this some time, but I'd be glad if you can pick it up earlier.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 3, 2017

@rrzefox Were there some quirks which would be visible with 4-16-64 or 2-8-32 approach or was it just a free form handcrafting? If the latter is the case, could you test one of the proposed "4x" schemes?

@rrzefox
Copy link

rrzefox commented Dec 6, 2017

For reference, this is how it looks with 4-16-32: Edit: Due to a bug, this actually showed 4-4-4 instead of the intended 4-16-32.
z0
z1
z2

While I also tried to make steps with a factor of four, I found deviating from that in the last step acceptable. I had tried 2-8-32 but not liked the result because it did not filter enough. But see for yourself, I deployed that now... I'll deploy 4-16-64 on Friday.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 6, 2017

OK, let's see the 4-16-64 then.

BTW - because Matthijs has changed the account name, the link to compare is different now:

http://bl.ocks.org/matthijsmelissen/raw/af7a602c222dbf1ff1a2c0d84ed755b7/#2.00/18.8/14.7

@matthijsmelissen
Copy link
Collaborator Author

4-16-32 works in fact quite well, I added it to the PR now.

@matthijsmelissen
Copy link
Collaborator Author

On zoom levels 0-8, don't render small water areas (smaller than 32px).

I changed the documentation, it is now 0-7.

@rrzefox
Copy link

rrzefox commented Dec 8, 2017

I've now deployed 4-16-64 for testing (Z0-3 rerendered)

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 8, 2017

It looks OK for me.

On z0 La Plata is not visible, but the border too, so it's not a problem. Lack of Ontario, Erie, Ladoga, Onega and Victoria on z0 are also not visible if you're not looking for them. Anything on z1+ looks decent in both 4-16-32 and 4-16-64 for me, so it's up to you which version do you like more, Matthijs.

@StyXman
Copy link
Contributor

StyXman commented Dec 8, 2017

Me being from Argentina I can say that this is a nice improvement, and I don't think anybody would complain about Río de la Plata not being present in z0.

@matthijsmelissen
Copy link
Collaborator Author

    [zoom >= 0][way_pixels >= 4],
    [zoom >= 1][way_pixels >= 16],
    [zoom >= 2][way_pixels >= 32],

@rrzefox This does not work. It will still render water areas of size 8 on z1 because the first line is still true.

@matthijsmelissen
Copy link
Collaborator Author

@rrzefox I have fixed this in my branch, would you be able to deploy my branch to your server?

I used 4-16-32, as filtering with 4-16-64 will almost certainly be too aggressive.

@matthijsmelissen matthijsmelissen mentioned this pull request Dec 9, 2017
@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 9, 2017

@rrzefox I have fixed this in my branch, would you be able to deploy my branch to your server?

It was already tested and the screenshots are here: #2952 (comment)

@matthijsmelissen
Copy link
Collaborator Author

Nope, that test contained the bug I mentioned in #2952 (comment) .

@matthijsmelissen
Copy link
Collaborator Author

Just to be clear, the 4-16-32 deploy actually showed 4-4-4, the 2-8-32 deploy actually showed 2-2-2, and the 4-16-64 deploy actually showed 4-4-4 again.

@rrzefox
Copy link

rrzefox commented Dec 10, 2017

The fixed 4-16-32 version has been deployed and Z0-7 rerendered.

@kocio-pl
Copy link
Collaborator

That's OK for me, we can merge it any time soon.

@matthijsmelissen
Copy link
Collaborator Author

@rrzefox Thanks!

@kocio-pl If you Approve it, I can merge it.

@matthijsmelissen matthijsmelissen merged commit 63c05ac into gravitystorm:master Dec 11, 2017
@kocio-pl
Copy link
Collaborator

I'm confused, it looks like z1 and z2 are wrong now - La Plata and smaller Big Lakes are missing now on both proposed and current versions (OSMF re-rendering is happening right now)...

http://bl.ocks.org/matthijsmelissen/raw/af7a602c222dbf1ff1a2c0d84ed755b7/#2.00/27.4/42.3

@kocio-pl
Copy link
Collaborator

Another thing that we could make better would be to not filter riverbanks, as suggested here:

http://www.openstreetmap.org/user/imagico/diary/42975#comment40531

@matthijsmelissen
Copy link
Collaborator Author

My intention was not to render riverbanks at all. The solution is exactly the opposite of what @imagico suggests: these riverbanks are way too big, and should be cut up in smaller parts.

@kocio-pl
Copy link
Collaborator

Current code still allows showing them, so maybe it'd be better to get rid of them completely on some initial zoom levels to avoid problems with some segments being visible and some others invisible.

@matthijsmelissen
Copy link
Collaborator Author

Makes sense.

@matthijsmelissen
Copy link
Collaborator Author

I'm confused, it looks like z1 and z2 are wrong now

Do you have any idea what could cause this?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 24, 2017

I guess we just got lost with testing all the possibilities on @rrzefox server. 😄 It was probably some small, stupid error but nobody checked it on his own machine.

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.

4 participants