Modify

Opened 11 months ago

Last modified 3 days ago

#17533 reopened enhancement

[PATCH] Check waterway flow direction

Reported by: gaben Owned by: team
Priority: normal Milestone: 20.03
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 (1)

flow direction.png (9.2 KB) - added by anonymous 11 months ago.

Download all attachments as: .zip

Change History (26)

Changed 11 months ago by anonymous

Attachment: flow direction.png added

comment:1 Changed 11 months ago by Don-vip

Description: modified (diff)

comment:2 Changed 11 months ago by Don-vip

Keywords: waterway direction added

comment:3 Changed 11 months 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 9 months ago by Don-vip (previous) (diff)

comment:4 Changed 9 months ago by anonymous

Can anyone test the rule?

comment:5 Changed 9 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 9 months ago by Klumbumbus

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

comment:7 Changed 5 months ago by gaben

Cc: gaben added

I'm the OP.

How can I test the proposed patch?

comment:8 Changed 5 months ago by Don-vip

Reporter: changed from anonymous to gaben

comment:9 in reply to:  7 Changed 5 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 5 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 5 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 5 months ago by gaben (previous) (diff)

comment:12 Changed 5 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 8 weeks 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 7 weeks 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 7 weeks ago by dmanzer95 (previous) (diff)

comment:15 Changed 6 weeks 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 6 weeks ago by stoecker

Milestone: 20.01

comment:17 in reply to:  15 ; Changed 6 weeks 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 6 weeks 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 6 weeks 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 4 weeks ago by Don-vip

Milestone: 20.0120.02

comment:21 Changed 2 weeks ago by simon04

Resolution: fixed
Status: newclosed

In 15833/josm:

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

comment:22 Changed 2 weeks 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 9 days ago by gaben

Resolution: fixed
Status: closedreopened

Tested in a VM, it's not working.

comment:24 Changed 3 days 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 3 days 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 3 days ago by skyper (previous) (diff)

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.