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

Use z_order for roads instead of priority table #2732

Merged
merged 3 commits into from
Aug 21, 2017

Conversation

pnorman
Copy link
Collaborator

@pnorman pnorman commented Aug 8, 2017

This is getting a start on #2046. There should be no rendering changes.

The prio join added extra filtering, but this isn't needed, and was in fact undesirable. The features to be rendered are specified in the MSS, and the data volume returned to Mapnik isn't noticeably impacted because the huge majority of lines matching the WHERE clause also match the features being rendered.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 8, 2017

The code is shorter with this change, which is great, but are there any other interesting implications?

Should it be used also in this code?

@pnorman
Copy link
Collaborator Author

pnorman commented Aug 8, 2017

The code is shorter with this change, which is great, but are there any other interesting implications?

I'm not sure what you mean, but I'm not aware of any problems.

Should it be used also in this code?

Yes.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 8, 2017

I'm not sure what you mean, but I'm not aware of any problems.

I wonder for example if z-order really gives the same result as priority? If I understand correctly, z-order means that we use layer=* tag, while prio does not use it. Am I right or it works in some other way?

@pnorman
Copy link
Collaborator Author

pnorman commented Aug 8, 2017

I wonder for example if z-order really gives the same result as priority? If I understand correctly, z-order means that we use layer=* tag, while prio does not use it. Am I right or it works in some other way?

layernotnull comes first in the sort, so that's used before either. z_order does not depend on layer.

@kocio-pl kocio-pl mentioned this pull request Aug 8, 2017
littlebtc added a commit to littlebtc/openstreetmap-carto that referenced this pull request Aug 8, 2017
@pnorman
Copy link
Collaborator Author

pnorman commented Aug 8, 2017

Just to note, if there are any changes in rendered output that someone spots, I'd be interested, because I'm not expecting any.

Copy link
Collaborator

@matthijsmelissen matthijsmelissen left a comment

Choose a reason for hiding this comment

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

I tested this and reviewed the code, all seems fine. About time we finally go through with this!

@@ -444,7 +444,7 @@ Layer:
table: |-
(SELECT
way,
(CASE WHEN substr(feature, length(feature)-3, 4) = 'link' THEN substr(feature, 0, length(feature)-4) ELSE feature END) AS feature,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this change in a separate PR for clarity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've done it in a separate commit, which should be clear enough

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, fine.

@kocio-pl kocio-pl merged commit fdf5019 into gravitystorm:master Aug 21, 2017
kreed added a commit to kreed/openstreetmap-carto that referenced this pull request Sep 1, 2017
Service tracks aren't handled by z_order

Closes gravitystorm#2779
@pnorman pnorman deleted the roads_simplify branch September 16, 2017 09:02
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.

3 participants