Add Detach Node operation #4320

[Psigio]

Hi - I've had an attempt at implementing the Detach Node operation as described in #4320. I've added the new operation, action & some unit tests for the action. It will only operate on nodes that are part of at least one way and have at least one tag. After detaching the node (and replacing with a duplicate without any tags in the way), it switches to move mode immediately so the user can move the detached node.

I'm not sure about the move mode switch, it's not perfect as the mouse pointer is offset from the node based upon the position in the context menu where the Detach Node operation is rendered.

Let me know if you want any changes made!

[mmd-osm]

I tested this patch, and I believe it causes serious side effects, if the node in question is part of a relation. Let's say, I'm detaching a via node of a turn restriction. This pull request creates a new node and replaces all occurrences in the involved ways, but it fails to update the relations. This will eventually break the turn restriction.

In JOSM, this detaching operation is quite involved. Maybe you could take a look how it is implemented there to identify all relevant use cases and how to deal with them.

[Psigio]

Thanks for the information @mmd-osm, I hadn't considered relations membership! I've taken a quick look at the JOSM action, and I'll ask a question back on the original issue as I'm not sure if there are already any examples of actions in ID that require additional user input.

[bhousel]

Thanks for the information @mmd-osm, I hadn't considered relations membership! I've taken a quick look at the JOSM action, and I'll ask a question back on the original issue as I'm not sure if there are already any examples of actions in ID that require additional user input.

I recently added a lot of this logic to connect.js for #4921
I think the logic needed to disable the Detatch operation to prevent relation breakage might be similar (or simpler, hopefully)

iD/modules/actions/connect.js

