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

Switch to ST_PointOnSurface? #1644

Closed
pnorman opened this issue Jul 8, 2015 · 18 comments
Closed

Switch to ST_PointOnSurface? #1644

pnorman opened this issue Jul 8, 2015 · 18 comments

Comments

@pnorman
Copy link
Collaborator

pnorman commented Jul 8, 2015

In vector tiles, polygons cannot be used for labeling, as polygons are clipped to the vector tile bounds.

This is a particular problem for non-PostGIS datasources, but for PostGIS ones, the standard solution is to use ST_PointOnSurface. #349 found that it was not really better or worse than Mapnik label placement.

#349 (comment) raised the possibility of performance concerns. I am not sure, but there are a couple of ways this can be addressed

  1. Appropriate use of !bbox!. (SELECT ST_PointOnSurface(way) AS way FROM planet_osm_polygon) AS p does not work because the geometry filtering condition used is ST_PointOnSurface(way) && !bbox!.

    The appropriate way to write the query is

    (SELECT
    ST_PointOnSurface(way)
    FROM planet_osm_polygon
    WHERE way && !bbox!
      AND name IS NOT NULL
    ) AS p

    This will then use indexes to limit the data fetched, with a second filtering taking place in the outer select added by Mapnik. A functional index on ST_PointOnSurface(way) WHERE name IS NOT NULL could also help.

    I don't know if @mrwojo did this when testing. If not, it could easily explain any performance differences.

  2. Where this is both a point and a polygon layer, these can be combined into one query with UNION ALL

@gravitystorm

This comment has been minimized.

@imagico
Copy link
Collaborator

imagico commented Jul 9, 2015

Note resorting to ST_PointOnSurface() to accommodate vector tiles workflows would codify the current bad quality label placement even if mapnik should in the future allow better quality - you would be limited in possible improvements to such offered by the database engine which are unlikely to be within its scope. Ultimately good quality labeling is simply diametrical to the idea of a strictly tiled approach to rendering.

@pnorman

This comment has been minimized.

@HolgerJeromin

This comment has been minimized.

@gravitystorm

This comment has been minimized.

@pnorman

This comment has been minimized.

@pnorman pnorman closed this as completed Jul 10, 2015
@HolgerJeromin

This comment has been minimized.

@imagico

This comment has been minimized.

@matkoniecz

This comment has been minimized.

@imagico

This comment has been minimized.

@pnorman
Copy link
Collaborator Author

pnorman commented May 2, 2017

I'm reopening this since a few things have changed and we should look at the decision in light of them

  • We're finding more cases where Mapnik's label placement results in strange incorrect results (water label placement offset #1465 and connected tickets)
  • Invalid geometries should be gone with osm2pgsql versions from the last few years
  • Combining the point and polygon queries into one layer is a step that forks want to take (e.g. @rorym's work)
  • Doing polygon to point conversion in SQL allows for different placement methods in the future (e.g. PIA)

@pnorman pnorman reopened this May 2, 2017
@imagico
Copy link
Collaborator

imagico commented May 2, 2017

The argument in #1644 (comment) still applies i think but this is based on the assumption that Mapnik would eventually offer more sophisticated label placement - which is unlikely given that Mapnik is essentially a dead horse.

The idea that you could generate points from polygons from within SQL that are universally significantly better for labeling is probably not realistic though - this too much depends on font size, actual label text, surrounding labels and other stuff.

@pnorman
Copy link
Collaborator Author

pnorman commented Jul 27, 2017

The idea that you could generate points from polygons from within SQL that are universally significantly better for labeling is probably not realistic though - this too much depends on font size, actual label text, surrounding labels and other stuff.

Both Mapnik and ST_PointOnSurface just generate a point and place the label. If at some point there were an option that considered label size and surroundings, we could look at it, but for now, that doesn't matter.

@imagico
Copy link
Collaborator

imagico commented Jul 27, 2017

Well - i guess my main point here is that ultimately ST_PointOnSurface() is going to be a dead end regarding quality label placement but Mapnik likely is as well so it really does not matter.

@kocio-pl
Copy link
Collaborator

There are more ideas to improve Mapnik algorithm:

mapnik/mapnik#3550 (comment)

The question is if someone will try to code a better solution and which one is acceptable regarding speed and complexity.

@matthijsmelissen
Copy link
Collaborator

I added this issue to the vector tiles project.

matthijsmelissen pushed a commit to matthijsmelissen/openstreetmap-carto that referenced this issue May 12, 2018
Resolves gravitystorm#1644.

This is a necessary step towards vector rendering (see gravitystorm#1644).

As this changes the label placement algorithm for polygons, this causes differences in label placement.

(SQL deliberately not indented to increase readability of the diff.)
matthijsmelissen pushed a commit to matthijsmelissen/openstreetmap-carto that referenced this issue May 13, 2018
Resolves gravitystorm#1644.

This is a necessary step towards vector rendering (see gravitystorm#1644).

As this changes the label placement algorithm for polygons, this causes differences in label placement.

(SQL deliberately not indented to increase readability of the diff.)
matthijsmelissen pushed a commit to matthijsmelissen/openstreetmap-carto that referenced this issue Jan 12, 2019
Resolves gravitystorm#1644.

This is a necessary step towards vector rendering (see gravitystorm#1644).

This is a second trial of gravitystorm#3228, based on the word by @pnorman in gravitystorm#3233.

This is a first step, once we have this than we can probably merge the point and poly layers (for example amenity-points and amenity-points-poly).
@jeisenbe
Copy link
Collaborator

PRs #3233, #3690, #3712, #3781, and #3874 have switched the layers springs, stations, amenity=points, buildings, and amenity-low-priority to use ST_PointOnSurface.

But it was noted that addr:housenumber now displays at a different location than building names in #962 (comment)

It looks like the addresses layer should also be switched. Are there other layers that should still be changed, or will this issue then be closed?

@jeisenbe
Copy link
Collaborator

I believe all relevant layers have been switched to ST_PointOnSurface, so this issue is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants