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

Add rendering for natural=cape #3452

Merged
merged 4 commits into from
Nov 25, 2018
Merged

Add rendering for natural=cape #3452

merged 4 commits into from
Nov 25, 2018

Conversation

Adamant36
Copy link
Contributor

@Adamant36 Adamant36 commented Oct 16, 2018

This PR adds rendering for natural=cape. As it is, bays are currently rendered but capes aren't. It would be cool if they were. The code is copied from bays. Also, this PR closes #3148
taghistory
https://www.openstreetmap.org/#map=15/38.2245/-122.9640
cape example 1 z15 toms point and sand point
https://www.openstreetmap.org/#map=16/37.9637/-122.4272
cape 2 z17

@jragusa
Copy link
Contributor

jragusa commented Oct 16, 2018

Some remarks:
1/ A black colour like other terrestrial locations should better.
2/ The second location (Point San Pablo) is in italic but not the first one.
3/ Both examples are nodes, you should also try on polygons.

@Adamant36
Copy link
Contributor Author

@jragusa,

  1. I Although its a terrestrial location, its related to water and some of them are in the water. So I think blue works. Although, black might work to. I'll wait for more opinions. Maybe @kocio-pl has some thoughts on it. Here's a map of locations if you want to take a look at it. https://taginfo.openstreetmap.org/tags/natural=cape#map

  2. I don't know what the rendering issue is about. I'll have a look into it. Thanks for pointing it out.

  3. It's only suppose to be mapped as a node. So there aren't any polygons. There's 118 ways but that is less then one percent of them and they are miss tagged. So, I'm not testing them.

@jeisenbe
Copy link
Collaborator

jeisenbe commented Oct 16, 2018 via email

@Adamant36
Copy link
Contributor Author

Adamant36 commented Oct 16, 2018

@jeisenbe, I didn't even know capes existed until I did this PR and when I searched on the internet to see what they where this wasn't the type of cape that came up. Since they stick out into the ocean though, I figure they might be under water half the time like sand bars, rock islands, or other similar coastal features. Do you know anything about that? If they are a mostly a land feature that isn't under water a lot of the time, I'm fine changing it to black or dark grey. I guess islands aren't in blue either, but then they don't go under water (hopefully). I know islets are rendered in black. So are islands. So maybe black makes more sense.

@kocio-pl
Copy link
Collaborator

I have no doubt that capes should be rendered the same as islands. Blue would be inconsistent with that and may look like a label placing bug.

@hansfn
Copy link

hansfn commented Oct 16, 2018

I would say it's mostly a land feature and prefer black - FWIW.

@Adamant36
Copy link
Contributor Author

Black it is. I'm completely neutral on it. Since I know nothing about capes in the first place.

@jeisenbe
Copy link
Collaborator

jeisenbe commented Oct 16, 2018 via email

@Adamant36
Copy link
Contributor Author

Updated to black
https://www.openstreetmap.org/#map=12/33.4122/-118.5212
z10 (their all capes)
cape z10
z13 (their all capes)
cape z13
z17
cape z17
I'm not sure if z10 is the best zoom to start rendering it, but that's what the default was. So I went with it.

@jeisenbe
Copy link
Collaborator

jeisenbe commented Oct 18, 2018 via email

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

@jeisenbe This part of the code:

