Modify

Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#20019 closed enhancement (fixed)

Warn about direction=forward/backward on invalid nodes.

Reported by: skyper Owned by: GerdP
Priority: normal Milestone: 20.12
Component: Core validator Version:
Keywords: template_report direction node Cc: francois.lacombe

Description (last modified by skyper)

What steps will reproduce the problem?

  1. Have an child end node or a child middle node of more than one way with [high|rail|water]way] and ^[.+:]?direction$=[forward|backward]
  2. run validator

What is the expected result?

A warning about invalid values of direction for the geometry

What happens instead?

No warning

Please provide any additional information below. Attach a screenshot if possible.

See #20013 for the origin.

Replying to Klumbumbus:

I think the validator should warn if a node with direction=* is

  • on a ways first/last node (possible in mapcss)
  • belongs to two different highway=* ways (not possible in mapcss)
  • Only warn about values forward and backward. Other values are valid.
  • Include railway and waterway in both test.
Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-10-31 01:21:14 +0100 (Sat, 31 Oct 2020)
Revision:17288
Build-Date:2020-10-31 02:30:50
URL:https://josm.openstreetmap.de/svn/trunk

Attachments (3)

20019.patch (4.5 KB) - added by GerdP 4 months ago.
alpha version, please suggest improvements regarding the i18n strings
20019.2.patch (4.6 KB) - added by GerdP 4 months ago.
20019.3.patch (4.8 KB) - added by GerdP 4 months ago.

Download all attachments as: .zip

Change History (52)

comment:1 Changed 4 months ago by GerdP

Owner: changed from team to GerdP
Status: newassigned

comment:2 Changed 4 months ago by Famlam

Personally I have this in my custom rules, perhaps it helps? It also covers the 'two-different-highway' case with just mapcss.

node[direction=~/^(forward|backward)$/]:connection,
node["traffic_signals:direction"=~/^(forward|backward)$/][highway=traffic_signals]:connection {
  throwWarning: tr("Node with {0} on a connection of multiple ways", "{0.key}");
}

p.s.: obviously it can be made more generic using just a single regex; something like (untested):
node[/(^|:)direction$/=~/^(forward|backward)$/]:connection

Last edited 4 months ago by Famlam (previous) (diff)

comment:3 Changed 4 months ago by GerdP

Didn't try it, but the rule doesn't care about the tags of the parent ways. It probably produces false positives when ways are sharing nodes? For example when landuse=* polygons are glued to highways?

comment:4 Changed 4 months ago by Famlam

Indeed, but I don't expect many (i.e.) grass fields or residential borders to have (i.e.) (directional) traffic signals to enter or leave that landuse area, so probably those are all false positives. (Except "keep off the grass" signs perhaps, but even then direction only refers to the highway-way direction, unless anticlockwise grass-area walking is permitted ;) )

comment:5 Changed 4 months ago by GerdP

Here is an example for what I had in mind:
https://www.openstreetmap.org/node/1465196576

comment:6 Changed 4 months ago by Famlam

Hmm, ok, personally I would argue give_way is unrelated to (and thus shouldn't be connected to) the landuse. But I see your point, and unfortunately mapcss indeed lacks count_parents_with_tag or such.

comment:7 in reply to:  5 Changed 4 months ago by Klumbumbus

Replying to GerdP:

Here is an example for what I had in mind:
https://www.openstreetmap.org/node/1465196576

Yes, thats what I meant. The validator should't warn about this node.

comment:8 Changed 4 months ago by GerdP

So its going to be a new test coded in java.

comment:9 Changed 4 months ago by GerdP

I've started to implement this.
I think if we find a node with .*[:]?direction=[forward|backward] and no parent way this should also be flagged:
Unconnected node with {0}
if the node is inside a download area. I am not sure what to do if there are parent ways but none has a key that matches the pattern [high|rail|water]way]. Ignore them to avoid false positives? Or create an information?

Changed 4 months ago by GerdP

Attachment: 20019.patch added

alpha version, please suggest improvements regarding the i18n strings

comment:10 Changed 4 months ago by GerdP

Milestone: 20.11

comment:11 Changed 4 months ago by GerdP

+        super(tr("Direction nodes"), tr("Check for nodes which have a direction"));
+                        .message(tr("Unconnected node with {0}", key)).build());
+                        .message(tr("Node with {0} on end of way", key)).build());
+                    .message(tr("Node with {0} on a connection of multiple ways", key)).build());
  • I tend to think that it would be better to show the full tag instead of only the key in the error messages, since values other than forward/backward are allowed.
  • I don't like "Direction node" but found no better phrase yet.
  • What about closed highways where the node is the start+end node?

