From a7d4d7ab88368d7c76ffb965bc48cc6a82f237fe Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Sun, 8 Oct 2023 11:02:12 +0200 Subject: [PATCH 01/24] Apply patch without modification from https://josm.openstreetmap.de/attachment/ticket/21881/josm21881_cycle_dector_v2.patch --- .../josm/data/osm/NodeGraph.java | 211 ++++++---- .../josm/data/validation/OsmValidator.java | 4 +- .../data/validation/tests/CycleDetector.java | 364 ++++++++++++++++++ test/data/CycleDetector_test_wikipedia.osm | 95 +++++ .../validation/tests/CycleDetectorTest.java | 38 ++ 5 files changed, 636 insertions(+), 76 deletions(-) create mode 100644 src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java create mode 100644 test/data/CycleDetector_test_wikipedia.osm create mode 100644 test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java diff --git a/src/org/openstreetmap/josm/data/osm/NodeGraph.java b/src/org/openstreetmap/josm/data/osm/NodeGraph.java index 0651c103cc2..d0ebd39a7f0 100644 --- a/src/org/openstreetmap/josm/data/osm/NodeGraph.java +++ b/src/org/openstreetmap/josm/data/osm/NodeGraph.java @@ -24,20 +24,37 @@ /** * A directed or undirected graph of nodes. + * * @since 12463 (extracted from CombineWayAction) */ public class NodeGraph { + private final Set edges; + private final Map> successors = new LinkedHashMap<>(); + private final Map> predecessors = new LinkedHashMap<>(); + private int numUndirectedEdges; + + /** + * The number of edges that were added. + */ + private int addedEdges; + + /** + * Constructs a new {@code NodeGraph}. + */ + public NodeGraph() { + edges = new LinkedHashSet<>(); + } /** * Builds a list of pair of nodes from the given way. - * @param way 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()); @@ -48,27 +65,27 @@ public static List buildNodePairs(Way way, boolean directed) { /** * 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) + * @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 inverse copy) * @return a list of pair of nodes from the given ways */ public static List buildNodePairs(List ways, boolean directed) { List pairs = new ArrayList<>(); - for (Way w: ways) { + for (Way w : ways) { pairs.addAll(buildNodePairs(w, directed)); } return pairs; } /** - * 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 */ public static List eliminateDuplicateNodePairs(List pairs) { List cleaned = new ArrayList<>(); - for (NodePair p: pairs) { + for (NodePair p : pairs) { if (!cleaned.contains(p) && !cleaned.contains(p.swap())) { cleaned.add(p); } @@ -76,18 +93,28 @@ public static List eliminateDuplicateNodePairs(List pairs) { 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) { + for (NodePair pair : pairs) { graph.add(pair); } 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 */)); + for (Way w : ways) { + graph.add(buildNodePairs(w, true)); } return graph; } @@ -99,7 +126,7 @@ public static NodeGraph createDirectedGraphFromWays(Collection ways) { */ public static NodeGraph createUndirectedGraphFromNodeList(List pairs) { NodeGraph graph = new NodeGraph(); - for (NodePair pair: pairs) { + for (NodePair pair : pairs) { graph.add(pair); graph.add(pair.swap()); } @@ -108,41 +135,99 @@ public static NodeGraph createUndirectedGraphFromNodeList(List pairs) /** * Create an undirected graph from the given ways, but prevent reversing of all - * non-new ways by fix one direction. + * non-new ways by fixing one direction. * @param ways Ways to build the graph from * @return node graph structure * @since 8181 */ public static NodeGraph createUndirectedGraphFromNodeWays(Collection ways) { NodeGraph graph = new NodeGraph(); - for (Way w: ways) { - graph.add(buildNodePairs(w, false /* undirected */)); + for (Way w : ways) { + graph.add(buildNodePairs(w, false)); } return graph; } + /** + * Create a nearly undirected graph from the given ways, but prevent reversing of all + * non-new ways by fixing one direction. + * The first new way gives the direction of the graph. + * @param ways Ways to build the graph from + * @return node graph structure + */ public static NodeGraph createNearlyUndirectedGraphFromNodeWays(Collection ways) { boolean dir = true; NodeGraph graph = new NodeGraph(); - for (Way w: ways) { + for (Way w : ways) { if (!w.isNew()) { /* let the first non-new way give the direction (see #5880) */ 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; - /** counts the number of edges that were added */ - private int addedEdges; - private final Map> successors = new LinkedHashMap<>(); - private final Map> predecessors = new LinkedHashMap<>(); + /** + * Add a node pair. + * @param pair node pair + */ + public void add(NodePair pair) { + addedEdges++; + edges.add(pair); + } + + /** + * Add a list of node pairs. + * @param pairs collection of node pairs + */ + public void add(Collection pairs) { + for (NodePair pair : pairs) { + add(pair); + } + } + /** + * Return the edges containing the node pairs of the graph. + * @return the edges containing the node pairs of the graph + */ + public Set getEdges() { + return edges; + } + + /** + * Return the graph's nodes. + * @return the graph's nodes + */ + public Set getNodes() { + Set nodes = new LinkedHashSet<>(2 * edges.size()); + for (NodePair pair : edges) { + nodes.add(pair.getA()); + nodes.add(pair.getB()); + } + return nodes; + } + + /** + * Creates a lookup table from the existing edges to make querying possible. + * @return a map containing all the graph data, where the key is queryable, values are direct successors of the key node + * @since xxx + */ + public Map> createMap() { + final Map> result = new HashMap<>((int) (edges.size() / 0.75) + 1); + + for (NodePair edge : edges) { + result.computeIfAbsent(edge.getA(), k -> new ArrayList<>()).add(edge.getB()); + } + + return result; + } + + /** + * See {@link #prepare()} + */ protected void rememberSuccessor(NodePair pair) { List l = successors.computeIfAbsent(pair.getA(), k -> new ArrayList<>()); if (!l.contains(pair)) { @@ -150,6 +235,9 @@ protected void rememberSuccessor(NodePair pair) { } } + /** + * See {@link #prepare()} + */ protected void rememberPredecessors(NodePair pair) { List l = predecessors.computeIfAbsent(pair.getB(), k -> new ArrayList<>()); if (!l.contains(pair)) { @@ -157,6 +245,12 @@ protected void rememberPredecessors(NodePair pair) { } } + /** + * Replies true if {@code n} is a terminal node of the graph. Internal variables should be initialized first. + * @param n Node to check + * @return {@code true} if it is a terminal node + * @see #prepare() + */ protected boolean isTerminalNode(Node n) { if (successors.get(n) == null) return false; if (successors.get(n).size() != 1) return false; @@ -174,42 +268,20 @@ protected void prepare() { successors.clear(); predecessors.clear(); - for (NodePair pair: edges) { + for (NodePair pair : edges) { if (!undirectedEdges.contains(pair) && !undirectedEdges.contains(pair.swap())) { undirectedEdges.add(pair); } rememberSuccessor(pair); rememberPredecessors(pair); } - numUndirectedEges = undirectedEdges.size(); - } - - /** - * Constructs a new {@code NodeGraph}. - */ - public NodeGraph() { - edges = new LinkedHashSet<>(); + numUndirectedEdges = undirectedEdges.size(); } /** - * Add a node pair. - * @param pair node pair + * Return the terminal nodes of the graph. + * @return the terminal nodes of the graph */ - public void add(NodePair pair) { - addedEdges++; - edges.add(pair); - } - - /** - * Add a list of node pairs. - * @param pairs list of node pairs - */ - public void add(Collection pairs) { - for (NodePair pair: pairs) { - add(pair); - } - } - protected Set getTerminalNodes() { return getNodes().stream().filter(this::isTerminalNode).collect(Collectors.toCollection(LinkedHashSet::new)); } @@ -229,17 +301,8 @@ protected List getOutboundPairs(Node node) { return Optional.ofNullable(successors.get(node)).orElseGet(Collections::emptyList); } - protected Set getNodes() { - Set nodes = new LinkedHashSet<>(2 * edges.size()); - for (NodePair pair: edges) { - nodes.add(pair.getA()); - nodes.add(pair.getB()); - } - return nodes; - } - protected boolean isSpanningWay(Collection way) { - return numUndirectedEges == way.size(); + return numUndirectedEdges == way.size(); } protected List buildPathFromNodePairs(Deque path) { @@ -248,10 +311,9 @@ protected List buildPathFromNodePairs(Deque path) { } /** - * 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 * @return the spanning path; empty list if no path is found */ @@ -259,8 +321,7 @@ protected List buildSpanningPath(Node startNode) { 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 +341,17 @@ protected List buildSpanningPath(Node startNode) { /** * 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,7 +385,7 @@ public List buildSpanningPathNoRemove() { /** * 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,12 +411,12 @@ private boolean isConnected() { /** * 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()); diff --git a/src/org/openstreetmap/josm/data/validation/OsmValidator.java b/src/org/openstreetmap/josm/data/validation/OsmValidator.java index 220f8ccaf40..81dbd623949 100644 --- a/src/org/openstreetmap/josm/data/validation/OsmValidator.java +++ b/src/org/openstreetmap/josm/data/validation/OsmValidator.java @@ -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 @@ private OsmValidator() { // 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 }; /** diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java new file mode 100644 index 00000000000..aaca9df2983 --- /dev/null +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -0,0 +1,364 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.data.validation.tests; + +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.Collections; +import java.util.Deque; +import java.util.HashMap; +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.NodePair; +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.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 primitives */ + private final Set visitedWays = new HashSet<>(); + + /** Currently used directional waterways from the OSM wiki */ + private static 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(tr("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 NodePair(node, successor)); + } + } + } + + Collection segments = new ArrayList<>(); + + for (NodePair pair : pairs) { + final Node n = pair.getA(); + final Node m = pair.getB(); + + 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 >=0 if the node is found or
-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) { + 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) { + if (visitedWays.contains(way)) + return Collections.emptySet(); + + final Set graph = new HashSet<>(); + Queue queue = new ArrayDeque<>(); + queue.offer(way); + + while (!queue.isEmpty()) { + Way currentWay = queue.poll(); + visitedWays.add(currentWay); + + for (Node node : currentWay.getNodes()) { + Collection referrers = node.referrers(Way.class) + .filter(this::isPrimitiveUsable) + .filter(candidate -> candidate != currentWay) + .collect(Collectors.toSet()); + + if (!referrers.isEmpty()) { + for (Way referrer : referrers) { + if (!visitedWays.contains(referrer)) { + queue.offer(referrer); + visitedWays.add(referrer); + } + } + graph.addAll(referrers); + } + } + } + return graph; + } + + + + /** + * Tarjan's strongly connected components algorithm for JOSM. + * + * @see + * Tarjan's strongly connected components algorithm + */ + public static final class Tarjan { + + /** + * Used to remember visited nodes and its metadata. Key is used for storing + * the unique ID of the nodes instead of the full data to save space. + */ + private final Map registry; + + /** Used to store the graph data as a map. */ + private final Map> graphMap; + + /** Used to store strongly connected components. */ + private final Collection> scc = new HashSet<>(); + + /** Used on algorithm runtime to keep track discovery progress. */ + private final Deque stack = new ArrayDeque<>(); + + /** Used on algorithm runtime to keep track discovery progress. */ + private int index = 0; + + public Tarjan(NodeGraph graph) { + graphMap = graph.createMap(); + + this.registry = new HashMap<>((int) (graph.getEdges().size() / 0.75) + 1); + } + + /** + * Returns the strongly connected components in the current graph. + * + * @return the strongly connected components in the current graph + */ + public Collection> getSCC() { + for (Node node : graphMap.keySet()) { + if (!registry.containsKey(node.getUniqueId())) { + strongConnect(node); + } + } + return scc; + } + + /** + * Returns the graph data as a map. + * @see NodeGraph#createMap() + * @return the graph data as a map + */ + public Map> getGraphMap() { + return graphMap; + } + + /** + * Calculates strongly connected components available from the given node, in an iterative fashion. + * + * @param u0 the node to generate strongly connected components from + */ + private void strongConnect(final Node u0) { + final Deque> work = new ArrayDeque<>(); + work.push(new Pair<>(u0, 0)); + boolean recurse; + + while (!work.isEmpty()) { + Pair popped = work.remove(); + Node u = popped.a; + int j = popped.b; + + if (j == 0) { + index++; + registry.put(u.getUniqueId(), new TarjanHelper(index)); + stack.push(u); + } + + recurse = false; + List successors = getSuccessors(u); + + for (int i = j; i < successors.size(); i++) { + Node v = successors.get(i); + if (!registry.containsKey(v.getUniqueId())) { + work.push(new Pair<>(u, i + 1)); + work.push(new Pair<>(v, 0)); + recurse = true; + break; + } else if (stack.contains(v)) { + TarjanHelper uHelper = registry.get(u.getUniqueId()); + TarjanHelper vHelper = registry.get(v.getUniqueId()); + uHelper.lowlink = Math.min(uHelper.lowlink, vHelper.index); + } + } + + if (!recurse) { + TarjanHelper uHelper = registry.get(u.getUniqueId()); + if (uHelper.lowlink == uHelper.index) { + List currentSCC = new ArrayList<>(); + Node v; + do { + v = stack.remove(); + currentSCC.add(v); + } while (!v.equals(u)); + scc.add(currentSCC); + } + if (!work.isEmpty()) { + Node v = u; + Pair peeked = work.peek(); + u = peeked.a; + TarjanHelper vHelper = registry.get(v.getUniqueId()); + uHelper = registry.get(u.getUniqueId()); + uHelper.lowlink = Math.min(uHelper.lowlink, vHelper.lowlink); + } + } + } + } + + /** + * Returns the next direct successors from the graph of the given node. + * + * @param node a node to start search from + * @return direct successors of the node or an empty list, if it's a terminal node + */ + private List getSuccessors(Node node) { + return graphMap.getOrDefault(node, Collections.emptyList()); + } + + /** + * Helper class for storing the Tarjan algorithm runtime metadata. + */ + private static class TarjanHelper { + private final int index; + private int lowlink; + + private TarjanHelper(int index) { + this.index = index; + this.lowlink = index; + } + } + } +} diff --git a/test/data/CycleDetector_test_wikipedia.osm b/test/data/CycleDetector_test_wikipedia.osm new file mode 100644 index 00000000000..1759548de4b --- /dev/null +++ b/test/data/CycleDetector_test_wikipedia.osm @@ -0,0 +1,95 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java b/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java new file mode 100644 index 00000000000..91334cf0ad6 --- /dev/null +++ b/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java @@ -0,0 +1,38 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.data.validation.tests; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.nio.file.Files; +import java.nio.file.Paths; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.openstreetmap.josm.TestUtils; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.io.OsmReader; +import org.openstreetmap.josm.testutils.JOSMTestRules; + +/** + * JUnit test for {@link CycleDetector} validation test. + */ +class CycleDetectorTest { + /** + * Setup test. + */ + @RegisterExtension + public JOSMTestRules test = new JOSMTestRules(); + + @Test() + void testCycleDetection() throws Exception { + CycleDetector cycleDetector = new CycleDetector(); + DataSet ds = OsmReader.parseDataSet( + Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "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()); + } +} \ No newline at end of file From d9432912fa0f2553721e1eb9c00fcee093a3d532 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Sun, 8 Oct 2023 12:13:45 +0200 Subject: [PATCH 02/24] Utilize the newer BasicPreferences class annotation --- .../josm/data/validation/tests/CycleDetectorTest.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java b/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java index 91334cf0ad6..7d33d8f4989 100644 --- a/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java +++ b/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java @@ -7,21 +7,16 @@ import java.nio.file.Paths; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.RegisterExtension; import org.openstreetmap.josm.TestUtils; import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.io.OsmReader; -import org.openstreetmap.josm.testutils.JOSMTestRules; +import org.openstreetmap.josm.testutils.annotations.BasicPreferences; /** * JUnit test for {@link CycleDetector} validation test. */ +@BasicPreferences class CycleDetectorTest { - /** - * Setup test. - */ - @RegisterExtension - public JOSMTestRules test = new JOSMTestRules(); @Test() void testCycleDetection() throws Exception { From a20cf9c2a6155648831cb1ba25003c88eb4f5b7d Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Sun, 8 Oct 2023 12:14:06 +0200 Subject: [PATCH 03/24] Format OsmValidator ID code comments --- .../josm/data/validation/OsmValidator.java | 88 +++++++++---------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/src/org/openstreetmap/josm/data/validation/OsmValidator.java b/src/org/openstreetmap/josm/data/validation/OsmValidator.java index 81dbd623949..68b445ca5ca 100644 --- a/src/org/openstreetmap/josm/data/validation/OsmValidator.java +++ b/src/org/openstreetmap/josm/data/validation/OsmValidator.java @@ -114,50 +114,50 @@ private OsmValidator() { private static final Class[] CORE_TEST_CLASSES = new Class[] {// NOPMD /* FIXME - unique error numbers for tests aren't properly unique - ignoring will not work as expected */ /* Error codes are class.getName().hashCode() + "_" + oldCode. There should almost never be a collision. */ - DuplicateNode.class, // ID 1 .. 99 - OverlappingWays.class, // ID 101 .. 199 - UntaggedNode.class, // ID 201 .. 299 - UntaggedWay.class, // ID 301 .. 399 - SelfIntersectingWay.class, // ID 401 .. 499 - DuplicatedWayNodes.class, // ID 501 .. 599 - CrossingWays.Ways.class, // ID 601 .. 699 - CrossingWays.Boundaries.class, // ID 601 .. 699 - CrossingWays.SelfCrossing.class, // ID 601 .. 699 - SimilarNamedWays.class, // ID 701 .. 799 - Coastlines.class, // ID 901 .. 999 - WronglyOrderedWays.class, // ID 1001 .. 1099 - UnclosedWays.class, // ID 1101 .. 1199 - TagChecker.class, // ID 1201 .. 1299 - UnconnectedWays.UnconnectedHighways.class, // ID 1301 .. 1399 - UnconnectedWays.UnconnectedRailways.class, // ID 1301 .. 1399 - UnconnectedWays.UnconnectedWaterways.class, // ID 1301 .. 1399 - UnconnectedWays.UnconnectedNaturalOrLanduse.class, // ID 1301 .. 1399 - UnconnectedWays.UnconnectedPower.class, // ID 1301 .. 1399 - DuplicateWay.class, // ID 1401 .. 1499 - NameMismatch.class, // ID 1501 .. 1599 - MultipolygonTest.class, // ID 1601 .. 1699 - RelationChecker.class, // ID 1701 .. 1799 - TurnrestrictionTest.class, // ID 1801 .. 1899 - DuplicateRelation.class, // ID 1901 .. 1999 - WayConnectedToArea.class, // ID 2301 .. 2399 - PowerLines.class, // ID 2501 .. 2599 - Addresses.class, // ID 2601 .. 2699 - Highways.class, // ID 2701 .. 2799 - BarriersEntrances.class, // ID 2801 .. 2899 - OpeningHourTest.class, // 2901 .. 2999 - MapCSSTagChecker.class, // 3000 .. 3099 - Lanes.class, // 3100 .. 3199 - ConditionalKeys.class, // 3200 .. 3299 - InternetTags.class, // 3300 .. 3399 - ApiCapabilitiesTest.class, // 3400 .. 3499 - LongSegment.class, // 3500 .. 3599 - PublicTransportRouteTest.class, // 3600 .. 3699 - // 3700 .. 3799 is automatically removed since it clashed with pt_assistant. - SharpAngles.class, // 3800 .. 3899 - ConnectivityRelations.class, // 3900 .. 3999 - DirectionNodes.class, // 4000 .. 4099 - RightAngleBuildingTest.class, // 4100 .. 4199 - CycleDetector.class, // 4200 .. 4299 + DuplicateNode.class, // ID 1 .. 99 + OverlappingWays.class, // ID 101 .. 199 + UntaggedNode.class, // ID 201 .. 299 + UntaggedWay.class, // ID 301 .. 399 + SelfIntersectingWay.class, // ID 401 .. 499 + DuplicatedWayNodes.class, // ID 501 .. 599 + CrossingWays.Ways.class, // ID 601 .. 699 + CrossingWays.Boundaries.class, // ID 601 .. 699 + CrossingWays.SelfCrossing.class, // ID 601 .. 699 + SimilarNamedWays.class, // ID 701 .. 799 + Coastlines.class, // ID 901 .. 999 + WronglyOrderedWays.class, // ID 1001 .. 1099 + UnclosedWays.class, // ID 1101 .. 1199 + TagChecker.class, // ID 1201 .. 1299 + UnconnectedWays.UnconnectedHighways.class, // ID 1301 .. 1399 + UnconnectedWays.UnconnectedRailways.class, // ID 1301 .. 1399 + UnconnectedWays.UnconnectedWaterways.class, // ID 1301 .. 1399 + UnconnectedWays.UnconnectedNaturalOrLanduse.class, // ID 1301 .. 1399 + UnconnectedWays.UnconnectedPower.class, // ID 1301 .. 1399 + DuplicateWay.class, // ID 1401 .. 1499 + NameMismatch.class, // ID 1501 .. 1599 + MultipolygonTest.class, // ID 1601 .. 1699 + RelationChecker.class, // ID 1701 .. 1799 + TurnrestrictionTest.class, // ID 1801 .. 1899 + DuplicateRelation.class, // ID 1901 .. 1999 + WayConnectedToArea.class, // ID 2301 .. 2399 + PowerLines.class, // ID 2501 .. 2599 + Addresses.class, // ID 2601 .. 2699 + Highways.class, // ID 2701 .. 2799 + BarriersEntrances.class, // ID 2801 .. 2899 + OpeningHourTest.class, // ID 2901 .. 2999 + MapCSSTagChecker.class, // ID 3000 .. 3099 + Lanes.class, // ID 3100 .. 3199 + ConditionalKeys.class, // ID 3200 .. 3299 + InternetTags.class, // ID 3300 .. 3399 + ApiCapabilitiesTest.class, // ID 3400 .. 3499 + LongSegment.class, // ID 3500 .. 3599 + PublicTransportRouteTest.class, // ID 3600 .. 3699 + // ID 3700 .. 3799 is automatically removed since it clashed with pt_assistant. + SharpAngles.class, // ID 3800 .. 3899 + ConnectivityRelations.class, // ID 3900 .. 3999 + DirectionNodes.class, // ID 4000 .. 4099 + RightAngleBuildingTest.class, // ID 4100 .. 4199 + CycleDetector.class, // ID 4200 .. 4299 }; /** From defb5ff04b8aebb7168b7d627600bd52d932c295 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Sun, 8 Oct 2023 12:49:32 +0200 Subject: [PATCH 04/24] Apply suggestions from https://josm.openstreetmap.de/ticket/21881#comment:27 --- .../josm/data/validation/tests/CycleDetector.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index aaca9df2983..c43ed8e6ef3 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -48,7 +48,7 @@ public class CycleDetector extends Test { private final Set visitedWays = new HashSet<>(); /** Currently used directional waterways from the OSM wiki */ - private static List directionalWaterways; + private List directionalWaterways; protected static final String PREFIX = ValidatorPrefHelper.PREFIX + "." + CycleDetector.class.getSimpleName(); @@ -151,7 +151,7 @@ private static Collection createSegments(Map> graph * 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 >=0 if the node is found or
-1 if node not part of the way + * @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++) { From 85735a7554cf91cc001b8ae63bc2cbcb59eb0582 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Sun, 8 Oct 2023 13:03:30 +0200 Subject: [PATCH 05/24] Improve documentation --- src/org/openstreetmap/josm/data/osm/NodeGraph.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/org/openstreetmap/josm/data/osm/NodeGraph.java b/src/org/openstreetmap/josm/data/osm/NodeGraph.java index d0ebd39a7f0..a3ed5fc6cf0 100644 --- a/src/org/openstreetmap/josm/data/osm/NodeGraph.java +++ b/src/org/openstreetmap/josm/data/osm/NodeGraph.java @@ -23,7 +23,7 @@ import org.openstreetmap.josm.tools.Pair; /** - * A directed or undirected graph of nodes. + * A directed or undirected graph of nodes. Nodes are connected via edges represented by NodePair instances. * * @since 12463 (extracted from CombineWayAction) */ @@ -211,8 +211,10 @@ public Set getNodes() { } /** - * Creates a lookup table from the existing edges to make querying possible. - * @return a map containing all the graph data, where the key is queryable, values are direct successors of the key node + * Constructs a lookup table from the existing edges in the graph to enable efficient querying. + * This method creates a map where each node is associated with a list of nodes that are directly connected to it. + * + * @return A map representing the graph structure, where nodes are keys, and values are their direct successors. * @since xxx */ public Map> createMap() { From 90f2e1059bc6d196111d6ebf14a1f4f1aa99e7a9 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Wed, 25 Oct 2023 23:17:24 +0200 Subject: [PATCH 06/24] Move Tarjan's algorithm to a new package --- .../josm/data/validation/algos/Tarjan.java | 160 ++++++++++++++++++ .../data/validation/algos/package-info.java | 6 + .../data/validation/tests/CycleDetector.java | 146 +--------------- 3 files changed, 167 insertions(+), 145 deletions(-) create mode 100644 src/org/openstreetmap/josm/data/validation/algos/Tarjan.java create mode 100644 src/org/openstreetmap/josm/data/validation/algos/package-info.java diff --git a/src/org/openstreetmap/josm/data/validation/algos/Tarjan.java b/src/org/openstreetmap/josm/data/validation/algos/Tarjan.java new file mode 100644 index 00000000000..2758efd9477 --- /dev/null +++ b/src/org/openstreetmap/josm/data/validation/algos/Tarjan.java @@ -0,0 +1,160 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.data.validation.algos; + +import org.openstreetmap.josm.data.osm.Node; +import org.openstreetmap.josm.data.osm.NodeGraph; +import org.openstreetmap.josm.tools.Pair; +import org.openstreetmap.josm.tools.Utils; + +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.Deque; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + + +/** + * Tarjan's strongly connected components algorithm for JOSM. + * + * @see + * Tarjan's strongly connected components algorithm + * @author gaben + * @since xxx + */ +public final class Tarjan { + + /** + * Used to remember visited nodes and its metadata. Key is used for storing + * the unique ID of the nodes instead of the full data to save space. + */ + private final Map registry; + + /** Used to store the graph data as a map. */ + private final Map> graphMap; + + /** Used to store strongly connected components. */ + private final Collection> scc = new HashSet<>(); + + /** Used on algorithm runtime to keep track discovery progress. */ + private final Deque stack = new ArrayDeque<>(); + + /** Used on algorithm runtime to keep track discovery progress. */ + private int index = 0; + + public Tarjan(NodeGraph graph) { + graphMap = graph.createMap(); + + this.registry = new HashMap<>(Utils.hashMapInitialCapacity(graph.getEdges().size())); + } + + /** + * Returns the strongly connected components in the current graph. + * + * @return the strongly connected components in the current graph + */ + public Collection> getSCC() { + for (Node node : graphMap.keySet()) { + if (!registry.containsKey(node.getUniqueId())) { + strongConnect(node); + } + } + return scc; + } + + /** + * Returns the graph data as a map. + * @see NodeGraph#createMap() + * @return the graph data as a map + */ + public Map> getGraphMap() { + return graphMap; + } + + /** + * Calculates strongly connected components available from the given node, in an iterative fashion. + * + * @param u0 the node to generate strongly connected components from + */ + private void strongConnect(final Node u0) { + final Deque> work = new ArrayDeque<>(); + work.push(new Pair<>(u0, 0)); + boolean recurse; + + while (!work.isEmpty()) { + Pair popped = work.remove(); + Node u = popped.a; + int j = popped.b; + + if (j == 0) { + index++; + registry.put(u.getUniqueId(), new TarjanHelper(index)); + stack.push(u); + } + + recurse = false; + List successors = getSuccessors(u); + + for (int i = j; i < successors.size(); i++) { + Node v = successors.get(i); + if (!registry.containsKey(v.getUniqueId())) { + work.push(new Pair<>(u, i + 1)); + work.push(new Pair<>(v, 0)); + recurse = true; + break; + } else if (stack.contains(v)) { + TarjanHelper uHelper = registry.get(u.getUniqueId()); + TarjanHelper vHelper = registry.get(v.getUniqueId()); + uHelper.lowlink = Math.min(uHelper.lowlink, vHelper.index); + } + } + + if (!recurse) { + TarjanHelper uHelper = registry.get(u.getUniqueId()); + if (uHelper.lowlink == uHelper.index) { + List currentSCC = new ArrayList<>(); + Node v; + do { + v = stack.remove(); + currentSCC.add(v); + } while (!v.equals(u)); + scc.add(currentSCC); + } + if (!work.isEmpty()) { + Node v = u; + Pair peeked = work.peek(); + u = peeked.a; + TarjanHelper vHelper = registry.get(v.getUniqueId()); + uHelper = registry.get(u.getUniqueId()); + uHelper.lowlink = Math.min(uHelper.lowlink, vHelper.lowlink); + } + } + } + } + + /** + * Returns the next direct successors from the graph of the given node. + * + * @param node a node to start search from + * @return direct successors of the node or an empty list, if it's a terminal node + */ + private List getSuccessors(Node node) { + return graphMap.getOrDefault(node, Collections.emptyList()); + } + + /** + * Helper class for storing the Tarjan algorithm runtime metadata. + */ + private static class TarjanHelper { + private final int index; + private int lowlink; + + private TarjanHelper(int index) { + this.index = index; + this.lowlink = index; + } + } +} diff --git a/src/org/openstreetmap/josm/data/validation/algos/package-info.java b/src/org/openstreetmap/josm/data/validation/algos/package-info.java new file mode 100644 index 00000000000..826eb072849 --- /dev/null +++ b/src/org/openstreetmap/josm/data/validation/algos/package-info.java @@ -0,0 +1,6 @@ +// License: GPL. For details, see LICENSE file. + +/** + * General purpose algorithm classes for JOSM data validation. + */ +package org.openstreetmap.josm.data.validation.algos; \ No newline at end of file diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index c43ed8e6ef3..482289e242e 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -8,8 +8,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.Deque; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -27,9 +25,9 @@ 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.validation.algos.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, @@ -219,146 +217,4 @@ private Collection buildGraph(Way way) { } return graph; } - - - - /** - * Tarjan's strongly connected components algorithm for JOSM. - * - * @see - * Tarjan's strongly connected components algorithm - */ - public static final class Tarjan { - - /** - * Used to remember visited nodes and its metadata. Key is used for storing - * the unique ID of the nodes instead of the full data to save space. - */ - private final Map registry; - - /** Used to store the graph data as a map. */ - private final Map> graphMap; - - /** Used to store strongly connected components. */ - private final Collection> scc = new HashSet<>(); - - /** Used on algorithm runtime to keep track discovery progress. */ - private final Deque stack = new ArrayDeque<>(); - - /** Used on algorithm runtime to keep track discovery progress. */ - private int index = 0; - - public Tarjan(NodeGraph graph) { - graphMap = graph.createMap(); - - this.registry = new HashMap<>((int) (graph.getEdges().size() / 0.75) + 1); - } - - /** - * Returns the strongly connected components in the current graph. - * - * @return the strongly connected components in the current graph - */ - public Collection> getSCC() { - for (Node node : graphMap.keySet()) { - if (!registry.containsKey(node.getUniqueId())) { - strongConnect(node); - } - } - return scc; - } - - /** - * Returns the graph data as a map. - * @see NodeGraph#createMap() - * @return the graph data as a map - */ - public Map> getGraphMap() { - return graphMap; - } - - /** - * Calculates strongly connected components available from the given node, in an iterative fashion. - * - * @param u0 the node to generate strongly connected components from - */ - private void strongConnect(final Node u0) { - final Deque> work = new ArrayDeque<>(); - work.push(new Pair<>(u0, 0)); - boolean recurse; - - while (!work.isEmpty()) { - Pair popped = work.remove(); - Node u = popped.a; - int j = popped.b; - - if (j == 0) { - index++; - registry.put(u.getUniqueId(), new TarjanHelper(index)); - stack.push(u); - } - - recurse = false; - List successors = getSuccessors(u); - - for (int i = j; i < successors.size(); i++) { - Node v = successors.get(i); - if (!registry.containsKey(v.getUniqueId())) { - work.push(new Pair<>(u, i + 1)); - work.push(new Pair<>(v, 0)); - recurse = true; - break; - } else if (stack.contains(v)) { - TarjanHelper uHelper = registry.get(u.getUniqueId()); - TarjanHelper vHelper = registry.get(v.getUniqueId()); - uHelper.lowlink = Math.min(uHelper.lowlink, vHelper.index); - } - } - - if (!recurse) { - TarjanHelper uHelper = registry.get(u.getUniqueId()); - if (uHelper.lowlink == uHelper.index) { - List currentSCC = new ArrayList<>(); - Node v; - do { - v = stack.remove(); - currentSCC.add(v); - } while (!v.equals(u)); - scc.add(currentSCC); - } - if (!work.isEmpty()) { - Node v = u; - Pair peeked = work.peek(); - u = peeked.a; - TarjanHelper vHelper = registry.get(v.getUniqueId()); - uHelper = registry.get(u.getUniqueId()); - uHelper.lowlink = Math.min(uHelper.lowlink, vHelper.lowlink); - } - } - } - } - - /** - * Returns the next direct successors from the graph of the given node. - * - * @param node a node to start search from - * @return direct successors of the node or an empty list, if it's a terminal node - */ - private List getSuccessors(Node node) { - return graphMap.getOrDefault(node, Collections.emptyList()); - } - - /** - * Helper class for storing the Tarjan algorithm runtime metadata. - */ - private static class TarjanHelper { - private final int index; - private int lowlink; - - private TarjanHelper(int index) { - this.index = index; - this.lowlink = index; - } - } - } } From 92500bd4d0003a9c70a42e08ff9c90df4ae092f6 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Wed, 25 Oct 2023 23:17:44 +0200 Subject: [PATCH 07/24] Use utils function to precompute HashSet size --- src/org/openstreetmap/josm/data/osm/NodeGraph.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/org/openstreetmap/josm/data/osm/NodeGraph.java b/src/org/openstreetmap/josm/data/osm/NodeGraph.java index a3ed5fc6cf0..049fffe1d99 100644 --- a/src/org/openstreetmap/josm/data/osm/NodeGraph.java +++ b/src/org/openstreetmap/josm/data/osm/NodeGraph.java @@ -21,6 +21,7 @@ import java.util.stream.Stream; import org.openstreetmap.josm.tools.Pair; +import org.openstreetmap.josm.tools.Utils; /** * A directed or undirected graph of nodes. Nodes are connected via edges represented by NodePair instances. @@ -218,7 +219,7 @@ public Set getNodes() { * @since xxx */ public Map> createMap() { - final Map> result = new HashMap<>((int) (edges.size() / 0.75) + 1); + final Map> result = new HashMap<>(Utils.hashMapInitialCapacity(edges.size())); for (NodePair edge : edges) { result.computeIfAbsent(edge.getA(), k -> new ArrayList<>()).add(edge.getB()); From 44682aad7c769294606185967ad66bb67fa955d9 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Mon, 18 Dec 2023 17:44:56 +0100 Subject: [PATCH 08/24] Adjust code as requested by Taylor --- src/org/openstreetmap/josm/data/osm/NodeGraph.java | 10 +++++----- .../josm/data/validation/algos/Tarjan.java | 7 +++---- .../josm/data/validation/algos/package-info.java | 2 +- .../21881}/CycleDetector_test_wikipedia.osm | 0 .../josm/data/validation/tests/CycleDetectorTest.java | 7 +++---- 5 files changed, 12 insertions(+), 14 deletions(-) rename test/data/{ => regress/21881}/CycleDetector_test_wikipedia.osm (100%) diff --git a/src/org/openstreetmap/josm/data/osm/NodeGraph.java b/src/org/openstreetmap/josm/data/osm/NodeGraph.java index 049fffe1d99..d054a2379fd 100644 --- a/src/org/openstreetmap/josm/data/osm/NodeGraph.java +++ b/src/org/openstreetmap/josm/data/osm/NodeGraph.java @@ -184,7 +184,7 @@ public void add(NodePair pair) { * Add a list of node pairs. * @param pairs collection of node pairs */ - public void add(Collection pairs) { + public void add(Iterable pairs) { for (NodePair pair : pairs) { add(pair); } @@ -194,15 +194,15 @@ public void add(Collection pairs) { * Return the edges containing the node pairs of the graph. * @return the edges containing the node pairs of the graph */ - public Set getEdges() { - return edges; + public Collection getEdges() { + return Collections.unmodifiableSet(edges); } /** * Return the graph's nodes. * @return the graph's nodes */ - public Set getNodes() { + public Collection getNodes() { Set nodes = new LinkedHashSet<>(2 * edges.size()); for (NodePair pair : edges) { nodes.add(pair.getA()); @@ -391,7 +391,7 @@ public List buildSpanningPathNoRemove() { * @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<>(); diff --git a/src/org/openstreetmap/josm/data/validation/algos/Tarjan.java b/src/org/openstreetmap/josm/data/validation/algos/Tarjan.java index 2758efd9477..64709d5c435 100644 --- a/src/org/openstreetmap/josm/data/validation/algos/Tarjan.java +++ b/src/org/openstreetmap/josm/data/validation/algos/Tarjan.java @@ -16,13 +16,12 @@ import java.util.List; import java.util.Map; - /** * Tarjan's strongly connected components algorithm for JOSM. * + * @author gaben * @see * Tarjan's strongly connected components algorithm - * @author gaben * @since xxx */ public final class Tarjan { @@ -67,8 +66,8 @@ public Collection> getSCC() { /** * Returns the graph data as a map. - * @see NodeGraph#createMap() * @return the graph data as a map + * @see NodeGraph#createMap() */ public Map> getGraphMap() { return graphMap; @@ -148,7 +147,7 @@ private List getSuccessors(Node node) { /** * Helper class for storing the Tarjan algorithm runtime metadata. */ - private static class TarjanHelper { + private static final class TarjanHelper { private final int index; private int lowlink; diff --git a/src/org/openstreetmap/josm/data/validation/algos/package-info.java b/src/org/openstreetmap/josm/data/validation/algos/package-info.java index 826eb072849..f69af0eedd0 100644 --- a/src/org/openstreetmap/josm/data/validation/algos/package-info.java +++ b/src/org/openstreetmap/josm/data/validation/algos/package-info.java @@ -3,4 +3,4 @@ /** * General purpose algorithm classes for JOSM data validation. */ -package org.openstreetmap.josm.data.validation.algos; \ No newline at end of file +package org.openstreetmap.josm.data.validation.algos; diff --git a/test/data/CycleDetector_test_wikipedia.osm b/test/data/regress/21881/CycleDetector_test_wikipedia.osm similarity index 100% rename from test/data/CycleDetector_test_wikipedia.osm rename to test/data/regress/21881/CycleDetector_test_wikipedia.osm diff --git a/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java b/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java index 7d33d8f4989..3a5a47b7836 100644 --- a/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java +++ b/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java @@ -18,11 +18,10 @@ @BasicPreferences class CycleDetectorTest { - @Test() + @Test void testCycleDetection() throws Exception { CycleDetector cycleDetector = new CycleDetector(); - DataSet ds = OsmReader.parseDataSet( - Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "CycleDetector_test_wikipedia.osm")), null); + DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(21881, "CycleDetector_test_wikipedia.osm"), null); cycleDetector.startTest(null); cycleDetector.visit(ds.allPrimitives()); cycleDetector.endTest(); @@ -30,4 +29,4 @@ void testCycleDetection() throws Exception { // we have 4 cycles in the test file assertEquals(4, cycleDetector.getErrors().size()); } -} \ No newline at end of file +} From fc6a89b07d57f7d6bb3ba5ea4a947ffe187157ba Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Mon, 18 Dec 2023 22:56:50 +0100 Subject: [PATCH 09/24] Revert "Format OsmValidator ID code comments" This reverts commit a20cf9c2a6155648831cb1ba25003c88eb4f5b7d. --- .../josm/data/validation/OsmValidator.java | 88 +++++++++---------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/src/org/openstreetmap/josm/data/validation/OsmValidator.java b/src/org/openstreetmap/josm/data/validation/OsmValidator.java index 68b445ca5ca..81dbd623949 100644 --- a/src/org/openstreetmap/josm/data/validation/OsmValidator.java +++ b/src/org/openstreetmap/josm/data/validation/OsmValidator.java @@ -114,50 +114,50 @@ private OsmValidator() { private static final Class[] CORE_TEST_CLASSES = new Class[] {// NOPMD /* FIXME - unique error numbers for tests aren't properly unique - ignoring will not work as expected */ /* Error codes are class.getName().hashCode() + "_" + oldCode. There should almost never be a collision. */ - DuplicateNode.class, // ID 1 .. 99 - OverlappingWays.class, // ID 101 .. 199 - UntaggedNode.class, // ID 201 .. 299 - UntaggedWay.class, // ID 301 .. 399 - SelfIntersectingWay.class, // ID 401 .. 499 - DuplicatedWayNodes.class, // ID 501 .. 599 - CrossingWays.Ways.class, // ID 601 .. 699 - CrossingWays.Boundaries.class, // ID 601 .. 699 - CrossingWays.SelfCrossing.class, // ID 601 .. 699 - SimilarNamedWays.class, // ID 701 .. 799 - Coastlines.class, // ID 901 .. 999 - WronglyOrderedWays.class, // ID 1001 .. 1099 - UnclosedWays.class, // ID 1101 .. 1199 - TagChecker.class, // ID 1201 .. 1299 - UnconnectedWays.UnconnectedHighways.class, // ID 1301 .. 1399 - UnconnectedWays.UnconnectedRailways.class, // ID 1301 .. 1399 - UnconnectedWays.UnconnectedWaterways.class, // ID 1301 .. 1399 - UnconnectedWays.UnconnectedNaturalOrLanduse.class, // ID 1301 .. 1399 - UnconnectedWays.UnconnectedPower.class, // ID 1301 .. 1399 - DuplicateWay.class, // ID 1401 .. 1499 - NameMismatch.class, // ID 1501 .. 1599 - MultipolygonTest.class, // ID 1601 .. 1699 - RelationChecker.class, // ID 1701 .. 1799 - TurnrestrictionTest.class, // ID 1801 .. 1899 - DuplicateRelation.class, // ID 1901 .. 1999 - WayConnectedToArea.class, // ID 2301 .. 2399 - PowerLines.class, // ID 2501 .. 2599 - Addresses.class, // ID 2601 .. 2699 - Highways.class, // ID 2701 .. 2799 - BarriersEntrances.class, // ID 2801 .. 2899 - OpeningHourTest.class, // ID 2901 .. 2999 - MapCSSTagChecker.class, // ID 3000 .. 3099 - Lanes.class, // ID 3100 .. 3199 - ConditionalKeys.class, // ID 3200 .. 3299 - InternetTags.class, // ID 3300 .. 3399 - ApiCapabilitiesTest.class, // ID 3400 .. 3499 - LongSegment.class, // ID 3500 .. 3599 - PublicTransportRouteTest.class, // ID 3600 .. 3699 - // ID 3700 .. 3799 is automatically removed since it clashed with pt_assistant. - SharpAngles.class, // ID 3800 .. 3899 - ConnectivityRelations.class, // ID 3900 .. 3999 - DirectionNodes.class, // ID 4000 .. 4099 - RightAngleBuildingTest.class, // ID 4100 .. 4199 - CycleDetector.class, // ID 4200 .. 4299 + DuplicateNode.class, // ID 1 .. 99 + OverlappingWays.class, // ID 101 .. 199 + UntaggedNode.class, // ID 201 .. 299 + UntaggedWay.class, // ID 301 .. 399 + SelfIntersectingWay.class, // ID 401 .. 499 + DuplicatedWayNodes.class, // ID 501 .. 599 + CrossingWays.Ways.class, // ID 601 .. 699 + CrossingWays.Boundaries.class, // ID 601 .. 699 + CrossingWays.SelfCrossing.class, // ID 601 .. 699 + SimilarNamedWays.class, // ID 701 .. 799 + Coastlines.class, // ID 901 .. 999 + WronglyOrderedWays.class, // ID 1001 .. 1099 + UnclosedWays.class, // ID 1101 .. 1199 + TagChecker.class, // ID 1201 .. 1299 + UnconnectedWays.UnconnectedHighways.class, // ID 1301 .. 1399 + UnconnectedWays.UnconnectedRailways.class, // ID 1301 .. 1399 + UnconnectedWays.UnconnectedWaterways.class, // ID 1301 .. 1399 + UnconnectedWays.UnconnectedNaturalOrLanduse.class, // ID 1301 .. 1399 + UnconnectedWays.UnconnectedPower.class, // ID 1301 .. 1399 + DuplicateWay.class, // ID 1401 .. 1499 + NameMismatch.class, // ID 1501 .. 1599 + MultipolygonTest.class, // ID 1601 .. 1699 + RelationChecker.class, // ID 1701 .. 1799 + TurnrestrictionTest.class, // ID 1801 .. 1899 + DuplicateRelation.class, // ID 1901 .. 1999 + WayConnectedToArea.class, // ID 2301 .. 2399 + PowerLines.class, // ID 2501 .. 2599 + Addresses.class, // ID 2601 .. 2699 + Highways.class, // ID 2701 .. 2799 + BarriersEntrances.class, // ID 2801 .. 2899 + OpeningHourTest.class, // 2901 .. 2999 + MapCSSTagChecker.class, // 3000 .. 3099 + Lanes.class, // 3100 .. 3199 + ConditionalKeys.class, // 3200 .. 3299 + InternetTags.class, // 3300 .. 3399 + ApiCapabilitiesTest.class, // 3400 .. 3499 + LongSegment.class, // 3500 .. 3599 + PublicTransportRouteTest.class, // 3600 .. 3699 + // 3700 .. 3799 is automatically removed since it clashed with pt_assistant. + SharpAngles.class, // 3800 .. 3899 + ConnectivityRelations.class, // 3900 .. 3999 + DirectionNodes.class, // 4000 .. 4099 + RightAngleBuildingTest.class, // 4100 .. 4199 + CycleDetector.class, // 4200 .. 4299 }; /** From 5e6e90beab21fd6b8c597d6d6eeb446bf078f38b Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Tue, 19 Dec 2023 19:50:45 +0100 Subject: [PATCH 10/24] Revert the reordering of NodeGraph class --- .../josm/data/osm/NodeGraph.java | 115 +++++++++--------- 1 file changed, 57 insertions(+), 58 deletions(-) diff --git a/src/org/openstreetmap/josm/data/osm/NodeGraph.java b/src/org/openstreetmap/josm/data/osm/NodeGraph.java index d054a2379fd..7b4b062eaea 100644 --- a/src/org/openstreetmap/josm/data/osm/NodeGraph.java +++ b/src/org/openstreetmap/josm/data/osm/NodeGraph.java @@ -29,26 +29,10 @@ * @since 12463 (extracted from CombineWayAction) */ public class NodeGraph { - private final Set edges; - private final Map> successors = new LinkedHashMap<>(); - private final Map> predecessors = new LinkedHashMap<>(); - private int numUndirectedEdges; - - /** - * The number of edges that were added. - */ - private int addedEdges; - - /** - * Constructs a new {@code NodeGraph}. - */ - public NodeGraph() { - edges = new LinkedHashSet<>(); - } /** * Builds a list of pair of nodes from the given way. - * @param way 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 inverse copy) * @return a list of pair of nodes from the given way @@ -66,7 +50,7 @@ public static List buildNodePairs(Way way, boolean directed) { /** * Builds a list of pair of nodes from the given ways. - * @param ways 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 inverse copy) * @return a list of pair of nodes from the given ways @@ -171,45 +155,12 @@ public static NodeGraph createNearlyUndirectedGraphFromNodeWays(Collection return graph; } - /** - * Add a node pair. - * @param pair node pair - */ - public void add(NodePair pair) { - addedEdges++; - edges.add(pair); - } - - /** - * Add a list of node pairs. - * @param pairs collection of node pairs - */ - public void add(Iterable pairs) { - for (NodePair pair : pairs) { - add(pair); - } - } - - /** - * Return the edges containing the node pairs of the graph. - * @return the edges containing the node pairs of the graph - */ - public Collection getEdges() { - return Collections.unmodifiableSet(edges); - } - - /** - * Return the graph's nodes. - * @return the graph's nodes - */ - public Collection getNodes() { - Set nodes = new LinkedHashSet<>(2 * edges.size()); - for (NodePair pair : edges) { - nodes.add(pair.getA()); - nodes.add(pair.getB()); - } - return nodes; - } + private final Set edges; + private int numUndirectedEdges; + /** The number of edges that were added. */ + private int addedEdges; + private final Map> successors = new LinkedHashMap<>(); + private final Map> predecessors = new LinkedHashMap<>(); /** * Constructs a lookup table from the existing edges in the graph to enable efficient querying. @@ -281,6 +232,40 @@ protected void prepare() { numUndirectedEdges = undirectedEdges.size(); } + /** + * Constructs a new {@code NodeGraph}. + */ + public NodeGraph() { + edges = new LinkedHashSet<>(); + } + + /** + * Add a node pair. + * @param pair node pair + */ + public void add(NodePair pair) { + addedEdges++; + edges.add(pair); + } + + /** + * Add a list of node pairs. + * @param pairs collection of node pairs + */ + public void add(Iterable pairs) { + for (NodePair pair : pairs) { + add(pair); + } + } + + /** + * Return the edges containing the node pairs of the graph. + * @return the edges containing the node pairs of the graph + */ + public Collection getEdges() { + return Collections.unmodifiableSet(edges); + } + /** * Return the terminal nodes of the graph. * @return the terminal nodes of the graph @@ -304,6 +289,19 @@ protected List getOutboundPairs(Node node) { return Optional.ofNullable(successors.get(node)).orElseGet(Collections::emptyList); } + /** + * Return the graph's nodes. + * @return the graph's nodes + */ + public Collection getNodes() { + Set nodes = new LinkedHashSet<>(2 * edges.size()); + for (NodePair pair : edges) { + nodes.add(pair.getA()); + nodes.add(pair.getB()); + } + return nodes; + } + protected boolean isSpanningWay(Collection way) { return numUndirectedEdges == way.size(); } @@ -317,6 +315,7 @@ protected List buildPathFromNodePairs(Deque path) { * Tries to find a spanning path starting from node {@code startNode}. *

* Traverses the path in depth-first order. + * * @param startNode the start node * @return the spanning path; empty list if no path is found */ @@ -347,7 +346,7 @@ protected List buildSpanningPath(Node startNode) { * the segment of a way) exactly once.

* Note that duplicated edges are removed first! * - * @return the path; {@code null} if no path was found + * @return the path; {@code null}, if no path was found */ public List buildSpanningPath() { prepare(); From 1a9ab2484ad09ce998e299dece54cf89f56be2e6 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Tue, 19 Dec 2023 19:58:05 +0100 Subject: [PATCH 11/24] Rename algorithms package --- .../josm/data/validation/{algos => algorithms}/Tarjan.java | 2 +- .../josm/data/validation/algorithms/package-info.java | 6 ++++++ .../josm/data/validation/algos/package-info.java | 6 ------ .../josm/data/validation/tests/CycleDetector.java | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) rename src/org/openstreetmap/josm/data/validation/{algos => algorithms}/Tarjan.java (98%) create mode 100644 src/org/openstreetmap/josm/data/validation/algorithms/package-info.java delete mode 100644 src/org/openstreetmap/josm/data/validation/algos/package-info.java diff --git a/src/org/openstreetmap/josm/data/validation/algos/Tarjan.java b/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java similarity index 98% rename from src/org/openstreetmap/josm/data/validation/algos/Tarjan.java rename to src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java index 64709d5c435..050c30f76c6 100644 --- a/src/org/openstreetmap/josm/data/validation/algos/Tarjan.java +++ b/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java @@ -1,5 +1,5 @@ // License: GPL. For details, see LICENSE file. -package org.openstreetmap.josm.data.validation.algos; +package org.openstreetmap.josm.data.validation.algorithms; import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.NodeGraph; diff --git a/src/org/openstreetmap/josm/data/validation/algorithms/package-info.java b/src/org/openstreetmap/josm/data/validation/algorithms/package-info.java new file mode 100644 index 00000000000..5788cfb0101 --- /dev/null +++ b/src/org/openstreetmap/josm/data/validation/algorithms/package-info.java @@ -0,0 +1,6 @@ +// License: GPL. For details, see LICENSE file. + +/** + * General purpose algorithm classes for OSM data validation. + */ +package org.openstreetmap.josm.data.validation.algorithms; diff --git a/src/org/openstreetmap/josm/data/validation/algos/package-info.java b/src/org/openstreetmap/josm/data/validation/algos/package-info.java deleted file mode 100644 index f69af0eedd0..00000000000 --- a/src/org/openstreetmap/josm/data/validation/algos/package-info.java +++ /dev/null @@ -1,6 +0,0 @@ -// License: GPL. For details, see LICENSE file. - -/** - * General purpose algorithm classes for JOSM data validation. - */ -package org.openstreetmap.josm.data.validation.algos; diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index 482289e242e..38fdb0e2430 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -25,7 +25,7 @@ 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.validation.algos.Tarjan; +import org.openstreetmap.josm.data.validation.algorithms.Tarjan; import org.openstreetmap.josm.gui.progress.ProgressMonitor; import org.openstreetmap.josm.spi.preferences.Config; From 032e95e18b08af21c4d753aa0537c3761cfd55b0 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Tue, 19 Dec 2023 20:05:29 +0100 Subject: [PATCH 12/24] Remove unused imports --- .../josm/data/validation/tests/CycleDetectorTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java b/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java index 3a5a47b7836..fe2f4748ec7 100644 --- a/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java +++ b/test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java @@ -3,9 +3,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; -import java.nio.file.Files; -import java.nio.file.Paths; - import org.junit.jupiter.api.Test; import org.openstreetmap.josm.TestUtils; import org.openstreetmap.josm.data.osm.DataSet; From 9976e551e755dc6ef505c422f24d2b498db318a5 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Tue, 19 Dec 2023 20:13:49 +0100 Subject: [PATCH 13/24] Fix RedundantFieldInitializer pmd warning --- .../openstreetmap/josm/data/validation/algorithms/Tarjan.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java b/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java index 050c30f76c6..a0038dd0e3d 100644 --- a/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java +++ b/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java @@ -42,7 +42,7 @@ public final class Tarjan { private final Deque stack = new ArrayDeque<>(); /** Used on algorithm runtime to keep track discovery progress. */ - private int index = 0; + private int index; public Tarjan(NodeGraph graph) { graphMap = graph.createMap(); From eb04f301c2b0feff70c61099ac8d1019f40ac1a0 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Wed, 20 Dec 2023 10:43:29 +0100 Subject: [PATCH 14/24] Improve CPU performance and memory consumption --- .../data/validation/algorithms/Tarjan.java | 14 ++++++---- .../data/validation/tests/CycleDetector.java | 26 +++++++++---------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java b/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java index a0038dd0e3d..ecd735c95d2 100644 --- a/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java +++ b/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java @@ -12,7 +12,6 @@ import java.util.Collections; import java.util.Deque; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; @@ -35,8 +34,8 @@ public final class Tarjan { /** Used to store the graph data as a map. */ private final Map> graphMap; - /** Used to store strongly connected components. */ - private final Collection> scc = new HashSet<>(); + /** Used to store strongly connected components. NOTE: single nodes are not stored to save memory. */ + private final Collection> scc = new ArrayList<>(); /** Used on algorithm runtime to keep track discovery progress. */ private final Deque stack = new ArrayDeque<>(); @@ -51,7 +50,7 @@ public Tarjan(NodeGraph graph) { } /** - * Returns the strongly connected components in the current graph. + * Returns the strongly connected components in the current graph. Single nodes are ignored to save memory. * * @return the strongly connected components in the current graph */ @@ -66,6 +65,7 @@ public Collection> getSCC() { /** * Returns the graph data as a map. + * * @return the graph data as a map * @see NodeGraph#createMap() */ @@ -120,7 +120,11 @@ private void strongConnect(final Node u0) { v = stack.remove(); currentSCC.add(v); } while (!v.equals(u)); - scc.add(currentSCC); + + // store the component only if it makes a cycle, otherwise it's a waste of memory + if (currentSCC.size() > 1) { + scc.add(currentSCC); + } } if (!work.isEmpty()) { Node v = u; diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index 38fdb0e2430..eb8f0da0a62 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -17,7 +17,6 @@ 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.osm.WaySegment; @@ -28,6 +27,7 @@ import org.openstreetmap.josm.data.validation.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, @@ -42,8 +42,8 @@ public class CycleDetector extends Test { /** All waterways for cycle detection */ private final Set usableWaterways = new HashSet<>(); - /** Already visited primitives */ - private final Set visitedWays = new HashSet<>(); + /** Already visited primitive unique IDs */ + private final Set visitedWays = new HashSet<>(); /** Currently used directional waterways from the OSM wiki */ private List directionalWaterways; @@ -111,23 +111,23 @@ public void clear() { * @return WaySegments from the Nodes */ private static Collection createSegments(Map> graphMap, Collection nodes) { - List pairs = new ArrayList<>(); + 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 NodePair(node, successor)); + pairs.add(new Pair<>(node, successor)); } } } Collection segments = new ArrayList<>(); - for (NodePair pair : pairs) { - final Node n = pair.getA(); - final Node m = pair.getB(); + 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()); @@ -187,7 +187,7 @@ private Collection> getGraphs() { * @return a collection of ways which belongs to the same graph */ private Collection buildGraph(Way way) { - if (visitedWays.contains(way)) + if (visitedWays.contains(way.getUniqueId())) return Collections.emptySet(); final Set graph = new HashSet<>(); @@ -196,19 +196,19 @@ private Collection buildGraph(Way way) { while (!queue.isEmpty()) { Way currentWay = queue.poll(); - visitedWays.add(currentWay); + visitedWays.add(currentWay.getUniqueId()); for (Node node : currentWay.getNodes()) { Collection referrers = node.referrers(Way.class) .filter(this::isPrimitiveUsable) .filter(candidate -> candidate != currentWay) - .collect(Collectors.toSet()); + .collect(Collectors.toList()); if (!referrers.isEmpty()) { for (Way referrer : referrers) { - if (!visitedWays.contains(referrer)) { + if (!visitedWays.contains(referrer.getUniqueId())) { queue.offer(referrer); - visitedWays.add(referrer); + visitedWays.add(referrer.getUniqueId()); } } graph.addAll(referrers); From db741d5a76199c0df6befcfa06638195a567da78 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Wed, 20 Dec 2023 14:54:07 +0100 Subject: [PATCH 15/24] Fix javadoc, add translation context --- .../josm/data/validation/algorithms/Tarjan.java | 5 +++++ .../josm/data/validation/tests/CycleDetector.java | 7 +++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java b/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java index ecd735c95d2..3dff63fbe1b 100644 --- a/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java +++ b/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java @@ -43,6 +43,11 @@ public final class Tarjan { /** Used on algorithm runtime to keep track discovery progress. */ private int index; + /** + * Initialize the Tarjan's algorithm. + * + * @param graph graph data in NodeGraph object format + */ public Tarjan(NodeGraph graph) { graphMap = graph.createMap(); diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index eb8f0da0a62..00879597d3f 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -2,6 +2,7 @@ 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; @@ -85,7 +86,7 @@ public void endTest() { if (possibleCycle.size() > 1) { errors.add( TestError.builder(this, Severity.ERROR, CYCLE_DETECTED) - .message(tr("Cycle in directional waterway network")) + .message(trc("graph theory", "Cycle in directional waterway network")) .primitives(possibleCycle) .highlightWaySegments(createSegments(graphMap, possibleCycle)) .build() @@ -106,8 +107,9 @@ public void 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 + * @param nodes nodes to build the way segments from * @return WaySegments from the Nodes */ private static Collection createSegments(Map> graphMap, Collection nodes) { @@ -147,6 +149,7 @@ private static Collection createSegments(Map> graph /** * 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 From 6068544318689ea9c1f634193a445f56347151af Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Wed, 20 Dec 2023 15:34:21 +0100 Subject: [PATCH 16/24] Break is enough --- src/org/openstreetmap/josm/data/osm/NodeGraph.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/org/openstreetmap/josm/data/osm/NodeGraph.java b/src/org/openstreetmap/josm/data/osm/NodeGraph.java index 7b4b062eaea..cf069a41732 100644 --- a/src/org/openstreetmap/josm/data/osm/NodeGraph.java +++ b/src/org/openstreetmap/josm/data/osm/NodeGraph.java @@ -51,7 +51,7 @@ public static List buildNodePairs(Way way, boolean directed) { /** * 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.

+ * @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 inverse copy) * @return a list of pair of nodes from the given ways */ From 53b386dd76aad6b1a613dbf990e0301e6632a910 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Wed, 27 Dec 2023 20:38:18 +0100 Subject: [PATCH 17/24] Move Tarjan class one level up --- .../josm/data/{validation => }/algorithms/Tarjan.java | 2 +- .../josm/data/{validation => }/algorithms/package-info.java | 2 +- .../openstreetmap/josm/data/validation/tests/CycleDetector.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) rename src/org/openstreetmap/josm/data/{validation => }/algorithms/Tarjan.java (98%) rename src/org/openstreetmap/josm/data/{validation => }/algorithms/package-info.java (66%) diff --git a/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java b/src/org/openstreetmap/josm/data/algorithms/Tarjan.java similarity index 98% rename from src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java rename to src/org/openstreetmap/josm/data/algorithms/Tarjan.java index 3dff63fbe1b..49a9b2f028b 100644 --- a/src/org/openstreetmap/josm/data/validation/algorithms/Tarjan.java +++ b/src/org/openstreetmap/josm/data/algorithms/Tarjan.java @@ -1,5 +1,5 @@ // License: GPL. For details, see LICENSE file. -package org.openstreetmap.josm.data.validation.algorithms; +package org.openstreetmap.josm.data.algorithms; import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.NodeGraph; diff --git a/src/org/openstreetmap/josm/data/validation/algorithms/package-info.java b/src/org/openstreetmap/josm/data/algorithms/package-info.java similarity index 66% rename from src/org/openstreetmap/josm/data/validation/algorithms/package-info.java rename to src/org/openstreetmap/josm/data/algorithms/package-info.java index 5788cfb0101..dcf2b45585a 100644 --- a/src/org/openstreetmap/josm/data/validation/algorithms/package-info.java +++ b/src/org/openstreetmap/josm/data/algorithms/package-info.java @@ -3,4 +3,4 @@ /** * General purpose algorithm classes for OSM data validation. */ -package org.openstreetmap.josm.data.validation.algorithms; +package org.openstreetmap.josm.data.algorithms; diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index 00879597d3f..e52aab4354d 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -25,7 +25,7 @@ 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.validation.algorithms.Tarjan; +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; From 3bd03645d83cec716bb545e8bebc39a22658a3c4 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Fri, 29 Dec 2023 17:15:05 +0100 Subject: [PATCH 18/24] Move visitedWays check one level up to better reflect the actual behaviour --- .../josm/data/validation/tests/CycleDetector.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index e52aab4354d..92f4ec7d7cb 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -8,7 +8,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -174,10 +173,14 @@ private Collection> getGraphs() { Collection> graphs = new ArrayList<>(); for (Way waterway : usableWaterways) { + if (visitedWays.contains(waterway.getUniqueId())) { + continue; + } Collection graph = buildGraph(waterway); - if (!graph.isEmpty()) + if (!graph.isEmpty()) { graphs.add(graph); + } } return graphs; @@ -190,9 +193,6 @@ private Collection> getGraphs() { * @return a collection of ways which belongs to the same graph */ private Collection buildGraph(Way way) { - if (visitedWays.contains(way.getUniqueId())) - return Collections.emptySet(); - final Set graph = new HashSet<>(); Queue queue = new ArrayDeque<>(); queue.offer(way); From b8d68da2ee303010a5de5c79f11994f747445cd5 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Mon, 15 Jan 2024 00:41:58 +0100 Subject: [PATCH 19/24] Change reporting from nodes to parent ways, as it will be required for #23397 --- .../data/validation/tests/CycleDetector.java | 37 +++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index 92f4ec7d7cb..450a3be5f6c 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -15,9 +15,11 @@ import java.util.Set; import java.util.stream.Collectors; +import org.openstreetmap.josm.data.osm.BBox; 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.QuadBuckets; import org.openstreetmap.josm.data.osm.Way; import org.openstreetmap.josm.data.osm.WaySegment; import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper; @@ -69,26 +71,39 @@ public void visit(Way w) { 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")); + Arrays.asList("river", "stream", "tidal_channel", "drain", "ditch", "fish_pass", "fairway")); } @Override public void endTest() { + final QuadBuckets quadBuckets = new QuadBuckets<>(); + quadBuckets.addAll(usableWaterways); + 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) { + for (List possibleCycle : scc) { // there is a cycle in the graph if a strongly connected component has more than one node if (possibleCycle.size() > 1) { + // build bbox to locate the issue + BBox bBox = new BBox(); + possibleCycle.forEach(node -> bBox.addPrimitive(node, 0)); + // find ways within this bbox + List waysWithinErrorBbox = quadBuckets.search(bBox); + List toReport = waysWithinErrorBbox.stream() + .filter(w -> possibleCycle.stream() + .anyMatch(w.getNodes()::contains)) + .collect(Collectors.toList()); + + Map> graphMap = tarjan.getGraphMap(); 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() + TestError.builder(this, Severity.ERROR, CYCLE_DETECTED) + .message(trc("graph theory", "Cycle in directional waterway network")) + .primitives(toReport) + .highlightWaySegments(createSegments(graphMap, possibleCycle)) + .build() ); } } @@ -203,9 +218,9 @@ private Collection buildGraph(Way way) { for (Node node : currentWay.getNodes()) { Collection referrers = node.referrers(Way.class) - .filter(this::isPrimitiveUsable) - .filter(candidate -> candidate != currentWay) - .collect(Collectors.toList()); + .filter(this::isPrimitiveUsable) + .filter(candidate -> candidate != currentWay) + .collect(Collectors.toList()); if (!referrers.isEmpty()) { for (Way referrer : referrers) { From 41c3e42d0951884cde309dac5c953fa811ac1364 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Mon, 15 Jan 2024 10:17:04 +0100 Subject: [PATCH 20/24] Partial selection: add all affected ways to TestError --- .../josm/data/validation/tests/CycleDetector.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index 450a3be5f6c..e22590e8952 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -84,6 +84,11 @@ public void endTest() { Tarjan tarjan = new Tarjan(nodeGraph); Collection> scc = tarjan.getSCC(); + // for partial selection, we need to manually add the rest of graph members to the lookup object + if (partialSelection) { + quadBuckets.addAll(graph); + } + for (List possibleCycle : scc) { // there is a cycle in the graph if a strongly connected component has more than one node if (possibleCycle.size() > 1) { @@ -93,8 +98,7 @@ public void endTest() { // find ways within this bbox List waysWithinErrorBbox = quadBuckets.search(bBox); List toReport = waysWithinErrorBbox.stream() - .filter(w -> possibleCycle.stream() - .anyMatch(w.getNodes()::contains)) + .filter(w -> possibleCycle.stream().filter(w.getNodes()::contains).count() > 1) .collect(Collectors.toList()); Map> graphMap = tarjan.getGraphMap(); From 5b3f5f497a10e6d549fe1a6fa97c2f6c51ed1a35 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Sun, 21 Apr 2024 08:51:14 +0200 Subject: [PATCH 21/24] Fix error highlight for self-intersecting ways --- .../josm/data/osm/WaySegment.java | 4 +-- .../data/validation/tests/CycleDetector.java | 25 ++++++++++++------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/org/openstreetmap/josm/data/osm/WaySegment.java b/src/org/openstreetmap/josm/data/osm/WaySegment.java index 48fe38c4a52..6b4b3de7c09 100644 --- a/src/org/openstreetmap/josm/data/osm/WaySegment.java +++ b/src/org/openstreetmap/josm/data/osm/WaySegment.java @@ -2,7 +2,7 @@ package org.openstreetmap.josm.data.osm; /** - * A segment consisting of 2 consecutive nodes out of a way. + * A segment consisting of two consecutive nodes out of a way. */ public final class WaySegment extends IWaySegment { @@ -36,7 +36,7 @@ public static WaySegment forNodePair(Way way, Node first, Node second) { } endIndex--; } - throw new IllegalArgumentException("Node pair is not part of way!"); + throw new IllegalArgumentException("The node pair is not consecutive part of the way!"); } /** diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index e22590e8952..88c925896c4 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -155,7 +155,7 @@ private static Collection createSegments(Map> graph intersect.retainAll(mWays); for (Way w : intersect) { - if (w.getNeighbours(n).contains(m) && getNodeIndex(w, n) + 1 == getNodeIndex(w, m)) { + if (isConsecutive(w, n, m)) { segments.add(WaySegment.forNodePair(w, n, m)); } } @@ -166,20 +166,21 @@ private static Collection createSegments(Map> graph } /** - * Returns the way index of a node. Only the first occurrence is considered in case it's a closed way. + * Returns the way index of a node. Only the first occurrence is considered in case it's a closed or self-intersecting 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 + * @param n the first node to look up + * @param m the second, possibly consecutive node + * @return {@code true} if the nodes are consecutive order in the way direction */ - private static int getNodeIndex(Way w, Node n) { - for (int i = 0; i < w.getNodesCount(); i++) { - if (w.getNode(i).equals(n)) { - return i; + private static boolean isConsecutive(Way w, Node n, Node m) { + for (int i = 0; i < w.getNodesCount() - 1; i++) { + if (w.getNode(i).equals(n) && w.getNode(i + 1).equals(m)) { + return true; } } - return -1; + return false; } /** @@ -237,6 +238,12 @@ private Collection buildGraph(Way way) { } } } + + // case for single, non-connected waterways + if (graph.isEmpty()) { + graph.add(way); + } + return graph; } } From 567da9cb034f1dc1734e20549dae2680fa8e0cd3 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Mon, 22 Apr 2024 11:22:05 +0200 Subject: [PATCH 22/24] Fix Javadoc --- .../josm/data/validation/tests/CycleDetector.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index 88c925896c4..1f0f2f89ecc 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -166,10 +166,10 @@ private static Collection createSegments(Map> graph } /** - * Returns the way index of a node. Only the first occurrence is considered in case it's a closed or self-intersecting way. + * Determines if the given nodes are consecutive part of the parent way. * * @param w parent way - * @param n the first node to look up + * @param n the first node to look up in the way direction * @param m the second, possibly consecutive node * @return {@code true} if the nodes are consecutive order in the way direction */ From d820d386fec9a18fe8a1c61de0865c09835aa7d7 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Thu, 25 Apr 2024 09:44:25 +0200 Subject: [PATCH 23/24] Remove tidal_channel and fairway from the list of compatible ways --- .../openstreetmap/josm/data/validation/tests/CycleDetector.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index 1f0f2f89ecc..8592b41f997 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -71,7 +71,7 @@ public void visit(Way w) { 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")); + Arrays.asList("river", "stream", "drain", "ditch", "fish_pass")); } @Override From c5472710e80350f924bff71e9c3bab15224d753e Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Thu, 25 Apr 2024 10:28:37 +0200 Subject: [PATCH 24/24] As per wiki, tidal_channel is directional "The direction of the way should be towards the sea (i.e. the direction of water flow at low tide)." --- .../openstreetmap/josm/data/validation/tests/CycleDetector.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java index 8592b41f997..cc9a56aca7f 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java +++ b/src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java @@ -71,7 +71,7 @@ public void visit(Way w) { public void startTest(ProgressMonitor progressMonitor) { super.startTest(progressMonitor); directionalWaterways = Config.getPref().getList(PREFIX + ".directionalWaterways", - Arrays.asList("river", "stream", "drain", "ditch", "fish_pass")); + Arrays.asList("river", "stream", "tidal_channel", "drain", "ditch", "fish_pass")); } @Override