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

Replace VMAP0-data by OSM-data #2969

Merged
merged 2 commits into from
Dec 11, 2017

Conversation

matthijsmelissen
Copy link
Collaborator

Currently, we use VMAP0-data instead of OSM-data to indicate built-up areas
on zoom levels 8 and 9. This PR replaces the VMAP0-data with the OSM
landcover data.

Resolves #256.

Currently, we use VMAP0-data instead of OSM-data to indicate built-up areas
on zoom levels 8 and 9. This PR replaces the VMAP0-data with the OSM
landcover data.

Resolves gravitystorm#256.
@HolgerJeromin
Copy link
Contributor

Is there documentation to be updated?

Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

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

Aside from the stray newline, a few other changes need to be made

I haven't looked at how effective this approach is and the cartographic results.

project.mml Outdated
('natural_' || (CASE WHEN "natural" IN ('wood', 'sand', 'scree', 'shingle', 'bare_rock') THEN "natural" ELSE NULL END)) AS "natural",
('wetland_' || (CASE WHEN "natural" IN ('wetland', 'mud') THEN (CASE WHEN "natural" IN ('mud') THEN "natural" ELSE tags->'wetland' END) ELSE NULL END)) AS wetland,

Copy link
Collaborator

Choose a reason for hiding this comment

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

stray newline

Copy link
Collaborator

Choose a reason for hiding this comment

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

You forgot to remove this empty line.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 7, 2017

I like the idea.

* Fix travis
* Fix documentation
* Remove layer from project.mml
* Remove empty newline
@matthijsmelissen matthijsmelissen mentioned this pull request Dec 9, 2017
@matthijsmelissen
Copy link
Collaborator Author

I have applied the requested changes.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 9, 2017

Rendering test of south of the Uzbekistan (as originally reported in #256):

z8
Before
bandpl5x
After
kmgxdcrj

z9
Before
4hfirjvs
After
wgfkhd8e

@matthijsmelissen
Copy link
Collaborator Author

Seems I forgot to push. Should be better now!

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 9, 2017

I haven't looked at how effective this approach is and the cartographic results.

This change is important in the areas with lacking landuse/natural tagging. In other places the change won't be too big (z8-9 means very big cities and we have them visible now).

The Uzbekistan sample shows that both data sets are different, but comparable.

@matthijsmelissen
Copy link
Collaborator Author

The Uzbekistan area seems fine to me.

Likely there are some places on earth where this will be a degradation (if landuse has not been drawn) but I'd consider it an improvement not to show settlements if we don't have them in OSM.

@matthijsmelissen
Copy link
Collaborator Author

All requested changes have been added. @pnorman didn't check the cartographic results, if you're fine with that this can be merged.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 11, 2017

@pnorman rarely comments here for some time, which makes it harder to discuss changes. But I also believe this is not about cartographic results (they are similar), but rather about data consistency and dropping external dependencies (both of which are obvious), so I think we should really merge it.

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.

None yet

4 participants