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 addr:flats #4082

Merged
merged 7 commits into from
Apr 7, 2020
Merged

Render addr:flats #4082

merged 7 commits into from
Apr 7, 2020

Conversation

richlv
Copy link
Contributor

@richlv richlv commented Mar 16, 2020

Fixes #3988

Changes proposed in this pull request:
Render addr:flats similar to addr:unit.
Accounting for combinations with addr:housenumber, addr:unit, addr:housename.

Test rendering with links to the example places:

  • haven't had a chance to figure this out.

@jeisenbe jeisenbe added new features Requests to render new features addressing labels Mar 18, 2020
@jeisenbe
Copy link
Collaborator

jeisenbe commented Mar 18, 2020

Thank you for the test code, @richlv.

Have you downloaded docker? The instructions on setting up a test rendering DOCKER.md are fairly clear, if you have some experience.

This comment gives more detailed instructions: #3782 (comment)

Or try out this guide: https://www.openstreetmap.org/user/SomeoneElse/diary/43041

You might also read https://ircama.github.io/osm-carto-tutorials/ if you want to set up a test server directly in Linux.

@Adamant36
Copy link
Contributor

It seems to work as intended. In the first picture they are all flat numbers. In the second, the flat numbers are mostly under the address numbers.
Flat 1
Flat 2

@richlv
Copy link
Contributor Author

richlv commented Mar 20, 2020

Thank you, that was in some aspects simpler than expected, and more complicated in others :)
Most of the difficulty had to do with #3474 .

Test area 1 (flat numbers on entrances) - https://www.openstreetmap.org/#map=18/56.98831/24.24307
ZL 18 before and after:
Screenshot 2020-03-20 at 10 35 25 Screenshot 2020-03-20 at 10 39 49

ZL 19 before and after:
Screenshot 2020-03-20 at 10 42 34 Screenshot 2020-03-20 at 10 43 05

Test area 2 (flats + housenumber) - https://www.openstreetmap.org/#map=19/57.07449/24.11386
ZL 17 before and after:
Screenshot 2020-03-20 at 12 20 18 Screenshot 2020-03-20 at 12 20 37

ZL 18 before and after:
Screenshot 2020-03-20 at 12 21 33 Screenshot 2020-03-20 at 12 21 45

@richlv
Copy link
Contributor Author

richlv commented Mar 20, 2020

Thank you for the test @Adamant36, it took me quite a while to get it all running, and then preparing all the screenshots - and having it tested on more areas is very helpful.

@richlv
Copy link
Contributor Author

richlv commented Mar 20, 2020

@Adamant36, this might be a data issue - in your second screenshot there are two instances of:

1921
5

Copy link
Collaborator

@jeisenbe jeisenbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should render both addr:unit and addr:flats on the same object.
Dividing the numbers onto 2 different lines looks a bit odd.

Also, the wiki page claims "If there is gap in numeration, mark numbers and intervals with semicolon: addr:flats=3-7;10;14;16-18." - and there are over 2000 addr:flats features which include a semicolon.

How should we handle this?

style/addressing.mss Outdated Show resolved Hide resolved
style/addressing.mss Outdated Show resolved Hide resolved
@richlv
Copy link
Contributor Author

richlv commented Mar 23, 2020

Regarding semicolon separated entries, it seems that the best is to display them all as-is - or am I missing some concern here?

@jeisenbe
Copy link
Collaborator

If we are rendering something like "addr:flats=3-7;10;14;16-18", it might be more legibile if we replace the semicolons with a more common separator, such as a comma, space, or both:

3-7;10;14;16-18
vs
3-7, 10, 14, 16-18
3-7,10,14,16-18
3-7 10 14 16-18

@richlv
Copy link
Contributor Author

richlv commented Mar 23, 2020

Converting semicolons could indeed increase readability, but then I thought a bit about looking at that as a mapper.
If openstreetmap-carto is primarily aimed at mappers, such a conversion would confuse me. I might see "3-7, 10" and think "oh, syntax error here, should fix".
As a mapper, I'd prefer these to be displayed as-is.

@jeisenbe
Copy link
Collaborator

As a mapper, I'd prefer these to be displayed as-is.

That's a good point.

Looked at most of them.

Majority are duplicates of either addr:flats or addr:entrance (presumably tagging for the renderer).

What do you mean by addr:entrance? I see it's documented as an old proposal only: https://wiki.openstreetmap.org/wiki/Proposed_features/Key:addr:entrance "addr:street=Zhelezni vrata + addr:housenumber=24 + addr:entrance=А"