comment:12 Changed 4 months ago by skyper

Milestone: 20.11

Please, always warn about new nodes (id=0)

If the node is new and/or inside download area it needs be a middle node of only one way with special primary key.

Have to check data but so far I only found man_made=pipeline and power=* as possible additional candidates. What other tags exist for infrastructure on ways?

comment:13 Changed 4 months ago by skyper

Milestone: 20.11

Changed 4 months ago by GerdP

Attachment: 20019.2.patch added

comment:14 Changed 4 months ago by GerdP

  • reports full tag
  • handles new nodes
  • adds man_made=pipeline as allowed parent way
  • complains if no allowed parent way was found (as "Unconnected node", searching for a better text)

I don't yet understand power=*. Do you have an example?

comment:15 Changed 4 months ago by Famlam

Minor thing: MULLTIPLE_WAYS_CODE typo in the variable name ;)

comment:16 Changed 4 months ago by GerdP

Thanks, will correct that.
BTW: Here is a link to find some test data with overpass: http://overpass-turbo.eu/s/ZJI

comment:17 Changed 4 months ago by GerdP

The regex doesn't work yet, it also complains about xyzdirection=forward.

Changed 4 months ago by GerdP

Attachment: 20019.3.patch added

comment:18 Changed 4 months ago by GerdP

20019.3.patch:

  • combine warnings in group Invalid usage of direction on node
  • only check keys if equal to direction or ending with :direction
  • suppress Unconnected node with {0} message when node is connected to ways which are not "suitable":
        private static boolean isSuitableParentWay(Way w) {
            return w.hasKey("highway", "railway", "waterway") || w.hasTag("man_made", "pipeline");
        }
    

comment:19 Changed 4 months ago by Famlam

The regex doesn't work yet, it also complains about xyzdirection=forward.

The regex should've been (^|:)direction, meaning "starts with OR contains ':', followed by 'direction', but this looks like a good solution too :)

Not sure what the overpass query checks, but it marks nearly every traffic light with direction in Nijmegen (NL), also good ones?

comment:20 Changed 4 months ago by GerdP

The overpass query checks nothing, it just collects input for the test. Once the basic code is accepted I'll try to prepare a test file for unit tests.

comment:21 Changed 4 months ago by skyper

Description: modified (diff)

comment:22 in reply to:  14 Changed 4 months ago by skyper

Cc: francois.lacombe added

Replying to GerdP:

I don't yet understand power=*. Do you have an example?

No, do not have an example. I was thinking about infrastructure on ways with nodes describing a lock or gate in one direction. As there are electric components like diodes, I thought that there is a chance one of these tags is used on nodes with a parent way power=*.

Have to dig around in France as I did not find a query to run on Europe or the globe, yet.

comment:23 Changed 4 months ago by francois.lacombe

Thank you for notice :)

I think there is not such object on power distribution nor transmission networks. Power could go forward and backward depending on loads. On meshed networks it's totally intermittent.
Power flows are usually directed by adjusting the impedance of lines not with diodes or one-way other component of any kind.

It's ok on pipelines since we find non-return valves.

comment:24 Changed 4 months ago by skyper

How about suggesting: Use cardinal directions or numbers, instead, or move relevant tags onto a middle way node connected to only one way with tags highway=*, railway=*, waterway=* or man_made=pipeline.

comment:25 Changed 4 months ago by Famlam

How about suggesting: Use cardinal directions or numbers, instead, or move relevant tags onto a middle way node connected to only one way with tags highway=*, railway=*, waterway=* or man_made=pipeline.

I'm not sure if I would understand middle way node or the sentence at all, sorry :)
I personally think the error messages are clear as they are in the patch

comment:26 Changed 4 months ago by GerdP

Cardinal numbers would be wrong. They are used when you map the position of the traffic-sign as a standalone node. Here we map the meaning of the sign for the way.

comment:27 Changed 4 months ago by skyper

Convinced me, both of you. Always forget that I cannot treat direction and *:direction equally.

comment:28 Changed 4 months ago by Don-vip

Milestone: 20.1120.12

Milestone renamed

comment:29 Changed 3 months ago by GerdP

In 17349/josm:

see #20019: Warn about direction=forward/backward on invalid nodes.

  • implement new test DirectionNodes which uses error codes 4000-4099

TODO: unit test

comment:30 Changed 3 months ago by GerdP

In 17352/josm:

see #20019: Warn about direction=forward/backward on invalid nodes.

  • fix parenthesis

comment:31 Changed 3 months ago by stoecker

In 17368/josm:

fix i18n, see #20019

