path) {
@@ -248,8 +312,8 @@
}
/**
- * Tries to find a spanning path starting from node startNode.
- *
+ * Tries to find a spanning path starting from node {@code startNode}.
+ *
* Traverses the path in depth-first order.
*
* @param startNode the start node
@@ -259,8 +323,7 @@
if (startNode != null) {
Deque path = new ArrayDeque<>();
Set dupCheck = new HashSet<>();
- Deque nextPairs = new ArrayDeque<>();
- nextPairs.addAll(getOutboundPairs(startNode));
+ Deque nextPairs = new ArrayDeque<>(getOutboundPairs(startNode));
while (!nextPairs.isEmpty()) {
NodePair cur = nextPairs.removeLast();
if (!dupCheck.contains(cur) && !dupCheck.contains(cur.swap())) {
@@ -280,17 +343,17 @@
/**
* Tries to find a path through the graph which visits each edge (i.e.
- * the segment of a way) exactly once.
- * Note that duplicated edges are removed first!
+ * the segment of a way) exactly once.
+ * Note that duplicated edges are removed first!
*
- * @return the path; null, if no path was found
+ * @return the path; {@code null}, if no path was found
*/
public List buildSpanningPath() {
prepare();
- if (numUndirectedEges > 0 && isConnected()) {
- // try to find a path from each "terminal node", i.e. from a
- // node which is connected by exactly one undirected edges (or
- // two directed edges in opposite direction) to the graph. A
+ if (numUndirectedEdges > 0 && isConnected()) {
+ // Try to find a path from each "terminal node", i.e. from a
+ // node which is connected by exactly one undirected edge (or
+ // two directed edges in the opposite direction) to the graph. A
// graph built up from way segments is likely to include such
// nodes, unless the edges build one or more closed rings.
// We order the nodes to start with the best candidates, but
@@ -324,10 +387,10 @@
/**
* Find out if the graph is connected.
- * @return true if it is connected.
+ * @return {@code true} if it is connected
*/
private boolean isConnected() {
- Set nodes = getNodes();
+ Collection nodes = getNodes();
if (nodes.isEmpty())
return false;
Deque toVisit = new ArrayDeque<>();
@@ -350,12 +413,12 @@
/**
* Sort the nodes by number of appearances in the edges.
- * @return set of nodes which can be start nodes in a spanning way.
+ * @return set of nodes which can be start nodes in a spanning way
*/
private Set getMostFrequentVisitedNodesFirst() {
if (edges.isEmpty())
return Collections.emptySet();
- // count appearance of nodes in edges
+ // count the appearance of nodes in edges
Map counters = new HashMap<>();
for (NodePair pair : edges) {
Integer c = counters.get(pair.getA());
Index: src/org/openstreetmap/josm/data/validation/OsmValidator.java
===================================================================
--- src/org/openstreetmap/josm/data/validation/OsmValidator.java (revision 18934)
+++ src/org/openstreetmap/josm/data/validation/OsmValidator.java (working copy)
@@ -44,6 +44,7 @@
import org.openstreetmap.josm.data.validation.tests.ConditionalKeys;
import org.openstreetmap.josm.data.validation.tests.ConnectivityRelations;
import org.openstreetmap.josm.data.validation.tests.CrossingWays;
+import org.openstreetmap.josm.data.validation.tests.CycleDetector;
import org.openstreetmap.josm.data.validation.tests.DirectionNodes;
import org.openstreetmap.josm.data.validation.tests.DuplicateNode;
import org.openstreetmap.josm.data.validation.tests.DuplicateRelation;
@@ -154,8 +155,9 @@
// 3700 .. 3799 is automatically removed since it clashed with pt_assistant.
SharpAngles.class, // 3800 .. 3899
ConnectivityRelations.class, // 3900 .. 3999
- DirectionNodes.class, // 4000-4099
+ DirectionNodes.class, // 4000 .. 4099
RightAngleBuildingTest.class, // 4100 .. 4199
+ CycleDetector.class, // 4200 .. 4299
};
/**
Index: src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java
===================================================================
--- src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java (nonexistent)
+++ src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java (working copy)
@@ -0,0 +1,223 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.data.validation.tests;
+
+import static org.openstreetmap.josm.tools.I18n.tr;
+import static org.openstreetmap.josm.tools.I18n.trc;
+
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Queue;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.NodeGraph;
+import org.openstreetmap.josm.data.osm.OsmPrimitive;
+import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.data.osm.WaySegment;
+import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper;
+import org.openstreetmap.josm.data.validation.Severity;
+import org.openstreetmap.josm.data.validation.Test;
+import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.data.algorithms.Tarjan;
+import org.openstreetmap.josm.gui.progress.ProgressMonitor;
+import org.openstreetmap.josm.spi.preferences.Config;
+import org.openstreetmap.josm.tools.Pair;
+
+/**
+ * Test for detecting cycles in a directed graph,
+ * currently used for waterways only. The processed graph consists of ways labeled as waterway.
+ *
+ * @author gaben
+ * @since xxx
+ */
+public class CycleDetector extends Test {
+ public static final int CYCLE_DETECTED = 4200;
+
+ /** All waterways for cycle detection */
+ private final Set usableWaterways = new HashSet<>();
+
+ /** Already visited primitive unique IDs */
+ private final Set visitedWays = new HashSet<>();
+
+ /** Currently used directional waterways from the OSM wiki */
+ private List directionalWaterways;
+
+ protected static final String PREFIX = ValidatorPrefHelper.PREFIX + "." + CycleDetector.class.getSimpleName();
+
+ public CycleDetector() {
+ super(tr("Cycle detector"), tr("Detects cycles in drainage systems."));
+ }
+
+ @Override
+ public boolean isPrimitiveUsable(OsmPrimitive p) {
+ return p.isUsable() && (p instanceof Way) && (((Way) p).getNodesCount() > 1) && p.hasTag("waterway", directionalWaterways);
+ }
+
+ @Override
+ public void visit(Way w) {
+ if (isPrimitiveUsable(w))
+ usableWaterways.add(w);
+ }
+
+ @Override
+ public void startTest(ProgressMonitor progressMonitor) {
+ super.startTest(progressMonitor);
+ directionalWaterways = Config.getPref().getList(PREFIX + ".directionalWaterways",
+ Arrays.asList("river", "stream", "tidal_channel", "drain", "ditch", "fish_pass", "fairway"));
+ }
+
+ @Override
+ public void endTest() {
+ for (Collection graph : getGraphs()) {
+ NodeGraph nodeGraph = NodeGraph.createDirectedGraphFromWays(graph);
+ Tarjan tarjan = new Tarjan(nodeGraph);
+ Collection> scc = tarjan.getSCC();
+ Map> graphMap = tarjan.getGraphMap();
+
+ for (Collection possibleCycle : scc) {
+ // there is a cycle in the graph if a strongly connected component has more than one node
+ if (possibleCycle.size() > 1) {
+ errors.add(
+ TestError.builder(this, Severity.ERROR, CYCLE_DETECTED)
+ .message(trc("graph theory", "Cycle in directional waterway network"))
+ .primitives(possibleCycle)
+ .highlightWaySegments(createSegments(graphMap, possibleCycle))
+ .build()
+ );
+ }
+ }
+ }
+
+ super.endTest();
+ }
+
+ @Override
+ public void clear() {
+ super.clear();
+ usableWaterways.clear();
+ visitedWays.clear();
+ }
+
+ /**
+ * Creates WaySegments from Nodes for the error highlight function.
+ *
+ * @param graphMap the complete graph data
+ * @param nodes nodes to build the way segments from
+ * @return WaySegments from the Nodes
+ */
+ private static Collection createSegments(Map> graphMap, Collection nodes) {
+ List> pairs = new ArrayList<>();
+
+ // build new graph exclusively from SCC nodes
+ for (Node node : nodes) {
+ for (Node successor : graphMap.get(node)) {
+ // check for outbound nodes
+ if (nodes.contains(successor)) {
+ pairs.add(new Pair<>(node, successor));
+ }
+ }
+ }
+
+ Collection segments = new ArrayList<>();
+
+ for (Pair pair : pairs) {
+ final Node n = pair.a;
+ final Node m = pair.b;
+
+ if (n != null && m != null && !n.equals(m)) {
+ List intersect = new ArrayList<>(n.getParentWays());
+ List mWays = m.getParentWays();
+ intersect.retainAll(mWays);
+
+ for (Way w : intersect) {
+ if (w.getNeighbours(n).contains(m) && getNodeIndex(w, n) + 1 == getNodeIndex(w, m)) {
+ segments.add(WaySegment.forNodePair(w, n, m));
+ }
+ }
+ }
+ }
+
+ return segments;
+ }
+
+ /**
+ * Returns the way index of a node. Only the first occurrence is considered in case it's a closed way.
+ *
+ * @param w parent way
+ * @param n the node to look up
+ * @return {@code >=0} if the node is found or
{@code -1} if node not part of the way
+ */
+ private static int getNodeIndex(Way w, Node n) {
+ for (int i = 0; i < w.getNodesCount(); i++) {
+ if (w.getNode(i).equals(n)) {
+ return i;
+ }
+ }
+
+ return -1;
+ }
+
+ /**
+ * Returns all directional waterways which connect to at least one other usable way.
+ *
+ * @return all directional waterways which connect to at least one other usable way
+ */
+ private Collection> getGraphs() {
+ // HashSet doesn't make a difference here
+ Collection> graphs = new ArrayList<>();
+
+ for (Way waterway : usableWaterways) {
+ if (visitedWays.contains(waterway.getUniqueId())) {
+ continue;
+ }
+ Collection graph = buildGraph(waterway);
+
+ if (!graph.isEmpty()) {
+ graphs.add(graph);
+ }
+ }
+
+ return graphs;
+ }
+
+ /**
+ * Returns a collection of ways, which belongs to the same graph.
+ *
+ * @param way starting way to extend the graph from
+ * @return a collection of ways which belongs to the same graph
+ */
+ private Collection buildGraph(Way way) {
+ final Set graph = new HashSet<>();
+ Queue queue = new ArrayDeque<>();
+ queue.offer(way);
+
+ while (!queue.isEmpty()) {
+ Way currentWay = queue.poll();
+ visitedWays.add(currentWay.getUniqueId());
+
+ for (Node node : currentWay.getNodes()) {
+ Collection referrers = node.referrers(Way.class)
+ .filter(this::isPrimitiveUsable)
+ .filter(candidate -> candidate != currentWay)
+ .collect(Collectors.toList());
+
+ if (!referrers.isEmpty()) {
+ for (Way referrer : referrers) {
+ if (!visitedWays.contains(referrer.getUniqueId())) {
+ queue.offer(referrer);
+ visitedWays.add(referrer.getUniqueId());
+ }
+ }
+ graph.addAll(referrers);
+ }
+ }
+ }
+ return graph;
+ }
+}
Index: test/data/regress/21881/CycleDetector_test_wikipedia.osm
===================================================================
--- test/data/regress/21881/CycleDetector_test_wikipedia.osm (nonexistent)
+++ test/data/regress/21881/CycleDetector_test_wikipedia.osm (working copy)
@@ -0,0 +1,95 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
Index: test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java
===================================================================
--- test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java (nonexistent)
+++ test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java (working copy)
@@ -0,0 +1,29 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.data.validation.tests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import org.junit.jupiter.api.Test;
+import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.io.OsmReader;
+import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
+
+/**
+ * JUnit test for {@link CycleDetector} validation test.
+ */
+@BasicPreferences
+class CycleDetectorTest {
+
+ @Test
+ void testCycleDetection() throws Exception {
+ CycleDetector cycleDetector = new CycleDetector();
+ DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(21881, "CycleDetector_test_wikipedia.osm"), null);
+ cycleDetector.startTest(null);
+ cycleDetector.visit(ds.allPrimitives());
+ cycleDetector.endTest();
+
+ // we have 4 cycles in the test file
+ assertEquals(4, cycleDetector.getErrors().size());
+ }
+}