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

Change z_order of link roads and add tests #374

Merged
merged 2 commits into from
Jul 11, 2015

Conversation

pnorman
Copy link
Collaborator

@pnorman 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.

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
Copy link
Collaborator Author

pnorman commented Jul 6, 2015

cc @gravitystorm @systemed as other cartographers using osm2pgsql

@matthijsmelissen
Copy link
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.

@pnorman
Copy link
Collaborator Author

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
Copy link
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.

@pnorman
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Change z_order of link roads and add tests
@pnorman pnorman merged commit b91b008 into osm2pgsql-dev:master Jul 11, 2015
@pnorman pnorman deleted the z_order branch July 18, 2015 08:25
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.

2 participants