When you say they are duplicates of addr:flats, is it something like addr:unit=1-20 which is clearly a range of numbers?

There are a few tagging mistakes (for example, "addr:unit=F6" + "addr:flats=Suite F6").

One concern I have is that the page has previously suggested that addr:flats= could be used in this way, to just mark a single "flat"/"unit", but this would dupicate the meaning of addr:unit.

I've also seen a number of cases where the addr:flats= value seems to include all of the units in the building, when there is only one entrance. So I'm not sure how helpful it is, in that case, to render it?

Eg, this was the first place I found in Kaliningrad with addr:flats, shown with your proposed rendering:

addr-flats-18:54 73610:20 47758

Every entrance has the housenumber, and then underneath there is 1-6 as the value of addr:flats. Several other spots in Kaliningrad are like this

@jeisenbe
Copy link
Collaborator

@imagico, can I get your cartographic opinion on this idea?

@richlv
Copy link
Contributor Author

richlv commented Mar 24, 2020

Majority are duplicates of either addr:flats or addr:entrance (presumably tagging for the renderer).

What do you mean by addr:entrance? I see it's documented as an old proposal only: https://wiki.openstreetmap.org/wiki/Proposed_features/Key:addr:entrance "addr:street=Zhelezni vrata + addr:housenumber=24 + addr:entrance=А"

Yes, and with a very low usage count.
A potential problem is that people have used addr:unit for entrances, for example - https://www.openstreetmap.org/node/5764175308 and https://www.openstreetmap.org/node/5263097242 .

If unit is used for individual flats and also for entrances, it puts it both above and below addr:flats.
That is, it can be a smaller identificator than addr:flats and lower. Consider:

  • addr:flats=1-20 + addr:unit=1 (on the same node, unit denotes numbered entrance).
  • addr:flats=1-20 + addr:unit=1 (on different nodes, addr:unit denotes individual flat number - perhaps because there's a business there).

It might be possible to shift such usage of addr:unit to addr:entrance (would likely need wiki updates and even rendering support), but that is out of scope here.

And then there's addr:flatnumber which might be a better for the latter usecase, but only used 194 times (and will cause semantic concern for some people if to be used on non-flats).

Though I'm afraid that this is getting too much in the weeds on a higher complexity level.
Sort of a reverse bikeshedding :)

When you say they are duplicates of addr:flats, is it something like addr:unit=1-20 which is clearly a range of numbers?

Yes, for example - https://www.openstreetmap.org/node/5470093221 .

One concern I have is that the page has previously suggested that addr:flats= could be used in this way, to just mark a single "flat"/"unit", but this would dupicate the meaning of addr:unit.

Saw your edit to the page. There's an overlap between addr:flats and addr:unit in either case (now it means shifting between the tags depending on the number of, er, numbers), but nothing we can resolve here.

I've also seen a number of cases where the addr:flats= value seems to include all of the units in the building, when there is only one entrance. So I'm not sure how helpful it is, in that case, to render it?

Eg, this was the first place I found in Kaliningrad with addr:flats, shown with your proposed rendering:

addr-flats-18:54 73610:20 47758

Every entrance has the housenumber, and then underneath there is 1-6 as the value of addr:flats. Several other spots in Kaliningrad are like this

That's an interesting numbering scheme that I have not seen before.
Looks like those are actually housenumbers, not entrance numbers.
I believe that rendering flats is still useful here, as it shows where they are mapped / not.
There are cases of a remotely similar numbering in Latvia, although in those cases there's only one entrance to a building, and it has all the flats assigned and signposted.

I suspect that trying to figure out how many entrances a building has and how many have addr:flats tagged to adjust rendering will be complicated and error prone.
And verrrry confusing for mappers :)

Unrelated to such entrances, there are also cases of mapping that is unclear, for example https://www.openstreetmap.org/node/3840036300 . It has "addr:flats=III" and "addr:unit=50".
Overall this is likely diminishing returns from looking at the combinations of flats+unit, the dozen or so exceptional cases are likely not worth spending too much time on.

@imagico
Copy link
Collaborator

imagico commented Mar 24, 2020

I am not really familiar with this kind of address tagging. To me addr:flats for collective mapping of several units and and addr:unit for the separate mapping of individual units seem to be competing concepts and it might be counterproductive to render both of them. I am also not sure how often flat numbers are actually verifiable data and how often the just come from address imports without local verifiability.

