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 place names for polygons if no point exists for area #2939

Closed
wants to merge 3 commits into from

Conversation

Komzpa
Copy link

@Komzpa Komzpa commented Nov 9, 2017

A patch to #2816 that doesn't create duplicate labels and compatible with current node-and-polygons data.

@mboeringa
Copy link

Thanks, @Komzpa , this is the only reasonable way forward in regards to this issue.

@matkoniecz
Copy link
Contributor

Is it tested full worldwide place data? It should be ensured that it is not causing performance problems.

@matthijsmelissen
Copy link
Collaborator

To me it seems like a quite complex solution (which every data consumer will have to apply). I'd really like to have rendering rules which stimulate easy to consume data for other providers.

This is not to dismiss @Komzpa's proposal, which is a great solution given the currently complex data.

@matkoniecz
Copy link
Contributor

matkoniecz commented Nov 9, 2017

I'd really like to have rendering rules which stimulate easy to consume data for other providers.

It will reveal more data problems than current rendering and will not hide any tagging errors that are currently visible.

@mboeringa
Copy link

To me it seems like a quite complex solution (which every data consumer will have to apply). I'd really like to have rendering rules which stimulate easy to consume data for other providers.

No, they won't necessarily.

That's the whole point of @Komzpa's solution:

Users can choose to ignore merging ways and nodes tagged with place in their own rendering setup and styles, and simply use place nodes as the single source of place data as they probably do now, as this pull is intended to favour to keep existing nodes, while still allowing label rendering of potentially newly added place areas / ways, instead of stimulating to drop the nodes as per the original proposal.

From a more technical point of view, the code is hardly more complex than the original proposal, except the added basic EXISTS and ST_Intersects usage.

@matthijsmelissen
Copy link
Collaborator

matthijsmelissen commented Nov 9, 2017

while still allowing label rendering of potentially newly added place areas / ways, instead of stimulating to drop the nodes as per the original proposal.

Yes, I agree this proposal is better than the original.

One of the remaining questions is, are we really happy with places tagged as area without them being tagged as nodes? (I know the wiki allows it.) And are we really happy with double-tagging as both area and node?

I have the feeling that the place-nodes and place-polygons don't have exactly the same semantics (that's why we cannot remove polygons and replace them by nodes), unlike the case with POI like supermarkets that can both be nodes and polygons.

I'm inclined to suggest using a differently named tag for polygons, and restrict the place tag to nodes. Which means no change on our side would be necessary. It would require a change to the wiki and to the data, though.

@matthijsmelissen
Copy link
Collaborator

@pnorman As this intends to supersede your PR, do you have any comments?

@matkoniecz
Copy link
Contributor

One of the remaining questions is, are we really happy with places tagged as area without them being tagged as nodes? (I know the wiki allows it.) And are we really happy with double-tagging as both area and node?

I am unhappy about both and have no good idea to fix situation - I would post it on ml or wiki if I had a good idea.

At this moment it seems that accepting both is a the best solution, so assuming that this PR works with full database (performance really should be tested before merging) I would prefer this PR.

@matkoniecz matkoniecz self-requested a review November 10, 2017 01:11
Copy link
Contributor

@matkoniecz matkoniecz left a comment

Choose a reason for hiding this comment

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

Check whatever worldwide database with all places has acceptable performance.

@matthijsmelissen
Copy link
Collaborator

I don't suppose we are requesting this from @Komzpa?

@matkoniecz
Copy link
Contributor

matkoniecz commented Nov 10, 2017

Yes, it is more of "somebody must to this before it is merged" rather than " @Komzpa as PR author must do this" as it is more complicated than standard before/after image.

Though, that reminds me - is it tested for at least some small-scale basic cases? https://github.com/gravitystorm/openstreetmap-carto/blob/master/CONTRIBUTING.md#pull-requests has

Pull requests that change the cartography should contain a few images selected to show the changes. The easiest way to make these is by taking screenshots and cropping them, then pasting them into the issue. Avoid adding an overwhelming number of screenshots.

@Komzpa - can you provide before/after for basic cases like

  • only node mapped
  • only area mapped
  • both node and area mapped
  • two different places with the same name (one mapped as node or node+area, second only as an area)
    ?

@mboeringa
Copy link

mboeringa commented Nov 10, 2017

I have the feeling that the place-nodes and place-polygons don't have exactly the same semantics (that's why we cannot remove polygons and replace them by nodes), unlike the case with POI like supermarkets that can both be nodes and polygons.

@math1985 . Yes, there is some truth in the difference of semantics, but remember, this is intentional as well.

Please recall @skylerbunny's nice proposal for the type=place relation (#2816 (comment), #2816 (comment), and especially https://wiki.openstreetmap.org/wiki/User:Skybunny/Relation:place), and that I commented on there as well (#2816 (comment)), where the existing place nodes would explicitly and intentionally assume the role=waypoint as part of the type=place or type=boundary relation to signify this semantic difference.

Assuming that the type=place relation (and the usage of role=waypoint on type=boundary relations as well) as described by @skylerbunny is accepted, I don't think there is any need to redefine place=x as area.

@mboeringa
Copy link

mboeringa commented Nov 10, 2017

Is it tested full worldwide place data? It should be ensured that it is not causing performance problems.

@matkoniecz .

Admittedly, it is not a big sample, but this one remark (http://gis.19327.n8.nabble.com/Place-names-tagging-rendering-and-processing-td5902489.html) by Andrey Novikov on the developer discussion mailing list as response to a question posted by @kocio-pl, seems to indicate at least one person successfully runs a similar algorithm as this PR against a rendering database. Although just a single sentence, Andrey clearly seems to describe a similar solution to the duplication problem as implemented in this PR.

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.

To me it seems like a quite complex solution (which every data consumer will have to apply). I'd really like to have rendering rules which stimulate easy to consume data for other providers.

I'm worried about this, and we're encouraging duplication in OSM by doing this.

I have not performed a technical review.

@skylerbunny
Copy link
Contributor

where the existing place nodes would explicitly and intentionally assume the role=waypoint as part of the type=place or type=boundary relation to signify this semantic difference.

Even more so:

  • for place areas that do not have a 'waypoint' node member now, and...
  • For data consumers that don't know or care about areas tagged as 'place'...

The 'place area' proposal covers both, because it encourages both a node and area when an area exists, and (since it tells people to put 'the important tags' on the node), data consumers don't have to change what they're doing because it's best practice to continue to tag a node. At that point the only missing piece is intelligent de-duping in rendering – something many other engines do – and here we are.

(As stated above, performance testing of the de-dup solution is good prior of course. But that's a different issue.)

Copy link
Contributor

@matkoniecz matkoniecz left a comment

Choose a reason for hiding this comment

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

@Komzpa - can you provide before/after for basic cases like

only node mapped
only area mapped
both node and area mapped
two different places with the same name (one mapped as node or node+area, second only as an area)
?

@Komzpa
Copy link
Author

Komzpa commented Nov 13, 2017

@pnorman with all respect, "I am worried about it" is not a technical PR change request.

We're not encouraging duplication, we're using already available data in OSM.
It is common to have two geometries of different type for an object, one as skeleton and one as coverage.

Building: polygon of building=yes, and a point of addr:*.
Road: polygon of area:highway=*, and a centerline of highway=*
River: polygon of waterway=riverbank, and a centerline of waterway=river.
Place: polygon of place=*, and a center node of place=*

@Komzpa
Copy link
Author

Komzpa commented Nov 13, 2017

@matkoniecz

only node: only node drawn.
only area: only area drawn.
both node and area: both drawn if they not intersect or have different place=, only node drawn if intersect and place= is the same.
two different places: all nodes drawn. if area intersects with point with same name= and same place=, it is not drawn.

@matkoniecz
Copy link
Contributor

@matkoniecz

Note that I asked about images - surprisingly often rendering is not working like one would expect.

@pnorman
Copy link
Collaborator

pnorman commented Nov 16, 2017

@pnorman with all respect, "I am worried about it" is not a technical PR change request.

Considering if this would require more complex work from other data consumers is on-topic for a PR review.

@mboeringa
Copy link

@pnorman and @matkoniecz : as far as the technical issues:

  • As I already stated, the SQL itself is hardly more complex than the original proposal. It is really minimal and quite understandable, no wizardry here, so from the style maintenance point I guess this is really one of the lesser changes and should not withhold merging.

  • From the performance point, I included one link to a @kocio-pl started "Developer Discussion" mailing thread in this Render place names for polygons if no point exists for area #2939 (comment) post, that, although admittedly based on just one short statement, seems to indicate that at least some others already de-duplicate in quite the same manner in their styles. As that user wrote:

"As a data consumer I discard area for labeling if it contains node with the same place= and name= keys. This is not 100% safe but removes major label duplications from my map."

This short sentence describes what this pull functionally implements.

@pnorman
Copy link
Collaborator

pnorman commented Nov 17, 2017

I would rather either decide that we accept place on either polygons or points, or decide that we don't accept it on polygons.

@mboeringa
Copy link

mboeringa commented Nov 18, 2017

I would rather either decide that we accept place on either polygons or points

I and a couple of others have already accepted this..., due to the clear technical need for both place nodes and place areas. It is really not an option to only have either nodes or areas, see the discussions and arguments in #2816 .

Place is just the one rare, or maybe the only real, exception where sticking to the "one feature, one OSM element" paradigm of OpenStreetMap, does more harm than good.

I also think that, even though place areas and place nodes may share the "place" tag as name as that is the only natural way to think of them, that, as @math1985 also pointed out, they are in fact semantically different. They are related, through being associated with some build up area, but they are not equivalent or entirely interchangeable. The node, as also defined by @skylerbunny's "place relation" waypoint role, serves a different purpose (routing and labelling) than the area (which is primarily for low / medium scale display and secondarily for labelling). This is a fundamental difference.

One could argue that they need separate tags or keys, but I would be against this, because it would bely the natural feeling people and OSM editors have that a place can be both a node and area. Creating separate tags would lead to a higher risk of improper usage and confusion among OSM users.

Rather, I would like to see the kind of proposal as @skylerbunny made for the optional type=boundary / boundary=place relation (https://wiki.openstreetmap.org/wiki/User:Skybunny/Relation:place), with the way and node features' distinct semantics being reflected in the relation role, while still maintaining full backwards compatibility with the old usage of place nodes for routing and labelling, but not blocking or preventing usage of place as areas.

@Hjart
Copy link

Hjart commented Nov 18, 2017

@mboeringa

Re:

Place is just the one rare, or maybe the only real, exception where sticking to the "one feature, one OSM element" paradigm of OpenStreetMap, does more harm than good.

I disagree. Please see my post in #2816

@rrzefox
Copy link

rrzefox commented Nov 18, 2017

I've deployed this now in the usual place (Edit: all tiles have rerendered now), but having a hard time finding examples where this would actually cause different rendering. Can you point to any examples? With this it should now be trivial to generate the example images matkoniecz requested.

@Hjart
Copy link

Hjart commented Nov 18, 2017

@rrzefox
Copy link

rrzefox commented Nov 19, 2017

@Hjart that (http://bl.ocks.org/math1985/raw/af7a602c222dbf1ff1a2c0d84ed755b7/#14.00/55.3651/9.0623) is one example for no node, only polygon, but not the most interesting one (as PR2816 should render this in exactly the same way). My question was mostly about places where this generates a different rendering than the "alternative" PR 2816.

@Hjart
Copy link

Hjart commented Nov 19, 2017

@rrzefox Ok. Ribe is still defined by both a node (https://www.openstreetmap.org/node/597643755) and an area (https://www.openstreetmap.org/relation/6334582). Hopefully that's what you are looking for.

I don't see anything in your link other than the words "proposed" and "current", btw

@mboeringa
Copy link

mboeringa commented Nov 19, 2017

My question was mostly about places where this generates a different rendering than the "alternative" PR 2816.

@rrzefox Unless you temporarily deploy #2816 as well on your server, and create some screenshots for comparison, you won't be able to see the difference between #2816 and #2939.

@pnorman 's #2816 should cause a duplicate label for the Ribe example @Hjart pointed out, while this PR #2939 by @Komzpa only renders a single label as it de-duplicates, meaning there is no visual difference for #2939 and current rendering - which is only based on the node - for the Ribe village.

However, as to #2939 , the below screenshot for Rodding shows that it will render the place tag also from areas, as was the wish of many. Notice the current rendering does not show the name of Rodding, as the current rendering is based on nodes only, and the Rodding's name is only available on a place=x way feature, not a node.

afbeelding

afbeelding

@SafwatHalaby
Copy link

If a place relation multipolygon exists and there is no center place node, will this render the name?

If a boundary relation exists, which also has a place tag (but it is not a multipolygon), will this render the name?

@Komzpa
Copy link
Author

Komzpa commented Nov 23, 2017

@SafwatHalaby relations are converted into polygons by osm2pgsql, so way place= is the same as type=boundary place= and the same as type=multipolygon place=.

@matthijsmelissen
Copy link
Collaborator

First of all, sorry for not getting back to this earlier.

This was a heated discussion, and a decision will need to be made one way or the other.

To me it seems like a quite complex solution (which every data consumer will have to apply). I'd really like to have rendering rules which stimulate easy to consume data for other providers.

I'm worried about this, and we're encouraging duplication in OSM by doing this.

For the reasons above, I am going to reject this PR.

@Komzpa thanks a lot for all the effort you put in, and joining the discussion, though!

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.

9 participants