.text-low-zoom[zoom < 10],
.text[zoom >= 10] {

does not mean it starts with z10. It starts from z0 till z9, if such feature is defined in text-low-zoom data layer (probably that name) in project.mml, and for z10+ we check for definition in a different data layers. It's a technical split for low zoom levels (to limit the amount of data we read from a database, so performance is sane) and all the other levels.

I think we should just copy island settings, because this way we can show only big enough entities on a given zoom level (and skip their name if they are too big there). Their size is tested in each case, so we don't have to make any ugly compromises and look at the averages etc.

@jeisenbe
Copy link
Collaborator

jeisenbe commented Oct 19, 2018 via email

@kocio-pl
Copy link
Collaborator

I see - I don't think of them as nodes, but you're right, only 118 out of 11 682 is tagged as ways! So we really have to add code for nodes and for this we have to choose some initial zoom level. I'm not sure what @Adamant36 meant with z10, but it should be tested on at least few different places anyway.

@Adamant36
Copy link
Contributor Author

@kocio-pl, so skip coding it like islands then and just do more zoom level testing?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Nov 6, 2018

I would not exclude areas, but zoom level testing for nodes seems like a priority.

@Adamant36
Copy link
Contributor Author

I would not exclude areas

Even if the wiki says to map them as a node and only 350ish out of 1100 of them are mapped as areas? I ask because I tried to render them as areas by copying the code from islands and it didn't work for some reason. Whereas, rendering them as nodes, without the island code, did. So....

@kocio-pl
Copy link
Collaborator

kocio-pl commented Nov 7, 2018

If wiki definition and usage are in sync, and you have a problem with rendering areas anyway, I guess it's better to go with a node only. Changing documentation would be needed to render areas.

@jragusa
Copy link
Contributor

jragusa commented Nov 7, 2018

@Adamant36 did you add way_pixels parameters in amenity-points.mss ? I checked and it's work fine.

@Adamant36
Copy link
Contributor Author

@jragusa, I'm not sure. I'll have to check. I agree with @kocio-pl that its probably better not to render them as ways anyway. Since the wiki doesn't say to. Plus, mapping them that way seems like it would be pretty inaccurate. Since who knows where the cap would end on the land side. I'm sure it wouldn't be based on official boundaries. So, I don't feel like encouraging them to be mapped that way. Therefore, points are fine. Its not like ways can't be added later once the wiki/etc gets worked if need be anyway.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Nov 8, 2018

Plus, mapping them that way seems like it would be pretty inaccurate. Since who knows where the cap would end on the land side.

But for bays it's OK, and I consider areas to be more accurate description than nodes in general (where is the accurate place for putting the node if you don't know about the area, anyway?). For me it's just lack of documentation, I hope someone will extend it.

@andrewharvey
Copy link

I think mapping them as both ways and points is going to be most accurate, the point will say where the tip is, but the way will give the whole area of the cape or point.

The downside of only rendering nodes and not ways is that is further discourages and makes it harder for people to choose to map as ways since they won't be rendered.

The tagging discussion is probably best place at the tagging mailing list.

@Adamant36
Copy link
Contributor Author

I have no problem adding rendering for ways. The wiki says its a way to map them on the sidebar. So, I don't think its necessarily wrong. There's just nothing in the "how to map" section about it. If someone wants to update the wiki documentation though, I'm fine with waiting and adding rendering for ways after that. I don't want to add rendering for ways if there is no documentation on the wiki for it though. Although, I guess I could.

@jragusa, in the mean time can you create a branch with the code you used to ways to render correctly or paste it here? Then I can compare it to how I did it, to see what I did wrong.

project.mml Outdated Show resolved Hide resolved
@Adamant36
Copy link
Contributor Author

@HolgerJeromin, thanks for bringing it back to my attention. I had planned to go over it sometime in the next couple of days. I'll go with your code and do some tests.

@Adamant36
Copy link
Contributor Author

Adamant36 commented Nov 19, 2018

Rendering of nodes and ways

https://www.openstreetmap.org/node/1210087541
cape node z15
https://www.openstreetmap.org/#map=15/42.6744/-70.7476 (way)
cape way z16

Tests of nodes at different zoom levels

Locke's Point z13
locke s point z13
Locke's Point z14
locke s point z14
Locke's Point z15
locke s point z15
Locke's Point z16
locke s point z16
Great Boars Head z14 (z13 got blocked by highway symbol)
z14
Great Boars Head z15
z15
Great Boars Head z16
z16
Little Boars Head z13
z13
Little Boars Head z14
z14
Little Boars Head z15
z15
Little Boars Head z16
z16

