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

Update leisure=golf_course color and render golf=* features (gravitystorm#661) #4381

Merged
merged 10 commits into from
Sep 11, 2021

Conversation

jgruca
Copy link
Contributor

@jgruca jgruca commented Apr 21, 2021

(Comment updated June 5 to current state of PR, test renderings also updated)

Fixes #661

Changes proposed in this pull request:

  • Change leisure=golf_course/miniature_golf_course to @campsite, eliminating @golf_course color
  • Update landcover layer to include golf area features
  • Update amenity-points layer to include golf point features
  • Add golf-line layer
  • Add style/golf.mss and specify rendering for:
    • golf=tee/fairway/driving_range (@grass, z>=12)
    • golf=green (@pitch, z>=13)
    • golf=bunker (@sand, z>=13)
    • golf=rough (@grass, z>=12; with texture, z>=15)
    • golf=hole as way (solid @address-color line, labeled with ref or name, z>=16)
    • golf=hole as point and golf=pin (symbols/golf_pin.svg; labeled with ref in @leisure-green, z>=16)

See #4212 for previous discussion of how these colors and textures were chosen. Corrections, feedback, critiques welcome.

Test rendering with links to the example places (before/after):

https://www.openstreetmap.org/#map=15/41.7849/-93.7247

z15
loc1-z15
z16
loc1-z16
z17
loc1-z17

https://www.openstreetmap.org/#map=14/41.8705/-93.2144

z14
loc2-z14
z15
loc2-z15
z16
loc2-z16
z17
loc2-z17

https://www.openstreetmap.org/#map=15/41.2767/-95.7367

z15
loc3-z15
z16
loc3-z16
z17
loc3-z17

- Change leisure=golf_course/miniature_golf_course to @campsite, eliminating @golf_course color
- Update landcover layer to include golf area features
- Update amenity-points layer to include golf point features
- Add golf-line layer
- Add style/golf.mss and specify rendering for:
  - golf=tee/fairway/driving_range (@grass, z>=12)
  - golf=green (@pitch, z>=13)
  - golf=bunker (@sand, z>=13)
  - golf=rough (@grass, z>=12; with texture, z>=15)
  - golf=hole as way (solid @leisure-green line, labeled with ref, z>=16)
  - golf=hole as point and golf=pin (symbols/golf_pin.svg; labeled with ref in @leisure-green, z>=16)
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.

I think the polygon fill colors are overall a good choice - though the rough color is IMO too close to scrub color and too different from grass color. A rough is AFAIK usually still just longer grass so a much darker color indicating something different is a bit misleading. See the following comparison with how the ac-style renders it:

this
ac-style

I also don't think it is a good idea to add further labels/linework/symbols in green color that are not strictly vegetation related. We would most certainly maneuver us seeing eyes into a situation similar to that with blue cycleways conflicting with the use of blue for water related features.

The hole symbol not being centered at the base of the flag seems to be a bit confusing to me.

project.mml Outdated Show resolved Hide resolved
style/golf.mss Show resolved Hide resolved
style/landcover.mss Show resolved Hide resolved
- Adjust rough color to be lighter
- Change golf-lines colors to be @admin-boundaries
- Modify golf_pin.svg with invisible bounding box to work around
  mapnik#1122
- Remove explicit bbox condition in SQL
- Leave @golf_course variable defined
@jgruca
Copy link
Contributor Author

jgruca commented Apr 22, 2021

I think the polygon fill colors are overall a good choice - though the rough color is IMO too close to scrub color and too different from grass color.

I've updated the rough color; see screenshots below.

I also don't think it is a good idea to add further labels/linework/symbols in green color that are not strictly vegetation related.

I tried changing these to @admin-boundaries and like the appearance; it also makes a little sense to me to use a boundary color for the play path since it is not observable on the ground, similar to a boundary. However, I'm open to suggestions on the best color to use here.

The hole symbol not being centered at the base of the flag seems to be a bit confusing to me.

This appears to be a bug in mapnik with a workaround. I've adjusted the SVG file so the bounding box of the marker is calculated correctly.

Updated images:

https://www.openstreetmap.org/#map=15/41.7849/-93.7247

z15
loc1-z15
z16
loc1-z16
z17
loc1-z17

https://www.openstreetmap.org/#map=14/41.8705/-93.2144

z14
loc2-z14
z15
loc2-z15
z16
loc2-z16
z17
loc2-z17

https://www.openstreetmap.org/#map=15/41.2767/-95.7367

z15
loc3-z15
z16
loc3-z16
z17
loc3-z17

@neuhausr
Copy link

I like this! My only criticism is that the color of the greens seems a little too close to the color of the water. Maybe modify it to be less blue. #B5E3B5, which I think is the current carto golf course fill, might work?

Also a question: if a hole has a name and a ref, how will that be handled? It would probably be ideal to include both ref and name, at least at higher zoom levels. (For example see the Jubilee Course at St Andrew's)

…ift pin SVG instead of changing SVG file, make sure golf=pin ref label actually renders
@jgruca
Copy link
Contributor Author

jgruca commented Apr 30, 2021

My only criticism is that the color of the greens seems a little too close to the color of the water.

I agree. This came up in the previous PR too, with @imagico agreeing to some extent. I'm trying to build consensus, though, and so far consensus seems to mean @pitch for the green. I'd be happy to hear from @imagico and @jeisenbe on acceptable alternatives; #B5E3B5 does look decent, as does @pitch sent through polygon-comp-op: lighten. But both of those options introduce single-use shades of green back into the style.

Also a question: if a hole has a name and a ref, how will that be handled? It would probably be ideal to include both ref and name, at least at higher zoom levels.

It looks like the tile.openstreetmap.fr renderer will display hole name on lines and fall back to ref, so I have copied that behavior on the latest commit. Thanks for pointing out Jubilee Course as an example, thought, because that made me realize my PR did not correctly render ref on golf=pin (a less common combo, but perhaps what you were talking about for a hole with a name and ref). I have fixed that as well.

image

@imagico
Copy link
Collaborator

imagico commented May 1, 2021

If the pitch color has issue with being too similar to other colors with a very different meaning the right way is to change the pitch color, not to introduce a new color to address the issue only for new color uses. Current pitch color was selected in #2363.

I don't think introducing yet another point symbol color and overloading the dark purple for administrative boundaries for other stuff are a good idea. Generally speaking dark purple is always a very problematic color where mixing with other (background) colors happens because it leads to all kind of weird colors in combination with the green tones in many areas dominating the base map (in particular also on golf courses). For the lines i would probably go either with a neutral color or try a very feint green drawn directly above the landcover and water layers. Not sure if that would work though, just an idea.

@jgruca
Copy link
Contributor Author

jgruca commented May 2, 2021

Thanks for the input. Using @address-color (#666666, dark gray) for the lines/labels/markers seems like a good neutral yet visible color, and semantically the hole ref seems similar to its address.

image

image

[...] the right way is to change the pitch color [...]

I don't disagree. Is that something that should be proposed in a separate PR?

@jgruca
Copy link
Contributor Author

jgruca commented Jun 5, 2021

I updated the first comment in this PR to match the current state of the PR. Comments welcome.

@imagico
Copy link
Collaborator

imagico commented Jul 17, 2021

Like with #4331 this should be reviewed by someone else because i am a bit prejudiced by having picked a different design approach to this elsewhere.

@imagico imagico dismissed their stale review July 17, 2021 11:59

Dismissing as stale because there have been fundamental changes since then

@michaelabon
Copy link

I'd love to have something like this merged, as someone who loves mapping golf courses. Most existing golf courses I see are mapping for the renderer, despite the wiki saying:

Do not use the tag landuse=grass in order to make the rendering more appealing. Use surface=grass.

I don't make sweeping changes to existing golf courses when I see people having used landuse=grass, but it makes me a little sad every time I see it. This PR would go a long way towards helping address the mis-tagging issue.

@pnorman
Copy link
Collaborator

pnorman commented Jul 28, 2021

Reviewing isn't just for maintainers. Everyone can review.

@pnorman pnorman self-requested a review July 28, 2021 22:43
@vincentvd1
Copy link

vincentvd1 commented Aug 10, 2021

I like the new color. Maybe one thing to consider. In issue #3143 it was also proposed to change the greens in order to be able to render landuse=meadow and natural=grassland differently. There it was proposed to give campsite the same color as the golfcourse (the old color) and use the campsite color for landuse=meadow ( you now used the old campsite color or golf courses).

I don't have a problem with the new golf course color, it is just because of the limited amount of available greens, it is good to also take this into account. Maybe (in a new PR for #3143) the new golf course color can be used for both golf courses and meadows. The campsite then needs to be changed to the green of leisure (see image at the end of #3143).

@imagico imagico mentioned this pull request Aug 29, 2021
Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

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

I still need to review the cartography, but found some technical issues.

project.mml Outdated
@@ -2024,7 +2041,8 @@ Layer:
OR leisure IN ('slipway', 'track')
OR waterway IN ('dam', 'weir')
OR "natural" IN ('arete', 'cliff', 'ridge'))
AND name IS NOT NULL
AND name IS NOT NULL)
OR tags @> 'golf=>hole'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to have negative performance impacts since it will be unable to use the planet_osm_line_name index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the best way to address this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking WHERE ([existing conditions] AND name IS NOT NULL) OR ([new conditions] AND ref IS NOT NULL) then rename the planet_osm_line_name index to planet_osm_line_label and make it have the condition name IS NOT NULL OR ref IS NOT NULL, but it could use more thought and testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I concur - we also have other queries from planet_osm_line which have a ref IS NOT NULL condition, that is roads-text-ref and roads-text-ref-minor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the index defined in indexes.sql/indexes.yml and reformatted the query's condition as described.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran EXPLAIN on a sample query, and it does appear the index is being used and has a positive effect on query cost.

With only planet_osm_line_name:

 Bitmap Heap Scan on planet_osm_line  (cost=55.42..3376.13 rows=7 width=85)
   Recheck Cond: (way && '<snip>'::geometry)
   Filter: ((((man_made = ANY ('{pier,breakwater,groyne,embankment}'::text[])) OR ((man_made = 'pipeline'::text) AND ((tags -> 'location'::text) =
ANY ('{overground,overhead,surface,outdoor}'::text[]))) OR (bridge = ANY ('{yes,aqueduct,cantilever,covered,trestle,viaduct}'::text[])) OR (tags @>
 '"attraction"=>"water_slide"'::hstore) OR (aerialway = ANY ('{cable_car,gondola,mixed_lift,goods,chair_lift,drag_lift,t-bar,j-bar,platter,rope_tow
,zip_line}'::text[])) OR (leisure = ANY ('{slipway,track}'::text[])) OR (waterway = ANY ('{dam,weir}'::text[])) OR ("natural" = ANY ('{arete,cliff,
ridge}'::text[]))) AND (name IS NOT NULL)) OR ((tags @> '"golf"=>"hole"'::hstore) AND (ref IS NOT NULL)))
   ->  Bitmap Index Scan on planet_osm_line_way_idx  (cost=0.00..55.42 rows=951 width=0)
         Index Cond: (way && '<snip>'::geometry)

After creating planet_osm_line_label:

 Bitmap Heap Scan on planet_osm_line  (cost=18.47..1116.75 rows=7 width=85)
   Recheck Cond: ((way && '<snip>'::geometry) AND ((name IS NOT NULL) OR (ref IS NOT NULL)))
   Filter: ((((man_made = ANY ('{pier,breakwater,groyne,embankment}'::text[])) OR ((man_made = 'pipeline'::text) AND ((tags -> 'location'::text) =
ANY ('{overground,overhead,surface,outdoor}'::text[]))) OR (bridge = ANY ('{yes,aqueduct,cantilever,covered,trestle,viaduct}'::text[])) OR (tags @>
 '"attraction"=>"water_slide"'::hstore) OR (aerialway = ANY ('{cable_car,gondola,mixed_lift,goods,chair_lift,drag_lift,t-bar,j-bar,platter,rope_tow
,zip_line}'::text[])) OR (leisure = ANY ('{slipway,track}'::text[])) OR (waterway = ANY ('{dam,weir}'::text[])) OR ("natural" = ANY ('{arete,cliff,
ridge}'::text[]))) AND (name IS NOT NULL)) OR ((tags @> '"golf"=>"hole"'::hstore) AND (ref IS NOT NULL)))
   ->  Bitmap Index Scan on planet_osm_line_label  (cost=0.00..18.47 rows=292 width=0)
         Index Cond: (way && '<snip>'::geometry)

style/golf.mss Show resolved Hide resolved
symbols/golf_pin.svg Outdated Show resolved Hide resolved
project.mml Outdated Show resolved Hide resolved
style/golf.mss Outdated Show resolved Hide resolved
style/golf.mss Outdated Show resolved Hide resolved
jgruca and others added 4 commits August 30, 2021 18:41
* Use consistent JSON operator
* Remove scaled-up marker
* Use marker-geometry-transform instead of marker-transform for offset
* Set default text label to eliminate extra rule
…e rows where ref IS NOT NULL

Update text-line layer query to take advantage of the updated index
Use invisible rectangle to center golf_pin.svg
Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

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

Just a couple minor technical changes, then it looks ready to go

project.mml Outdated Show resolved Hide resolved
project.mml Outdated Show resolved Hide resolved
jgruca and others added 2 commits September 11, 2021 14:31
Co-authored-by: Paul Norman <penorman@mac.com>
Co-authored-by: Paul Norman <penorman@mac.com>
@pnorman pnorman merged commit ac82b4a into gravitystorm:master Sep 11, 2021
@jgruca
Copy link
Contributor Author

jgruca commented Sep 11, 2021

Thanks, everyone!

@ZeLonewolf
Copy link
Contributor

I am seeing the following error, and I'm wondering if it might be related to this PR?

kosmtik_1  | Postgis Plugin: ERROR:  Unexpected end of string
kosmtik_1  | LINE 4: ...'waterway_' || waterway, 'natural_' || "natural", 'golf_' ||...
kosmtik_1  |                                                              ^
kosmtik_1  | in executeQuery Full sql was: 'SELECT * FROM (SELECT
kosmtik_1  |   way,
kosmtik_1  |     NULL as way_pixels,
kosmtik_1  |     COALESCE('aerialway_' || aerialway, 'attraction_' || CASE WHEN tags @> 'attraction=>water_slide' THEN 'water_slide' END, 'leisure_' || leisure, 'man_made_' || man_made, 'waterway_' || waterway, 'natural_' || "natural", 'golf_' || tags->'golf') AS feature,
kosmtik_1  |     access,
kosmtik_1  |     name,
kosmtik_1  |     tags->'operator' as operator,
kosmtik_1  |     ref,
kosmtik_1  |     NULL AS way_area,
kosmtik_1  |     CASE WHEN building = 'no' OR building IS NULL THEN 'no' ELSE 'yes' END AS is_building
kosmtik_1  |   FROM planet_osm_line
kosmtik_1  |   WHERE ((man_made IN ('pier', 'breakwater', 'groyne', 'embankment')
kosmtik_1  |       OR (man_made = 'pipeline'
kosmtik_1  |          AND tags-> 'location' IN ('overground', 'overhead', 'surface', 'outdoor')
kosmtik_1  |          OR bridge IN ('yes', 'aqueduct', 'cantilever', 'covered', 'trestle', 'viaduct'))
kosmtik_1  |       OR tags @> 'attraction=>water_slide'
kosmtik_1  |       OR aerialway IN ('cable_car', 'gondola', 'mixed_lift', 'goods', 'chair_lift', 'drag_lift', 't-bar', 'j-bar', 'platter', 'rope_tow', 'zip_line')
kosmtik_1  |       OR leisure IN ('slipway', 'track')
kosmtik_1  |       OR waterway IN ('dam', 'weir')
kosmtik_1  |       OR "natural" IN ('arete', 'cliff', 'ridge'))
kosmtik_1  |     AND name IS NOT NULL)
kosmtik_1  |     OR (tags @> 'golf=>hole' AND ref IS NOT NULL)
kosmtik_1  | ) AS text_line LIMIT 0'
kosmtik_1  |   encountered during parsing of layer 'text-line' in Layer
kosmtik_1  | [httpserver] /openstreetmap-carto/tile/4/5/6.png?t=1631407022029 500

Copy link
Contributor

@ZeLonewolf ZeLonewolf left a comment

Choose a reason for hiding this comment

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

This change appears to be causing SQL errors that I don't fully understand. When I remove the "natural", 'golf_' || tags->'golf' code, it compiles normally.

@pnorman
Copy link
Collaborator

pnorman commented Sep 12, 2021

Fixed in 871804d

@DaveF63
Copy link

DaveF63 commented Sep 29, 2021

Even though I don't like golf, this is a good addition to the OSM 'standard' map. It's these details which makes it stand out from its competitors.

I concur with the feeling that golf=green (@pitch) is a bit too blue, especially given it's shape & size making it look similar to a pond. There /are/ differences between it & water features, but they have to be studied intently to notice them.

Aside: I've never been keen on the current @pitch colour being used on sports pitches. Should be a bit 'greener' IMO.

Edit: To check - Are all landuse/landcover/natural/surface tags being ignored?

@jgruca jgruca deleted the render-golf-features-2 branch September 29, 2021 13:24
@boothym
Copy link
Contributor

boothym commented Sep 29, 2021

What about reusing the old golf course colour - rgb(181 227 181) - for golf greens, as that is less blue than the current pitch colour?

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

Successfully merging this pull request may close these issues.

Add rendering for golf=*
10 participants