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

Render offices as dots + names #3061

Closed
wants to merge 8 commits into from

Conversation

andrzej-r
Copy link
Contributor

@andrzej-r andrzej-r commented Feb 10, 2018

Superseded by #3163

RE #108
Fixes #271
Fixes #1922
Fixes #2570

I've added rendering of office tags (all except office=yes), using a simple marker and text from the name tag, both in amenity-brown colour.

There is no 'office' column in the table, so I had to use tags->'office' instead (advised by folks from #osm-dev, apparently to add this column to the table the whole database would have to be reimported, which is not an option).

This is the first time I touch database queries, so please review this part carefully.

Examples before and after:
office2_l17
office2_l18
office2_l19

@kocio-pl
Copy link
Collaborator

kocio-pl commented Feb 10, 2018

Thanks, that looks sane to me, however my idea was to use black, violet and brown dots, not just brown - see #1697 (comment). What do you think about it?

How did you create that list? There are not all the office values and it doesn't look like just top values from Taginfo.

I will test if the rendering is not too early, but I suspect for example z19+ would be better than z17+ in well mapped urban areas.

It also resolves #1922, #271 and #2570.

@Tomasz-W
Copy link

What about dark-blue dot for office=* ? This colour reminds me suits or paper folders with documents.

@andrzej-r
Copy link
Contributor Author

My goal is to first fix an immediate issue of office= labels not showing up. This discourages people from mapping POIs and causes building names to be overused as e.g. company names.

Changing default colours etc is of course possible, but let's not go overboard with trying to fix visual aspects all at once. There is a lot of work on classification, icons etc needed, which can be spread across multiple PRs on case-by-case basis. For example, some office= values (e.g. office=government, political_party,tax,educational_institution) do not belong to a "business" category, so they can be moved to other categories, if they suit them better.

I like the current zoom levels because they match the rendering of regular shop= and amenity= tags (dot at >=17, dot + name (if fits) at >=18). I don't mind deferring showing names to >=19 but it should done for all amenity tags in a single separate change.

The list of tags was taken from a wiki docs for the office= tag (https://wiki.openstreetmap.org/wiki/Key:office). Indeed, if other values are already in use (BTW, what is "taginfo"?) we should add them to the list (and Wiki!).

In testing, I haven't spotted other unmapped office tag values, but that's not necessarily easy to do. If anything, I was surprised how many office tags have suddenly showed up in Cambridge, UK. Much more that I expected (perhaps that's because both Maps.me and OsmAnd are already displaying them).

@andrzej-r andrzej-r mentioned this pull request Feb 10, 2018
@kocio-pl
Copy link
Collaborator

The list of tags was taken from a wiki docs for the office= tag (https://wiki.openstreetmap.org/wiki/Key:office). Indeed, if other values are already in use (BTW, what is "taginfo"?) we should add them to the list (and Wiki!).

Taginfo is an OSM service counting uses of tags - the numbers in the wiki infobox are embedded from it, for example. You can read more about it here:

https://wiki.openstreetmap.org/wiki/Taginfo

The list on the wiki is a good place to start and I agree that we should stick to it and add new types only when they are defined there. I want to look closer to see if we could tune this code a bit.

I was surprised how many office tags have suddenly showed up in Cambridge, UK. Much more that I expected (perhaps that's because both Maps.me and OsmAnd are already displaying them).

That's exactly why I'm so cautious with z17, which is crowded in some places and we try to offload it when possible.

I like the current zoom levels because they match the rendering of regular shop= and amenity= tags (dot at >=17, dot + name (if fits) at >=18). I don't mind deferring showing names to >=19 but it should done for all amenity tags in a single separate change.

With so many different kind of data it's not possible to match everything. I also think that most of the shops and amenities are about direct interaction (buying things or using services), while most of the offices are not, hence z18 might be used for selected ones and z19 for the rest of them.

Added missing office=* values.
Broken down office tags into sub-categories.
Rendering tweaks (colour, filters) based on sub-categories.
@andrzej-r
Copy link
Contributor Author

I've added more categories (from taginfo and iD) and broken them down into categories (office, shops, public) to facilitate difference in rendering (currently they only differ in colours). No icons so far.

I've increased zoom levels for some subcategories (so, for example, offices are rendered 1 zoom level later than surrounding shops, which IMHO is wrong) but I object moving offices to z19 without a matching change to other shops and (importantly) building names. Building names are currently rendered from z16-17, which creates incentive for mapping companies as building names. For this reason all single-occupant office buildings in my area use a company name, even if a valid building name exists.

Should we increase the zoom level (which I agree is a good idea), this should be done for many categories of buildings and POIs in a single changeset, which is outside the scope of this change.

By the way, is there any plan of increasing maximum zoom level in the osm map beyond 19?

@kocio-pl
Copy link
Collaborator

Great, I will try to test it soon.

By the way, is there any plan of increasing maximum zoom level in the osm map beyond 19?

I haven't heard of it and I don't expect it, since it would require more than double size of current disc storage on OSMF servers:

https://wiki.openstreetmap.org/wiki/Tile_disk_usage

In theory we're independent project, but in practice OSM.org website is our default deployment target. However I recently approved the code which has additional properties starting from z20 (but it's still usable with z-max=z19) - see #2936 (comment).

@SomeoneElseOSM
Copy link
Contributor

SomeoneElseOSM commented Feb 11, 2018

With regard to the aside about tile disk usage, here's a bit of anecdata for a tile server that I look after that goes up to 24 (and does have features that only appear at those high zooms). Sizes are from "du" per zoom level.

772032 16
762888 17
733576 15
617144 18
585020 14
346940 13
300916 20
144968 12
71240 19
55588 11
53420 21
37864 10
21264 22
14460 9
9276 23
6640 8
4980 24

This is heavily dependent on how people use the server - if people were to start scraping zoom 24 for example, the disk usage for that zoom level would go up hugely.

@kocio-pl
Copy link
Collaborator

Looking at some random places in my city, this rendering is better than I expected - it's not too crowded and mostly sane.

However when the office takes the whole building, it looks weird with a dot in the middle and the name in different colors. I guess it's better to skip offices with is_building=yes in this PR, as it's a different, quite general problem (the name on the building can relate to different tags).

@andrzej-r
Copy link
Contributor Author

I haven't noticed that. Can you point me to a specific location so I can try it myself?

There may be quite a few corner cases like this, so it would be good to test this change with on a larger scale.

@kocio-pl
Copy link
Collaborator

Sure, that's why independent testing before merging is a good thing. 😄

Look here for example:
https://www.openstreetmap.org/way/196558064

I also plan to review categories soon.

@andrzej-r
Copy link
Contributor Author

I can reproduce this behaviour (see the screenshot below) but I am unsure if that is something we should be fixing. It is consistent with how shops and other amenity points are currently implemented (e.g. what happens when a building has both name and shop tags?)

I would say it is in fact a desirable behaviour, and it only looks weird because we are used to all names being rendered as building names. Objectively, the name in @kocio-pl's example is a company name, not a building name.

My suggestion is to leave it as is, and let mappers decide if this is indeed the result they wanted. Perhaps the name tag contains a building name, not a business name, in which case a better solution would be to remove the office tag and use building=commercial instead.

office2_l19_single

@Tomasz-W
Copy link

I think black color should be used for govermental offices, and for commercial offices dark-blue would be better. Can you test it?

@andrzej-r
Copy link
Contributor Author

There is a bit of a problem with government-like offices. All government offices are currently using public-service=amenity-brown=#734a08. I'm not opposed to switching to black=#000000 but that would affect many existing objects. BTW, at the moment the only objects with blackish labels (visible in the screenshot) are place-of-worship, which are hardcoded to #000033.

My suggestion: leave government offices in public-service category for now. We can change it later on case by case basis, e.g. when we get suitable icons. The current list of government/public offices is:

    [feature = 'office_administrative'],
    [feature = 'office_adoption_agency'],
    [feature = 'office_government'],
    [feature = 'office_employment_agency'],
    [feature = 'office_educational_institution'],
    [feature = 'office_government'],
    [feature = 'office_ngo'],
    [feature = 'office_political_party'],
    [feature = 'office_quango'],
    [feature = 'office_religion'],
    [feature = 'office_tax']

This is how the above example looks like with office=darkblue=#00008b. I quite like it.

office2_l19_darkblue

@kocio-pl
Copy link
Collaborator

Before I check the groups - I think I can accept the dot for the whole building, but the space between the dot and the label is just too big. It's also the problem with shop dots, but it's consistent with other shops at least. Here the dot is even smaller than with shops, so the distance should be smaller too.

What bothers me is that the dot takes my attention to the center and then I can't immediately recognize the label position.

@andrzej-r
Copy link
Contributor Author

Would making the dot bigger be an option? That would match rendering of shops. The screenshot shows a shop and a shop-like office ("Plus"), both rendered slightly differently.

The spacing probably asssumes the dot may actually be an icon. I will check if it can be tighten without affecting icon-label spacing.

@kocio-pl
Copy link
Collaborator

I like the idea to have smaller dots for offices. We may use the icons for some of them, so they can be recognized by their own shape, but this distinction for the rest of them is nice.

@andrzej-r
Copy link
Contributor Author

I've left the dot size smaller and brought the label closer to it. Because the office labels are now handled differently from shop labels the relevant parts of the code were separated.

This doesn't mean I agree with making shops any more prominent than offices, just trying to get offices rendered at all.

Comparison screenshot:
office2_l18_spacing

@kocio-pl
Copy link
Collaborator

Thanks, now it's better for me.

@daganzdaanda
Copy link

daganzdaanda commented Feb 21, 2018

Could you test an denser area with more offices?
Also, as you noted, your code does render office nodes like shops at the moment ("Plus" https://www.openstreetmap.org/node/4413514591 ) Rendering really should be the same whether it's a way or node or relation.

@kocio-pl
Copy link
Collaborator

An example place with more offices (private and public):
https://overpass-turbo.eu/s/wmQ

@andrzej-r
Copy link
Contributor Author

Thanks for the overpass link. I've checked several locations and it looks like London is particularly rich in office points and buildings:
office2_l18_london1
office2_l18_london2
office2_l18_london3

@kocio-pl
Copy link
Collaborator

Still reasonable for me - that's very promising!

@andrzej-r
Copy link
Contributor Author

Over the next couple of weeks starting this Sunday I'll have a limited access to the Internet. Is there anything you want me to try before that?

How does the deployment procedure look like for the osm map? I read they pick up a new version of Carto every first Sunday of the month. Is that a released version (if so, when is the next release?) or a git master?

@kocio-pl
Copy link
Collaborator

I still didn't have the time to examine categories, so I have nothing more to ask you about currently.

How does the deployment procedure look like for the osm map? I read they pick up a new version of Carto every first Sunday of the month. Is that a released version (if so, when is the next release?) or a git master?

No, OSMF server admins run z0-z12 render updating then. It has nothing to do with style change, these levels are just not constantly updated (as z13-z19 are). Another occasion is when a new release is deployed, of course.

Deployment on OSMF servers is done after we do new style version release and let them know about it (see https://github.com/gravitystorm/openstreetmap-carto/blob/master/RELEASES.md#notifications ). I try to keep our regular - major and minor - releases once a month, especially on Fridays (because re-rendering tiles on OSMF servers may start in the weekend, when the load is lower), but it's just a matter of what I think is proper. Other team members can do it whenever they feel and bugfix releases are done when it's needed and possible.

I plan next release for tomorrow evening and hope that admins will deploy it soon after, but that's entirely their decision.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Feb 26, 2018

The example where the name on the building is rendered twice on z17- as a building and an office (and another one); what do we want to do with it?

xbear88m
4rdsyz9k

I think dark blue is a good choice as a new color - it's similar to how we render railway/metro stations, but we have no choice but reuse colors for different types of objects:
w0jlf8sc

Different buildings related problem - lack of office dots on some buildings on z17 (however they are visible on z18):
az q6leu

@andrzej-r
Copy link
Contributor Author

andrzej-r commented Feb 27, 2018

I will fix that spurious text-dy change - it wasn't intended. Can't do that before in the next 2 weeks, though.

Can you narrow down the cause of double labels and missing dots? The tags look OK to me (at first sight, at least).

Not sure what you meant by the colour. Is it OK to use it or should we reuse an existing colour?

I still think so office points should be rendered in public-service or shop colours. But if more people object to it I will change them to the office colour.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Feb 27, 2018

I will fix that spurious text-book change - it wasn't intended. Can't do that before in the next 2 weeks, though.

OK, it's a totally new set of tags, so it's better to test more than usual - no hurry.

Not sure what you meant by the colour. Is it OK to use it or should we reuse an existing colour?

I think it makes sense, I just wanted to show that it's a reuse.

Can you narrow down the cause of double labels and missing dots? The tags look OK to me (at first sight, at least).

I think that it's a multiple features match and since the office label shows in a different place than a building label, both are just rendered, because it's possible. Most of the time we try to show multiple the labels in the same place, so as a result only one is visible, but in general features are not exclusive.

@andrzej-r
Copy link
Contributor Author

I think (I cannot test it right now) both errors are caused by a mismatch in point and label firtering options. We should either add [zoom >= 17][way_pixels > 3000] to the point options (lines 918, 946) or remove it from label options (lines 2159, 2213). Or simply render all office points from z17.

@andrzej-r
Copy link
Contributor Author

  1. I've reverted the accidental text-dy change
  2. I've fixed double labels showing up in some office buildings at z17. It was indeed caused by mismatch in filtering options between labels and points:
    office2_l17_london5
    Note the rendering error (a building label partially showing up). Not sure if that's a style issue or if my kosmtik is acting up. Kosmtik shows the same rendering issue in existing structures, which are rendered correctly on the website:
    office2_l17_london6
    I would appreciate if someone could test it independently.
    office2_l17_london7
  3. Matched sizes of office icons and other POI icons at zoom>=18:
    office2_l18_london7

@andrzej-r andrzej-r mentioned this pull request Mar 12, 2018
@andrzej-r
Copy link
Contributor Author

Can I have this PR deployed in the test server, please? It seems to work OK in my setup but we need to look at broader selection of points and check if there are no issues with database performance.

@kocio-pl
Copy link
Collaborator

You need to resolve the project.mml conflicts before that.

@andrzej-r
Copy link
Contributor Author

I've resolved conflicts. Automated merging should work fine now but since I'm not familiar with other changes in master I haven't tested the result.

@kocio-pl
Copy link
Collaborator

Thanks. I don't know what other changes in master have to do with your PR, but I'm not aware of any problem witch could make testing hard or unreliable, so you can do it.

@rrzefox, could you deploy this code on your server to test it worldwide?

@matthijsmelissen
Copy link
Collaborator

Do we really need to test everything on a worldwide server? This seems particularly lime a PR that can be tested just as well with local extracts.

Of course if @rrzefox wants to help that is really great, but I don’t want to aak too much from him either.

@kocio-pl
Copy link
Collaborator

This is not typical "add one" code, so I'd like to see it in different places with a fresh eye without planning. I believe just asking is OK, since I have no idea how much would be "too much" for him.

@andrzej-r
Copy link
Contributor Author

I initially thought the same but then there were some unexpected issues found in some combinations of tags. Some independent testing would be useful.

Having said that, if there are no big issues found I'd advise to merge this pr asap and work on resolving smaller issues from there. The master branch has already started to diverge.

@Adamant36
Copy link
Contributor

I can probably test it in a couple of dense areas in California if someone wants to provide the code or the file with the code in there already.

@kocio-pl
Copy link
Collaborator

The code branch to be merged is here: https://github.com/andrzej-r/openstreetmap-carto/tree/office2

@andrzej-r
Copy link
Contributor Author

Can anyone comment on the visual issue I mentioned in #3061 (comment), point 2? I think (because I could see it also in existing objects rendered by kosmtic but not by the website) it is caused by my test setup.

Other than that I don't see any issues myself but of course a fresh pair would likely spot something.

Also, I would advise someone to go through the code and review it. Especially the part related to the database access.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Mar 19, 2018

The problem of double labels on office buildings is gone now:

4x48c5o8

@andrzej-r
Copy link
Contributor Author

Do you also get that clipped "Co" label at z17?

@kocio-pl
Copy link
Collaborator

Yes, but only in the Kosmtik window:
screenshot-2018-3-19 openstreetmap carto kosmtik

export rendering is clean:
zhi5i4cz

@andrzej-r
Copy link
Contributor Author

Looks like #2671 has created new conflicts. I will leave this PR as is to avoid needless code churn. Ping me when you are ready to merge it - I'll create a new PR against a fresh master.

@andrzej-r
Copy link
Contributor Author

Closing this PR as it is now superseded by #3163.

@andrzej-r andrzej-r closed this Apr 3, 2018
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.

8 participants