Modify

Opened 2 years ago

Last modified 16 months ago

#16998 new enhancement

[PATCH] parent_osm_id should have a parent_osm_ids

Reported by: taylor.smock Owned by: team
Priority: minor Milestone:
Component: Core mappaint Version: tested
Keywords: Cc: Don-vip

Description (last modified by Don-vip)

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"));
}

Attachments (7)

0001-Initial-work-on-getting-get_parent_ids-working.patch (2.1 KB) - added by taylor.smock 2 years ago.
Untested java method for parent_osm_ids (building)
0001-Initial-work-on-getting-get_parent_ids-working.2.patch (2.6 KB) - added by taylor.smock 2 years ago.
Java method for parent_osm_ids v2
0001-Initial-work-on-getting-get_parent_ids-working.3.patch (2.7 KB) - added by taylor.smock 2 years ago.
Java method for parent_osm_ids v3 -- now has a regex for selecting specific parents
parent_osm_ids.patch (3.0 KB) - added by taylor.smock 2 years ago.
16998-test.osm (7.6 KB) - added by GerdP 16 months ago.
test cases
16998.patch (4.7 KB) - added by GerdP 16 months ago.
roundabout: check number of connected highways
16998.1.patch (4.8 KB) - added by taylor.smock 16 months ago.
Return primitives instead of ids, add method to convert primitives to strings

Download all attachments as: .zip

Change History (21)

comment:1 Changed 2 years ago by Don-vip

Description: modified (diff)

comment:2 Changed 2 years ago by 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.

comment:3 Changed 2 years ago by 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)

comment:4 Changed 2 years ago by 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.

comment:5 Changed 2 years ago by 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>.

Changed 2 years ago by taylor.smock

Untested java method for parent_osm_ids (building)

Changed 2 years ago by taylor.smock

Java method for parent_osm_ids v2

Changed 2 years ago by taylor.smock

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

comment:6 Changed 2 years ago by taylor.smock

Summary: parent_osm_id should have a parent_osm_ids[PATCH] parent_osm_id should have a parent_osm_ids

Changed 2 years ago by taylor.smock

Attachment: parent_osm_ids.patch added

comment:7 Changed 16 months ago by taylor.smock

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

comment:8 Changed 16 months ago by stoecker

Cc: Don-vip added
Component: External ruleCore mappaint

comment:9 in reply to:  7 Changed 16 months ago by 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.

comment:10 Changed 16 months ago by 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.

comment:11 Changed 16 months ago by 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.

Changed 16 months ago by GerdP

Attachment: 16998-test.osm added

test cases

Changed 16 months ago by GerdP

Attachment: 16998.patch added

roundabout: check number of connected highways

comment:12 Changed 16 months ago by 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.

comment:13 Changed 16 months ago by 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?

comment:14 in reply to:  12 Changed 16 months ago by 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...)

Changed 16 months ago by taylor.smock

Attachment: 16998.1.patch added

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to taylor.smock
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket
The owner will be changed from team to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.