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

Move parking back to amenity-points layers, change way_pixels limit and fix initial zoom level #3923

Merged
merged 3 commits into from
Oct 25, 2019

Conversation

jeisenbe
Copy link
Collaborator

@jeisenbe jeisenbe commented Oct 5, 2019

Fixes #3908
Partial revert of #3874
Follow-up to fix bug in #3612

Changes proposed in this pull request:

1) Move parking back to amenity-points layers

  • Moves amenity=parking, amenity=motorcycle_parking, amenity=bicycle_parking and amenity=parking_entrance to amenity-points and text-point layers instead of amenity-low-priority and text-low-priority
  • This fixes bugs where the building text or address number is shown instead of the icon and parking text, introduced by Fix low priority layers #3874
  • Parking had previously been rendered in both amenity-points and amenity-low-priority.
  • Recently PR Fix low priority layers #3874 had consolidated parking rendering in low-priority only, but this leads to problems when there is a building or an address on the same feature
  • This commit moves parking back to the amenity-points layer, but near the end.
  • The minzoom for text-low-priority is now changed back to z17 since there are no longer any features rendered sooner that this in the layer.

2) Change initial zoom level to be z14 for both text label and icon

  • This bug was introduced by my PR Render parking at z14 and greater #3612 where the initial zoom was changed to z14 for the icon, but not for the text.
  • In PR Render parking at z14 and greater #3612 it was planned to render parking from z14 only, however this was not changed for the text labels, only for the icon.
  • This commit fixes the parking rendering at z10 to z13, though in practice almost no parking lots are large enough to have been rendered at these levels.

3) Change way_pixels limit for parking icon and text

  • Start showing icon at 750 pixels instead of 900, but don't show text label till 3000 way_pixels, as with most other features
  • Only amenity=marketplace and parking have an icon that appears based on way_pixels. The other uses 3000 way_pixels, which is the limit also used for all text labels.
  • Since the icon is only 14 x 14 pixels, it's reasonable to show it one zoom level sooner than the text, at 750 way_pixels (1/4 of 3000 - so 1 zoom level sooner)

Test renderings

Hanau, Germany

https://www.openstreetmap.org/way/221032567#map=17/50.13289/8.92118
z16 compare current (left), after (right)
z16-parkhaus-compare

z17
z17-parkhaus-after

z18
z18-parkhaus-compare

Montpellier, France

https://www.openstreetmap.org/#map=17/43.61338/3.88557

z16 comparison (left before, right after)
z16-parking-joffre-compare

z17
z17-parking-joffre-compare

Parking had previously been rendered in both amenity-points and amenity-low-priority.
Recently PR gravitystorm#3874 had consolidated parking rendering in low-priority only, but this leads to problems when there is a building or an address on the same feature
This commit moves parking back to the amenity-points layer, but near the end. The minzoom for text-low-priority is now changed back to z17
In PR gravitystorm#3612 it was planned to render parking from z14 only, however this was not changed for the text labels, only for the icon.
This commit fixes the parking rendering at z10 to z13, though in practice almost no parking lots are large enough to have been rendered at these levels.
Start showing icon at 750 pixels instead of 900, but don't show text label till 3000 way_pixels, as with most other features
@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Oct 5, 2019

I tried the previous rendering at 900 way_pixels for the icon and text, but I think this often renders the text outside of the parking area. This can be confusing, and over-emphasizes small parking lots.

Here's Hanau (same as above) at z17 but with 900 way_pixels limit:
https://www.openstreetmap.org/way/221032567#map=17/50.13289/8.92118
z17-parking-compare-900-way_pixels

This doesn't work well for long, narrow parking areas.

@jeisenbe jeisenbe added the bug label Oct 5, 2019
@jeisenbe jeisenbe mentioned this pull request Oct 19, 2019
@jeisenbe
Copy link
Collaborator Author

Does anyone have time to review this PR? I would love to get it in the next release if possible.

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

Seems to work fine. A few comments:

  • this does essentially disable the idea of a separate low priority point symbol layer for all but the highest zoom levels - but this was not really working anyway so this is in a way a good thing.
  • moving the parking features to the end of the COALESCE() only has the effect of giving these tags less priority than other tags on the same feature, it does not give the parking features and their symbols lower priority than other features. So this change will likely aggravate the problem of a lack of proper prioritization of POI symbols.
  • if this change actually fixes a bug can be debated because the tagging of name tags in cases where it can refer to different things (name of building vs. name of parking) is ambiguous. It is fine with me to solve it this way - though the order of the COALESCE() (see above) becomes a highly significant choice on priorities here. Placing parkings last there could be confusing because in the landcover layer these have fairly high priority so symbol and label might often not match the fill color.

@matkoniecz
Copy link
Contributor

amenity-points and text-point layers instead of amenity-low-priority and text-low-priority

It was intended as to prevent parking icon from blocking for example shop icon or museum icon. Is it now achieved using a different method or considered as unimportant or it was not working anyway?

@jeisenbe
Copy link
Collaborator Author

The problem was that parking garages, tagged building=* + amenity=parking were rendered with the building text and no icon, since amenity=parking was only in the low-priority layer after my last PR. Also, any other amenity-points icon would block parking features, even though they would otherwise render rather early if they are large enough (z10 minimum in theory, though in practice at z14 for very large parking lots). This worsens issue #3880.

To properly get the priority of icons right in all cases, we need to fix #3880 in a general way, or consider how we want to set the priority of all the amenity-points icons (and text) in a consistent way.

@matkoniecz
Copy link
Contributor

any other amenity-points icon would block parking features

That is why parkings were in both amenity and amenity-low-priority (maybe it was overengineered and supporting only just parkings and therefore not a proper way to do this) but it seemed to work.

@jeisenbe
Copy link
Collaborator Author

After PR #3612 amenity=parking icons and text were rendering in the regular amenity-points layers as soon as z14 if imported as a large enough polygon (>900 square pixels at each zoom level). From z17 points and small areas were also rendered in the low-priority layers. Potential duplicate rendering was blocked by the later icons and text colliding with the previously rendered icon or text. But this then led to a visible duplicate rendering when we switched the amenity-points layers to use ST_PointOnSurface, before switching the low-priority layer.

I initially tried fixing this by switching all parking into the low-priority layer only, but #3908 is a problem - icons and the parking text style never showed for parking garages, which are usually also tagged building=

In theory we could restore the previous duplicate rendering, with only larger polygons rendered in the main amenity-points layers, and nodes/small areas only rendered in low-priority, but this would risk #3908 reappearing in the future if changes were made to how the label/icon position is calculated for one of the layers.

But I would recommend merging this PR now, which will fix the rendering in most cases, and then discuss next week whether the nodes / small parking areas should instead be rendered in the low-priority layer. Alternately, we might think about how to solve the problems with prioritization in a general way by fixing #3880

@jeisenbe
Copy link
Collaborator Author

@matkoniecz - would you like to request changes formally, or do we have consensus to merge this PR today, before the release of v4.24.0?

@matkoniecz
Copy link
Contributor

I just wanted to check whatever it was a deliberate decision to drop this part. I am not going to block merge.

@jeisenbe
Copy link
Collaborator Author

OK, I will merge this now and we can see if we can solve the prioritization problem in general, but if not then we can consider using the low-priority layer again.

@jeisenbe jeisenbe merged commit 7020aea into gravitystorm:master Oct 25, 2019
@jeisenbe jeisenbe deleted the parking branch October 25, 2019 13:36
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.

amenity=parking icon no longer rendered on buildings
3 participants