Cartographically it seems that undifferentiated multi-line labels could be hard to read - it is likely often not clear if this is one label or separate label. Also z17 seems to early for anything but a simple house number - even in areas with moderate size buildings at moderate latitudes it is quite frequent that even those do not all fit to be shown.

@richlv
Copy link
Contributor Author

richlv commented Mar 24, 2020

I am not really familiar with this kind of address tagging. To me addr:flats for collective mapping of several units and and addr:unit for the separate mapping of individual units seem to be competing concepts and it might be counterproductive to render both of them. I am also not sure how often flat numbers are actually verifiable data and how often the just come from address imports without local verifiability.

Very verifiable in many cases, https://www.mapillary.com/map/im/e5bOY0-2Jhgc_CKht9xJcQ .
Agreed that they are somewhat overlapping, but that seems to be out of scope with both tags well in use.

Cartographically it seems that undifferentiated multi-line labels could be hard to read - it is likely often not clear if this is one label or separate label. Also z17 seems to early for anything but a simple house number - even in areas with moderate size buildings at moderate latitudes it is quite frequent that even those do not all fit to be shown.

addr:unit is currently rendered on the same line with just a space, and starts at z17. Similarly currently multi-line rendering is already used:

text-name: [addr_housenumber] + " " + [addr_unit] + "\n" + [addr_housename];

I just latched onto the existing rendering.

@jeisenbe
Copy link
Collaborator

To me addr:flats for collective mapping of several units and and addr:unit for the separate mapping of individual units seem to be competing concepts and it might be counterproductive to render both of them.

It's a bit odd that tagging has settled on plural "flats" and singular "unit", but in taginfo there is a pretty clear separation. Only a couple thousand addr:unit features include 2 numbers separated by a dash in their value (out of >500k), but the great majority of the 177k addr:flats features are in this format, as suggested by the description of the tag.

I'm glad that addr:unit isn't being used in this way, since it makes sense for addr:unit to only be used for a single unit or flat.

Cartographically it seems that undifferentiated multi-line labels could be hard to read - it is likely often not clear if this is one label or separate label. Also z17 seems to early for anything but a simple house number - even in areas with moderate size buildings at moderate latitudes it is quite frequent that even those do not all fit to be shown.

Perhaps @richlv could try rendering these from z18 only, and in a slightly different style (smaller text size perhaps?). At z17 too often they overlap and are blocked by adjacent address labels.

While the multi-line rendering makes sense when there is both a number and a name, these are all numbers, and probably should be all on one line, as with addr:housenumber + addr:unit currently.

@richlv
Copy link
Contributor Author

richlv commented Mar 25, 2020

Before making additional changes would like to confirm the intent.

  • Given that addr:flats is at a higher level and based on the comments addr:flats and addr:unit would not be used on the same object, addr:flats would always win over addr:unit.
  • Both would be rendered starting with z18 (demoting current addr:unit rendering from z17).

Does the above sound correct?

You mentioned them overlapping and being blocked at z17 - is this with the multiline approach?
Do you have a link to a particularly bad location in this regard?
Maybe we can keep them at z17 if multiline approach is not used.

@jeisenbe
Copy link
Collaborator

Check https://www.openstreetmap.org/#map=17/54.73472/20.47319 - this is the main place the tag is used in Kaliningrad, where I first checked:

z18 - vertical
addr-flats-18:54 73375:20 47085

z17 - vertical
addr-flats-17

z18 - horizontal
addr-flats-18:54 73375:20 47085

z17 - horizontal
addr-flats-horizontal-z17

Given that addr:flats is at a higher level and based on the comments addr:flats and addr:unit would not be used on the same object, addr:flats would always win over addr:unit.

That sounds right

Both would be rendered starting with z18 (demoting current addr:unit rendering from z17).

Possibly

@richlv
Copy link
Contributor Author

richlv commented Mar 27, 2020

Demoted addr:flats and addr:unit to z18, moved addr:flats to the same line, made addr:flats override addr:unit.

Does that look acceptable?

@jeisenbe
Copy link
Collaborator

Thank you.

I would also like to see a slightly different style (smaller text size perhaps?) for addr:unit and addr:flats, to more clearly distinguish it from addr:housenumber.

@imagico - any thoughts about this cartographically?

@jeisenbe
Copy link
Collaborator

In Northern Ireland, the tag addr:flats is used in several different ways;

  1. As recommended in the wiki: a comma-separated list or range of apartment number
    addr-flats-29