comment:32 Changed 3 months ago by GerdP

Oops, thanks!

comment:34 Changed 3 months ago by GerdP

I started to collect test cases for unit tests. I wonder what to with traffic_sign=city_limit like n427123883. It produces
Invalid usage of direction on node - Node with direction=backward on a connection of multiple ways (1)
I think this is a false positive, but maybe I'm wrong?

Version 1, edited 3 months ago by GerdP (previous) (next) (diff)

comment:35 Changed 3 months ago by skyper

What happens if you reverse the direction of one connected way. Is it denied or do I get a warning?

Still think forward/backward on nodes are ticking time bombs and instead of adjusting each editor software a proper tagging and/or tagging system is the cleaner solution.

city_limits are even more special as they can be valid for two directions. Still forward/backward will not work unless both ways are reversed and if these ways have several nodes with forward/backward more ways have to be taken into account which might be even not downloaded.

comment:36 in reply to:  35 Changed 3 months ago by GerdP

Replying to skyper:

What happens if you reverse the direction of one connected way. Is it denied or do I get a warning?

A dialog pops up, but it doesn't warn that the node is connected to other ways.

Still think forward/backward on nodes are ticking time bombs and instead of adjusting each editor software a proper tagging and/or tagging system is the cleaner solution.

city_limits are even more special as they can be valid for two directions. Still forward/backward will not work unless both ways are reversed and if these ways have several nodes with forward/backward more ways have to be taken into account which might be even not downloaded.

I've also asked in the German Forum because it was discussed this year, see https://forum.openstreetmap.org/viewtopic.php?id=70180

comment:37 Changed 3 months ago by aceman

Hi, I get this new warning on a junction of a footway and a road where there is a traffic_lights crossing. It is common to tag that shared now with highway=traffic_signals and crossing=traffic_signals. The traffic_signals:direction should only apply to the road direction, not for pedestrians. If this warning is to stay, what is the correct way to tag this?

comment:38 Changed 3 months ago by GerdP

I also think the wording is too aggressive. In a discussion in the German forum I suggested to change it to something like "Disputed usage" instead of "Invalid usage".

comment:39 Changed 3 months ago by GerdP

Resolution: fixed
Status: assignedclosed

In 17411/josm:

fix #20019: Warn about direction=forward/backward on invalid nodes.

  • show error for unconnected node Unconnected node with {0}. Use angle or cardinal direction
  • show warning for node that is connected, but not to a suitable way Node with {0} should be connected to a linear way
  • show information for Node with {0} on end of way and Node with {0} on a connection of multiple ways, both in group Disputed usage of direction on node
  • special handling for highways: if there is a major highway as defined in Highways.CLASSIFIED_HIGHWAYS, ignore minor ways like footway, path to reduce false positives at traffic lights.

comment:40 Changed 3 months ago by GerdP

In 17412/josm:

see #20019: Warn about direction=forward/backward on invalid nodes.

  • simplify source
  • improve test coverage

comment:41 Changed 3 months ago by GerdP

In 17413/josm:

see #20019: Warn about direction=forward/backward on invalid nodes.

  • revert changes in DirectionNodes.java from 17412
  • improve test coverage (nodes with positive ids)

comment:42 Changed 3 months ago by GerdP

Please try trunk/nodist/data/direction-nodes.osm and also some real world data.
At the moment, the value in i18n Strings {0} contains the tag. Maybe it would be better to use tag key + =forward/backward?

comment:43 Changed 3 months ago by skyper

Actually, I like the tags in the string, as I do not care about railway tagging that much and you have individual warnings to add to the ignore list.

comment:44 Changed 3 months ago by GerdP

I don't understand this comment. How is railway tagging related? The strings are not relevant for the ignore list.

comment:45 Changed 3 months ago by skyper

It makes a difference if railway:signal:direction or traffic_signals:direction is warned about. Maybe the warning could be split into several depending on the major tag of the parent ways?

comment:46 Changed 3 months ago by GerdP

I think you got comment:42 wrong. I wonder if we should report two different error messages for forward and backward.
You probably never want to ignore only the forward message?

comment:47 Changed 3 months ago by skyper

Yes, you are right, sorry. We only need the key and not the tag (key_value). The values can be directly in the message or even left out.

comment:48 Changed 3 months ago by GerdP

I think we need them in the message to distinguish fron the angle or cardinal direction

comment:49 Changed 3 months ago by GerdP

In 17415/josm:

see #20019: Warn about direction=forward/backward on invalid nodes.

  • use tag key + "=forward|backward" in messages so that we don't get separate messages for each direction

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
as The resolution will be set.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.