[patch] remove node from way

[Oli-Wan]

I'd like an entry in the tools menu to remove one node from a way: keep both the way and the node with all their respective properties and relation memberships, but have the node no longer be part of the way.

This boils down to deleting a single "<nd ref=...>" line, but (as far as I know) cannot be done in JOSM directly. Instead, you have to duplicate the node, manually edit the new node's relation memberships, then delete the old node, which ist needlessly cumbersome for such a trivial task.

[stoecker]

I think this is already implemented. Select node and choose the unglue tool ('G').

[Oli-Wan]

Replying to stoecker:

I think this is already implemented. Select node and choose the unglue tool ('G').

Thanks for the hint, but unfortunately, unglue does not do the job. There is in fact a workaround (see below), but I think this is still too complicated.

What unglue does is the following: Suppose you have two ways sharing a common node; then moving the node will affect both ways. Unglue-ing duplicates the node, such that the one way contains the original node, the other contains the duplicate. You can now move around both ways independently. (Works analogously for more than two ways sharing one common node.)

To elaborate on my request: Imagine a single way consisting of n>=3 points. (The n=3 case is actually trivial.) Now suppose you want to take one of the points out of the way, such that you stick with a way consisting of n-1 points and a single point.

You need to do that whenever you come across some POI which someone has accidentally made part of a way -- or maybe on purpose, in a place where this is not [any longer] sensible. For the latter case, take a bus stop which has been relocated to a different street or bus lane or whatever, while the name and all of its route relation memberships remain as before.

Suppose you have points A,B,C along some way, and you want to take out B. The workaround is:

1. split (P) the way three times - at A,B,C
2. delete the ways connecting A to B and B to C
3. extend the remaining way ending in A to C (or vice versa)
4. then combine (C) the two ways meeting up at C

This involves at least ten clicks or keystrokes (seven plus a rectangle selection, if you are lucky), while it could be done in just two (or three, via the menu).

[stoecker]

Please try what I wrote:

• select the node (nothing else)
• unglue

You get this node unglued and a new node entered in way. This node can then be removed.

Probably the node needs to have tags for this to work.

[Oli-Wan]

Okay, well thanks.

I did try your suggestion in the first place, but that just produced an error message.

However, you are perfectly right: if (and only if) the node has some tag, it works. I just opened a josm window and tried the trick whatever ways I found.

I do not know whether the dependency on a tag is a bug or a feature. Anyway, I think it would be a good idea to make the error message somewhat more explicit.
Currently, it displays only "This node is not glued to anything else", which suggests that unglue can only be applied to a shared node, and gives no hint about the "one-point use" of unglue.

What is the JOSM policy on error messages? Keep them as short as possible, and put all advanced information in the documentation? Or is there room for adding something like "Did you try to...?", separated by two newlines? I think that would be very helpful here.

Thanks again.

[Oli-Wan]

On second thought, whoever tries the feature, will try it on a tagged point. So the issue probably almost never arises...

[stanton]

I just came across the same need: to detach a node from a way.

Extra thought: often I come across a node that is in the middle of a way, has some tags attached and is possibly a member of some relations. I would like to move that node to the side of the way but also keep a node in the way (because the node may not be in line with its neighbors, or it may be the last node in the way, and removing it would change the shape of the way when all that was intended was to move the tags and relation memberships somewhere else).

Suggestion: I would like a command that does the following:

• Create a new node at the same position as the selected one
• Remove the selected node from all ways that use it
• Insert the new node into all these ways in the same position as the selected node

Maybe we can present the user with a choice ("Do you want to insert a blank node in the same position?") and if the user chooses not to, omit the first two steps.

I think it's important to keep the tags on the old node (and insert a fresh node in the way, rather than moving the data to a new node and keeping the old one in the way) in order to preserve the history of the old node (the history of a tag-less node in a way is probably less important than that of a node with tags and relation memberships).

[stanton]

I've tried Unglue on a few occasions and found that it messes up relations:

• The node on the way maintains its position in all relations it is a member of.
• The detached node is inserted into each of these, but at the very end.

When I take a tram stop (which is a node on the way) and unglue it so that I can move it e.g. to the other side of a crossing, this behavior messes up the order of stops in my relations (the order matters for some renderers).

Suggestions:

• Insert the new node into relations either before or behind the existing one, but not at the end. (Oh yes, if the existing node is a member in multiple positions of that relation, add the new one next to each occurrence of the old one).
• Present the user with a choice (similar to the one when merging two ways) listing all relations which contain the unglued node and a drop-down box for selecting which nodes to keep (choices: Keep node on the way, Keep unglued node, Keep both, Remove both)

