From 6ed20d776221b6a530c6d00b66d1c87fbc513332 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Wed, 25 Apr 2018 22:26:34 -0400 Subject: [PATCH] actionMergeNodes can just use actionConnect --- modules/actions/merge_nodes.js | 77 +++++++++++++--------------- test/spec/actions/merge_nodes.js | 88 ++++++++++++++++++++++---------- 2 files changed, 95 insertions(+), 70 deletions(-) diff --git a/modules/actions/merge_nodes.js b/modules/actions/merge_nodes.js index f26865d85..8ed4fbbaa 100644 --- a/modules/actions/merge_nodes.js +++ b/modules/actions/merge_nodes.js @@ -1,55 +1,48 @@ -import _sum from 'lodash-es/sum'; -import _extend from 'lodash-es/extend'; - -import { osmNode } from '../osm/node'; +import { actionConnect } from './connect'; +import { geoVecAdd, geoVecScale } from '../geo'; -export function actionMergeNodes(ids) { - function getSelectedEntities(graph) { - return ids.map(function(id) { return graph.entity(id); }); +// `actionMergeNodes` is just a combination of: +// +// 1. move all the nodes to a common location +// 2. `actionConnect` them + +export function actionMergeNodes(nodeIDs) { + + // If there is a single "interesting" node, use that as the location. + // Otherwise return the average location of all the nodes. + function chooseLoc(graph) { + if (!nodeIDs.length) return null; + var sum = [0,0]; + var interestingCount = 0; + var interestingLoc; + + for (var i = 0; i < nodeIDs.length; i++) { + var node = graph.entity(nodeIDs[i]); + if (node.hasInterestingTags()) { + interestingLoc = (++interestingCount === 1) ? node.loc : null; + } + sum = geoVecAdd(sum, node.loc); + } + + return interestingLoc || geoVecScale(sum, 1 / nodeIDs.length); } - function calcAverageLoc(nodes) { - return [ - _sum(nodes.map(function(node) { return node.loc[0]; })) / nodes.length, - _sum(nodes.map(function(node) { return node.loc[1]; })) / nodes.length - ]; - } - - function collectTags(entities) { - return entities.reduce(function(tags, entity) { return _extend(tags, entity.tags); }, {}); - } - - function replaceWithinWays(newNode) { - return function (graph, node) { - return graph.parentWays(node).reduce(function (graph, way) { - return graph.replace(way.replaceNode(node.id, newNode.id)); - }, graph); - }; - } - - function removeFromGraph(graph, entity) { - return graph.remove(entity); - } var action = function(graph) { - var nodes = getSelectedEntities(graph), - newNode = new osmNode({ id: newNodeId, loc: calcAverageLoc(nodes), tags: collectTags(nodes) }); + var toLoc = chooseLoc(graph); - graph = graph.replace(newNode); - graph = nodes.reduce(replaceWithinWays(newNode), graph); - graph = nodes.reduce(removeFromGraph, graph); - return graph; + for (var i = 0; i < nodeIDs.length; i++) { + var node = graph.entity(nodeIDs[i]); + graph = graph.replace(node.move(toLoc)); + } + + return actionConnect(nodeIDs)(graph); }; + action.disabled = function (graph) { - function isNotWayNode (entity) { return entity.type !== 'node' || graph.parentWays(entity) <= 0; } - - var entities = getSelectedEntities(graph); - - if (entities.some(isNotWayNode)) { - return 'not_eligible'; - } + return actionConnect(nodeIDs).disabled(graph); }; return action; diff --git a/test/spec/actions/merge_nodes.js b/test/spec/actions/merge_nodes.js index 48b53771c..5bde52beb 100644 --- a/test/spec/actions/merge_nodes.js +++ b/test/spec/actions/merge_nodes.js @@ -1,9 +1,5 @@ describe('iD.actionMergeNodes', function () { - function has(arr, findVal) { - return arr.some(function(val) { return val === findVal; }); - } - describe('#disabled', function () { it('enabled for both internal and endpoint nodes', function() { // @@ -28,6 +24,54 @@ describe('iD.actionMergeNodes', function () { }); + it('merges two isolated nodes, averaging loc', function() { + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [4, 4] }) + ]); + + graph = iD.actionMergeNodes(['a', 'b'])(graph); + + expect(graph.hasEntity('a')).to.be.undefined; + + var survivor = graph.hasEntity('b'); + expect(survivor).to.be.an.instanceof(iD.osmNode); + expect(survivor.loc).to.eql([2, 2], 'average loc'); + }); + + + it('merges two isolated nodes, merging tags, and keeping loc of the interesting node', function() { + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0], tags: { highway: 'traffic_signals' }}), + iD.osmNode({ id: 'b', loc: [4, 4] }) + ]); + + graph = iD.actionMergeNodes(['a', 'b'])(graph); + + expect(graph.hasEntity('a')).to.be.undefined; + + var survivor = graph.hasEntity('b'); + expect(survivor).to.be.an.instanceof(iD.osmNode); + expect(survivor.tags).to.eql({ highway: 'traffic_signals' }, 'merge all tags'); + expect(survivor.loc).to.eql([0, 0], 'use loc of interesting node'); + }); + + + it('merges two isolated nodes, merging tags, and averaging loc of both interesting nodes', function() { + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, -2], tags: { highway: 'traffic_signals' } }), + iD.osmNode({ id: 'b', loc: [0, 2], tags: { crossing: 'zebra' } }) + ]); + graph = iD.actionMergeNodes(['a', 'b'])(graph); + + expect(graph.hasEntity('a')).to.be.undefined; + + var survivor = graph.hasEntity('b'); + expect(survivor.tags).to.eql({ highway: 'traffic_signals', crossing: 'zebra' }, 'merge all tags'); + expect(survivor.loc).to.eql([0, 0], 'average loc of both interesting nodes'); + }); + + it('merges two nodes along a single way', function() { // // scenerio: merge b,c: @@ -45,10 +89,10 @@ describe('iD.actionMergeNodes', function () { expect(graph.hasEntity('b')).to.be.undefined; - var c = graph.hasEntity('c'); - expect(c).to.be.an.instanceof(iD.osmNode); - expect(c.loc).to.eql([1, 2]); - expect(graph.parentWays(c).length).to.equal(1); + var survivor = graph.hasEntity('c'); + expect(survivor).to.be.an.instanceof(iD.osmNode); + expect(survivor.loc).to.eql([1, 2]); + expect(graph.parentWays(survivor).length).to.equal(1); }); @@ -76,10 +120,10 @@ describe('iD.actionMergeNodes', function () { expect(graph.hasEntity('b')).to.be.undefined; - var d = graph.hasEntity('d'); - expect(d).to.be.an.instanceof(iD.osmNode); - expect(d.loc).to.eql([0, 1]); - expect(graph.parentWays(d).length).to.equal(2); + var survivor = graph.hasEntity('d'); + expect(survivor).to.be.an.instanceof(iD.osmNode); + expect(survivor.loc).to.eql([0, 1]); + expect(graph.parentWays(survivor).length).to.equal(2); }); @@ -114,22 +158,10 @@ describe('iD.actionMergeNodes', function () { expect(graph.hasEntity('b')).to.be.undefined; expect(graph.hasEntity('d')).to.be.undefined; - var d = graph.hasEntity('e'); - expect(d).to.be.an.instanceof(iD.osmNode); - expect(d.loc).to.eql([0, 2]); - expect(graph.parentWays(d).length).to.equal(3); + var survivor = graph.hasEntity('e'); + expect(survivor).to.be.an.instanceof(iD.osmNode); + expect(survivor.loc).to.eql([0, 0]); + expect(graph.parentWays(survivor).length).to.equal(3); }); - - it('merges tags of nodes', function() { - var graph = iD.coreGraph([ - iD.osmNode({ id: 'a', loc: [0, -2], tags: { highway: 'traffic_signals' } }), - iD.osmNode({ id: 'b', loc: [0, 2], tags: { crossing: 'zebra' } }) - ]); - graph = iD.actionMergeNodes(['a', 'b'])(graph); - - var b = graph.hasEntity('b'); - expect(b.loc).to.eql([0, 0]); - expect(b.tags).to.eql({ highway: 'traffic_signals', crossing: 'zebra' }); - }); });