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

Added styles for generic shop=* icon #604

Merged
merged 13 commits into from
Jul 22, 2014
Merged

Added styles for generic shop=* icon #604

merged 13 commits into from
Jul 22, 2014

Conversation

dkniffin
Copy link
Contributor

@dkniffin dkniffin commented Jun 4, 2014

See #117 for details. This is a new pull request for the same thing.

@dkniffin
Copy link
Contributor Author

dkniffin commented Jun 4, 2014

I was not able to test it unfortunately. I can't seem to get my tilemill setup working correctly for some reason. But it should be good, hopefully.

@vholten
Copy link
Contributor

vholten commented Jun 4, 2014

I tested it and found the following:

  • "where amenity in ('alcohol', 'bakery', ..." should be "where shop in ('alcohol', 'bakery', ..."
  • bakery is included twice

With the change of the first point made,

  • names of shops tagged on buildings do not render (but the icon does)
  • shops which are tagged on nodes render correctly

@dkniffin
Copy link
Contributor Author

dkniffin commented Jun 4, 2014

Made those changes.

@vholten
Copy link
Contributor

vholten commented Jun 4, 2014

I forgot one: on the same line, "or shop is not null" should be "or amenity is not null".
But even after all these changes, names of shops tagged on buildings do not render.

@dkniffin
Copy link
Contributor Author

dkniffin commented Jun 4, 2014

Done

@matthijsmelissen
Copy link
Collaborator

Thanks for rebasing this. I tested this on Luxembourg, and it is an enormous improvement compared to the previous rendering. That said, there are still some improvements to be made:

  • I think it would be better to start rendering generic shops from z17 instead of z16, just like most other shops.
  • Currently generic shop names on areas (such as buldings) don't shop up. Moreover, shop names on points do shop up even if they are not in the white list. The solution for both issues is to amenity-points,amenity-points-poly,text,text-poly
  • Vacant should be removed from the whitelist. Perhaps drugstore as well.
  • Perhaps we should document somewhere (maybe even in the code) how we arrived at the current list of shops, and why we excluded certain types of shops such as travel_agency, tyres, estate_agent, and photo - assuming they are not excluded by mistake.

@pnorman
Copy link
Collaborator

pnorman commented Jun 7, 2014

Perhaps we should document somewhere (maybe even in the code)

We can't add comments to the MML file, as it's JSON which (stupidly) doesn't support comments. We could add them to the SQL, but ugh, escaping.

@matthijsmelissen
Copy link
Collaborator

Closed this for now - looking forward to the improvements.

@dkniffin
Copy link
Contributor Author

Ok, I made some more changes. (You'll have to re-open this pull request to see 'em)

