From 21f171863cb4caef002fa85c3fe8db051eef57ca Mon Sep 17 00:00:00 2001 From: Thomas Petillon Date: Sat, 11 Apr 2020 16:26:17 +0200 Subject: [PATCH] Keep the oldest IDs alive when merging nodes into a way --- modules/actions/merge.js | 73 ++++++++++++++++++----- test/spec/actions/merge.js | 117 +++++++++++++++++++++++++++++++++---- 2 files changed, 164 insertions(+), 26 deletions(-) diff --git a/modules/actions/merge.js b/modules/actions/merge.js index d44cfdc18..70fc6fca3 100644 --- a/modules/actions/merge.js +++ b/modules/actions/merge.js @@ -1,5 +1,6 @@ +import { osmEntity } from '../osm'; import { osmTagSuggestingArea } from '../osm/tags'; -import { utilArrayGroupBy, utilArrayUniq } from '../util'; +import { utilArrayGroupBy, utilArrayUniq, utilCompareIDs } from '../util'; export function actionMerge(ids) { @@ -29,21 +30,65 @@ export function actionMerge(ids) { var nodes = utilArrayUniq(graph.childNodes(target)); var removeNode = point; - for (var i = 0; i < nodes.length; i++) { - var node = nodes[i]; - if (graph.parentWays(node).length > 1 || - graph.parentRelations(node).length || - node.hasInterestingTags()) { - continue; + if (!point.isNew()) { + // Try to preserve the original point if it already has + // an ID in the database. + + var inserted = false; + + var canBeReplaced = function(node) { + return !(graph.parentWays(node).length > 1 || + graph.parentRelations(node).length); + }; + + var replaceNode = function(node) { + graph = graph.replace(point.update({ tags: node.tags, loc: node.loc })); + target = target.replaceNode(node.id, point.id); + graph = graph.replace(target); + removeNode = node; + inserted = true; + }; + + var i; + var node; + + // First, try to replace a new child node on the target way. + for (i = 0; i < nodes.length; i++) { + node = nodes[i]; + if (canBeReplaced(node) && node.isNew()) { + replaceNode(node); + break; + } } - // Found an uninteresting child node on the target way. - // Move orig point into its place to preserve point's history. #3683 - graph = graph.replace(point.update({ tags: {}, loc: node.loc })); - target = target.replaceNode(node.id, point.id); - graph = graph.replace(target); - removeNode = node; - break; + if (!inserted && point.hasInterestingTags()) { + // No new child node found, try to find an existing, but + // uninteresting child node instead. + for (i = 0; i < nodes.length; i++) { + node = nodes[i]; + if (canBeReplaced(node) && + !node.hasInterestingTags()) { + replaceNode(node); + break; + } + } + + if (!inserted) { + // Still not inserted, try to find an existing, interesting, + // but more recent child node. + for (i = 0; i < nodes.length; i++) { + node = nodes[i]; + if (canBeReplaced(node) && + utilCompareIDs(point.id, node.id) < 0) { + replaceNode(node); + break; + } + } + } + + // If the point still hasn't been inserted, we give up. + // There are more interesting or older nodes on the way. + } } graph = graph.remove(removeNode); diff --git a/test/spec/actions/merge.js b/test/spec/actions/merge.js index 0c80063ea..c0e407c68 100644 --- a/test/spec/actions/merge.js +++ b/test/spec/actions/merge.js @@ -37,22 +37,115 @@ describe('iD.actionMerge', function () { expect(graph.entity('r').members).to.eql([{id: 'w', role: 'r', type: 'way'}]); }); - it('preserves original point if possible', function () { + it('preserves existing point id when possible', function () { var graph = iD.coreGraph([ - iD.osmNode({id: 'a', loc: [1, 0], tags: {a: 'a'}}), - iD.osmNode({id: 'p', loc: [0, 0], tags: {p: 'p'}}), - iD.osmNode({id: 'q', loc: [0, 1]}), - iD.osmWay({id: 'w', nodes: ['p', 'q'], tags: {w: 'w'}}) + iD.osmNode({id: 'n1', loc: [1, 0], tags: {n1: 'n1'}}), + iD.osmNode({id: 'a', loc: [0, 0], tags: {a: 'a'}}), + iD.osmNode({id: 'b', loc: [0, 1]}), + iD.osmWay({id: 'w', nodes: ['a', 'b'], tags: {w: 'w'}}) ]), - action = iD.actionMerge(['a', 'w']); + action = iD.actionMerge(['n1', 'w']); graph = action(graph); - expect(graph.hasEntity('a')).to.be.ok; - expect(graph.hasEntity('p')).to.be.ok; - expect(graph.hasEntity('q')).to.be.undefined; + expect(graph.hasEntity('n1')).to.be.ok; + expect(graph.hasEntity('a')).to.be.undefined; + expect(graph.hasEntity('b')).to.be.ok; + expect(graph.entity('w').tags).to.eql({n1: 'n1', w: 'w'}); + expect(graph.entity('w').nodes).to.eql(['n1', 'b']); + expect(graph.entity('n1').loc[0]).to.eql(0); + expect(graph.entity('n1').loc[1]).to.eql(0); + }); + + it('preserves existing point ids when possible', function () { + var graph = iD.coreGraph([ + iD.osmNode({id: 'n1', loc: [1, 0], tags: {n1: 'n1'}}), + iD.osmNode({id: 'n2', loc: [2, 0], tags: {n2: 'n2'}}), + iD.osmNode({id: 'a', loc: [0, 1]}), + iD.osmNode({id: 'b', loc: [0, 2], tags: {b: 'b'}}), + iD.osmNode({id: 'c', loc: [0, 3]}), + iD.osmWay({id: 'w', nodes: ['a', 'b', 'c'], tags: {w: 'w'}}) + ]), + action = iD.actionMerge(['n1', 'n2', 'w']); + + graph = action(graph); + expect(graph.hasEntity('n1')).to.be.ok; + expect(graph.hasEntity('n2')).to.be.ok; + expect(graph.hasEntity('a')).to.be.undefined; + expect(graph.hasEntity('b')).to.be.undefined; + expect(graph.hasEntity('c')).to.be.ok; + expect(graph.entity('n2').tags).to.eql({b: 'b'}); + expect(graph.entity('w').tags).to.eql({n1: 'n1', n2: 'n2', w: 'w'}); + expect(graph.entity('w').nodes).to.eql(['n1', 'n2', 'c']); + expect(graph.entity('n1').loc[0]).to.eql(0); + expect(graph.entity('n1').loc[1]).to.eql(1); + expect(graph.entity('n2').loc[0]).to.eql(0); + expect(graph.entity('n2').loc[1]).to.eql(2); + }); + + it('preserves existing node ids when possible', function () { + var graph = iD.coreGraph([ + iD.osmNode({id: 'a', loc: [1, 0], tags: {a: 'a'}}), + iD.osmNode({id: 'b', loc: [2, 0]}), + iD.osmNode({id: 'n1', loc: [0, 1]}), + iD.osmNode({id: 'n2', loc: [0, 2], tags: {n2: 'n2'}}), + iD.osmWay({id: 'w', nodes: ['n1', 'n2'], tags: {w: 'w'}}) + ]), + action = iD.actionMerge(['a', 'b', 'w']); + + graph = action(graph); + expect(graph.hasEntity('a')).to.be.undefined; + expect(graph.hasEntity('b')).to.be.undefined; + expect(graph.hasEntity('n1')).to.be.ok; + expect(graph.hasEntity('n2')).to.be.ok; expect(graph.entity('w').tags).to.eql({a: 'a', w: 'w'}); - expect(graph.entity('w').nodes).to.eql(['p', 'a']); - expect(graph.entity('a').loc[0]).to.eql(0); - expect(graph.entity('a').loc[1]).to.eql(1); + expect(graph.entity('w').nodes).to.eql(['n1', 'n2']); + expect(graph.entity('n1').loc[0]).to.eql(0); + expect(graph.entity('n1').loc[1]).to.eql(1); + expect(graph.entity('n2').loc[0]).to.eql(0); + expect(graph.entity('n2').loc[1]).to.eql(2); + }); + + it('preserves interesting existing node ids when possible', function () { + var graph = iD.coreGraph([ + iD.osmNode({id: 'n1', loc: [1, 0], tags: {n1: 'n1'}}), + iD.osmNode({id: 'n2', loc: [0, 1], tags: {n2: 'n2'}}), + iD.osmNode({id: 'n3', loc: [0, 2]}), + iD.osmWay({id: 'w', nodes: ['n2', 'n3'], tags: {w: 'w'}}) + ]), + action = iD.actionMerge(['n1', 'w']); + + graph = action(graph); + expect(graph.hasEntity('n1')).to.be.ok; + expect(graph.hasEntity('n2')).to.be.ok; + expect(graph.hasEntity('n3')).to.be.undefined; + expect(graph.entity('w').tags).to.eql({n1: 'n1', w: 'w'}); + expect(graph.entity('w').nodes).to.eql(['n2', 'n1']); + expect(graph.entity('n1').loc[0]).to.eql(0); + expect(graph.entity('n1').loc[1]).to.eql(2); + }); + + it('preserves oldest interesting existing node ids', function () { + var graph = iD.coreGraph([ + iD.osmNode({id: 'n3', loc: [1, 0], tags: {n3: 'n3'}}), + iD.osmNode({id: 'n6', loc: [2, 0], tags: {n6: 'n6'}}), + iD.osmNode({id: 'n2', loc: [0, 1], tags: {n2: 'n2'}}), + iD.osmNode({id: 'n5', loc: [0, 2], tags: {n5: 'n5'}}), + iD.osmNode({id: 'n1', loc: [0, 3], tags: {n1: 'n1'}}), + iD.osmNode({id: 'n4', loc: [0, 4], tags: {n4: 'n4'}}), + iD.osmWay({id: 'w', nodes: ['n2', 'n5', 'n1', 'n4'], tags: {w: 'w'}}) + ]), + action = iD.actionMerge(['n3', 'n6', 'w']); + + graph = action(graph); + expect(graph.hasEntity('n1')).to.be.ok; + expect(graph.hasEntity('n2')).to.be.ok; + expect(graph.hasEntity('n3')).to.be.ok; + expect(graph.hasEntity('n4')).to.be.ok; + expect(graph.hasEntity('n5')).to.be.undefined; + expect(graph.hasEntity('n6')).to.be.undefined; + expect(graph.entity('w').tags).to.eql({n3: 'n3', n6: 'n6', w: 'w'}); + expect(graph.entity('w').nodes).to.eql(['n2', 'n3', 'n1', 'n4']); + expect(graph.entity('n3').loc[0]).to.eql(0); + expect(graph.entity('n3').loc[1]).to.eql(2); }); });