Check waterway flow direction

[gaben]

What steps will reproduce the problem?

1. Recreate the following as on the picture
2. Run the validator

What is the expected result?

JOSM should warn about the middle drain because it has drawn in the wrong direction (also see #12646).

What happens instead?

Nothing.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-03-26 22:46:08 +0100 (Tue, 26 Mar 2019)
Build-Date:2019-03-27 02:30:53
Revision:14936
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14936 hu) Windows 10 64-Bit
OS Build number: Windows 10 Pro 1809 (17763)
Memory Usage: 718 MB / 1820 MB (516 MB allocated, but free)
Java version: 1.8.0_201-b09, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1200
Maximum Screen Size: 1920x1200
VM arguments: [-Djava.security.manager, -Djava.security.policy=file:<java.home>\lib\security\javaws.policy, -DtrustProxy=true, -Djnlpx.home=<java.home>\bin, -Djnlpx.origFilenameArg=%UserProfile%\AppData\LocalLow\Sun\Java\Deployment\cache\6.0\31\583aa85f-6f9da138, -Djnlpx.remove=false, -Djava.util.Arrays.useLegacyMergeSort=true, -Djnlpx.heapsize=NULL,2048m, -Djnlpx.splashport=50584, -Djnlp.application.href=https://josm.openstreetmap.de/download/josm-latest.jnlp, -Djnlpx.jvm=<java.home>\bin\javaw.exe]
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (34867)
+ apache-commons (34506)
+ continuosDownload (82)
+ ejml (34389)
+ geotools (34513)
+ jaxb (34678)
+ jts (34524)
+ opendata (34911)
+ reverter (34946)
+ tag2link (34867)
+ tageditor (34867)
+ turnlanes-tagging (280)
+ utilsplugin2 (34932)
+ wikipedia (v1.1.1)

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/Maxspeed&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1

Validator rules:
+ https://josm.openstreetmap.de/josmfile?page=Rules/OsmoseValidations&zip=1

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.

[dmanzer95]

MapCSS validation check for incorrect drain flow

way[waterway=drain] >[index!=-1] node:connection {
        set notlastnode;
}

way[waterway=drain] node:connection!.notlastnode {
        throwWarning: "result";
}

way[waterway=drain] >[index!=1] node:connection {
        set notfirstnode;
}

way[waterway=drain] node:connection!.notfirstnode {
        throwWarning: "result2";
}

[anonymous]

Can anyone test the rule?

[Don-vip]

@Klumbumbus do we have core support to detect first/last node in mapcss without requiring to set custom class? It would simplify the proposed patch.

[Klumbumbus]

only with >[index=1] and >[index=-1]

[gaben]

I'm the OP.

How can I test the proposed patch?

[dmanzer95]

Replying to gaben:

I'm the OP.

How can I test the proposed patch?

you can save as a .mapcss file and add it locally in JOSM preferences

[dmanzer95]

i just tested on a larger data set and it needs some more work - throwing a lot a false positives

[dmanzer95]

refined the check slightly - ready for testing

*[waterway][waterway!=drain] node:connection {
        set exclude;
}

way[waterway=drain] >[index!=-1] node:connection {
        set notlastnode;
}

way[waterway=drain] node:connection!.notlastnode!.exclude {
        throwWarning: "result";
}

way[waterway=drain] >[index!=1] node:connection {
        set notfirstnode;
}

way[waterway=drain] node:connection!.notfirstnode!.exclude {
        throwWarning: "result2";
}

[Don-vip]

OK, some remarks:

• please add explicit child selector ">". The syntax you used works but is only here for compatibility
• the classes should have more specific names, so we can have similar rules
• of course the message warning must be reworded too :)

[gaben]

From user perspective the rule working good, I already fixed a fair amount of long waterways with many short wrong direction segments. So it's definitely worth implementing it properly :)

[dmanzer95]

Replying to Don-vip:

OK, some remarks:

• please add explicit child selector ">". The syntax you used works but is only here for compatibility
• the classes should have more specific names, so we can have similar rules
• of course the message warning must be reworded too :)

Hi Don-vip, I've added your recommendations. Does this look better?

*[waterway][waterway!=drain] > node:connection {
        set notDrain;
}

way[waterway=drain] >[index!=-1] node:connection {
        set notDrainLastNode;
}

way[waterway=drain] > node:connection!.notDrainLastNode!.notDrain {
        throwWarning: tr("incorrect drain flow direction");
}

way[waterway=drain] >[index!=1] node:connection {
        set notDrainFirstNode;
}

way[waterway=drain] > node:connection!.notDrainFirstNode!.notDrain {
        throwWarning: tr("incorrect drain flow direction");
}  

[dmanzer95]

I'm unfamiliar with creating a pull request in this repo. Can someone please point me in the right direction? Thank you

