Index: src/org/openstreetmap/josm/data/validation/tests/LoopDetector.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/LoopDetector.java (nonexistent) +++ src/org/openstreetmap/josm/data/validation/tests/LoopDetector.java (working copy) @@ -0,0 +1,256 @@ +package org.openstreetmap.josm.data.validation.tests; + +import static org.openstreetmap.josm.gui.MainApplication.getLayerManager; +import static org.openstreetmap.josm.tools.I18n.tr; + +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Deque; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import org.openstreetmap.josm.data.osm.IPrimitive; +import org.openstreetmap.josm.data.osm.Node; +import org.openstreetmap.josm.data.osm.NodeGraph; +import org.openstreetmap.josm.data.osm.NodePair; +import org.openstreetmap.josm.data.osm.OsmPrimitive; +import org.openstreetmap.josm.data.osm.Way; +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.gui.progress.ProgressMonitor; +import org.openstreetmap.josm.tools.Logging; + + +/** + * Test for detecting cycles in a directed graph, currently used for waterwoys only. + * The graph consist of OSM dataset ways. + * + * @since xxx + */ +public class LoopDetector extends Test { + public static final int LOOP_DETECTED = 4100; + + /** + * Currently used directional waterways from the OSM wiki + */ + private static final Set directionalWaterways = new HashSet<>( + Arrays.asList("river", "stream", "tidal_channel", "drain", "ditch", "fish_pass", "fairway")); + + private final Set visitedWays = new HashSet<>(); + + public LoopDetector() { + super(tr("Loop detector"), tr("Detects loops in connected directional ways.")); + } + + @Override + public boolean isPrimitiveUsable(OsmPrimitive p) { + return p.isUsable() && (p instanceof Way) && ((Way) p).getNodesCount() > 1 && p.hasTag("waterway", directionalWaterways); + } + + @Override + public void startTest(ProgressMonitor progressMonitor) { + super.startTest(progressMonitor); + setShowElements(true); + // TODO: osm partial selection validator doesn't work + + Collection> graphs = getGraphs(); + + // FIXME debug variables + int i =0, g = 0; + + for (Collection graph : graphs) { + NodeGraph nodeGraph = NodeGraph.createDirectedGraphFromWays(graph); + Tarjan tarjan = new Tarjan(nodeGraph); + Logging.debug("Graph looping... " + ++g); + Collection> scc = tarjan.getSCC(); + for (Collection possibleCycle : scc) { + // contains a cycle (or loop) if the strongly connected components set size is larger than 1 + if (possibleCycle.size() > 1) { + Logging.debug("Cycle detected! " + ++i); + errors.add( + TestError.builder(this, Severity.ERROR, LOOP_DETECTED) + .message(tr("Cycle in directional waterway network")) + .primitives(possibleCycle) + .build() + ); + } + } + progressMonitor.worked(1); + } + } + + @Override + public void clear() { + super.clear(); + visitedWays.clear(); + } + + /** + * Returns all directional waterways which connects to at least one other usable way. + * + * @return all directional waterways which connects to at least one other usable way + */ + private Collection> getGraphs() { + // 70127 waterway + Set usableWaterways = getLayerManager() + .getActiveDataSet() + .getWays() + .stream() + .filter(this::isPrimitiveUsable) + .collect(Collectors.toSet()); + + // HashSet doesn't make a difference here + Collection> graphs = new ArrayList<>(); + + for (Way current : usableWaterways) { + Collection graph = buildGraph(current); + + if (!graph.isEmpty()) + graphs.add(graph); + } + + Logging.debug("All usable waterways " + usableWaterways.size() + ", visited ways " + visitedWays.size() + ", graphs size " + graphs.size()); + usableWaterways.removeAll(visitedWays); + Logging.debug(String.format("Ignored ways (%s): %s", usableWaterways.size(), + usableWaterways.stream().map(IPrimitive::getId).collect(Collectors.toList()))); + + 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) { + if (visitedWays.contains(way)) + return new HashSet<>(); + + final Set graph = new HashSet<>(); + + for (Node node : way.getNodes()) { + final Collection referrers = node.referrers(Way.class).filter(this::isPrimitiveUsable).collect(Collectors.toSet()); + referrers.remove(way); + + if (!referrers.isEmpty()) { + visitedWays.add(way); + for (Way referrer : referrers) { + graph.addAll(buildGraph(referrer)); + } + } + graph.addAll(referrers); + } + return graph; + } + + /** + * Tarjan's algorithm implementation for JOSM. + * + * @see + * Tarjan's strongly connected components algorithm + */ + public static class Tarjan { + + /** + * Helper class for storing algorithm runtime metadata. + */ + private static class TarjanHelper { + private final int index; + private int lowlink; + + public TarjanHelper(int index, int lowlink) { + this.index = index; + this.lowlink = lowlink; + } + } + + /** + * A simple key-value registry to store visited nodes and its metadata + */ + private final Map registry = new HashMap<>(); + + private int index = 0; + private final Collection> scc = new HashSet<>(); + private final Deque stack = new ArrayDeque<>(); + private final NodeGraph graph; + + public Tarjan(NodeGraph graph) { + this.graph = graph; + } + + /** + * Returns the strongly connected components in the current graph. + * + * @return the strongly connected components in the current graph + */ + public Collection> getSCC() { + for (Node node : graph.getNodes()) { + if (!registry.containsKey(node)) { + Logging.debug("Started SCC for n" + node.getId()); + strongConnect(node); + Logging.debug("...done"); + } + } + return scc; + } + + /** + * Calculates strongly connected components available from the given node. + * + * @param v + */ + private void strongConnect(Node v) { + registry.put(v, new TarjanHelper(index, index)); + index++; + stack.push(v); + + // FIXME: performance issue here, possibly because of constant getSuccessors() call + for (Node w : getSuccessors(v)) { + if (!registry.containsKey(w)) { + strongConnect(w); + TarjanHelper vHelper = registry.get(v); + TarjanHelper wHelper = registry.get(w); + vHelper.lowlink = Math.min(vHelper.lowlink, wHelper.lowlink); + } else if (stack.contains(w)) { + TarjanHelper vHelper = registry.get(v); + TarjanHelper wHelper = registry.get(w); + vHelper.lowlink = Math.min(vHelper.lowlink, wHelper.index); + } + } + + final TarjanHelper vHelper = registry.get(v); + if (vHelper.lowlink == vHelper.index) { + Collection currentSCC = new HashSet<>(); + Node w; + do { + w = stack.remove(); + currentSCC.add(w); + } while (!w.equals(v)); + scc.add(currentSCC); + } + } + + /** + * Returns direct successors from the graph of the given node. + * + * @param node a node to start search from + * @return a collection of nodes from the graph which are direct neighbors of the given node + */ + private Collection getSuccessors(Node node) { + final Collection successors = new HashSet<>(); + for (NodePair pair : graph.getEdges()) { + if (pair.getA().equals(node)) { + successors.add(pair.getB()); + } + } + return successors; + } + } +} Index: src/org/openstreetmap/josm/data/validation/OsmValidator.java =================================================================== --- src/org/openstreetmap/josm/data/validation/OsmValidator.java (revision 18437) +++ src/org/openstreetmap/josm/data/validation/OsmValidator.java (working copy) @@ -53,6 +53,7 @@ import org.openstreetmap.josm.data.validation.tests.InternetTags; import org.openstreetmap.josm.data.validation.tests.Lanes; import org.openstreetmap.josm.data.validation.tests.LongSegment; +import org.openstreetmap.josm.data.validation.tests.LoopDetector; import org.openstreetmap.josm.data.validation.tests.MapCSSTagChecker; import org.openstreetmap.josm.data.validation.tests.MultipolygonTest; import org.openstreetmap.josm.data.validation.tests.NameMismatch; @@ -154,6 +155,7 @@ SharpAngles.class, // 3800 .. 3899 ConnectivityRelations.class, // 3900 .. 3999 DirectionNodes.class, // 4000-4099 + LoopDetector.class, // 4100-4199 }; /** @@ -365,7 +367,7 @@ public static JTree buildJTreeList() { DefaultMutableTreeNode root = new DefaultMutableTreeNode(tr("Ignore list")); final Pattern elemId1Pattern = Pattern.compile(":([rwn])_"); - final Pattern elemId2Pattern = Pattern.compile("^[0-9]+$"); + final Pattern elemId2Pattern = Pattern.compile("^\\d+$"); for (Entry e: ignoredErrors.entrySet()) { String key = e.getKey(); // key starts with a code, it maybe followed by a string (eg. a MapCSS rule) and @@ -457,7 +459,7 @@ } if (tr("Ignore list").equals(description)) description = ""; - if (!key.matches("^[0-9]+(_.*|$)")) { + if (!key.matches("^\\d+(_.*|$)")) { description = key; key = ""; } @@ -470,7 +472,7 @@ } else if (item.matches("^([rwn])_.*")) { // single element entry = key + ":" + item; - } else if (item.matches("^[0-9]+(_.*|)$")) { + } else if (item.matches("^\\d+(_.*|)$")) { // no element ids entry = item; } Index: src/org/openstreetmap/josm/data/osm/NodeGraph.java =================================================================== --- src/org/openstreetmap/josm/data/osm/NodeGraph.java (revision 18437) +++ src/org/openstreetmap/josm/data/osm/NodeGraph.java (working copy) @@ -32,12 +32,12 @@ * Builds a list of pair of nodes from the given way. * @param way way * @param directed if {@code true} each pair of nodes will occur once, in the way nodes order. - * if {@code false} each pair of nodes will occur twice (the pair and its inversed copy) + * if {@code false} each pair of nodes will occur twice (the pair and its inverse copy) * @return a list of pair of nodes from the given way */ public static List buildNodePairs(Way way, boolean directed) { List pairs = new ArrayList<>(); - for (Pair pair: way.getNodePairs(false /* don't sort */)) { + for (Pair pair: way.getNodePairs(false)) { pairs.add(new NodePair(pair)); if (!directed) { pairs.add(new NodePair(pair).swap()); @@ -50,7 +50,7 @@ * Builds a list of pair of nodes from the given ways. * @param ways ways * @param directed if {@code true} each pair of nodes will occur once, in the way nodes order. - * if {@code false} each pair of nodes will occur twice (the pair and its inversed copy) + * if {@code false} each pair of nodes will occur twice (the pair and its inverse copy) * @return a list of pair of nodes from the given ways */ public static List buildNodePairs(List ways, boolean directed) { @@ -62,7 +62,7 @@ } /** - * Builds a new list of pair nodes without the duplicated pairs (including inversed copies). + * Builds a new list of pair nodes without the duplicated pairs (including inverse copies). * @param pairs existing list of pairs * @return a new list of pair nodes without the duplicated pairs */ @@ -76,6 +76,11 @@ return cleaned; } + /** + * Create a directed graph from the given node pairs. + * @param pairs Node pairs to build the graph from + * @return node graph structure + */ public static NodeGraph createDirectedGraphFromNodePairs(List pairs) { NodeGraph graph = new NodeGraph(); for (NodePair pair: pairs) { @@ -84,10 +89,15 @@ return graph; } + /** + * Create a directed graph from the given ways. + * @param ways ways to build the graph from + * @return node graph structure + */ public static NodeGraph createDirectedGraphFromWays(Collection ways) { NodeGraph graph = new NodeGraph(); for (Way w: ways) { - graph.add(buildNodePairs(w, true /* directed */)); + graph.add(buildNodePairs(w, true)); } return graph; } @@ -116,7 +126,7 @@ public static NodeGraph createUndirectedGraphFromNodeWays(Collection ways) { NodeGraph graph = new NodeGraph(); for (Way w: ways) { - graph.add(buildNodePairs(w, false /* undirected */)); + graph.add(buildNodePairs(w, false)); } return graph; } @@ -130,16 +140,20 @@ graph.add(buildNodePairs(w, dir)); dir = false; } else { - graph.add(buildNodePairs(w, false /* undirected */)); + graph.add(buildNodePairs(w, false)); } } return graph; } - private final Set edges; - private int numUndirectedEges; + public Set getEdges() { + return edges; + } + private int numUndirectedEdges; + /** counts the number of edges that were added */ private int addedEdges; + private final Set edges; private final Map> successors = new LinkedHashMap<>(); private final Map> predecessors = new LinkedHashMap<>(); @@ -181,7 +195,7 @@ rememberSuccessor(pair); rememberPredecessors(pair); } - numUndirectedEges = undirectedEdges.size(); + numUndirectedEdges = undirectedEdges.size(); } /** @@ -202,7 +216,7 @@ /** * Add a list of node pairs. - * @param pairs list of node pairs + * @param pairs collection of node pairs */ public void add(Collection pairs) { for (NodePair pair: pairs) { @@ -229,7 +243,7 @@ return Optional.ofNullable(successors.get(node)).orElseGet(Collections::emptyList); } - protected Set getNodes() { + public Set getNodes() { Set nodes = new LinkedHashSet<>(2 * edges.size()); for (NodePair pair: edges) { nodes.add(pair.getA()); @@ -239,7 +253,7 @@ } protected boolean isSpanningWay(Collection way) { - return numUndirectedEges == way.size(); + return numUndirectedEdges == way.size(); } protected List buildPathFromNodePairs(Deque path) { @@ -287,7 +301,7 @@ */ public List buildSpanningPath() { prepare(); - if (numUndirectedEges > 0 && isConnected()) { + 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 edges (or // two directed edges in opposite direction) to the graph. A @@ -324,7 +338,7 @@ /** * 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(); @@ -350,7 +364,7 @@ /** * 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())