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

Change z_order of link roads and add tests #374

Merged
merged 2 commits into from Jul 11, 2015

Conversation

Projects
None yet
2 participants
@pnorman
Collaborator

pnorman commented Jul 5, 2015

Most maps prefer placing link roads below non-link roads. This allows them to do so without complex SQL or multiple layer solutions which break layer tag usage.

z_order per layer has also been increased from 10 to 100 to allow for this more fine-grained ordering.

Other types of roads (tracks, paths, etc) have been added. This does not impact those not using z_order for layers with these, but does allow their use.

It also removes highway=minor handling, which is a disused tag.

Fixes #132

@math1985, as you are more familiar with appropriate ordering of roads, could you review this? I'd like to deal with #132 for 0.88.0 which is probably coming soon.

I'm not attempting to replicate the osm-carto behavior for railways, as what we're doing with a UNION ALL is fairly specific and might not be generally applicable.

Change z_order of link roads and add tests
Mosat maps prefer placing link roads below non-link roads. This allows
them to do so without complex SQL or multiple layer solutions which
break layer tag usage.

z_order per layer has also been increased from 10 to 100 to allow
for this more fine-grained ordering.

Other types of roads (tracks, paths, etc) have been added. This
does not impact those not using z_order for layers with these, but
does allow their use.
@pnorman

This comment has been minimized.

Show comment
Hide comment
@pnorman

pnorman Jul 6, 2015

Collaborator

cc @gravitystorm @systemed as other cartographers using osm2pgsql

Collaborator

pnorman commented Jul 6, 2015

cc @gravitystorm @systemed as other cartographers using osm2pgsql

@matthijsmelissen

This comment has been minimized.

Show comment
Hide comment
@matthijsmelissen

matthijsmelissen Jul 7, 2015

Contributor

The proposed code looks good.

I'd still consider also including z_order for railways. Anyone who wants to order roads and railways properly and who does not want to create 11 layers in the .mml-file for all layers -5 to 5, will have roads and railways in one layer, so the ordering of railways must be set somewhere. The UNION BY is only necessary for these renderings that cover areas that have trams and highways sharing objects. If necessary, renderers could manually exclude trams from the layer (on the cost of ordering problems), like in the old default Mapnik layer, so the UNION BY is not necessary. If UNION BY is not used, setting the order for railways is still useful. I therefore think defining order for railways is useful for many users, and won't harm others.

I'd also include highway=proposed and highway=construction.

Contributor

matthijsmelissen commented Jul 7, 2015

The proposed code looks good.

I'd still consider also including z_order for railways. Anyone who wants to order roads and railways properly and who does not want to create 11 layers in the .mml-file for all layers -5 to 5, will have roads and railways in one layer, so the ordering of railways must be set somewhere. The UNION BY is only necessary for these renderings that cover areas that have trams and highways sharing objects. If necessary, renderers could manually exclude trams from the layer (on the cost of ordering problems), like in the old default Mapnik layer, so the UNION BY is not necessary. If UNION BY is not used, setting the order for railways is still useful. I therefore think defining order for railways is useful for many users, and won't harm others.

I'd also include highway=proposed and highway=construction.

@pnorman

This comment has been minimized.

Show comment
Hide comment
@pnorman

pnorman Jul 8, 2015

Collaborator

z_order is included for railways - see pnorman@a2f5b94#diff-0bc6697e58518b9e4450d6a128bd8790R84

I've had trouble replicating the broken z_order behavior with osm2pgsql master, since osm-carto now works around it. I see that the @yohanboniface's HOT style has problems at http://www.openstreetmap.org/#map=17/51.67413/5.32917&layers=H, so I'll try that.

Collaborator

pnorman commented Jul 8, 2015

z_order is included for railways - see pnorman@a2f5b94#diff-0bc6697e58518b9e4450d6a128bd8790R84

I've had trouble replicating the broken z_order behavior with osm2pgsql master, since osm-carto now works around it. I see that the @yohanboniface's HOT style has problems at http://www.openstreetmap.org/#map=17/51.67413/5.32917&layers=H, so I'll try that.

@matthijsmelissen

This comment has been minimized.

Show comment
Hide comment
@matthijsmelissen

matthijsmelissen Jul 8, 2015

Contributor

z_order is included for railways

Sorry, I meant the value added to the z_order, as in this PR. It would be nice if a naive rendering of all roads+railways ordered by z_order would result in railways rendered on top of roads - currently, railways would be rendered below roads.

Contributor

matthijsmelissen commented Jul 8, 2015

z_order is included for railways

Sorry, I meant the value added to the z_order, as in this PR. It would be nice if a naive rendering of all roads+railways ordered by z_order would result in railways rendered on top of roads - currently, railways would be rendered below roads.

@pnorman

This comment has been minimized.

Show comment
Hide comment
@pnorman

pnorman Jul 9, 2015

Collaborator

Since the HOT style still has z_ordering issues, I installed it was able to verify the changes work

image
image

Collaborator

pnorman commented Jul 9, 2015

Since the HOT style still has z_ordering issues, I installed it was able to verify the changes work

image
image

@pnorman

This comment has been minimized.

Show comment
Hide comment
@pnorman

pnorman Jul 9, 2015

Collaborator

I'm not convinced about changing railways to always be above all roads, instead of being above minor roads and below major ones.

Collaborator

pnorman commented Jul 9, 2015

I'm not convinced about changing railways to always be above all roads, instead of being above minor roads and below major ones.

@pnorman

This comment has been minimized.

Show comment
Hide comment
@pnorman

pnorman Jul 11, 2015

Collaborator

After talking to other cartographers, most people are either working around the current osm2pgsql z_order or consider it a bug. This method seems the best of the changes.

Collaborator

pnorman commented Jul 11, 2015

After talking to other cartographers, most people are either working around the current osm2pgsql z_order or consider it a bug. This method seems the best of the changes.

pnorman added a commit that referenced this pull request Jul 11, 2015

Merge pull request #374 from pnorman/z_order
Change z_order of link roads and add tests

@pnorman pnorman merged commit b91b008 into openstreetmap:master Jul 11, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pnorman pnorman deleted the pnorman:z_order branch Jul 18, 2015

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