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 text-poly-low-zoom to use ST_PointOnSurface #3921

Merged
merged 1 commit into from
Jan 28, 2020

Conversation

jeisenbe
Copy link
Collaborator

@jeisenbe jeisenbe commented Oct 4, 2019

Related to #3201 - migration to server-side vector tiles

Changes proposed in this pull request:

  • Change text-poly-low-zoom layer to use ST_PointOnSurface - this is necessary to use this style with vector tiles, and this makes the low zoom rendering consistent with the label placement at higher zoom levels (z10 and up
  • Also remove unneeded text-placment: interior for the polygon features affected - this code is no longer needed, because the labels are placed on a point, not in a polygon, by Mapnik at the time of rendering.

Test rendering:

z8 Libya
https://www.openstreetmap.org/#map=8/27.330/17.216
z8-libya-rocks-compare-st

z9 Wan am namus
https://www.openstreetmap.org/#map=9/24.8628/17.7742
z9-wan-am-namus-compare

Also remove unneeded text-placment: interior
@@ -1992,7 +1992,8 @@ Layer:
name,
CASE WHEN building = 'no' OR building IS NULL THEN 'no' ELSE 'yes' END AS is_building -- always no with the where conditions
FROM planet_osm_polygon
WHERE (landuse IN ('forest', 'military', 'farmland')
WHERE way && !bbox!
AND (landuse IN ('forest', 'military', 'farmland')
Copy link
Collaborator

Choose a reason for hiding this comment

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

AND has higher precedence than OR In SQL, so we need a pair or brackers around all the OR statements.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In addition to the pair here?

  AND (landuse IN....
...
  OR leisure IN ('nature_reserve'))

Where should the brackets be? It seems to work currently

Copy link
Collaborator

Choose a reason for hiding this comment

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

The brackets look good to me as-is.

@jeisenbe
Copy link
Collaborator Author

@matthijsmelissen - the current code works. I don't see where the brackets are missing. Could you advise?

@jeisenbe
Copy link
Collaborator Author

@matthijsmelissen there is a pair of brackets around all the OR statements:

WHERE way && !bbox!
AND (landuse IN ('forest', 'military', 'farmland')
  OR military IN ('danger_area'
...
  OR leisure IN ('nature_reserve'))
AND building IS NULL
...

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Nov 5, 2019

@matthijsmelissen could you respond to the comments above?

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Jan 8, 2020

@matthijsmelissen please respond to my answer to your comment above, it has been 3 months now.

@matthijsmelissen
Copy link
Collaborator

I might have mislooked, brackets look fine to me.

@jeisenbe
Copy link
Collaborator Author

Thank you for the help. If you have time, would you be able to approve this PR?

@jeisenbe jeisenbe mentioned this pull request Jan 25, 2020
@pnorman pnorman merged commit 534fa8b into gravitystorm:master Jan 28, 2020
@jeisenbe jeisenbe requested a review from pnorman January 28, 2020 06:49
@jeisenbe jeisenbe deleted the textlowzoom branch January 28, 2020 06:53
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.

3 participants