-
Notifications
You must be signed in to change notification settings - Fork 823
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 addr:unit #2494
Render addr:unit #2494
Conversation
Thanks for the contribution, but this will have to wait until after the database reload and the lua branch gets merged, and we aren't doing PRs for new features against the lua branch yet, so I'm going to close the PR. |
Is it now time to reopen this PR? |
Yes, but it looks like it will have to wait few more months to update database (see #1286) - probably 6 months after 4.0.0, which would be about the end of November. |
@tpikonen Do you plan to update this code before nearest database reload window opens? |
I rebased the code against the current master. I do not have a local renderer installed at the moment, so I can't test this now. It's should be ok though :) |
Thanks! Could you also squeeze these 3 commits into one? |
Ok, commits are now squeezed. |
The nearest major PostgreSQL version (10) is expected on 9.11.2017, so this is probably when we should try to release osm-carto with this PR (once it's tested and accepted) to synchronize database reload with schema change: |
This code needs updating to work against the current database schema, or schema changes should be discussed independently of this particular change. |
@pnorman could you elaborate on the "needs updating to work against the current database schema"? I suppose you mean the changes made to the project.mml in this PR? I'm afraid I know next to nothing about the DB in OSM, so some help would be needed. |
This PR should contain no changes to openstreetmap-carto.style and instead pull the addr:unit tag from the tags column. |
I have general question then - when should we update database schema? Do you think we should add the code for hstore and once in a while (when we decide to get some elements into the schema) convert the code to the canonical form - or you mean something different? |
I'm not sure what you mean. We'd want some reasonable benefit to any change, but this is best discussed in its own issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just adding a review for tracking purposes.
This needs implementing without changes to openstreetmap-carto.style.
@tpikonen With this change the code works with current database schema: kocio-pl@27f0618. I'm just not sure if this should be this way (using |
Render addr:unit with addr:housenumber, if it exists. Render addr:unit by itself when zoom >= 18.
@kocio-pl Thanks for fixing the DB variables, I added the changes to the PR. I'm no SQL expert either, but from my reading of the PGSQL hstore docs this should now work without changes to the database schema. |
This branch implements rendering of addr:unit tags on ways (buildings) and nodes (entrances etc.), as discussed in issue #2488
The image below, rendered from http://www.openstreetmap.org/#map=18/60.16315/24.87713 shows rendering of addr:unit with addr:housenumber on buildings and without addr:housenumber on entrances.