[Don-vip]

Replying to dmanzer95:

I'm unfamiliar with creating a pull request in this repo. Can someone please point me in the right direction?

Until we switch to git/gitlab you have to attach a patch to this ticket. See https://josm.openstreetmap.de/wiki/DevelopersGuide/PatchGuide

[anonymous]

Replying to Don-vip:

Replying to dmanzer95:

I'm unfamiliar with creating a pull request in this repo. Can someone please point me in the right direction?

Until we switch to git/gitlab you have to attach a patch to this ticket. See https://josm.openstreetmap.de/wiki/DevelopersGuide/PatchGuide

Thanks! What's the timeline for this migration?

[Don-vip]

No ETA, there's a lot of work I need to do and little time to do it :(

[simon04]

In 15833/josm:

fix #17533 - Check waterway flow direction (patch by dmanzer95, modified)

[gaben]

Can someone confirm the fix? Tried applying @dmanzer95's patch now, which doesn't work at all and the integrated either :/ The proposed patch worked before, I don't know what happened. I'm using r15840.

[gaben]

Tested in a VM, it's not working.

[Don-vip]

@dmanzer95 can you please check? It doesn't work for me either.

Also, why is the check restricted to waterway=drain? The test would be useful for all flowing waterways.

[skyper]

Replying to Don-vip:

@dmanzer95 can you please check? It doesn't work for me either.

Also, why is the check restricted to waterway=drain? The test would be useful for all flowing waterways.

Should be the opposite. Drain and some canal might give false positives.

[Don-vip]

In 16131/josm:

see #17533 - revert r15833, doesn't work

[Don-vip]

@anyone interested in the feature: please submit a working patch

[dmanzer95]

Hello, all.

I have also tested the proposed patch in the latest version and can confirm that it does not currently work.

My theory of why it worked before, but doesn't now is because it appears that the MapCSS child selector is functioning differently in the latest version of JOSM. If others can confirm, I will open an additional ticket

In previous versions of JOSM, this MapCSS line would select all nodes on a way with the tag waterway=drain

way[waterway=drain] > node

In the latest version, this line does not function the same way, and does not select those nodes.
Did the JOSM MapCSS implementation change? https://josm.openstreetmap.de/wiki/Help/Styles/MapCSSImplementation

[gaben]

@dmanzer95: Can confirm, the functionality lost between [15632:15644].

[Don-vip]

So likely an unexpected regression from r15640. Strange

[GerdP]

Seems untagged nodes are no longer checked in MapCSSTagChecker?

[GerdP]

Other tests like highway node connected to building also don't work.
Work around:

src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java

@@ -969 +969 @@
             }
         }
     }
+
+    @Override
+    public boolean isPrimitiveUsable(OsmPrimitive p) {
+        return p.isUsable() && (!(p instanceof Way) || (((Way) p).getNodesCount() > 1)); // test only Ways with at least 2 nodes
+    }
 }

[GerdP]

Better solution might be to revert the changes regarding p.isTagged()

[Don-vip]

Replying to GerdP:

Better solution might be to revert the changes regarding p.isTagged()

Yep, I'm doing that

[Don-vip]

In 16154/josm:

see #17533 - see #18455 - partial revert of r15640, causing untagged nodes to be skipped entirely

[Don-vip]

@dmanzer95 can you please check the patch works again, and address comment:24 / comment:25? Thanks!

[Klumbumbus]

• Currently we draw direction arrows for, waterway=stream|river|ditch|drain|tidal_channel, see here​
• About missing conections (unconnected ends) we warn for waterway=stream|river|drain, see here​ (ditch was removed with #16334 and tidal_channel not added as they end more often than the others into the ocean without further connection.)

[dmanzer95]

I've tested version 15937 and it does not work. Which version should I be testing?

Addressing comments 24 and 25: Testing all waterways is much more complex. I can look into it using MapCSS, but it might be better executed as a java check

[Don-vip]

Replying to dmanzer95:

I've tested version 15937 and it does not work. Which version should I be testing?

r16154+, see comment:35

[GerdP]

sample file with drain and river

[GerdP]

The example in 17533-sample.osm doesn't produce a warning, neither for waterway=drain nor for waterway=river.
Tested with r16406.
My understanding is that the river should produce two warnings, but the drains have no direction.

[gaben]

Replying to dmanzer95:

might be better executed as a java check

The problem with that, it makes the check JOSM exclusive. At least until a standalone validator implemented (see #15182).

So anyone knows how to start such a check? Does it need pathfinding like A*?

[gaben]

iD already has this feature: ​https://github.com/openstreetmap/iD/issues/6216

[gaben]

So the once applied patch seems to work again after r16154. Can you please test it (from r15833)?

[skyper]

Actually, I lost track of the tickets regarding flow directions or looping of waterways.