[skyper]

Replying to stanton:

I've tried Unglue on a few occasions and found that it messes up relations:

• The node on the way maintains its position in all relations it is a member of.
• The detached node is inserted into each of these, but at the very end.

When I take a tram stop (which is a node on the way) and unglue it so that I can move it e.g. to the other side of a crossing, this behavior messes up the order of stops in my relations (the order matters for some renderers).

Suggestions:

• Insert the new node into relations either before or behind the existing one, but not at the end. (Oh yes, if the existing node is a member in multiple positions of that relation, add the new one next to each occurrence of the old one).
• Present the user with a choice (similar to the one when merging two ways) listing all relations which contain the unglued node and a drop-down box for selecting which nodes to keep (choices: Keep node on the way, Keep unglued node, Keep both, Remove both)

You are right this should be fixed.

Is the original issue still a problem ?

[giuseppe.bilotta@…]

Patch to implement the feature

[giuseppe.bilotta@…]

I've just uploaded a patch to implement the required feature as an action available from the Tools menu (associated to the Alt+J) shortcut. A single action to achieve the removal (without otherwise touching the node(s) or the way) is IMO better than having to go through the unglue+delete sequeunce, or the extract(utils2 plugin)+delete sequence. The binary blob is for an icon, I'm no graphic artist so feel free to replace it with whatever you want ;-)

[stoecker]

Nothing for core I believe.

[gbilotta <giuseppe.bilotta@…>]

Replying to stoecker:

Nothing for core I believe.

I disagree. The original classification was more appropriate IMHO. This is is a rather fundamental feature that should go in core, not in a plugin.

[AlfonZ]

Here are some things that I find unexpected. Please refer to 5133-test.osm​.

The warning The selected nodes do not share the same way is used in cases when it's misleading in regards to what is selected/why the selection is not right for the action.

• when one lone node is selected
• having two-nodes way, selecting one of them
• having two-nodes way, selecting both of them - in this case the warning is plain incorrect
• selecing a way and a node not belonging to it

Other unexpected behaviour:

• Having a closed way, removing one of the inner (i.e. not the first/last) node leaves the way closed - OK. Removing first/last node opens the way.
• A 3-nodes closed way, removing the inner node, creates self-overlapping way with 2 nodes and 2 segments.
• Two ways sharing one node, selecting the node and short way. Removes the node from longer, not selected way.
• Selecting node(s) and its(their) way. Removes the node(s) from the way - OK. Selecting any number of extra ways that don't touch the node(s) - does the same and ignores the extra selected ways.

Replying to gbilotta <giuseppe.bilotta@…>:

Replying to stoecker:

Nothing for core I believe.

I disagree. The original classification was more appropriate IMHO. This is is a rather fundamental feature that should go in core, not in a plugin.

I thought it's not unusual to add new features into plugins. They might be moved into core at a later time.

[gbilotta <giuseppe.bilotta@…>]

Thanks for the feedback, here's a couple of comments.

Replying to AlfonZ:

Replying to gbilotta <giuseppe.bilotta@…>:

Replying to stoecker:

Nothing for core I believe.

I disagree. The original classification was more appropriate IMHO. This is is a rather fundamental feature that should go in core, not in a plugin.

I thought it's not unusual to add new features into plugins. They might be moved into core at a later time.

I must confess I'm not particularly familiar with the details of JOSM development, this being my first attempt at contributing and whatnot. If the standard m.o. is to first let a feature be distributed in the form of a plugin before moving into core, that's ok; I just thought that this particular feature was important enough to be considered for core (of course, I understand that what can be considered important is highly subjective ;-))

Here are some things that I find unexpected. Please refer to 5133-test.osm​.

The warning The selected nodes do not share the same way is used in cases when it's misleading in regards to what is selected/why the selection is not right for the action.

• when one lone node is selected
• having two-nodes way, selecting one of them
• having two-nodes way, selecting both of them - in this case the warning is plain incorrect
• selecing a way and a node not belonging to it

For what it's worth, this part of the code was copied over from the SplitWayAction logic (you'll notice that it gets the same wrong/misleading message in the same circumstances). (In fact, it was my intention to refactor this logic into a separate tool to be shared by both actions, but this being my first attempt at programming Java I have opted for the simpler approach for the time being.) I'll try making the reply more generic, and special-casing some circumstances.

Other unexpected behaviour:

• Having a closed way, removing one of the inner (i.e. not the first/last) node leaves the way closed - OK. Removing first/last node opens the way.