addr-flats-30
(In this example the entrances are mapped this way)

  1. To map the housenumber of a townhouse or apartment
  • Here each door goes to 2 units, but the sequence of numbers is the same as those of the addr:housenumber for the townhouses to the northeast and southwest.
    z19
    addr-flats-26
    z18 - rather crowded still.
    addr-flats-21
  1. To add the name of the apartment building
    addr-flats-30
    (The name of the buildings in this image are tagged as addr:flats=Lismore Court rather than name=* or addr:housename, surprisingly)

Believe it or not, in this example the comma-separate list of numbers is the value of addr:housenumber and addr:flats is the name of the building! So right idea, wrong way:
addr-flats-28

@jeisenbe
Copy link
Collaborator

(One existing issue which I've noticed here is that it looks strange to render the housenumber or unit number or flats numbers right on top of the entrance icon, the brown square. See #3038. Also see #3071 for more issues about the entrance rendering)

@jeisenbe
Copy link
Collaborator

Looking at other places, it's hard to find many cities or countries which have addr:flats features. I didn't find any in Luxembourg or the parts of Australia, Netherland, Beligium or France which I've downloaded, or the countries I tried in Central America and Asia. It seems to be mostly used in eastern Europe.

In Buenos Aires, there are 3 shop or office features which also have addr:flats= but the value is a single number, like addr:unit. 2 other features have just a number on an entrance door.

There's only one in Brussels, on a church, oddly.

In Asturias, Spain there are 3 nodes on a building like https://www.openstreetmap.org/node/3008585185 which have addr:flats=3 even though the housenumber is different.

Two others have both addr:housenumber and addr:flats with a single number (like addr:unit): https://www.openstreetmap.org/node/3677624217 and https://www.openstreetmap.org/node/5740130190

In Oregon it is used on 9 nodes, but all of these also have amenity=* and a name, so would not be rendered. And here again addr:flats is a synonym for addr:unit: https://overpass-turbo.eu/s/RZZ

In Estonia there are only 7 nodes. In Lithuania there are 72 nodes, and most seem to be used as intended: with a range of flat numbers separated by a dash: https://overpass-turbo.eu/s/S00

Most Eastern European countries have a few hundred, though Latvia has over 800: https://overpass-turbo.eu/s/S01

So the great majority are in Belarus, Russia and Ukraine: eg https://overpass-turbo.eu/s/S02 - over half in Russia alone: https://overpass-turbo.eu/s/S03

Perhaps this tag is not widespread enough to be supported by this global style.

@richlv
Copy link
Contributor Author

richlv commented Mar 28, 2020

Yeah, the point of the pull request is to motivate contributors to collect this information :)

Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to treat addr:unit and addr:flats the same, it might make sense to do it in SQL rather than outline all the different combinations in MSS.

Something like

(SELECT 
    way
    array_to_string(array[
      NULLIF(array_to_string(array[addr_housenumber,COALESCE(addr_unit, addr_flats)]),''), -- Combine hounumber and unit/flats to one line
      addr_housename], E'\n') AS address -- Add housename on a new line if necessary
  FROM
  (SELECT
      ST_PointOnSurface(way) AS way,
      osm_id,
      "addr:housenumber" AS addr_housenumber
      "addr:housename" AS addr_housename,
      tags->'addr:unit' AS addr_unit,
      tags->'addr:flats' AS addr_flats,
      way_area
    FROM planet_osm_polygon
    WHERE way && !bbox!
      AND building IS NOT NULL
      AND way_area < 4000000*POW(!scale_denominator!*0.001*0.28,2)
  UNION ALL
      way,
      osm_id,
      "addr:housenumber" AS addr_housenumber
      "addr:housename" AS addr_housename,
      tags->'addr:unit' AS addr_unit,
      tags->'addr:flats' AS addr_flats,
      NULL
    FROM planet_osm_point
    WHERE way && !bbox!) AS addr_points
  WHERE "addr:housenumber" IS NOT NULL
    OR "addr:housename" IS NOT NULL
    OR tags ? 'addr:unit'
    OR tags ? 'addr:flats'
  ORDER BY way_area DESC NULLS LAST,
    osm_id
) AS addresses

project.mml Outdated Show resolved Hide resolved
project.mml Outdated Show resolved Hide resolved
style/addressing.mss Outdated Show resolved Hide resolved
@richlv
Copy link
Contributor Author

richlv commented Apr 2, 2020

@pnorman, thank you for showing the way to merge the info in SQL, looks like a great way.
It might reduce the styling flexibility, though - keeping as-is for now.