Lines 60 to 211 in de12396

	action.disabled = function(graph) {
	var seen = {};
	var restrictionIDs = [];
	var survivor;
	var node, way;
	var relations, relation, role;
	var i, j, k;
	// Choose a survivor node, prefer an existing (not new) node - #4974
	for (i = 0; i < nodeIDs.length; i++) {
	survivor = graph.entity(nodeIDs[i]);
	if (survivor.version) break; // found one
	}
	// 1. disable if the nodes being connected have conflicting relation roles
	for (i = 0; i < nodeIDs.length; i++) {
	node = graph.entity(nodeIDs[i]);
	relations = graph.parentRelations(node);
	for (j = 0; j < relations.length; j++) {
	relation = relations[j];
	role = relation.memberById(node.id).role || '';
	// if this node is a via node in a restriction, remember for later
	if (relation.isValidRestriction()) {
	restrictionIDs.push(relation.id);
	}
	if (seen[relation.id] !== undefined && seen[relation.id] !== role) {
	return 'relation';
	} else {
	seen[relation.id] = role;
	}
	}
	}
	// gather restrictions for parent ways
	for (i = 0; i < nodeIDs.length; i++) {
	node = graph.entity(nodeIDs[i]);
	var parents = graph.parentWays(node);
	for (j = 0; j < parents.length; j++) {
	var parent = parents[j];
	relations = graph.parentRelations(parent);
	for (k = 0; k < relations.length; k++) {
	relation = relations[k];
	if (relation.isValidRestriction()) {
	restrictionIDs.push(relation.id);
	}
	}
	}
	}
	// test restrictions
	restrictionIDs = _uniq(restrictionIDs);
	for (i = 0; i < restrictionIDs.length; i++) {
	relation = graph.entity(restrictionIDs[i]);
	if (!relation.isComplete(graph)) continue;
	var memberWays = relation.members
	.filter(function(m) { return m.type === 'way'; })
	.map(function(m) { return graph.entity(m.id); });
	memberWays = _uniq(memberWays);
	var f = relation.memberByRole('from');
	var t = relation.memberByRole('to');
	var isUturn = (f.id === t.id);
	// 2a. disable if connection would damage a restriction
	// (a key node is a node at the junction of ways)
	var nodes = { from: [], via: [], to: [], keyfrom: [], keyto: [] };
	for (j = 0; j < relation.members.length; j++) {
	collectNodes(relation.members[j], nodes);
	}
	nodes.keyfrom = _uniq(nodes.keyfrom.filter(hasDuplicates));
	nodes.keyto = _uniq(nodes.keyto.filter(hasDuplicates));
	var filter = keyNodeFilter(nodes.keyfrom, nodes.keyto);
	nodes.from = nodes.from.filter(filter);
	nodes.via = nodes.via.filter(filter);
	nodes.to = nodes.to.filter(filter);
	var connectFrom = false;
	var connectVia = false;
	var connectTo = false;
	var connectKeyFrom = false;
	var connectKeyTo = false;
	for (j = 0; j < nodeIDs.length; j++) {
	var n = nodeIDs[j];
	if (nodes.from.indexOf(n) !== -1) { connectFrom = true; }
	if (nodes.via.indexOf(n) !== -1) { connectVia = true; }
	if (nodes.to.indexOf(n) !== -1) { connectTo = true; }
	if (nodes.keyfrom.indexOf(n) !== -1) { connectKeyFrom = true; }
	if (nodes.keyto.indexOf(n) !== -1) { connectKeyTo = true; }
	}
	if (connectFrom && connectTo && !isUturn) { return 'restriction'; }
	if (connectFrom && connectVia) { return 'restriction'; }
	if (connectTo && connectVia) { return 'restriction'; }
	// connecting to a key node -
	// if both nodes are on a member way (i.e. part of the turn restriction),
	// the connecting node must be adjacent to the key node.
	if (connectKeyFrom || connectKeyTo) {
	if (nodeIDs.length !== 2) { return 'restriction'; }
	var n0 = null;
	var n1 = null;
	for (j = 0; j < memberWays.length; j++) {
	way = memberWays[j];
	if (way.contains(nodeIDs[0])) { n0 = nodeIDs[0]; }
	if (way.contains(nodeIDs[1])) { n1 = nodeIDs[1]; }
	}
	if (n0 && n1) { // both nodes are part of the restriction
	var ok = false;
	for (j = 0; j < memberWays.length; j++) {
	way = memberWays[j];
	if (way.areAdjacent(n0, n1)) {
	ok = true;
	break;
	}
	}
	if (!ok) {
	return 'restriction';
	}
	}
	}
	// 2b. disable if nodes being connected will destroy a member way in a restriction
	// (to test, make a copy and try actually connecting the nodes)
	for (j = 0; j < memberWays.length; j++) {
	way = memberWays[j].update({}); // make copy
	for (k = 0; k < nodeIDs.length; k++) {
	if (nodeIDs[k] === survivor.id) continue;
	if (way.areAdjacent(nodeIDs[k], survivor.id)) {
	way = way.removeNode(nodeIDs[k]);
	} else {
	way = way.replaceNode(nodeIDs[k], survivor.id);
	}
	}
	if (way.isDegenerate()) {
	return 'restriction';
	}
	}
	}
	return false;

[Psigio]

Thanks @bhousel - I've done a few experiments using the logic you added in the actions/connect file (I've moved it out to a new util file so I can re-use it without repeating it), and it seems ok. My knowledge of turn restrictions is a bit basic, so I'll do a bit more reading up to see if I'm covering all the cases or whether a modified version of the logic is necessary for the detach node operation.

[Psigio]

I've done a bit more reading on restrictions at https://wiki.openstreetmap.org/wiki/Relation:restriction and I think that the code can be simplified from the connect.js logic. I've got an experimental branch which I'll work on for a bit to check it manages to prevent breaking the turn restrictions and once I've got some unit tests, I'll update the pull request.

[bhousel]

cool thanks @Psigio ! I forgot to comment yesterday - I'm not sure whether it makes sense to split out the logic into a new util file. I think it might depend on what the action is trying to do. But if you can find a way to make it generic , go for it!

[Psigio]

Hi,

I've done a bit more on this. The operation will now prevent the detach operation if the target node is a via or location_hint role within a restriction relationship. It won't prevent a node within a via way being detached as this shouldn't affect the restriction.

The action now also deals with other relationships too, it will swap the membership of the detached node to the new replacement one.

Hopefully this is a bit better now, happy to make any other changes that are necessary!

[bhousel]

Thanks again @Psigio - I just merged this!

I made a bunch of cleanups in 119792f, but all minor stuff:
- change keyboard shortcut to 'E' to not conflict with anything
- move disabled check from operation into action and simplify
- use actionMoveNode to place the detached node at the mouse cursor
- disable the operation if the node is connected to hidden features
- lots of code simplification
- make the icon more centered

Another cool thing with using actionMoveNode to put the detached node at the mouse cursor, is that this action can be performed in a transitioned way. It feels magic to be able to do this just by pressing 'E' ✨: