[PATCH] parent_osm_id should have a parent_osm_ids

[taylor.smock]

I'm in the process of writing a validator to find mistagged roundabouts (e.g., two roads come in at the same node) and that works, if and only if the names of the roads are different. Ideally, I would just run a count(parent_osm_ids) since those *are* guaranteed to be different.

Example of my code:

way[junction=roundabout][name] > node {set .is_in_named_roundabout}
node.is_in_roundabout!.is_in_named_roundabout[count(parent_tags("name")) > 1],
node.is_in_roundabout.is_in_named_roundabout[count(parent_tags("name")) > 2] {
        throwWarning: tr("Roundabouts should not have more than two additional ways at any node ({0})", parent_tags("name"));
}

[GerdP]

Maybe a test like that is better coded in Java? My understanding is that we have to separate ways that belong to the roundabout from those that are connected to the roundabout. The latter should start or end at the roundabout and there should not be two or more ways
connected to a roundabout (count excluding the roundabout ways). Also, there might be a test that the roundabout ways really form a closed way in case that the roundabout is split and that those ways all have the same highway tag.
If you agree I volunteer to code that.

[taylor.smock]

I absolutely don't mind if you code it.
I just thought it would be easier to add a parent_osm_ids() since there is already a parent_tags() function that (presumably) gets a list of parent_osm_ids and then runs a method to get tags on them, and I can see other potential uses for it as well.
As soon as I can upload my validator.mapcss code to github, and my employer agrees to put it under a compatible license (probably GPLv2+), I'll drop a link to the repo here so you don't have to redo some of my work.

What I have done as far as validation checks for roundabouts:
Checks if the way ends at the roundabout
Checks that the way is either a oneway or has a traffic calming island at the node on the roundabout
Looks for circular ways and asks if they are roundabouts (just looks for closed ways that are highways)

[GerdP]

As a co-maintainer of mkgmap I also know some ideas to check roundabouts. I am surprised reg. the traffic_island node. I think that node should not be on a way tagged junction=roundabout, only close to it.
Circular ways don't have to be roundabouts, junction=circular is just one possible alternative.

[taylor.smock]

The check I'm running on circular ways is looking for junction!=roundabout and junction!=circular. If I see other junctions that are typically circular, I'm definitely adding it to my validator. as an additional junction!=<new_junction>.

[taylor.smock]

Untested java method for parent_osm_ids (building)

[taylor.smock]

Java method for parent_osm_ids v2

[taylor.smock]

Java method for parent_osm_ids v3 -- now has a regex for selecting specific parents

[taylor.smock]

Should I write tests for this, or should I close this ticket as wontfix?

[GerdP]

Replying to taylor.smock:

Should I write tests for this, or should I close this ticket as wontfix?

Sorry, seems I forgot about this ticket. I don't fully understand what you want to test. My understanding is that you can analyse the parent ways of each node of a roundabout. There should never be more than one parent way which doesn't have the junction=roundabout tag.

[taylor.smock]

I pretty much wanted to find nodes on a roundabout which had more than one parent highway without a junction=roundabout tag, yes. I just wanted to point it out, since often it is just poor geometry.

I think it might be generally applicable to other things, but that is the reason why I originally wanted it.

[GerdP]

OK, looking at it. Already found a special case: A connected highway might not end at the roundabout node, in that case it has to be counted twice.

[GerdP]

test cases

[GerdP]

roundabout: check number of connected highways

[GerdP]

Please check attached patch:

• does it find all problem cases?
• does it produce false positives?
• should the primitives in the error messages contain all parts of the roundabout connected to the problem node?
• are the warning messages ok?

Should I create a new ticket for this test? Does it make your patch obsolete?

Not related to this patch: Why don't we check highway=service roundabouts?

I'll add unit test(s) if you agree with the code.

[GerdP]

Reg. attachement parent_osm_ids: I know it is very unlikely but you can have a parent way and a parent relation with the same id. Also, you don't know if the parent is a way or relation. Maybe use the typical prefix 'w' or 'r'? Or return a list of
OsmPrimitve instead of Long?
May you can add a sample use case?

[taylor.smock]

Replying to GerdP:

Please check attached patch:

• does it find all problem cases?
• does it produce false positives?
• should the primitives in the error messages contain all parts of the roundabout connected to the problem node?
• are the warning messages ok?

Should I create a new ticket for this test? Does it make your patch obsolete?

Not related to this patch: Why don't we check highway=service roundabouts?

I'll add unit test(s) if you agree with the code.

It looks good to me. The major issues I wanted to find were ways that did not end at the roundabout (I have a check in mapcss for that, but it pointed at the node IIRC, not the parent ways), and it finds ways that are connected to the same point on the roundabout.

I'll send a jar to some people I know who do a lot more road editing than I do. I don't see a lot of false positives (the wiki is fairly clear that incoming/outgoing ways should never connect at the same node, to ensure that routers recognize that they have to route you through a roundabout). Maybe if there is a case where multiple roads merge right before the roundabout, and the mapper decides to have them connect to the same node? I've never seen that, but that doesn't mean it doesn't exist.

It should (probably) be a separate patch. I (still) think it would be useful to get parent OSM primitive ids, e.g. you want to ensure that a node has a max of 100 parents or something.

I think I'll make modifications to the parent_osm_ids to return a List<OsmPrimitive> and add a method to change it to a List<String> with the nwr prefix (although I don't anticipate any n prefixes...)

[taylor.smock]

Return primitives instead of ids, add method to convert primitives to strings

[gaben]

Ping, #21801 partially depends on this.

[gaben]

Another ping comment. The patch seems well defined, so should be fine.

[taylor.smock]

In 18829/josm:

Fix #16998: Add parent_osm_primitives and convert_primitives_to_string

[taylor.smock]

Ticket retargeted after milestone deleted

[taylor.smock]

Ticket retargeted after milestone deleted