gravitystorm / openstreetmap-carto

A general-purpose OpenStreetMap mapnik style, in CartoCSS
Other
1.53k stars 818 forks source link

Rendering of leisure=beach_resort #2440

Closed Karthoo closed 6 years ago

Karthoo commented 7 years ago

Leisure=beach_resort is an important tag in touristical regions. The wiki tells us, that we have to tag this instead of natural=beach, if the beach area is managed. Because of that, there is a white hole on the map in some coastal areas and that doesn't look very good. Maybe you could use the same colour like on natural=beach, but with an parasol symbol in the center.

dieterdreist commented 7 years ago

2016-11-14 21:42 GMT+01:00 doktorpixel14 notifications@github.com:

The wiki tells us, that we have to tag this instead of natural=beach,

that's orthogonal. You'll tag the beach as natural=beach and a beach resort as leisure=beach_resort, and both tags can be valid at the same spot.

Karthoo commented 7 years ago

"Use natural=beach for areas of sand next to the sea or to water which are not managed."

So should we change this in the Wiki? Because in my opinion, it sounds like you can't use both tags in one place.

dieterdreist commented 7 years ago

2016-11-15 19:13 GMT+01:00 doktorpixel14 notifications@github.com:

"Use natural=beach for areas of sand next to the sea or to water which are not managed."

So should we change this in the Wiki?

The natural=beach tag has no such limitations and even explicitly mentions spatial overlap of beaches and beach ressorts: https://wiki.openstreetmap.org/wiki/Tag:natural%3Dbeach

Karthoo commented 7 years ago

Okay, so we clarified that, but I think, that it would be still a good idea to render this tag ;-)

d1g commented 7 years ago

instead of natural=beach, if the beach area is managed

It says opposite: "Use additional objects tagged with natural=beach and name=* for entire beaches."

So should we change this in the Wiki? Because in my opinion, it sounds like you can't use both tags in one place.

Wiki never said "instead", please read carefully.

Karthoo commented 7 years ago

It says opposite

Yes, after dieterdreist changed it :)

Karthoo commented 6 years ago

beach_resort What do you think of this symbol? The water is based on the already existing "swimming" icon.

kocio-pl commented 6 years ago

Looks OK for me, but the most important part is testing icons with 14 px, because even nice design can break with so small matrix.

Karthoo commented 6 years ago

I already have it as an svg but can't attach it to posts here

kocio-pl commented 6 years ago

