Changeset 10991 in josm


Ignore:
Timestamp:
2016-09-11T02:15:50+02:00 (8 years ago)
Author:
Don-vip
Message:

fix #13570 - Coastline validator doesn't report some problems and is slow (patch by Gerd Petermann, modified)

Location:
trunk
Files:
3 added
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/org/openstreetmap/josm/data/validation/tests/Coastlines.java

    r10448 r10991  
    88import java.util.Collection;
    99import java.util.Collections;
     10import java.util.HashSet;
    1011import java.util.Iterator;
    1112import java.util.LinkedList;
    1213import java.util.List;
    13 
    14 import org.openstreetmap.josm.Main;
     14import java.util.Set;
     15
    1516import org.openstreetmap.josm.command.ChangeCommand;
    1617import org.openstreetmap.josm.command.Command;
    … …  
    2122import org.openstreetmap.josm.data.validation.Test;
    2223import org.openstreetmap.josm.data.validation.TestError;
    23 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
    2424import org.openstreetmap.josm.gui.progress.ProgressMonitor;
     25import org.openstreetmap.josm.tools.Geometry;
    2526
    2627/**
    … …  
    2930 * @author frsantos
    3031 * @author Teemu Koskinen
     32 * @author Gerd Petermann
    3133 */
    3234public class Coastlines extends Test {
    … …  
    3537    protected static final int REVERSED_COASTLINE = 902;
    3638    protected static final int UNCONNECTED_COASTLINE = 903;
    37 
    38     private List<Way> coastlines;
    39 
    40     private Area downloadedArea;
     39    protected static final int WRONG_ORDER_COASTLINE = 904;
     40
     41    private List<Way> coastlineWays;
    4142
    4243    /**
    … …  
    4950    @Override
    5051    public void startTest(ProgressMonitor monitor) {
    51 
    5252        super.startTest(monitor);
    53 
    54         OsmDataLayer layer = Main.getLayerManager().getEditLayer();
    55 
    56         if (layer != null) {
    57             downloadedArea = layer.data.getDataSourceArea();
    58         }
    59 
    60         coastlines = new LinkedList<>();
     53        coastlineWays = new LinkedList<>();
    6154    }
    6255
    6356    @Override
    6457    public void endTest() {
    65         for (Way c1 : coastlines) {
    66             Node head = c1.firstNode();
    67             Node tail = c1.lastNode();
    68 
    69             if (c1.getNodesCount() == 0 || head.equals(tail)) {
    70                 continue;
    71             }
    72 
    73             int headWays = 0;
    74             int tailWays = 0;
    75             boolean headReversed = false;
    76             boolean tailReversed = false;
    77             boolean headUnordered = false;
    78             boolean tailUnordered = false;
    79             Way next = null;
    80             Way prev = null;
    81 
    82             for (Way c2 : coastlines) {
    83                 if (c1 == c2) {
    84                     continue;
    85                 }
    86 
    87                 if (c2.containsNode(head)) {
    88                     headWays++;
    89                     next = c2;
    90 
    91                     if (head.equals(c2.firstNode())) {
    92                         headReversed = true;
    93                     } else if (!head.equals(c2.lastNode())) {
    94                         headUnordered = true;
    95                     }
    96                 }
    97 
    98                 if (c2.containsNode(tail)) {
    99                     tailWays++;
    100                     prev = c2;
    101 
    102                     if (tail.equals(c2.lastNode())) {
    103                         tailReversed = true;
    104                     } else if (!tail.equals(c2.firstNode())) {
    105                         tailUnordered = true;
    106                     }
    107                 }
    108             }
    109 
    110             // To avoid false positives on upload (only modified primitives
    111             // are visited), we have to check possible connection to ways
    112             // that are not in the set of validated primitives.
    113             if (headWays == 0) {
    114                 Collection<OsmPrimitive> refs = head.getReferrers();
    115                 for (OsmPrimitive ref : refs) {
    116                     if (ref != c1 && isCoastline(ref)) {
    117                         // ref cannot be in <code>coastlines</code>, otherwise we would
    118                         // have picked it up already
    119                         headWays++;
    120                         next = (Way) ref;
    121 
    122                         if (head.equals(next.firstNode())) {
    123                             headReversed = true;
    124                         } else if (!head.equals(next.lastNode())) {
    125                             headUnordered = true;
     58        checkConnections();
     59        checkDirection();
     60        coastlineWays = null;
     61        super.endTest();
     62    }
     63
     64    /**
     65     * Check connections between coastline ways.
     66     * The nodes of a coastline way have to fullfil these rules:
     67     * 1) the first node must be connected to the last node of a coastline way (which might be the same way)
     68     * 2) the last node must be connected to the first node of a coastline way (which might be the same way)
     69     * 3) all other nodes must not be connected to a coastline way
     70     * 4) the number of referencing coastline ways must be 1 or 2
     71     * Nodes outside the download area are special cases, we may not know enough about the connected ways.
     72     */
     73    private void checkConnections() {
     74        Area downloadedArea = null;
     75        for (Way w : coastlineWays) {
     76            if (downloadedArea == null) {
     77                downloadedArea = w.getDataSet().getDataSourceArea();
     78            }
     79            int numNodes = w.getNodesCount();
     80            for (int i = 0; i < numNodes; i++) {
     81                Node n = w.getNode(i);
     82                List<OsmPrimitive> refs = n.getReferrers();
     83                Set<Way> connectedWays = new HashSet<>();
     84                for (OsmPrimitive p : refs) {
     85                    if (p != w && isCoastline(p)) {
     86                        connectedWays.add((Way) p);
     87                    }
     88                }
     89                if (i == 0) {
     90                    if (connectedWays.isEmpty() && n != w.lastNode() && n.getCoor().isIn(downloadedArea)) {
     91                        addError(UNCONNECTED_COASTLINE, w, null, n);
     92                    }
     93                    if (connectedWays.size() == 1 && n != connectedWays.iterator().next().lastNode()) {
     94                        checkIfReversed(w, connectedWays.iterator().next(), n);
     95                    }
     96                    if (connectedWays.size() == 1 && w.isClosed() && connectedWays.iterator().next().isClosed()) {
     97                        addError(UNORDERED_COASTLINE, w, connectedWays, n);
     98                    }
     99                } else if (i == numNodes - 1) {
     100                    if (connectedWays.isEmpty() && n != w.firstNode() && n.getCoor().isIn(downloadedArea)) {
     101                        addError(UNCONNECTED_COASTLINE, w, null, n);
     102                    }
     103                    if (connectedWays.size() == 1 && n != connectedWays.iterator().next().firstNode()) {
     104                        checkIfReversed(w, connectedWays.iterator().next(), n);
     105                    }
     106                } else if (!connectedWays.isEmpty()) {
     107                    addError(UNORDERED_COASTLINE, w, connectedWays, n);
     108                }
     109            }
     110        }
     111    }
     112
     113    /**
     114     * Check if two or more coastline ways form a closed clockwise way
     115     */
     116    private void checkDirection() {
     117        HashSet<Way> done = new HashSet<>();
     118        for (Way w : coastlineWays) {
     119            if (done.contains(w))
     120                continue;
     121            List<Way> visited = new ArrayList<>();
     122            done.add(w);
     123            visited.add(w);
     124            List<Node> nodes = new ArrayList<>(w.getNodes());
     125            Way curr = w;
     126            while (nodes.get(0) != nodes.get(nodes.size()-1)) {
     127                boolean foundContinuation = false;
     128                for (OsmPrimitive p : curr.lastNode().getReferrers()) {
     129                    if (p != curr && isCoastline(p)) {
     130                        Way other = (Way) p;
     131                        if (done.contains(other))
     132                            continue;
     133                        if (other.firstNode() == curr.lastNode()) {
     134                            foundContinuation = true;
     135                            curr = other;
     136                            done.add(curr);
     137                            visited.add(curr);
     138                            nodes.remove(nodes.size()-1); // remove duplicate connection node
     139                            nodes.addAll(curr.getNodes());
     140                            break;
    126141                        }
    127142                    }
    128143                }
    129             }
    130             if (tailWays == 0) {
    131                 Collection<OsmPrimitive> refs = tail.getReferrers();
    132                 for (OsmPrimitive ref : refs) {
    133                     if (ref != c1 && isCoastline(ref)) {
    134                         tailWays++;
    135                         prev = (Way) ref;
    136 
    137                         if (tail.equals(prev.lastNode())) {
    138                             tailReversed = true;
    139                         } else if (!tail.equals(prev.firstNode())) {
    140                             tailUnordered = true;
    141                         }
    142                     }
    143                 }
    144             }
    145 
    146             List<OsmPrimitive> primitives = new ArrayList<>();
    147             primitives.add(c1);
    148 
    149             if (headWays == 0 || tailWays == 0) {
    150                 List<OsmPrimitive> highlight = new ArrayList<>();
    151 
    152                 if (headWays == 0 && head.getCoor().isIn(downloadedArea)) {
    153                     highlight.add(head);
    154                 }
    155                 if (tailWays == 0 && tail.getCoor().isIn(downloadedArea)) {
    156                     highlight.add(tail);
    157                 }
    158 
    159                 if (!highlight.isEmpty()) {
    160                     errors.add(new TestError(this, Severity.ERROR, tr("Unconnected coastline"),
    161                             UNCONNECTED_COASTLINE, primitives, highlight));
    162                 }
    163             }
    164 
    165             boolean unordered = false;
    166             boolean reversed = headWays == 1 && headReversed && tailWays == 1 && tailReversed;
    167 
    168             if (headWays > 1 || tailWays > 1) {
    169                 unordered = true;
    170             } else if (headUnordered || tailUnordered) {
    171                 unordered = true;
    172             } else if (reversed && next == prev) {
    173                 unordered = true;
    174             } else if ((headReversed || tailReversed) && headReversed != tailReversed) {
    175                 unordered = true;
    176             }
    177 
    178             if (unordered) {
    179                 List<OsmPrimitive> highlight = new ArrayList<>();
    180 
    181                 if (headWays > 1 || headUnordered || headReversed || reversed) {
    182                     highlight.add(head);
    183                 }
    184                 if (tailWays > 1 || tailUnordered || tailReversed || reversed) {
    185                     highlight.add(tail);
    186                 }
    187 
    188                 errors.add(new TestError(this, Severity.ERROR, tr("Unordered coastline"),
    189                         UNORDERED_COASTLINE, primitives, highlight));
    190             } else if (reversed) {
    191                 errors.add(new TestError(this, Severity.ERROR, tr("Reversed coastline"),
    192                         REVERSED_COASTLINE, primitives));
    193             }
    194         }
    195 
    196         coastlines = null;
    197         downloadedArea = null;
    198 
    199         super.endTest();
     144                if (!foundContinuation)
     145                    break;
     146            }
     147            // simple closed ways are reported by WronglyOrderedWays
     148            if (visited.size() > 1 && nodes.get(0) == nodes.get(nodes.size()-1) && Geometry.isClockwise(nodes)) {
     149                errors.add(new TestError(this, Severity.WARNING, tr("Reversed coastline: land not on left side"),
     150                        WRONG_ORDER_COASTLINE, visited));
     151            }
     152        }
     153    }
     154
     155    /**
     156     * Check if a reversed way would fit, if yes, add fixable "reversed" error, "unordered" else
     157     * @param w way that might be reversed
     158     * @param other other way that is connected to w
     159     * @param n1 node at which w and other are connected
     160     */
     161    private void checkIfReversed(Way w, Way other, Node n1) {
     162        boolean headFixedWithReverse = false;
     163        boolean tailFixedWithReverse = false;
     164        int otherCoastlineWays = 0;
     165        Way connectedToFirst = null;
     166        for (int i = 0; i < 2; i++) {
     167            Node n = (i == 0) ? w.firstNode() : w.lastNode();
     168            List<OsmPrimitive> refs = n.getReferrers();
     169            for (OsmPrimitive p : refs) {
     170                if (p != w && isCoastline(p)) {
     171                    Way cw = (Way) p;
     172                    if (i == 0 && cw.firstNode() == n) {
     173                        headFixedWithReverse = true;
     174                        connectedToFirst = cw;
     175                    } else if (i == 1 && cw.lastNode() == n) {
     176                        if (cw != connectedToFirst)
     177                            tailFixedWithReverse = true;
     178                    } else
     179                        otherCoastlineWays++;
     180                }
     181            }
     182        }
     183        if (otherCoastlineWays == 0 && headFixedWithReverse && tailFixedWithReverse)
     184            addError(REVERSED_COASTLINE, w, null, null);
     185        else
     186            addError(UNORDERED_COASTLINE, w, Collections.singletonList(other), n1);
     187    }
     188
     189    /**
     190     * Add error if not already done
     191     * @param errCode the error code
     192     * @param w the way that is in error
     193     * @param otherWays collection of other ways in error or null
     194     * @param n the node to be highlighted or null
     195     */
     196    private void addError(int errCode, Way w, Collection<Way> otherWays, Node n) {
     197        String msg;
     198        switch (errCode) {
     199        case UNCONNECTED_COASTLINE:
     200            msg = tr("Unconnected coastline");
     201            break;
     202        case UNORDERED_COASTLINE:
     203            msg = tr("Unordered coastline");
     204            break;
     205        case REVERSED_COASTLINE:
     206            msg = tr("Reversed coastline");
     207            break;
     208        default:
     209            msg = tr("invalid coastline"); // should not happen
     210        }
     211        Set<OsmPrimitive> primitives = new HashSet<>();
     212        primitives.add(w);
     213        if (otherWays != null)
     214            primitives.addAll(otherWays);
     215        // check for repeated error
     216        for (TestError e : errors) {
     217            if (e.getCode() != errCode)
     218                continue;
     219            if (errCode != REVERSED_COASTLINE && !e.getHighlighted().contains(n))
     220                continue;
     221            if (e.getPrimitives().size() != primitives.size())
     222                continue;
     223            if (!e.getPrimitives().containsAll(primitives))
     224                continue;
     225            return; // we already know this error
     226        }
     227        if (errCode != REVERSED_COASTLINE)
     228            errors.add(new TestError(this, Severity.ERROR, msg, errCode, primitives, Collections.singletonList(n)));
     229        else
     230            errors.add(new TestError(this, Severity.ERROR, msg, errCode, primitives));
    200231    }
    201232
    … …  
    206237
    207238        if (isCoastline(way)) {
    208             coastlines.add(way);
     239            coastlineWays.add(way);
    209240        }
    210241    }

  • trunk/src/org/openstreetmap/josm/gui/DefaultNameFormatter.java

    r10748 r10991  
    136136    }
    137137
     138    /**
     139     * Formats a name for an {@link OsmPrimitive}.
     140     *
     141     * @param osm the primitive
     142     * @return the name
     143     * @since 10991
     144     */
     145    public String format(OsmPrimitive osm) {
     146        if (osm instanceof Node) {
     147            return format((Node) osm);
     148        } else if (osm instanceof Way) {
     149            return format((Way) osm);
     150        } else if (osm instanceof Relation) {
     151            return format((Relation) osm);
     152        }
     153        return null;
     154    }
     155
    138156    @Override
    139157    public String format(Node node) {

  • trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MultipolygonTestTest.java

    r10945 r10991  
    55import static org.junit.Assert.assertFalse;
    66import static org.junit.Assert.assertTrue;
    7 import static org.junit.Assert.fail;
    87
    9 import java.io.FileInputStream;
    10 import java.io.InputStream;
    118import java.util.ArrayList;
    12 import java.util.Arrays;
    139import java.util.List;
    14 import java.util.Set;
    15 import java.util.TreeSet;
     10import java.util.stream.Collectors;
    1611
    1712import org.junit.Rule;
    … …  
    2217import org.openstreetmap.josm.data.osm.Relation;
    2318import org.openstreetmap.josm.data.osm.Way;
    24 import org.openstreetmap.josm.data.validation.TestError;
    25 import org.openstreetmap.josm.gui.DefaultNameFormatter;
    2619import org.openstreetmap.josm.gui.mappaint.ElemStyles;
    27 import org.openstreetmap.josm.io.OsmReader;
    2820import org.openstreetmap.josm.testutils.JOSMTestRules;
    2921
    … …  
    9082    @Test
    9183    public void testMultipolygonFile() throws Exception {
    92         try (InputStream is = new FileInputStream("data_nodist/multipolygon.osm")) {
    93             for (Relation r : OsmReader.parseDataSet(is, null).getRelations()) {
    94                 if (r.isMultipolygon()) {
    95                     String name = DefaultNameFormatter.getInstance().format(r);
    96                     String codes = r.get("josm_error_codes");
    97                     if (codes != null) {
    98                         List<TestError> errors = new ArrayList<>();
    99                         for (org.openstreetmap.josm.data.validation.Test test : Arrays.asList(MULTIPOLYGON_TEST, RELATION_TEST)) {
    100                             test.initialize();
    101                             test.startTest(null);
    102                             test.visit(r);
    103                             test.endTest();
    104                             errors.addAll(test.getErrors());
    105                         }
    106                         Set<Integer> expectedCodes = new TreeSet<>();
    107                         for (String code : codes.split(",")) {
    108                             expectedCodes.add(Integer.parseInt(code));
    109                         }
    110                         Set<Integer> actualCodes = new TreeSet<>();
    111                         for (TestError error : errors) {
    112                             Integer code = error.getCode();
    113                             assertTrue(name + " does not expect JOSM error code " + code + ": " + error.getDescription(),
    114                                     expectedCodes.contains(code));
    115                             actualCodes.add(code);
    116                         }
    117                         assertEquals(name + " " + expectedCodes + " => " + actualCodes,
    118                                 expectedCodes.size(), actualCodes.size());
    119                     } else if (r.hasKey("name") && (r.getName().startsWith("06") || r.getName().startsWith("07"))) {
    120                         fail(name + " lacks josm_error_codes tag");
    121                     }
    122                 }
    123             }
    124         }
     84        ValidatorTestUtils.testSampleFile("data_nodist/multipolygon.osm",
     85                ds -> ds.getRelations().stream().filter(Relation::isMultipolygon).collect(Collectors.toList()),
     86                name -> name.startsWith("06") || name.startsWith("07"), MULTIPOLYGON_TEST, RELATION_TEST);
    12587    }
    12688}
Note: See TracChangeset for help on using the changeset viewer.