-
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
Use ST_PointOnSurface for amenity points #3712
Use ST_PointOnSurface for amenity points #3712
Conversation
This requies explicit statements about which features are point-only and which are poly-only.
This avoids features being sent to Mapnik that won't match any of the MSS rules while simplifying the WHERE clause drastically.
This avoids having to maintain two long complex mostly-identical queries.
Removing the use of two layers is a huge improvement to style complexity, bringing compile time from 17s to 11s, and XML lines from 56.5k to 47.2k. The diff also gets rid of 36k of SQL. Part of this reducing duplication between points and polygons, part is simplifying the WHERE clause, and part is a common query for points and text. |
cc @rory |
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.
Great to see progress here, and great to see such a performance gain!
I might be misunderstanding, but it seems some features do not appear in the queries anymore. Was this deliberate?
I haven't done a full review yet.
No. I thought that all features that appeared in one query should appear in the other or else it would lead to mismatches between labels and icons. I'll have to go back and do a line-by-line check. |
So, I'm going through the text query and finding more differences than I expected. This makes a common query less valuable than I had thought, but I've left it that way. If we want to change that, I can bring back the existing text queries and merge those two in a different PR. |
These were in the text queries and are needed if a common query is to be used.
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.
Looks good and tests fine and should make future improvements, in particular the synchronization between starting zoom level, drawing order and coalesce priorities, significantly easier.
Two things i noticed though:
- This seems to be the first use of yaml references for SQL code - would probably make sense to indicate that with a comment (something like this SQL query is also used in the text layer further down). Can be added in a later PR just as well of course.
- If i read the code correctly there is one major difference in rendering results this will result in: The feature COALESCE() includes everything that is in the text layers which is a lot more than in the amenity-points/polys layers. Therefore if for example a
tourism=viewpoint
is also taggednatural=bare_rock
(or anything like that) this will not be shown any more with this change. Now this is not necessarily a bad thing - this would to some extent discourage the ambiguous practice to tag multiple things on the same feature leading to it not being clear where supplemental tags apply to. But still this might lead to some confusion.
I had to think this over, and you're partially right. It'll go by the priority in the I'm not sure if the old way is any better. There, a name could end up in one style while an icon in another style (e.g. |
This ticket has been mentioned in #3635 (comment) as part of the prioritization strategy, but this was not mentioned as a goal of this code change. What are the consequences in this respect and is there really a strategy? |
I'm not sure what you're asking. |
Two questions:
|
It shouldn't, except if there were inconsistencies between the queries.
It was a goal to make it easier to make changes, but I wasn't thinking of prioritization, and I'm still not sure what you mean by strategic goal. |
Thanks, now I have the things straighten up enough. |
I'm curious about performance impact of this change. Has anyone tested this? Particularly I mean dropping the where clause used for filtering in amenity points/poly queries. This one with comment "-- The upcoming where clause is needed for performance only". |
There's a bunch of whitespace noise, so it might help to view https://github.com/gravitystorm/openstreetmap-carto/compare/master...pnorman:amenity_point_unification?w=1