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

Fix low priority layers #3874

Merged
merged 7 commits into from
Sep 20, 2019
Merged

Fix low priority layers #3874

merged 7 commits into from
Sep 20, 2019

Conversation

jeisenbe
Copy link
Collaborator

@jeisenbe jeisenbe commented Sep 7, 2019

Fix low priority layers

Fixes #3860

Changes proposed in this pull request:

  • Remove duplicate parking, motorcycle_parking, bicycle_parking, wayside_shrine, wayside_cross and man_made_cross from amenity-points layer.
  • Create text-low-priority layer to render text labels of parkings and wayside shrines/crosses
  • Move parking_entrance to low-priority-point layer, to match other parking features
  • Fix rendering text labels for parking on nodes
  • Adjust initial zoom level for amenity=parking nodes from z17 to z18 to match bicycle_parking and motorcycle_parking - (the code would be more complex otherwise). - removed
  • Use ST_PointOnSurface to combine amenity-low-priority and amenity-low-priority-poly into one layer, and re-use the same SQL table for low-priority text labels layer.

Test rendering:

https://www.openstreetmap.org/#map=18/-12.35622/130.88415

Before
z18-pasteur-road-before

After
z18-pasteur-road-after

After (ST_PointOnSurface):
z18-pasteur-after-st-point-on-surface

Move parking_entrance to low-priority, remove parking duplicate, Remove duplicate wayside_cross rendering, Make #text-low-priority section
Currently amenity=parking is duplicated in the amenity-points layer and the amenity-low-priority layers, this commit removes the duplicate from amenity-points.
Also amenity=parking_entrance is moved from amenity-points to the amenity-low-priority layers to match the other parking features
New low priority text layers are created as well, and text code is fixed so parking lot text renders on nodes at >z18 now
@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Sep 7, 2019

I've also created another branch which has the same changes, but with both amenity-low-priority point and poly layers combined into one with ST_PointOnSurface, and the text layer based off of this.

See: https://github.com/jeisenbe/openstreetmap-carto/tree/parking-combined and commit f438059

I can add that commit to this PR if desired, or we can discuss it later, since it's not required to fix these bugs.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Sep 7, 2019

Rendering with ST_PointOnSurface; position is slightly less central:

z18-pasteur-after-st-point-on-surface

@imagico
Copy link
Collaborator

imagico commented Sep 7, 2019

I think if you change this you should also use the same approach as in amenity-points. If ST_PointOnSurface() leads to acceptable label positions is a different issue - if it is good enough for amenity-points it is also good enough for low priority. 😉

In any case please make sure the rendering rules are the same as before and any change in actual rendering behavior is made separately. From a quick look it seems you are not rendering parkings with way_pixels<=900 any more, not even at z18+.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 7, 2019

As a general note: I was wondering what is the difference between the weighted-centroid algorithm carefully tailored by @talaj for Mapnik and stock ST_PointOnSurface() function. Sadly the effect is disappointing for me in many places. However for me it is still acceptable (good enough, as you said), so I think we should stick to this at least to avoid double position rendering.

@talaj
Copy link

talaj commented Sep 7, 2019

According to this answer ST_PointOnSurface() works the same as the former interior algorithm in Mapnik was working. What is the problem with the new interior algorithm?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 7, 2019

It was both substantially higher speed and much less code complexity - see #3712 (comment).

Hm, it would be great to deploy new algorithm directly in PostGIS too, if possible. What do you think about it?

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Sep 8, 2019

I think if you change this you should also use the same approach as in amenity-points

Done. I've pushed the commit that was shown.

please make sure the rendering rules are the same as before and any change in actual rendering behavior is made separately. From a quick look it seems you are not rendering parkings with way_pixels<=900 any more, not even at z18+.

D@mn it Jim, I'm a doctor, not a computer engineer!

Oops. I'll fix that.

(If we reconsider this, should teeny tiny parking smaller than 30 x 30 pixels have an icon or text label even at z18? I suspect z18 was picked back before we had z19 rendered on openstreetmap.org - Perhaps this should be changed to z19 in a separate PR.)

Always show amenity=parking icon at z17 and other parking icons at z18 even if area is too small
Show text at z18 and above always. This matches the previous behavior.
@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Sep 8, 2019

it seems you are not rendering parkings with way_pixels<=900 any more, not even at z18+

I've pushed a new commit which fixes this, and tested the rendering. Should be ready for review/merge now.

Fix rendering of text labels for wayside_shrine by adding text-dy 10
@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Sep 8, 2019

