Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JOSM #22799 - Smarter DistributeAction action for single node #129

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

JOSM #22799 - Smarter DistributeAction action for single node #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
90896db
Apply patch without modification from https://josm.openstreetmap.de/a…
gabortim Oct 8, 2023
4e8bef4
Add unit test for DistributeAction
gabortim Oct 8, 2023
5face9f
Add single node unit test for DistributeAction
gabortim Oct 8, 2023
09ec5ca
Use List interface instead of Collection
gabortim Oct 12, 2023
b495fdf
Fix typo and documentation
gabortim Oct 25, 2023
c076aa8
Move nodePairFurthestApart() methods to Geometry utility class
gabortim Oct 25, 2023
24a43fe
Add a todo before I forget
gabortim Oct 25, 2023
c9d0d47
Make test method parameters more flexible
gabortim Oct 26, 2023
f5ac297
Simplify DistributeAction by using Geometry class methods
gabortim Oct 26, 2023
c8e3d65
Rename local variables
gabortim Oct 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 3 additions & 87 deletions src/org/openstreetmap/josm/actions/AlignInLineAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
package org.openstreetmap.josm.actions;

import static org.openstreetmap.josm.gui.help.HelpUtil.ht;
import static org.openstreetmap.josm.tools.Geometry.nodePairFurthestApart;
import static org.openstreetmap.josm.tools.I18n.tr;

import java.awt.event.ActionEvent;
Expand Down Expand Up @@ -80,91 +81,6 @@ static class InvalidSelection extends Exception {
}
}

/**
* Return 2 nodes making up the line along which provided nodes must be aligned.
*
* @param nodes Nodes to be aligned.
* @return A array of two nodes.
* @throws IllegalArgumentException if nodes is empty
*/
private static Node[] nodePairFurthestApart(List<Node> nodes) {
// Detect if selected nodes are on the same way.

// Get ways passing though all selected nodes.
Set<Way> waysRef = null;
for (Node n: nodes) {
Collection<Way> ref = n.getParentWays();
if (waysRef == null)
waysRef = new HashSet<>(ref);
else
waysRef.retainAll(ref);
}

if (waysRef == null) {
throw new IllegalArgumentException();
}

// Nodes belongs to multiple ways, return most distant nodes.
if (waysRef.size() != 1)
return nodeFurthestAppart(nodes);

// All nodes are part of the same way. See #9605.
Way way = waysRef.iterator().next();

if (way.isClosed()) {
// Align these nodes on the line passing through the most distant nodes.
return nodeFurthestAppart(nodes);
}

Node nodea = null;
Node nodeb = null;

// The way is open, align nodes on the line passing through the extremity nodes (most distant in the way
// sequence). See #9605#comment:3.
Set<Node> remainNodes = new HashSet<>(nodes);
for (Node n : way.getNodes()) {
if (!remainNodes.contains(n))
continue;
if (nodea == null)
nodea = n;
if (remainNodes.size() == 1) {
nodeb = remainNodes.iterator().next();
break;
}
remainNodes.remove(n);
}

return new Node[] {nodea, nodeb};
}

/**
* Return the two nodes the most distant from the provided list.
*
* @param nodes List of nodes to analyze.
* @return An array containing the two most distant nodes.
*/
private static Node[] nodeFurthestAppart(List<Node> nodes) {
Node node1 = null, node2 = null;
double minSqDistance = 0;
int nb;

nb = nodes.size();
for (int i = 0; i < nb - 1; i++) {
Node n = nodes.get(i);
for (int j = i + 1; j < nb; j++) {
Node m = nodes.get(j);
double sqDist = n.getEastNorth().distanceSq(m.getEastNorth());
if (sqDist > minSqDistance) {
node1 = n;
node2 = m;
minSqDistance = sqDist;
}
}
}

return new Node[] {node1, node2};
}