You can export it as PNG (which is needed anyway, because it's how it will look on the map) and SVG can be uploaded as gist, because it's just code (XML) after all.

Karthoo commented 6 years ago

I think it also looks good in 14x14px. So how can I test it? beach_resort_14x14

kocio-pl commented 6 years ago

It still looks nice and readable for me.

The best way is to make a fork with a branch, change the code to use this icon and render some real area. I guess it's easiest to install Docker-based testing environment:

https://github.com/gravitystorm/openstreetmap-carto/blob/master/DOCKER.md

If you need some help just write.

Karthoo commented 6 years ago

Unfortunately Docker doesn't work on my computer, beacause I can't activate "Hyper-V". So it would be nice if someone else could try it out for me... (I expect that it doesn't take that much time, does it?)

kocio-pl commented 6 years ago

There's also traditional way, Docker is meant only to make the process easier:

https://github.com/gravitystorm/openstreetmap-carto/blob/master/INSTALL.md

If you make the code, I can try it too.

kocio-pl commented 6 years ago

If you're on Windows, you can probably try VirtualBox and install Linux inside, like Ubuntu 16.04 for example - 64-bit arch might be needed for Kosmtik. Docker would be slower then, but it's still the easiest way and this test is quite simple, so it doesn't need anything big and fast.

Tomasz-W commented 6 years ago

@doktorpixel14 Can you upload your SVG file to Gist?

https://gist.github.com/

Karthoo commented 6 years ago

Yes, you can find it here: https://gist.github.com/doktorpixel14/fe72d5aa4d070385b6ba3f993bec0ca3

Karthoo commented 6 years ago

I did some testing now with this being the result: beach_resort_17 Zoom level 17 ... beach_resort_18 ... and 18

The problem with this two examples shown here is that they have 'natural=beach' and 'leisure=beach_resort' tagged on one way. So normally there should also be the rendering for the beach. How could I solve that?

kocio-pl commented 6 years ago

They should be independent, I guess. Where is this place - could you post a link?

Karthoo commented 6 years ago

https://www.openstreetmap.org/way/182023373

Karthoo commented 6 years ago

Another idea that came to my mind is to add an outline to beach resorts which are mapped as an area. It would be useful to see where the resort ends and where the next one begins or where there is a public beach again.

kocio-pl commented 6 years ago

Try to check in the Data inspector in Kosmtik how is it being selected. The area rendering is defined in https://github.com/gravitystorm/openstreetmap-carto/blob/master/landcover.mss.

Karthoo commented 6 years ago

I don't really get how to use the data inspector. What do the colours mean? I only see different brown shades.

kocio-pl commented 6 years ago

You go close to the item you want to inspect (to avoid clutter) and click on the map on it, so the list of data attributes appears - that's the data as defined in project.mml. To make the selection more strict, you can turn on the view of only the data layers you want to test.

Karthoo commented 6 years ago

If I click on an item nothing happens. Do I have to select a tool or something like that first?

kocio-pl commented 6 years ago

When the Data inspector is on, you can control it by the right side menu. I just turned it on and clicked in some less dense area, here is how it looks like:

screenshot-2018-3-29 openstreetmap carto kosmtik

Tomasz-W commented 6 years ago

Thanks for testing it!

Another idea that came to my mind is to add an outline to beach resorts which are mapped as an area. It would be useful to see where the resort ends and where the next one begins or where there is a public beach again.

As I know, most of these resorts doesn't have a physical borders, so I think outline would be unnessesary and little bit confusing.

Karthoo commented 6 years ago

So is it an error if it looks like this: image

kocio-pl commented 6 years ago

Yes, there's a problem with layers, as you can see. My Docker is out of order now and I don't know why, so I can't check the source of it, but maybe try to rebuild the kosmtik container.

Karthoo commented 6 years ago

Hmm, didn't help.

kocio-pl commented 6 years ago

Does anybody else with Docker could test if it builds properly? That's probably something with Mapnik, CartoCSS changes and Kosmtik npm version, so packaging and dependencies in general.

kocio-pl commented 6 years ago

Could you publish the code at least, so we could test it too?

Karthoo commented 6 years ago

So here is what I changed:

https://github.com/gravitystorm/openstreetmap-carto/compare/master...doktorpixel14:master

kocio-pl commented 6 years ago

The most convenient way is to just use your fork and push the changes to a branch named like "beach_resort" and then just let us know which is it (for example https://github.com/kocio-pl/openstreetmap-carto/tree/castle ).

Karthoo commented 6 years ago

Just follow the link in my previous post. I already did that.

kocio-pl commented 6 years ago

Here is the fix: https://github.com/gravitystorm/openstreetmap-carto/commit/74915de812a948a22eff77edbd84fc6e1de00efe

The problem was you have added it to the landcover layer and it was selected for landcover instead of beach, but beach resort doesn't have any landcover defined by itself, so we get nothing to show as a background. BTW: there are also other elements added lately which also have no landcover defined, I'll try to fix it soon.

The only layer you need for selecting areas for icons is amenity-points-poly- the rest is amenity-points for nodes, text-poly for area labels and text-point for node labels, just like you did.

kocio-pl commented 6 years ago

I would try with z16+ - these are bigger, general areas, and that would be also on pair with similar picnic zone (tourism=picnic_site) or camping (tourism=camp_site).

Karthoo commented 6 years ago

Yes, that's probably better, because you would also see it before the resort's amenities then.

Karthoo commented 6 years ago

So here are some updated screenshots:

beach_resort_16 New zoom level 16 ...

beach_resort_18 ... and updated zoom level 18, now with the 'natural=beach' being rendered too.

Would it be better to not show the name on lvl 16?

kocio-pl commented 6 years ago

I'm not sure, but if you have no specific needs or known pitfalls, try to follow similar cases (like mentioned camping or picnic sites) for the sake of conceptual consistency and code compactness.

Karthoo commented 6 years ago

Okay, I followed the example of the picnic sites by hiding names in zoom level 16. In addition, I made the code for the label a bit more compact.

So would you declare it ready for an pull request or is there still a step missing?

kocio-pl commented 6 years ago

It is more or less ready in my opinion, there are however some technical things related to a project management:

Karthoo commented 6 years ago

And how do I merge the commits?

kocio-pl commented 6 years ago

Probably like this:

https://stackoverflow.com/questions/5308816/how-to-use-git-merge-squash

I do it in a different way - resetting the branch in a visual editor (namely gitk) in a mixed mode and then reapplying adding files and commit. Maybe I will test this way too, however I don't need to do it too much, so I don't care.

Karthoo commented 6 years ago

Is this really necessary? I don't find a way that works for me ...

kocio-pl commented 6 years ago

Do you have a problem with git merge --squash?

It's not mandatory, but it's always good to not have many changes that are not standalone and are just part of the solution. If you fail, we can go on with what you have for now.

Karthoo commented 6 years ago

Yes. I've created a new repository with a new branch where I changed the things again. But if I'm trying to use squash I get this output:

image

kocio-pl commented 6 years ago

Maybe beach_resort option should be not used?

Karthoo commented 6 years ago

Then it says 'already up-to-date'

kocio-pl commented 6 years ago

So OK, I will squash them later and try to do it on my own to learn. Or maybe somebody with better git knowledge can help you?