Modify

Opened 2 years ago

Last modified 6 months ago

#17533 reopened enhancement

Check waterway flow direction

Reported by: gaben Owned by: team
Priority: normal Milestone:
Component: Core validator Version: latest
Keywords: waterway direction Cc: Klumbumbus, gaben

Description (last modified by Don-vip)

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.

Attachments (2)

flow direction.png (9.2 KB) - added by anonymous 2 years ago.
17533-sample.osm (3.3 KB) - added by GerdP 11 months ago.
sample file with drain and river

Download all attachments as: .zip

Change History (43)

Changed 2 years ago by anonymous

Attachment: flow direction.png added

comment:1 Changed 2 years ago by Don-vip

Description: modified (diff)

comment:2 Changed 2 years ago by Don-vip

Keywords: waterway direction added

comment:3 Changed 2 years ago by 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";
}
Last edited 23 months ago by Don-vip (previous) (diff)

comment:4 Changed 23 months ago by anonymous

Can anyone test the rule?

comment:5 Changed 23 months ago by Don-vip

Cc: Klumbumbus added
Summary: Check waterway flow direction[PATCH] Check waterway flow direction

@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.

comment:6 Changed 23 months ago by Klumbumbus

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

comment:7 Changed 19 months ago by gaben

Cc: gaben added

I'm the OP.

How can I test the proposed patch?

comment:8 Changed 19 months ago by Don-vip

Reporter: changed from anonymous to gaben

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

comment:10 Changed 19 months ago by dmanzer95

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

comment:11 Changed 19 months ago by 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";
}
Last edited 19 months ago by gaben (previous) (diff)

comment:12 Changed 19 months ago by 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 :)

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

comment:14 in reply to:  12 Changed 16 months ago by 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");
}  
Last edited 16 months ago by dmanzer95 (previous) (diff)

comment:15 Changed 15 months ago by dmanzer95

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

comment:16 Changed 15 months ago by stoecker

Milestone: 20.01

comment:17 in reply to:  15 ; Changed 15 months ago by 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

comment:18 in reply to:  17 Changed 15 months ago by 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?

comment:19 Changed 15 months ago by Don-vip

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

comment:20 Changed 15 months ago by Don-vip

Milestone: 20.0120.02

comment:21 Changed 15 months ago by simon04

Resolution: fixed
Status: newclosed

In 15833/josm:

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

comment:22 Changed 15 months ago by 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.

comment:23 Changed 14 months ago by gaben

Resolution: fixed
Status: closedreopened

Tested in a VM, it's not working.

comment:24 Changed 14 months ago by Don-vip

Milestone: 20.0220.03

@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.

comment:25 in reply to:  24 Changed 14 months ago by 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.

Last edited 14 months ago by skyper (previous) (diff)

comment:26 Changed 13 months ago by Don-vip

In 16131/josm:

see #17533 - revert r15833, doesn't work

comment:27 Changed 13 months ago by Don-vip

Milestone: 20.03
Summary: [PATCH] Check waterway flow directionCheck waterway flow direction

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

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

comment:29 Changed 13 months ago by gaben

@dmanzer95: Can confirm, the functionality lost between r15632 and r15644.

comment:30 Changed 13 months ago by Don-vip

So likely an unexpected regression from r15640. Strange

comment:31 Changed 13 months ago by GerdP

Seems untagged nodes are no longer checked in MapCSSTagChecker?

comment:32 Changed 13 months ago by GerdP

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

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

     
    969969            }
    970970        }
    971971    }
     972
     973    @Override
     974    public boolean isPrimitiveUsable(OsmPrimitive p) {
     975        return p.isUsable() && (!(p instanceof Way) || (((Way) p).getNodesCount() > 1)); // test only Ways with at least 2 nodes
     976    }
    972977}

comment:33 Changed 13 months ago by GerdP

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

comment:34 in reply to:  33 Changed 13 months ago by Don-vip

Replying to GerdP:

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

Yep, I'm doing that

comment:35 Changed 13 months ago by Don-vip

In 16154/josm:

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

comment:36 Changed 13 months ago by Don-vip

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

comment:37 Changed 13 months ago by 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.)
Last edited 13 months ago by Klumbumbus (previous) (diff)

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

comment:39 in reply to:  38 Changed 13 months ago by 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

Changed 11 months ago by GerdP

Attachment: 17533-sample.osm added

sample file with drain and river

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

comment:41 in reply to:  38 Changed 6 months ago by 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*?

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened 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 gaben
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket

Add Comment


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

 
Note: See TracTickets for help on using tickets.