/**
* Operation depends on the selected objects:
*/
Expand All @@ -190,7 +106,7 @@ public void actionPerformed(ActionEvent e) {
* Builds "align in line" command depending on the selected objects.
* @param ds data set in which the command operates
* @return the resulting command to execute to perform action
* @throws InvalidSelection if a polygon is selected, or if a node is used by 3 or more ways
* @throws InvalidSelection if a polygon is selected, or if a node is used by three or more ways
* @since 13108
*/
public Command buildCommand(DataSet ds) throws InvalidSelection {
Expand Down Expand Up @@ -226,7 +142,7 @@ public Command buildCommand(DataSet ds) throws InvalidSelection {
}

/**
* Align nodes in case 3 or more nodes are selected.
* Align nodes in case three or more nodes are selected.
*
* @param nodes Nodes to be aligned.
* @return Command that perform action.
Expand Down
109 changes: 67 additions & 42 deletions src/org/openstreetmap/josm/actions/DistributeAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
package org.openstreetmap.josm.actions;

import static org.openstreetmap.josm.gui.help.HelpUtil.ht;
import static org.openstreetmap.josm.tools.Geometry.getFurthestPrimitive;
import static org.openstreetmap.josm.tools.Geometry.nodeFurthestApart;
import static org.openstreetmap.josm.tools.I18n.tr;

import java.awt.event.ActionEvent;
import java.awt.event.KeyEvent;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.Iterator;
Expand Down Expand Up @@ -49,7 +52,9 @@ public DistributeAction() {
* Select method according to user selection.
* Case 1: One Way (no self-crossing) and at most 2 nodes contains by this way:
* Distribute nodes keeping order along the way
* Case 2: Other
* Case 2: One Node part of at least one way, not a start or end node
* Distribute the selected node relative to neighbors
* Case 3: Other
* Distribute nodes
*/
@Override
Expand All @@ -59,8 +64,8 @@ public void actionPerformed(ActionEvent e) {

// Collect user selected objects
Collection<OsmPrimitive> selected = getLayerManager().getEditDataSet().getSelected();
Collection<Way> ways = new LinkedList<>();
Collection<Node> nodes = new HashSet<>();
List<Way> ways = new ArrayList<>();
List<Node> nodes = new ArrayList<>();
for (OsmPrimitive osm : selected) {
if (osm instanceof Node) {
nodes.add((Node) osm);
Expand All @@ -81,13 +86,15 @@ public void actionPerformed(ActionEvent e) {
cmds = distributeWay(ways, nodes);
} else if (checkDistributeNodes(ways, nodes)) {
cmds = distributeNodes(nodes);
} else if (checkDistributeNode(nodes)) {
cmds = distributeNode(nodes);
} else {
new Notification(
tr("Please select :\n" +
"* One no self-crossing way with at most two of its nodes;\n" +
"* Three nodes."))
tr("Please select:<ul>" +
"<li>One no self-crossing way with at most two of its nodes;</li>" +
"<li>One node in the middle of a way;</li>" +
"<li>Three nodes.</li></ul>"))
.setIcon(JOptionPane.INFORMATION_MESSAGE)
.setDuration(Notification.TIME_SHORT)
.show();
return;
}
Expand Down Expand Up @@ -184,6 +191,46 @@ private static Collection<Command> distributeWay(Collection<Way> ways,
return cmds;
}

/**
* Test if single node oriented algorithm applies to the selection.
* @param nodes The selected node. Collection type and naming kept for compatibility with similar methods.
* @return true in this case
*/
private static boolean checkDistributeNode(List<Node> nodes) {
if (nodes.size() == 1) {
Node node = nodes.get(0);
int goodWays = 0;
for (Way way : node.getParentWays()) {
// the algorithm is applicable only if there is one way which:
// - is open and the selected node is a middle node, or
// - is closed and has at least 4 nodes (as 3 doesn't make sense and error-prone)
if (!way.isFirstLastNode(node) || (way.isClosed() && way.getRealNodesCount() > 3))
goodWays++;
}
return goodWays == 1;
}
return false;
}

/**
* Distribute a single node relative to way neighbours.
* @see DistributeAction#distributeNodes(Collection)
* @param nodes a single node in a collection to distribute
* @return Commands to execute to perform action
*/
private static Collection<Command> distributeNode(List<Node> nodes) {
final Node nodeToDistribute = nodes.get(0);
Way parent = nodeToDistribute.getParentWays().get(0);

List<Node> neighbours = new ArrayList<>(parent.getNeighbours(nodeToDistribute));

// insert in the middle
neighbours.add(1, nodeToDistribute);

// call the distribution method with 3 nodes
return distributeNodes(neighbours);
}

/**
* Test if nodes oriented algorithm applies to the selection.
* @param ways Selected ways
Expand All @@ -197,7 +244,7 @@ private static boolean checkDistributeNodes(Collection<Way> ways, Collection<Nod
/**
* Distribute nodes when only nodes are selected.
* The general algorithm here is to find the two selected nodes
* that are furthest apart, and then to distribute all other selected
* that are the furthest apart, and then to distribute all other selected
* nodes along the straight line between these nodes.
* @param nodes nodes to distribute
* @return Commands to execute to perform action
Expand All @@ -206,59 +253,37 @@ private static boolean checkDistributeNodes(Collection<Way> ways, Collection<Nod
private static Collection<Command> distributeNodes(Collection<Node> nodes) {
// Find from the selected nodes two that are the furthest apart.
// Let's call them A and B.
double distance = Double.NEGATIVE_INFINITY;

Node nodea = null;
Node nodeb = null;

Collection<Node> itnodes = new LinkedList<>(nodes);
for (Node n : nodes) {
itnodes.remove(n);
for (Node m : itnodes) {
double dist = Math.sqrt(n.getEastNorth().distance(m.getEastNorth()));
if (dist > distance) {
nodea = n;
nodeb = m;
distance = dist;
}
}
}
Node[] furthestApart = nodeFurthestApart(new ArrayList<>(nodes));
Node nodeA = furthestApart[0];
Node nodeB = furthestApart[1];

if (nodea == null || nodeb == null) {
if (nodeA == null || nodeB == null) {
throw new IllegalArgumentException();
}

// Remove the nodes A and B from the list of nodes to move
nodes.remove(nodea);
nodes.remove(nodeb);
nodes.remove(nodeA);
nodes.remove(nodeB);

// Find out co-ords of A and B
double ax = nodea.getEastNorth().east();
double ay = nodea.getEastNorth().north();
double bx = nodeb.getEastNorth().east();
double by = nodeb.getEastNorth().north();
double ax = nodeA.getEastNorth().east();
double ay = nodeA.getEastNorth().north();
double bx = nodeB.getEastNorth().east();
double by = nodeB.getEastNorth().north();

// A list of commands to do
Collection<Command> cmds = new LinkedList<>();

// Amount of nodes between A and B plus 1
// Number of nodes between A and B plus 1
int num = nodes.size()+1;

// Current number of node
int pos = 0;
while (!nodes.isEmpty()) {
pos++;
Node s = null;

// Find the node that is furthest from B (i.e. closest to A)
distance = Double.NEGATIVE_INFINITY;
for (Node n : nodes) {
double dist = Math.sqrt(nodeb.getEastNorth().distance(n.getEastNorth()));
if (dist > distance) {
s = n;
distance = dist;
}
}
Node s = getFurthestPrimitive(nodeB, nodes);

if (s != null) {
// First move the node to A's position, then move it towards B
Expand Down
Loading