The biggest change I made was I included a new script in the root of the repo called shop_values.rb . This script generates the list of shops that should show up. The script queries the taginfo api to get all values for shop. It then takes all values with more than 100 instances, filters out empty string values, and anything in the "blacklist" (currently [ "no", "vacant", "fixme", "drugstore", "other", "*" ]. Yes, there 503 occurrences of a literal star as a value. smh). Then it outputs those in sql format (to be copy-pasted into the project.mml file.

I also made the other changes suggested by math1985, so shops in areas should show up, and hopefully the names show up now.

Unfortunately, I still have not found time to fix my local Tilemill, so I'd appreciate it if someone could test these commits for me 😄 Thanks.

@matthijsmelissen
Copy link
Collaborator

Thanks, I re-opened the issue (but didn't have time yet to review the code).

@HolgerJeromin
Copy link
Contributor

Please add fish and fishmonger to the blacklist. Seafood is there and the right tag. Thanks

@matthijsmelissen
Copy link
Collaborator

Please add fish and fishmonger to the blacklist. Seafood is there and the right tag. Thanks

+1.

I think shop=betting can also go to the ignore list: see http://wiki.openstreetmap.org/wiki/Tag:shop%3Dbetting (Full disclosure: I was improved in this proposal).

@matthijsmelissen
Copy link
Collaborator

Some more:

  • shop=gambling is discouraged.
  • shop=organic is discouraged.
  • shop=pharmacy should be amenity=pharmacy.
  • shop=delicatessen is duplicate with shop=deli.
  • shop=jewellery is duplicate with shop=jewelry.

@dkniffin
Copy link
Contributor Author

Just made those changes.

I worked on trying to get my tilemill working, but still no luck yet. Urg.

@vholten
Copy link
Contributor

vholten commented Jun 12, 2014

More for the blacklist:

  • "bags", # prefer shop=bag
  • "antique", # prefer shop=antiques
  • "pets", # prefer shop=pet

In these three cases the preferred versions have a wiki page and the alternatives don't. The preferred versions are also used (much) more often according to taginfo.

Also, there's a typo in "jewellry" currently on the blacklist; should be "jewellery".

@Rovastar
Copy link
Contributor

for reference some tag usage of shape=jew*

jewelry 13 095
jewellery 112
jewelers 28
jewellers 17
jeweller 11
jewelery 11
jewlery 9

@dkniffin
Copy link
Contributor Author

Here's what I'm thinking will be in the blacklist:

no
vacant
empty
disused
unknown
closed
fixme
FIXME
FixMe
other
* (literal star)

I'm hesitant about the last two.

Edit by pnorman: markdown fix

@dkniffin
Copy link
Contributor Author

Ok, I've made that change. I would like to keep this blacklist simple for the moment, so unless there's one like the ones included (values that mean "not a shop" or similar), I won't be making any more changes to it for this PR.

If someone could please test this version of the PR, I'd be grateful. :)

Assuming it looks good (no syntax errors, etc) I think we're ready for it to be merged.

@dkniffin
Copy link
Contributor Author

@math1985, would you mind testing this again for me?

@matthijsmelissen
Copy link
Collaborator

Tested, looks fine. As far as I'm concerned, it can be merged.

@dkniffin
Copy link
Contributor Author

@gravitystorm What do you think?

@matthijsmelissen
Copy link
Collaborator

@gravitystorm What is the status of this PR?

@gravitystorm gravitystorm merged commit 57869a3 into gravitystorm:master Jul 22, 2014
@gravitystorm
Copy link
Owner

I've merged this now - thanks @oddityoverseer13 !

As well as the usual merge conflicts, I moved the script and also had to fix the handling of the icons. We're using a mixture of marker and point symbolizers now, and this patch was causing the specific pointsymbolizer icons to disappear. But it's all working now.

@dkniffin
Copy link
Contributor Author

cool. how soon until we'll see this on osm.org? The readme says it gets an update at each point release, and it looks like those are roughly once a month. So August? just curious.

@matthijsmelissen
Copy link
Collaborator

We're using a mixture of marker and point symbolizers now, and this patch was causing the specific pointsymbolizer icons to disappear. But it's all working now.

I'm not sure what you mean with that. What was exactly wrong?

Edit: I think I see it now. See also some background and the correction.

@matthijsmelissen
Copy link
Collaborator

cool. how soon until we'll see this on osm.org? The readme says it gets an update at each point release, and it looks like those are roughly once a month. So August? just curious.

@gravitystorm usually creates a release each time he accepts a bunch of PRs, so that would be probably today or tomorrow. After that, the server admins need to manually update the carto version on the server. Usually they do that within one or two days, but human intervention is necessary so no guarantees. Then it still takes a couple of hours for the server to update the tiles.

@dkniffin
Copy link
Contributor Author

@math1985 cool thanks

@daganzdaanda
Copy link

This is showing up now on new tiles. It is a real improvement!
Even when there is no space for a name, the pink dot shows that there is a shop. Before, nothing or just the housenumber showed up. There were houses with lots of repeated housenumbers on the map, which was irritating and unclear.

Great work! Thank you, all!

@daganzdaanda
Copy link

Just noticed a minor regression(?):
shop=department_store used to have its own symbol, now it is just a dot.

@Klumbumbus
Copy link

Great improvement. Thanks.

@dkniffin
Copy link
Contributor Author

@daganzdaanda It looks like it still has an icon to me. can you provide a link and/or screenshot?

@Klumbumbus
Copy link

@daganzdaanda It looks like it still has an icon to me. can you provide a link and/or screenshot?

Example: There are two department shops in the middle of the view: "Nanu-Nana" and "Douglas". Move the drag button on the bottom to see the change
http://bl.ocks.org/tyrasd/raw/6164696/#18.00/50.83358/12.92131

@dkniffin
Copy link
Contributor Author

hmm. strange. Here's the one I was looking at (jcpenny in the middle of view: http://bl.ocks.org/tyrasd/raw/6164696/#17.00/39.85821/-105.05881

(nice app, btw. hadn't seen that.)

@HolgerJeromin
Copy link
Contributor

same in aachen:
http://www.openstreetmap.org/way/83274324#map=19/50.77397/6.07842
zoom level 19 is rerendered and has a dot, zoom 18 is old style and has an icon.
and:
http://www.openstreetmap.org/way/79620527#map=18/50.77427/6.08960

@Klumbumbus
Copy link

same in aachen:
http://www.openstreetmap.org/way/83274324#map=19/50.77397/6.07842
zoom level 19 is rerendered and has a dot, zoom 18 is old style and has an icon.

This was maybe not yet new rendered. Now it has the dot also at zoom 18.

@gravitystorm
Copy link
Owner

Just noticed a minor regression(?):
shop=department_store used to have its own symbol, now it is just a dot.

Please open a new ticket!

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.