This was actually intentional, left as a way to (re)open closed ways, although I can see why it is surprising. So the question is: should the behavior always be to keep the way closed, or should it be to always open it? See also the next point.

• A 3-nodes closed way, removing the inner node, creates self-overlapping way with 2 nodes and 2 segments.

Interesting, I hadn't tested this case. I can fix it so that it generates a simple way with only 1 segment, although this creates an inconsistency with the former: removing a node from a 3-nodes closed way opens it, but if the number of nodes is higher the way is kept closed. Shouldn't the behavior be more consistent?

• Two ways sharing one node, selecting the node and short way. Removes the node from longer, not selected way.

This is actually not due to short/long way, but due to terminal/non-terminal node, and it's a side-effect of my copying the getApplicableWays logic from the SplitWay action (which obviously prefers non-terminal nodes). Keep in mind that in your example, however, the short way only has two nodes, so the fall-back to the other way makes sense, sort-of (you can't remove a node from a way with only two nodes).

• Selecting node(s) and its(their) way. Removes the node(s) from the way - OK. Selecting any number of extra ways that don't touch the node(s) - does the same and ignores the extra selected ways.

This follows the logic of the SplitWay action, so I suggest this be kept this way, for consistency (or maybe it should be changed in SplitWay too).

[stoecker]

Replying to gbilotta <giuseppe.bilotta@…>:

I disagree. The original classification was more appropriate IMHO. This is is a rather fundamental feature that should go in core, not in a plugin.

I thought it's not unusual to add new features into plugins. They might be moved into core at a later time.

I must confess I'm not particularly familiar with the details of JOSM development, this being my first attempt at contributing and whatnot. If the standard m.o. is to first let a feature be distributed in the form of a plugin before moving into core, that's ok; I just thought that this particular feature was important enough to be considered for core (of course, I understand that what can be considered important is highly subjective ;-))

It depends. Thought we have too many functions in core already, so usually the policy is to move such "fancy" stuff to utilsplugin and see if importance is really high enough. Experienced users always find that this or that shortcut tool is important, but if it can be reached in a different way, then it also helps to confuse.

This was actually intentional, left as a way to (re)open closed ways, although I can see why it is surprising. So the question is: should the behavior always be to keep the way closed, or should it be to always open it? See also the next point.

Don't handle first/last node different. This is unwanted in any case.

• A 3-nodes closed way, removing the inner node, creates self-overlapping way with 2 nodes and 2 segments.

Interesting, I hadn't tested this case. I can fix it so that it generates a simple way with only 1 segment, although this creates an inconsistency with the former: removing a node from a 3-nodes closed way opens it, but if the number of nodes is higher the way is kept closed. Shouldn't the behavior be more consistent?

Why don't you use take the normal delete actions? They already care properly for these cases: Way.removeNode().

[gbilotta <giuseppe.bilotta@…>]

Replying to stoecker:

Replying to gbilotta <giuseppe.bilotta@…>:

I must confess I'm not particularly familiar with the details of JOSM development, this being my first attempt at contributing and whatnot. If the standard m.o. is to first let a feature be distributed in the form of a plugin before moving into core, that's ok; I just thought that this particular feature was important enough to be considered for core (of course, I understand that what can be considered important is highly subjective ;-))

It depends. Thought we have too many functions in core already, so usually the policy is to move such "fancy" stuff to utilsplugin and see if importance is really high enough. Experienced users always find that this or that shortcut tool is important, but if it can be reached in a different way, then it also helps to confuse.

I understand, but in this case I would say that unglue (or extract) + delete are not really "reaching the tool in a different way", but rather clumsy (and fragile) workarounds for a missing, fundamental (IMHO) functionality (disconnecting nodes from ways).

Why don't you use take the normal delete actions? They already care properly for these cases: Way.removeNode().

I tried that initially, but then the undo/redo didn't work correctly. Or rather, I couldn't find a good explanation of how to make it work correctly (as I mentioned, this is my first approach to JOSM coding). Am I supposed to get the original node list, then do removeNodes, get the resulting node list, restore the original one and then register the undo/redo action with the result of the removeNodes, or is there a smarter way?

[gbilotta <giuseppe.bilotta@…>]

Feature implementation, v2

[gbilotta <giuseppe.bilotta@…>]

New version of the patch uploaded. I think I addressed all the issues raised by my previous patch, keep the feedback coming ;-)

[stoecker]