@jeisenbe, the point of this pull request is to motivate contributors to collect this information, which often is easily verifiable.
Without it being exposed, motivation to map it is low, and spotting mistakes is very hard.
How could I increase the interest in this pull request?

@jeisenbe
Copy link
Collaborator

jeisenbe commented Apr 3, 2020

I'm willing to support this PR if the other maintainers agree.

But right now there are changes requested above, which need to be addressed: #4082 (review)

Also, I still don't think addr:unit and addr:flats should ever be rendered on the same feature. Please fix this.

@richlv
Copy link
Contributor Author

richlv commented Apr 3, 2020

I'm willing to support this PR if the other maintainers agree.

But right now there are changes requested above, which need to be addressed: #4082 (review)

Do you mean merging information in SQL?
I didn't do that for now as it might limit the future flexibility in how we might want to change rendering style.
Should I do it now?

Also, I still don't think addr:unit and addr:flats should ever be rendered on the same feature. Please fix this.

That shouldn't be the case now, unless I have misunderstood how all this thing works.
Does it render unit+flats anywhere?

@jeisenbe
Copy link
Collaborator

jeisenbe commented Apr 3, 2020

That shouldn't be the case now, unless I have misunderstood how all this thing works. Does it render unit+flats anywhere?

Sorry, I did not realize that had been fixed. In testing, when both addr:flats= and addr:unit are tagged (often the addr:unit is duplicating the ref of the entrance, in the former USSR), then it appears addr:flats only will be rendered, now.

I'm not quite sure why addr:flats is rendered first, rather than addr:unit - perhaps due to alphabetical order?

Re: merging information in SQL - @pnorman was this a required change, or just a suggestion?

@richlv
Copy link
Contributor Author

richlv commented Apr 3, 2020

I'm not quite sure why addr:flats is rendered first, rather than addr:unit - perhaps due to alphabetical order?

That is intentional, as flats is higher semantically. I'm not aware of any case where these have been used together in a way that is consistent with the current wiki documentation, though - it's either duplicates, or addr:unit being used for entrances (like A, B, C...), and addr:flats being used for, well, flats in each entrance (a few buildings in Finland).

Given that tagging would likely have to be changed in all instances, I'm not too attached to the priority one way or another.

@pnorman
Copy link
Collaborator

pnorman commented Apr 3, 2020

Re: merging information in SQL - @pnorman was this a required change, or just a suggestion?

If others feel the nested MSS is understandable I'm okay keeping the complexity there.

@jeisenbe
Copy link
Collaborator

jeisenbe commented Apr 4, 2020

That is intentional, as flats is higher semantically

Oh, I agree. What I meant to say is that I don't quite understand why the current mss code works properly.

@pnorman do you understand why addr:flats is getting priority over addr:units when both are tagged on the same feature (with or without addr:housename)?

@pnorman
Copy link
Collaborator

pnorman commented Apr 4, 2020

@pnorman do you understand why addr:flats is getting priority over addr:units when both are tagged on the same feature (with or without addr:housename)?

The relevant MSS is

["addr_unit" != null]["addr_housenumber" = null] {
  text-name: [addr_unit];
}
["addr_flats" != null]["addr_housenumber" = null] {
  text-name: [addr_flats];
}

It's been awhile since I done much MSS with priorities, but my recollection is that the case of addr_unit and addr_flats matches both parts of the MSS, both parts have the same number of selectors, so it goes with the last one when generating the XML.

@jeisenbe
Copy link
Collaborator

jeisenbe commented Apr 4, 2020

Thank you, now that makes sense, though it is a little surprising.

@pnorman pnorman merged commit e7fc0c9 into gravitystorm:master Apr 7, 2020
@jeisenbe
Copy link
Collaborator

jeisenbe commented Apr 7, 2020

Thank you @richlv and congratulations on your first PR with Openstreetmap Carto!

If you might enjoy contributing further, we have a list of "Good First Issues" as well as some harder issues labeled "Help Wanted" if you are looking for a challenge.

@richlv
Copy link
Contributor Author

richlv commented Apr 8, 2020

Aaaaamazing. Thank you so much for all the tips and help.
I'm very excited to see this go in :)

A question, though - what is still missing for the rendering on osm.org to update?
For example, DB sync, code deployment or anything else?

@richlv richlv deleted the patch-1 branch April 8, 2020 18:21
@HolgerJeromin
Copy link
Contributor

This style releases every few weeks a new version. The next one will be on this friday (#4106).
After that the rollout on osm.org is done a few days later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressing new features Requests to render new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

render addr:flats same as addr:unit
6 participants