diff --git a/data/core.yaml b/data/core.yaml index d1d0b3290..292f6e658 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -116,6 +116,7 @@ en: line: Connected a way to a line. area: Connected a way to an area. relation: These features can't be connected because they have conflicting relation roles. + restriction: "These features can't be connected because it would damage a \"{relation}\" relation." disconnect: title: Disconnect description: Disconnect these lines/areas from each other. @@ -131,7 +132,7 @@ en: annotation: "Merged {n} features." not_eligible: These features can't be merged. not_adjacent: These features can't be merged because their endpoints aren't connected. - restriction: These features can't be merged because at least one is a member of a "{relation}" relation. + restriction: "These features can't be merged because at least one is a member of a \"{relation}\" relation." incomplete_relation: These features can't be merged because at least one hasn't been fully downloaded. conflicting_tags: These features can't be merged because some of their tags have conflicting values. move: diff --git a/dist/locales/en.json b/dist/locales/en.json index f537bbf01..a9a387d15 100644 --- a/dist/locales/en.json +++ b/dist/locales/en.json @@ -151,7 +151,8 @@ "line": "Connected a way to a line.", "area": "Connected a way to an area." }, - "relation": "These features can't be connected because they have conflicting relation roles." + "relation": "These features can't be connected because they have conflicting relation roles.", + "restriction": "These features can't be connected because it would damage a \"{relation}\" relation." }, "disconnect": { "title": "Disconnect", diff --git a/modules/actions/connect.js b/modules/actions/connect.js index 70800024d..7db528b22 100644 --- a/modules/actions/connect.js +++ b/modules/actions/connect.js @@ -1,3 +1,5 @@ +import _uniq from 'lodash-es/uniq'; + import { actionDeleteNode } from './delete_node'; @@ -15,7 +17,7 @@ import { actionDeleteNode } from './delete_node'; // https://github.com/openstreetmap/potlatch2/blob/master/net/systemeD/halcyon/connection/actions/MergeNodesAction.as // https://github.com/openstreetmap/josm/blob/mirror/src/org/openstreetmap/josm/actions/MergeNodesAction.java // -export function actionConnect(nodeIds) { +export function actionConnect(nodeIDs) { var action = function(graph) { var survivor; var node; @@ -23,14 +25,14 @@ export function actionConnect(nodeIds) { var i, j; // Choose a survivor node, prefer an existing (not new) node - #4974 - for (i = 0; i < nodeIds.length; i++) { - survivor = graph.entity(nodeIds[i]); + for (i = 0; i < nodeIDs.length; i++) { + survivor = graph.entity(nodeIDs[i]); if (survivor.version) break; // found one } // Replace all non-surviving nodes with the survivor and merge tags. - for (i = 0; i < nodeIds.length; i++) { - node = graph.entity(nodeIds[i]); + for (i = 0; i < nodeIDs.length; i++) { + node = graph.entity(nodeIDs[i]); if (node.id === survivor.id) continue; parents = graph.parentWays(node); @@ -57,27 +59,174 @@ export function actionConnect(nodeIds) { action.disabled = function(graph) { var seen = {}; + var restrictionIDs = []; + var survivor; + var node; + var relations, relation, role; + var i, j, k; - for (var i = 0; i < nodeIds.length; i++) { - var node = graph.entity(nodeIds[i]); + // 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 + } - var toCheck = [node].concat(graph.parentWays(node)); - for (var j = 0; j < toCheck.length; j++) { - var entity = toCheck[j]; + // 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); - var relations = graph.parentRelations(entity); - for (var k = 0; k < relations.length; k++) { - var relation = relations[k]; - var role = relation.memberById(entity.id).role || ''; + for (j = 0; j < relations.length; j++) { + relation = relations[j]; + role = relation.memberById(node.id).role || ''; - if (seen[relation.id] !== undefined && seen[relation.id] !== role) { - return 'relation'; - } else { - seen[relation.id] = role; + // if this node is a via node in a restriction, remember for later + if (relation.isRestriction()) { + 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.isRestriction()) { + restrictionIDs.push(relation.id); } } } } + + + // test restrictions + restrictionIDs = _uniq(restrictionIDs); + for (i = 0; i < restrictionIDs.length; i++) { + relation = graph.entity(restrictionIDs[i]); + + var memberWays = []; + for (j = 0; j < relation.members.length; j++) { + var m = relation.members[j]; + var entity = graph.hasEntity(m.id); + if (m.type === 'way') { + memberWays.push(entity); + } + } + + // 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 f = keyNodeFilter(nodes.keyfrom, nodes.keyto); + nodes.from = nodes.from.filter(f); + nodes.via = nodes.via.filter(f); + nodes.to = nodes.to.filter(f); + + 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) { return 'restriction'; } + if (connectFrom && connectVia) { return 'restriction'; } + if (connectTo && connectVia) { return 'restriction'; } + + // connecting to a key node - must be an adjacent node + if (connectKeyFrom || connectKeyTo) { + if (nodeIDs.length !== 2) { return 'restriction'; } + var ok = false; + for (j = 0; j < memberWays.length; j++) { + ok = memberWays[j].areAdjacent(nodeIDs[0], nodeIDs[1]); + if (ok) 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 removing non surviving nodes) + for (j = 0; j < memberWays.length; j++) { + var way = memberWays[j].update({}); + for (k = 0; k < nodeIDs.length; k++) { + if (nodeIDs[k] !== survivor.id) { + way = way.removeNode(nodeIDs[k]); + } + } + if (way.isDegenerate()) { + return 'restriction'; + } + } + } + + return false; + + + // if a key node appears multiple times (indexOf !== lastIndexOf) it's a FROM-VIA or TO-VIA junction + function hasDuplicates(n, i, arr) { + return arr.indexOf(n) !== arr.lastIndexOf(n); + } + + function keyNodeFilter(froms, tos) { + return function(n) { + return froms.indexOf(n) === -1 && tos.indexOf(n) === -1; + }; + } + + function collectNodes(member, collection) { + var entity = graph.hasEntity(member.id); + if (!entity) return; + + var role = member.role || ''; + if (!collection[role]) { + collection[role] = []; + } + + if (member.type === 'node') { + collection[role].push(member.id); + if (role === 'via') { + collection.keyfrom.push(member.id); + collection.keyto.push(member.id); + } + + } else if (member.type === 'way') { + collection[role].push.apply(collection[role], entity.nodes); + if (role === 'from' || role === 'via') { + collection.keyfrom.push(entity.first()); + collection.keyfrom.push(entity.last()); + } + if (role === 'to' || role === 'via') { + collection.keyto.push(entity.first()); + collection.keyto.push(entity.last()); + } + } + } }; diff --git a/test/spec/actions/connect.js b/test/spec/actions/connect.js index 59bad0ad1..88b37e947 100644 --- a/test/spec/actions/connect.js +++ b/test/spec/actions/connect.js @@ -174,98 +174,231 @@ describe('iD.actionConnect', function() { describe('#disabled', function () { - it('returns falsy when connecting members of the same relation and same roles (different ways)', function () { - // - // "Route 1" "Route 1" - // 'forward' 'forward' - // a ----- b c ===== d - // + it('returns falsy when connecting members of the same relation and same roles', function () { var graph = iD.coreGraph([ iD.osmNode({id: 'a'}), iD.osmNode({id: 'b'}), iD.osmNode({id: 'c'}), - iD.osmNode({id: 'd'}), - iD.osmWay({id: '-', nodes: ['a', 'b']}), - iD.osmWay({id: '=', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r1', tags: { name: 'Route 1'}, members: [ - { id: '-', type: 'way', role: 'forward' }, - { id: '=', type: 'way', role: 'forward' } + iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), + iD.osmRelation({id: 'r1', members: [ + { id: 'b', type: 'node', role: 'foo' }, + { id: 'c', type: 'node', role: 'foo' } ]}) ]); expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.be.not.ok; }); - it('returns falsy when connecting members of the different relation and different roles (different ways)', function () { - // - // "Route 1" "Route 2" - // 'forward' 'backward' - // a ----- b c ===== d - // + it('returns falsy when connecting members of the different relation and different roles', function () { var graph = iD.coreGraph([ iD.osmNode({id: 'a'}), iD.osmNode({id: 'b'}), iD.osmNode({id: 'c'}), - iD.osmNode({id: 'd'}), - iD.osmWay({id: '-', nodes: ['a', 'b']}), - iD.osmWay({id: '=', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r1', tags: { name: 'Route 1'}, members: [ - { id: '-', type: 'way', role: 'forward' } - ]}), - iD.osmRelation({id: 'r2', tags: { name: 'Route 2'}, members: [ - { id: '=', type: 'way', role: 'backward' } - ]}) + iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), + iD.osmRelation({id: 'r1', members: [{ id: 'b', type: 'node', role: 'foo' } ]}), + iD.osmRelation({id: 'r2', members: [{ id: 'c', type: 'node', role: 'bar' } ]}) ]); expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.be.not.ok; }); - it('returns \'relation\' when connecting members of the same relation but different roles (different ways)', function () { - // - // "Route 1" "Route 1" - // 'forward' 'backward' - // a ----- b c ===== d - // + it('returns \'relation\' when connecting members of the same relation but different roles', function () { var graph = iD.coreGraph([ iD.osmNode({id: 'a'}), iD.osmNode({id: 'b'}), iD.osmNode({id: 'c'}), - iD.osmNode({id: 'd'}), - iD.osmWay({id: '-', nodes: ['a', 'b']}), - iD.osmWay({id: '=', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r1', tags: { name: 'Route 1'}, members: [ - { id: '-', type: 'way', role: 'forward' }, - { id: '=', type: 'way', role: 'backward' } + iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), + iD.osmRelation({id: 'r1', members: [ + { id: 'b', type: 'node', role: 'foo' }, + { id: 'c', type: 'node', role: 'bar' } ]}) ]); expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.eql('relation'); }); - it('returns \'relation\' when connecting members of the same relation but different roles (joined way)', function () { + it('returns falsy when connecting nodes that would not break a via-node restriction', function () { // - // from via to - // a --- b === c ~~~ d + // a --- b --- c r1: `no_right_turn` + // | FROM '-' + // d VIA 'c' + // | TO '|' + // e // var graph = iD.coreGraph([ iD.osmNode({id: 'a'}), iD.osmNode({id: 'b'}), iD.osmNode({id: 'c'}), iD.osmNode({id: 'd'}), - iD.osmWay({id: '-', nodes: ['a', 'b']}), - iD.osmWay({id: '=', nodes: ['b', 'c']}), - iD.osmWay({id: '~', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r', members: [ + iD.osmNode({id: 'e'}), + iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), + iD.osmWay({id: '|', nodes: ['c', 'd', 'e']}), + iD.osmRelation({id: 'r1', tags: { type: 'restriction', restriction: 'no_right_turn' }, members: [ { id: '-', type: 'way', role: 'from' }, - { id: '=', type: 'way', role: 'via' }, - { id: '~', type: 'way', role: 'to' } + { id: 'c', type: 'node', role: 'via' }, + { id: '|', type: 'way', role: 'to' } ]}) ]); - expect(iD.actionConnect(['a', 'b']).disabled(graph)).to.eql('relation'); - expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.eql('relation'); - expect(iD.actionConnect(['c', 'd']).disabled(graph)).to.eql('relation'); + // allowed: adjacent connections that don't destroy a way + expect(iD.actionConnect(['a', 'b']).disabled(graph)).to.be.not.ok; + expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.be.not.ok; + expect(iD.actionConnect(['c', 'd']).disabled(graph)).to.be.not.ok; + expect(iD.actionConnect(['d', 'e']).disabled(graph)).to.be.not.ok; }); - }); + it('returns falsy when connecting nodes that would not break a via-way restriction', function () { + // + // a --- b --- c r1: `no_u_turn` + // | FROM '=' + // d VIA '|' + // | TO '-' + // g === f === e + // + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmNode({id: 'c'}), + iD.osmNode({id: 'd'}), + iD.osmNode({id: 'e'}), + iD.osmNode({id: 'f'}), + iD.osmNode({id: 'g'}), + iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), + iD.osmWay({id: '|', nodes: ['c', 'd', 'e']}), + iD.osmWay({id: '=', nodes: ['e', 'f', 'g']}), + iD.osmRelation({id: 'r1', tags: { type: 'restriction', restriction: 'no_u_turn' }, members: [ + { id: '=', type: 'way', role: 'from' }, + { id: '|', type: 'way', role: 'via' }, + { id: '-', type: 'way', role: 'to' } + ]}) + ]); + + // allowed: adjacent connections that don't destroy a way + expect(iD.actionConnect(['a', 'b']).disabled(graph)).to.be.not.ok; + expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.be.not.ok; + expect(iD.actionConnect(['c', 'd']).disabled(graph)).to.be.not.ok; + expect(iD.actionConnect(['d', 'e']).disabled(graph)).to.be.not.ok; + expect(iD.actionConnect(['e', 'f']).disabled(graph)).to.be.not.ok; + expect(iD.actionConnect(['f', 'g']).disabled(graph)).to.be.not.ok; + }); + + it('returns \'restriction\' when connecting nodes that would break a via-node restriction', function () { + // + // a --- b --- c r1: `no_right_turn` + // | FROM '-' + // d VIA 'c' + // | TO '|' + // e + // + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmNode({id: 'c'}), + iD.osmNode({id: 'd'}), + iD.osmNode({id: 'e'}), + iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), + iD.osmWay({id: '|', nodes: ['c', 'd', 'e']}), + iD.osmRelation({id: 'r1', tags: { type: 'restriction', restriction: 'no_right_turn' }, members: [ + { id: '-', type: 'way', role: 'from' }, + { id: 'c', type: 'node', role: 'via' }, + { id: '|', type: 'way', role: 'to' } + ]}) + ]); + + // prevented: + // extra connections to the VIA node, or any connections between FROM and TO + expect(iD.actionConnect(['a', 'c']).disabled(graph)).to.eql('restriction', 'extra connection FROM-VIA'); + expect(iD.actionConnect(['e', 'c']).disabled(graph)).to.eql('restriction', 'extra connection TO-VIA'); + expect(iD.actionConnect(['b', 'd']).disabled(graph)).to.eql('restriction', 'extra connection FROM-TO'); + }); + + it('returns \'restriction\' when connecting nodes that would break a via-way restriction', function () { + // + // a --- b --- c r1: `no_u_turn` + // | FROM '=' + // d VIA '|' + // | TO '-' + // g === f === e + // + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmNode({id: 'c'}), + iD.osmNode({id: 'd'}), + iD.osmNode({id: 'e'}), + iD.osmNode({id: 'f'}), + iD.osmNode({id: 'g'}), + iD.osmWay({id: '-', nodes: ['a', 'b', 'c']}), + iD.osmWay({id: '|', nodes: ['c', 'd', 'e']}), + iD.osmWay({id: '=', nodes: ['e', 'f', 'g']}), + iD.osmRelation({id: 'r1', tags: { type: 'restriction', restriction: 'no_u_turn' }, members: [ + { id: '=', type: 'way', role: 'from' }, + { id: '|', type: 'way', role: 'via' }, + { id: '-', type: 'way', role: 'to' } + ]}) + ]); + + // prevented: + // extra connections to any node along VIA way + expect(iD.actionConnect(['a', 'c']).disabled(graph)).to.eql('restriction', 'extra connection TO-VIA c'); + expect(iD.actionConnect(['b', 'd']).disabled(graph)).to.eql('restriction', 'extra connection TO-VIA d'); + expect(iD.actionConnect(['b', 'e']).disabled(graph)).to.eql('restriction', 'extra connection TO-VIA e'); + expect(iD.actionConnect(['c', 'e']).disabled(graph)).to.eql('restriction', 'extra connection VIA-VIA'); + expect(iD.actionConnect(['f', 'c']).disabled(graph)).to.eql('restriction', 'extra connection FROM-VIA c'); + expect(iD.actionConnect(['f', 'd']).disabled(graph)).to.eql('restriction', 'extra connection FROM-VIA d'); + expect(iD.actionConnect(['g', 'e']).disabled(graph)).to.eql('restriction', 'extra connection FROM-VIA e'); + }); + + it('returns \'restriction\' when connecting would destroy a way in a via-node restriction', function () { + // + // a --- b r1: `no_right_turn` + // | FROM '-' + // | VIA 'b' + // c TO '|' + // + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmNode({id: 'c'}), + iD.osmWay({id: '-', nodes: ['a', 'b']}), + iD.osmWay({id: '|', nodes: ['b', 'c']}), + iD.osmRelation({id: 'r1', tags: { type: 'restriction', restriction: 'no_right_turn' }, members: [ + { id: '-', type: 'way', role: 'from' }, + { id: 'b', type: 'node', role: 'via' }, + { id: '|', type: 'way', role: 'to' } + ]}) + ]); + + expect(iD.actionConnect(['a', 'b']).disabled(graph)).to.eql('restriction', 'destroy FROM'); + expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.eql('restriction', 'destroy TO'); + }); + + it('returns \'restriction\' when connecting would destroy a way in via-way restriction', function () { + // + // a --- b r1: `no_u_turn` + // | FROM '=' + // | VIA '|' + // d === c TO '-' + // + var graph = iD.coreGraph([ + iD.osmNode({id: 'a'}), + iD.osmNode({id: 'b'}), + iD.osmNode({id: 'c'}), + iD.osmNode({id: 'd'}), + iD.osmWay({id: '-', nodes: ['a', 'b']}), + iD.osmWay({id: '|', nodes: ['b', 'c']}), + iD.osmWay({id: '=', nodes: ['c', 'd']}), + iD.osmRelation({id: 'r1', tags: { type: 'restriction', restriction: 'no_u_turn' }, members: [ + { id: '=', type: 'way', role: 'from' }, + { id: '|', type: 'way', role: 'via' }, + { id: '-', type: 'way', role: 'to' } + ]}) + ]); + + expect(iD.actionConnect(['a', 'b']).disabled(graph)).to.eql('restriction', 'destroy TO'); + expect(iD.actionConnect(['b', 'c']).disabled(graph)).to.eql('restriction', 'destroy VIA'); + expect(iD.actionConnect(['c', 'd']).disabled(graph)).to.eql('restriction', 'destroy FROM'); + }); + + }); });