Changeset 17393 in josm


Ignore:
Timestamp:
2020-12-07T11:52:15+01:00 (3 months ago)
Author:
GerdP
Message:

see #10205: Strange align nodes in circle behavior

  • allow to define center node also when a single unclosed way is selected
  • add robustness and more unit tests for evaluation of valid selections
  • if only nodes and no way are selected, order the nodes using the angle to the agv. east-north position. This should produce a predictable result
  • if way(s) are selected the order the nodes is determined by the occurence in the way(s). Self-Intersecting polygons are rejected if no center node is given
  • don't throw InvalidSelection when selection is valid but no point was moved, let buildCommand() return null instead

With a selection that gives a center point and way(s) which are not even close to a circular shape the result might still be surprising. Sometimes the way nodes are arranged around the center node, sometimes they are moved so that a circle arc with nearly the same length is produced. The result changes significantly when the way nodes are also selected. Subject to further improvements.

Location:
trunk
Files:
3 added
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/src/org/openstreetmap/josm/actions/AlignInCircleAction.java

    r17386 r17393  
    1414import java.util.List;
    1515import java.util.Set;
     16import java.util.SortedMap;
     17import java.util.TreeMap;
    1618import java.util.stream.Collectors;
    1719
     
    3032import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon;
    3133import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon.JoinedWay;
     34import org.openstreetmap.josm.data.validation.tests.CrossingWays;
    3235import org.openstreetmap.josm.data.validation.tests.SelfIntersectingWay;
    3336import org.openstreetmap.josm.gui.Notification;
     
    4346 * @author Teemu Koskinen
    4447 * @author Alain Delplanque
     48 * @author Gerd Petermann
    4549 *
    4650 * @since 146
     
    107111        try {
    108112            Command cmd = buildCommand(getLayerManager().getEditDataSet());
    109             UndoRedoHandler.getInstance().add(cmd);
     113            if (cmd != null)
     114                UndoRedoHandler.getInstance().add(cmd);
     115            else {
     116                new Notification(tr("nothing changed"))
     117                .setIcon(JOptionPane.INFORMATION_MESSAGE)
     118                .setDuration(Notification.TIME_SHORT)
     119                .show();
     120
     121            }
    110122        } catch (InvalidSelection except) {
    111123            Logging.debug(except);
     
    115127                .show();
    116128        }
    117 
    118129    }
    119130
     
    140151     * --> Align these nodes, all are fix
    141152     * @param ds data set in which the command operates
    142      * @return the resulting command to execute to perform action
     153     * @return the resulting command to execute to perform action, or null if nothing was changed
    143154     * @throws InvalidSelection if selection cannot be used
    144155     * @since 17386
     
    147158    public static Command buildCommand(DataSet ds) throws InvalidSelection {
    148159        Collection<OsmPrimitive> sel = ds.getSelected();
    149         List<Node> nodes = new LinkedList<>();
     160        List<Node> selectedNodes = new LinkedList<>();
    150161        // fixNodes: All nodes for which the angle relative to center should not be modified
    151162        Set<Node> fixNodes = new HashSet<>();
     
    156167        for (OsmPrimitive osm : sel) {
    157168            if (osm instanceof Node) {
    158                 nodes.add((Node) osm);
     169                selectedNodes.add((Node) osm);
    159170            } else if (osm instanceof Way) {
    160171                ways.add((Way) osm);
     
    162173        }
    163174
    164         if (ways.size() == 1 && !ways.get(0).isClosed()) {
     175        // nodes on selected ways
     176        List<Node> onWay = new ArrayList<>();
     177        if (!ways.isEmpty()) {
     178            List<Node> potentialCenter = new ArrayList<>();
     179            for (Node n : selectedNodes) {
     180                if (ways.stream().anyMatch(w -> w.containsNode(n))) {
     181                    onWay.add(n);
     182                } else {
     183                    potentialCenter.add(n);
     184                }
     185            }
     186            if (potentialCenter.size() == 1) {
     187                // center is given
     188                center = potentialCenter.get(0).getEastNorth();
     189                if (onWay.size() == 1) {
     190                    radius = center.distance(onWay.get(0).getEastNorth());
     191                }
     192            } else if (potentialCenter.size() > 1) {
     193                throw new InvalidSelection(tr("Please select only one node as center."));
     194            }
     195
     196        }
     197
     198        final List<Node> nodes;
     199        if (ways.isEmpty()) {
     200            nodes = sortByAngle(selectedNodes);
     201            fixNodes.addAll(nodes);
     202        } else if (ways.size() == 1 && !ways.get(0).isClosed()) {
    165203            // Case 1
    166204            Way w = ways.get(0);
    167             if (SelfIntersectingWay.isSelfIntersecting(w)) {
    168                 throw new InvalidSelection(tr("Self-intersecting way"));
    169             }
    170 
    171205            fixNodes.add(w.firstNode());
    172206            fixNodes.add(w.lastNode());
    173             fixNodes.addAll(nodes);
    174             fixNodes.addAll(collectNodesWithExternReferrers(ways));
     207            fixNodes.addAll(onWay);
    175208            // Temporary closed way used to reorder nodes
    176209            Way closedWay = new Way(w);
    177             closedWay.addNode(w.firstNode());
    178             nodes = collectNodesAnticlockwise(Collections.singletonList(closedWay));
    179             closedWay.setNodes(null); // see #19885
    180         } else if (!ways.isEmpty() && checkWaysArePolygon(ways)) {
    181             // Case 2
    182             List<Node> inside = new ArrayList<>();
    183             List<Node> outside = new ArrayList<>();
    184 
    185             for (Node n: nodes) {
    186                 boolean isInside = ways.stream().anyMatch(w -> w.getNodes().contains(n));
    187                 if (isInside)
    188                     inside.add(n);
    189                 else
    190                     outside.add(n);
    191             }
    192 
    193             if (outside.size() == 1 && inside.isEmpty()) {
    194                 center = outside.get(0).getEastNorth();
    195             } else if (outside.size() == 1 && inside.size() == 1) {
    196                 center = outside.get(0).getEastNorth();
    197                 radius = center.distance(inside.get(0).getEastNorth());
    198             } else if (inside.size() == 2 && outside.isEmpty()) {
    199                 // 2 nodes inside, define diameter
    200                 EastNorth en0 = inside.get(0).getEastNorth();
    201                 EastNorth en1 = inside.get(1).getEastNorth();
    202                 center = new EastNorth((en0.east() + en1.east()) / 2, (en0.north() + en1.north()) / 2);
     210            try {
     211                closedWay.addNode(w.firstNode());
     212                nodes = collectNodesAnticlockwise(Collections.singletonList(closedWay));
     213            } finally {
     214                closedWay.setNodes(null); // see #19885
     215            }
     216        } else if (Multipolygon.joinWays(ways).size() == 1) {
     217            // Case 2:
     218            if (onWay.size() == 2) {
     219                // 2 way nodes define diameter
     220                EastNorth en0 = onWay.get(0).getEastNorth();
     221                EastNorth en1 = onWay.get(1).getEastNorth();
    203222                radius = en0.distance(en1) / 2;
    204             }
    205 
    206             fixNodes.addAll(inside);
    207             fixNodes.addAll(collectNodesWithExternReferrers(ways));
     223                if (center == null) {
     224                    center = en0.getCenter(en1);
     225                }
     226            }
     227            fixNodes.addAll(onWay);
    208228            nodes = collectNodesAnticlockwise(ways);
    209             if (nodes.size() < 4) {
    210                 throw new InvalidSelection(tr("Not enough nodes in selected ways."));
    211             }
    212         } else if (ways.isEmpty() && nodes.size() > 3) {
    213             // Case 3
    214             fixNodes.addAll(nodes);
    215             // No need to reorder nodes since all are fix
    216229        } else {
    217             if (ways.isEmpty() && nodes.size() <= 3)
    218                 throw new InvalidSelection(tr("Please select at least four nodes."));
    219230            throw new InvalidSelection();
    220231        }
     232        fixNodes.addAll(collectNodesWithExternReferrers(ways));
    221233
    222234        // Check if one or more nodes are outside of download area
     
    224236            throw new InvalidSelection(tr("One or more nodes involved in this action is outside of the downloaded area."));
    225237
     238
    226239        if (center == null) {
    227             // Compute the center of nodes
    228             center = Geometry.getCenter(nodes);
     240            if (nodes.size() < 4) {
     241                throw new InvalidSelection(tr("Not enough nodes to calculate center."));
     242            }
     243            if (validateGeometry(nodes)) {
     244                // Compute the center of nodes
     245                center = Geometry.getCenter(nodes);
     246            }
    229247            if (center == null) {
    230                 throw new InvalidSelection(tr("Cannot determine center of selected nodes."));
     248                throw new InvalidSelection(tr("Cannot determine center of circle for this geometry."));
    231249            }
    232250        }
     
    282300        }
    283301        if (cmds.isEmpty())
    284             throw new InvalidSelection(tr("nothing changed"));
     302            return null;
    285303        return new SequenceCommand(tr("Align Nodes in Circle"), cmds);
     304    }
     305
     306    private static List<Node> sortByAngle(final List<Node> nodes) {
     307        EastNorth sum = new EastNorth(0, 0);
     308        for (Node n : nodes) {
     309            EastNorth en = n.getEastNorth();
     310            sum = sum.add(en.east(), en.north());
     311        }
     312        final EastNorth simpleCenter = new EastNorth(sum.east()/nodes.size(), sum.north()/nodes.size());
     313
     314        SortedMap<Double, List<Node>> orderedMap = new TreeMap<>();
     315        for (Node n : nodes) {
     316            double angle = new PolarCoor(n.getEastNorth(), simpleCenter).angle;
     317            orderedMap.computeIfAbsent(angle, k-> new ArrayList<>()).add(n);
     318        }
     319        return orderedMap.values().stream().flatMap(List<Node>::stream).collect(Collectors.toList());
     320    }
     321
     322    private static boolean validateGeometry(List<Node> nodes) {
     323        Way test = new Way();
     324        test.setNodes(nodes);
     325        if (!test.isClosed()) {
     326            test.addNode(test.firstNode());
     327        }
     328
     329        try {
     330            if (CrossingWays.isSelfCrossing(test))
     331                return false;
     332            return !SelfIntersectingWay.isSelfIntersecting(test);
     333        } finally {
     334            test.setNodes(null); // see #19855
     335        }
    286336    }
    287337
     
    304354        Collection<JoinedWay> rings = Multipolygon.joinWays(ways);
    305355        if (rings.size() != 1)
    306             throw new InvalidSelection();
     356            throw new InvalidSelection(); // we should never get here
    307357        List<Node> nodes = new ArrayList<>(rings.iterator().next().getNodes());
    308         if (nodes.get(0) != nodes.get(nodes.size()-1))
     358        if (nodes.get(0) != nodes.get(nodes.size() - 1))
    309359            throw new InvalidSelection();
    310360        if (Geometry.isClockwise(nodes))
     
    324374        updateEnabledStateOnModifiableSelection(selection);
    325375    }
    326 
    327     /**
    328      * Determines if ways can be joined into a single polygon.
    329      * @param ways The ways collection to check
    330      * @return true if all ways can be joined into a single polygon
    331      */
    332     private static boolean checkWaysArePolygon(Collection<Way> ways) {
    333         if (Multipolygon.joinWays(ways).size() != 1)
    334             return false;
    335         // For each way, nodes strictly between first and last should't be reference by an other way
    336         for (Way way: ways) {
    337             for (Node node: way.getNodes()) {
    338                 if (way.isFirstLastNode(node)) continue;
    339                 if (ways.stream().filter(wayOther -> way != wayOther).anyMatch(wayOther -> node.getReferrers().contains(wayOther))) {
    340                     return false;
    341                 }
    342             }
    343         }
    344         return true;
    345     }
    346 
    347376}
  • trunk/src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java

    r17348 r17393  
    2727import org.openstreetmap.josm.data.validation.util.ValUtil;
    2828import org.openstreetmap.josm.gui.progress.ProgressMonitor;
     29import org.openstreetmap.josm.tools.CheckParameterUtil;
    2930import org.openstreetmap.josm.tools.Logging;
    3031
     
    469470    }
    470471
     472    /**
     473     * Check if the given way is self crossing
     474     * @param way the way to check
     475     * @return {@code true} if one or more segments of the way are crossing
     476     * @see SelfIntersectingWay
     477     * @since xxx
     478     */
     479    public static boolean isSelfCrossing(Way way) {
     480        CheckParameterUtil.ensureParameterNotNull(way, "way");
     481        SelfCrossing test = new SelfCrossing();
     482        test.visit(way);
     483        return !test.getErrors().isEmpty();
     484    }
    471485}
  • trunk/test/unit/org/openstreetmap/josm/actions/AlignInCircleActionTest.java

    r17386 r17393  
    33
    44import static org.junit.jupiter.api.Assertions.assertEquals;
     5import static org.junit.jupiter.api.Assertions.assertFalse;
    56import static org.junit.jupiter.api.Assertions.assertNotNull;
    6 import static org.junit.jupiter.api.Assertions.assertThrows;
     7import static org.junit.jupiter.api.Assertions.assertNull;
     8import static org.junit.jupiter.api.Assertions.assertTrue;
    79
    810import java.nio.file.Files;
     
    1416import org.junit.jupiter.api.extension.RegisterExtension;
    1517import org.openstreetmap.josm.TestUtils;
     18import org.openstreetmap.josm.actions.AlignInCircleAction.InvalidSelection;
    1619import org.openstreetmap.josm.command.Command;
    1720import org.openstreetmap.josm.data.osm.DataSet;
    1821import org.openstreetmap.josm.data.osm.Node;
     22import org.openstreetmap.josm.data.osm.OsmPrimitive;
    1923import org.openstreetmap.josm.data.osm.Way;
    2024import org.openstreetmap.josm.io.OsmReader;
    2125import org.openstreetmap.josm.testutils.JOSMTestRules;
     26import org.opentest4j.AssertionFailedError;
    2227
    2328import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
     
    7176    /**
    7277     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/20041">Bug #20041</a>.
    73      * Don't create move commands when no node is visibly moved
     78     * Don't create move commands when no node is visibly moved.
    7479     * @throws Exception if an error occurs
    7580     */
     
    8893        if (roundabout != null) {
    8994            ds.setSelected(roundabout);
    90             assertThrows(AlignInCircleAction.InvalidSelection.class, () -> AlignInCircleAction.buildCommand(ds));
     95            assertNull(AlignInCircleAction.buildCommand(ds));
    9196        }
    9297    }
     
    150155    }
    151156
     157    /**
     158     * Various cases of selections in file
     159     * @throws Exception if an error occurs
     160     */
     161    @Test
     162    void testSelectionEvaluation() throws Exception {
     163        DataSet ds = OsmReader.parseDataSet(
     164                Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleCases.osm")), null);
     165
     166        for (int i = 0; i < 80; i++) {
     167            final String selVal = Integer.toString(i);
     168            Set<OsmPrimitive> sel = ds.allPrimitives().stream().filter(p -> p.hasTag("sel", selVal))
     169                    .collect(Collectors.toSet());
     170            if (sel.isEmpty())
     171                continue;
     172            ds.setSelected(sel);
     173            boolean selValid = sel.stream().noneMatch(p -> p.hasKey("invalid-selection"));
     174            try {
     175                AlignInCircleAction.buildCommand(ds);
     176                assertTrue(selValid, "sel=" + selVal + " is not valid?");
     177            } catch (InvalidSelection e) {
     178                assertFalse(selValid, "sel=" + selVal + " is not invalid?");
     179            } catch (Exception e) {
     180                throw new AssertionFailedError("test failed: sel=" + selVal,e);
     181            }
     182        }
     183    }
    152184}
Note: See TracChangeset for help on using the changeset viewer.