Ok, now it's ready: latest commit removes duplicate rendering of wayside_shrine and adds text-dy: 10 needed for rendering of wayside_shrine name labels.

Now all the features in the amenity-low-priority layer should only render in this layer, I believe, except for waste_disposal areas which were intentionally treated differently than nodes.

@talaj
Copy link

talaj commented Sep 8, 2019

it would be great to deploy new algorithm directly in PostGIS too

@kocio-pl It is possible. GEOS library should accept it. Once it's there, it should be doable to convince PostGIS to use it.

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.

Overall looks fine at a quick glance - except for the mentioned things.

More generally speaking the whole concept of low and high priority POIs does not really work very well if the low priority ones start appearing at z10 already and then - because they are low priority - vanish as new POI types appear in the high priority layer as you zoom in. But that is a different can of worms obviously.

amenity-points.mss Outdated Show resolved Hide resolved
project.mml Show resolved Hide resolved
@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 8, 2019

GEOS library should accept it. Once it's there, it should be doable to convince PostGIS to use it.

Considering weak choices the old algorithm is making, I think that would be a win/win for us and general audience of GEOS users (which includes some important projects: https://en.wikipedia.org/wiki/JTS_Topology_Suite#Applications_Using_GEOS ).

The only question is who could do it. @talaj Would you like to try that or you prefer not to?

@talaj
Copy link

talaj commented Sep 8, 2019

Would you like to try that or you prefer not to?

@kocio-pl I would like to do that but my spare time and energy is quite limited, so I cannot promise anything.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 8, 2019

I perfectly understand you, there's no need to make any promises. If you would like to use some help to make it happen, don't hesitate to ask.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Sep 8, 2019

I've pushed a new commit which uses an id based selector for #text-low-priority and #amenity-low-priority, since there are no longer separate point and polygon layers.

@jeisenbe
Copy link
Collaborator Author

@imagico the changes you requested have been made. Ready to approve?

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.

Looks good - but there is some weirdness taken over from the previous zoom level logic that does not make a lot of sense.

One potential problem this change could create is that oneway arrows will have priority over parking symbols which is not really such a sensible prioritization. But that is definitely a minor issue compared to what this fixes.

[feature = 'amenity_bicycle_parking'],
[feature = 'amenity_motorcycle_parking'],
[feature = 'amenity_parking_entrance'] {
[zoom >= 10][way_pixels > 900],
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this is not introduced by your change this is weird - for large parking labels seems to start at z10 while the symbol does not start before z14 - which does not make much sense.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Sep 16, 2019 via email

@jeisenbe jeisenbe mentioned this pull request Sep 17, 2019
This change allows points to be rendered the same as areas with way_pixels=0, simplifiying the CSS code
@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Sep 18, 2019

I've pushed a new commit which uses COALESCE(way_area/NULLIF(POW(!scale_denominator!*0.001*0.28,2),0), 0) AS way_pixels as in PR #3878 so that text labels render on parking nodes again at z18 and higher. I've tested it. @imagico, would you have the time to review this new commit?

@imagico
Copy link
Collaborator

imagico commented Sep 18, 2019

There is no way_pixels condition on z18 and above for parkings so i don't think this change makes sense here.

@jeisenbe
Copy link
Collaborator Author

OK, I realize now that the prior commits had already fixed the rendering on nodes, which was broken when done from the amenity-points layer. I was thinking that it would be good to use the same code COALESCE(way_area/NULLIF(POW(!scale_denominator!*0.001*0.28,2),0), 0) AS way_pixels in the amenity-low-priority layer so that in the future, contributors will not be confused why this layer works differently than amenity-points.

But that's not really necessary for this PR, so I will revert to the previously reviewed commit.

@jeisenbe jeisenbe merged commit 792a6c5 into gravitystorm:master Sep 20, 2019
@jeisenbe jeisenbe deleted the parking branch September 20, 2019 04:24
jeisenbe added a commit to jeisenbe/openstreetmap-carto that referenced this pull request Sep 20, 2019
jeisenbe added a commit to jeisenbe/openstreetmap-carto that referenced this pull request Oct 5, 2019
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
jeisenbe added a commit that referenced this pull request Oct 25, 2019
…nd fix initial zoom level (#3923)

* Move parking back to amenity-points layers

Parking had previously been rendered in both amenity-points and amenity-low-priority.
Recently PR #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
The minzoom for text-low-priority is changed back to z17

* Change parking text intial zoom to z14

In PR #3612 it was planned to render parking from z14 only, however this was not yet changed for the text labels, only for the icon.

* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

amenity=parking rendered with two parking icons
4 participants