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

Render ways with both highway and railway tag #894

Merged

Conversation

matthijsmelissen
Copy link
Collaborator

Make sure that ways that have both a highway and a railway tag are rendered
both as highway and railway.

This resolves #874.

This assumes #866 and #884 are merged first.

Thanks to @kkofler.

Re-add highway=platform, and changes ordering of railway=platform versus paths.
This solves gravitystorm#871.

This assumes gravitystorm#866 is merged first.
Make sure that ways that have both a highway and a railway tag are rendered
both as highway and railway.

This resolves gravitystorm#874.

This assumes gravitystorm#866 and gravitystorm#884 are merged first.

Thanks to @kkofler.
@matthijsmelissen
Copy link
Collaborator Author

This is urgent as it resolves an important regression (railways on top of roads were not rendered).

@matthijsmelissen
Copy link
Collaborator Author

@pnorman Could you check the performance of this?

@matthijsmelissen
Copy link
Collaborator Author

It would also be good if some people could properly test this for potential newly introduced bugs.

@pnorman
Copy link
Collaborator

pnorman commented Aug 18, 2014

@pnorman Could you check the performance of this?

Checking fd1df07, then 40a747d. In future please indicate the commits you want me to compare, as it's not trivial for a PR that depends on other PRs.

@pnorman
Copy link
Collaborator

pnorman commented Aug 19, 2014

It's slower. With changes, 8.987 MT/s, without that last change, 9.021 MT/s, a decrease of 0.38%. This result is statistically significant. The slowdown seems distributed across all layers.

It's probably worth the slowdown. We've gained more speed in the last few months than we'll be losing here, including more speed from the roads refactorings which introduced this bug. Assigning to Andy.

It's worth noting that reusing the same layer for roads_fill and roads_casing would probably increase speed by far more than any loss here.

@pnorman pnorman assigned gravitystorm and unassigned pnorman Aug 19, 2014
@matthijsmelissen
Copy link
Collaborator Author

It's worth noting that reusing the same layer for roads_fill and roads_casing would probably increase speed by far more than any loss here.

The problem with that is that highway-area-fill is still rendered in between these two layers. To merge roads_fill and roads_casing, we would need to merge the linear roads and areas SQL (for example with a union), which is probably undesirable.

@matthijsmelissen
Copy link
Collaborator Author

@gravitystorm Do you already know when you will have time for your next round of merges? I think there are quite a lot of people waiting for this fix.

@gravitystorm gravitystorm merged commit fd1df07 into gravitystorm:master Aug 27, 2014
@matthijsmelissen matthijsmelissen deleted the highway-railway-overlap branch September 25, 2014 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Railway=tram no longer rendered when the way also has a highway tag
3 participants