Personally, id go with either z13 or z14 (I'm leaning slightly more toward z14). Also, big thanks to @jragusa for the help.

@jragusa
Copy link
Contributor

jragusa commented Nov 19, 2018

There is some large capes so why not starting from z10 as well as natural=bay and natural=strait ?

Examples in Sweden: Avanäs and Grötlingboudd

@Adamant36
Copy link
Contributor Author

@jragusa, those are both mapped as ways. Which will already be rendered starting at z10 like islands. The question of rendering zoom level is for nodes. From the small amount of research I've done, most capes that are mapped as nodes are smaller ones.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Nov 19, 2018

I guess this code makes some unnecessary queries to the database. Since we render only text, all non-text layers should not be needed. Could you check this?

This means that only these data layers should include capes:

  • text-poly-low-zoom (for z0-z9)
  • text-poly (z10+)
  • text-point (z10+)

The numbers in the brackets are indicating which zoom levels are available for styling in MSS files. Since z16+ is a subset of z10+, all the node capes will be rendered. For areas there's no zoom level limits (other than size), since the code:

.text-low-zoom[zoom < 10],
.text[zoom >= 10] {

means "for z<10 take the data from the class text-low-zoom (which is located in data layer text-poly-low-zoom):

- id: text-poly-low-zoom
class: text-low-zoom
geometry: polygon

and for z10+ take the data from the class text (which is located in data layer text-poly - also in text-point, but it will not be used in this context):

- id: text-poly
class: text
geometry: polygon

- id: text-point
class: text
geometry: point

You can see that polygons work also on z<10, since some island names are visible from z4:

https://www.openstreetmap.org/#map=4/77.31/13.93

screenshot_2018-11-20 openstreetmap

@Adamant36
Copy link
Contributor Author

@kocio-pl, you were correct so I updated it. I thought a few of them seemed unnecessary. I just wasn't sure which ones. So I appreciate the explanation.

What do you think the starting zoom level for nodes should be?

@kocio-pl
Copy link
Collaborator

Great!

What do you think the starting zoom level for nodes should be?

I'm not sure and only extensive testing can give the answer. Bays on nodes are rendered on z14+, so I would go with the same value initially.

@Adamant36
Copy link
Contributor Author

There's some tests a couple messages up. Personally I like 14, even more so known that's what it is for bays, but I'll do some more tests anyway.

@kocio-pl
Copy link
Collaborator

Hi, did you have an opportunity to test it?

@Adamant36
Copy link
Contributor Author

Pontius Point Z10
pontius point z10
Pontius Point Z11
pontius point z11
Pontius Point Z12
pontious point z12
Pontius Point Z13
pontius point z13
Pontius Point Z14
pointious point z14
Pontius Point Z15
pontius point z15
Rattle Snake Point z10
rattle snake point z10
Rattle Snake Point z11
rattle snake point z11
Rattle Snake Point z12 (doesn't display due to highway shield)
rattle snake point z12 doesn't display due to highway shield
Rattle Snake Point z13
rattle snake point z13
Rattle Snake Point z14
rattle snake point z14
Rattle Snake Point z15
rattle snake point z15
Random Place With a Bunch of Them Z10
random place with a bunch of them z10
Random Place With a Bunch of Them Z11
random place with a bunch of them z11
Random Place With a Bunch of Them Z12
random place with a bunch of them z12
Random Place With a Bunch of Them Z13
random place with a bunch of them z13
Random Place With a Bunch of Them Z14
random place with a bunch of them z14
Random Place With a Bunch of Them Z15
random place with a bunch of them z15
Random Place With a Bunch of Them Two Z10
random place with a bunch of them two z10
Random Place With a Bunch of Them Two Z11
random place with a bunch of them two z11
Random Place With a Bunch of Them Two Z12
random place with a bunch of them two z12
Random Place With a Bunch of Them Two Z13
random place with a bunch of them two z13
Random Place With a Bunch of Them Two Z14
random place with a bunch of them two z14
Random Place With a Bunch of Them two Z15
random place with a bunch of them two z15

@kocio-pl
Copy link
Collaborator

Oneida Lake tells me that z14+ should be OK. We can be a bit shy here and not go to z13, because nodes are less sure for rendering than areas (we're just guessing how big are them).

https://www.openstreetmap.org/#map=14/43.1564/-75.8424

@Adamant36
Copy link
Contributor Author

@kocio-pl, sounds good. I updated it. It should be good to go.

@kocio-pl kocio-pl merged commit 6b88c40 into gravitystorm:master Nov 25, 2018
@kocio-pl
Copy link
Collaborator

kocio-pl commented Nov 25, 2018

Thanks, testing works OK.

I have found a nice example of node bay and node cape being close to each other, so it's good to show them from the same zoom level and z14+ seems to be good:

https://www.openstreetmap.org/#map=14/53.9445/21.7355

screenshot_2018-11-25 openstreetmap carto kosmtik

@Adamant36 Adamant36 deleted the cape branch November 28, 2018 09:42
@geozeisig
Copy link

If I interpreted it correctly caps will be rendered similar to islands depending on size ([way_pixels > 3000]). That is, if the area is large enough, the cap is rendered from Z4. So I wrote on the wiki page that large caps should be mapped with an area. But that was reverted. Does anyone know if you can map caps with areas?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 5, 2018

I think yes, you just wrote how to do it, but area type is still explicitly allowed in the infobox. It's just @imagico has different opinion on area tagging, but that should be discussed on the wiki discussion page or Tagging list, not here.

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 natural=cape (headlands)
8 participants