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

Rendering of leisure=beach_resort #2440

Closed
Karthoo opened this issue Nov 14, 2016 · 51 comments
Closed

Rendering of leisure=beach_resort #2440

Karthoo opened this issue Nov 14, 2016 · 51 comments

Comments

@Karthoo
Copy link
Contributor

Karthoo commented Nov 14, 2016

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
Copy link

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
Copy link
Contributor Author

Karthoo commented Nov 15, 2016

"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.

@kocio-pl kocio-pl added this to the New features milestone Nov 15, 2016
@dieterdreist
Copy link

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
Copy link
Contributor Author

Karthoo commented Nov 15, 2016

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

@d1g
Copy link

d1g commented Nov 17, 2016

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
Copy link
Contributor Author

Karthoo commented Nov 17, 2016

It says opposite

Yes, after dieterdreist changed it :)

@Karthoo
Copy link
Contributor Author

Karthoo commented Oct 27, 2017

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

@kocio-pl
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Oct 27, 2017

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

@kocio-pl
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Oct 27, 2017

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

@kocio-pl
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Oct 27, 2017

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
Copy link
Collaborator

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
Copy link
Collaborator

kocio-pl commented Oct 27, 2017

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
Copy link

@doktorpixel14 Can you upload your SVG file to Gist?

https://gist.github.com/

@Karthoo
Copy link
Contributor Author

Karthoo commented Mar 18, 2018

@Karthoo
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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
Copy link
Collaborator

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

@Karthoo
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

@Karthoo
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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

@kocio-pl
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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

@kocio-pl
Copy link
Collaborator

kocio-pl commented Mar 29, 2018

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
Copy link

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
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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

@kocio-pl
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

Hmm, didn't help.

@kocio-pl
Copy link
Collaborator

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
Copy link
Collaborator

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

@Karthoo
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

So here is what I changed:

master...doktorpixel14:master

@kocio-pl
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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

@kocio-pl
Copy link
Collaborator

Here is the fix: 74915de

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
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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

@Karthoo
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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
Copy link
Collaborator

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

  • it's always better to have a specific branch for every feature you introduce/change instead of master branch of your fork - this one is kind of special and serves as default branch
  • before creating a pull request, please merge all the commits into one and name it clearly so somebody making a fork of the osm-carto could easily pick/filter/review features

@Karthoo
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

And how do I merge the commits?

@kocio-pl
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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

@kocio-pl
Copy link
Collaborator

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
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

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
Copy link
Collaborator

Maybe beach_resort option should be not used?

@Karthoo
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

Then it says 'already up-to-date'

@kocio-pl
Copy link
Collaborator

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?

@Karthoo
Copy link
Contributor Author

Karthoo commented Mar 29, 2018

Ah, I found a simple way to merge the commits via the web interface by myself.

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

No branches or pull requests

6 participants