I tried that initially, but then the undo/redo didn't work correctly. Or rather, I couldn't find a good explanation of how to make it work correctly (as I mentioned, this is my first approach to JOSM coding). Am I supposed to get the original node list, then do removeNodes, get the resulting node list, restore the original one and then register the undo/redo action with the result of the removeNodes, or is there a smarter way?

There are many ways. E.g. using another Command (one replacing the way and not it's nodes) and so on. :-)

I think your current patch misses the case, where resulting way has only one node or zero nodes left. You need to delete the way in that case.

[gbilotta <giuseppe.bilotta@…>]

Replying to stoecker:

I tried that initially, but then the undo/redo didn't work correctly. Or rather, I couldn't find a good explanation of how to make it work correctly (as I mentioned, this is my first approach to JOSM coding). Am I supposed to get the original node list, then do removeNodes, get the resulting node list, restore the original one and then register the undo/redo action with the result of the removeNodes, or is there a smarter way?

There are many ways. E.g. using another Command (one replacing the way and not it's nodes) and so on. :-)

Hm, I think I'll stick with the approach I've found for the time being, sounds more robust than using some other Command.

I think your current patch misses the case, where resulting way has only one node or zero nodes left. You need to delete the way in that case.

In my implementation, if a way would be left with less than 2 distinct nodes, the disconnect is simply aborted, so you are never left with degenerate ways.

[gbilotta <giuseppe.bilotta@…>]

Is there anything else that needs to be done for this? (Also, if necessary I can provide an updated one rebased off current dev head.)

[bastiK]

This part in your patch

+        // I'm sure there's a better way to handle this
+        List<Node> orgNodes = selectedWay.getNodes();
+        selectedWay.removeNodes(new HashSet<Node>(selectedNodes));
+        List<Node> newNodes = selectedWay.getNodes();
+        selectedWay.setNodes(orgNodes);
+        Main.main.undoRedo.add(new ChangeNodesCommand(selectedWay, newNodes));

should be something like

        List<Node> orgNodes = selectedWay.getNodes();
        List<Node> newNodes = new ArrayList<Node>(orgNodes);
        newNodes.removeAll(selectedNodes);
        Main.main.undoRedo.add(new ChangeNodesCommand(selectedWay, newNodes));

Apart from this minor issue, I would say the patch is ready to be committed.

The only question: core or Utilsplugin2?
As the functionality is somewhat covered by the existing Unglue Way feature, I would vote for Utilsplugin2, but it's a tough call.

@team: What do you think?

[stoecker]

When in core, then expert mode.

[gbilotta <giuseppe.bilotta@…>]

Replying to bastiK:

This part in your patch

+        // I'm sure there's a better way to handle this
+        List<Node> orgNodes = selectedWay.getNodes();
+        selectedWay.removeNodes(new HashSet<Node>(selectedNodes));
+        List<Node> newNodes = selectedWay.getNodes();
+        selectedWay.setNodes(orgNodes);
+        Main.main.undoRedo.add(new ChangeNodesCommand(selectedWay, newNodes));

should be something like

        List<Node> orgNodes = selectedWay.getNodes();
        List<Node> newNodes = new ArrayList<Node>(orgNodes);
        newNodes.removeAll(selectedNodes);
        Main.main.undoRedo.add(new ChangeNodesCommand(selectedWay, newNodes));

The reason why I go through removeNodes() is that the method in Way automatically takes care of corner cases such as the closed way. To do that operating on the plain nodes list would require me to duplicate the removeNodes() logic here. I would prefer to avoid that, if possible. Should I refactor that logic into a separate utility function, maybe?

[bastiK]

I guess it's okay then. It would be a little cleaner to do this on a copied Way object without DataSet association, so it doesn't reindex the way 3 times (DataSet.reindexWay).

[gbilotta <giuseppe.bilotta@…>]

Feature implementation, v3

[gbilotta <giuseppe.bilotta@…>]

Ok, new version of the patch (v3): this time, I introduce the new RemoveNodesCommand command, as suggested in a previous comment, and the disconnect action makes use of this new command to handle the undo, in a much more straightforward way than my previous convoluted (and rightly criticized) implementation. The logic should be otherwise unchanged. Comments welcome.

[gbilotta <giuseppe.bilotta@…>]

Is anything else needed for the integration of this patch?

[bastiK]

The patch is fine, but it still has to be decided where to put it (core or Utilsplugin2).

[stoecker]

In 6253/josm:

fix #5133 - add command to remove nodes from ways - patch by Giuseppe Bilotta

[Don-vip]

Ticket #8066 has been marked as a duplicate of this ticket.

[stoecker]

Milestone renamed