diff --git a/modules/actions/add_member.js b/modules/actions/add_member.js index d5f5b26b7..ab15f343c 100644 --- a/modules/actions/add_member.js +++ b/modules/actions/add_member.js @@ -1,49 +1,105 @@ -import { osmJoinWays } from '../osm'; +import _groupBy from 'lodash-es/groupBy'; + +import { osmJoinWays, osmWay } from '../osm'; -export function actionAddMember(relationId, member, memberIndex, insertHint) { +export function actionAddMember(relationId, member, memberIndex, insertPair) { + + // Relation.replaceMember() removes duplicates, and we don't want that.. #4696 + function replaceMemberAll(relation, needleID, replacement) { + var members = []; + for (var i = 0; i < relation.members.length; i++) { + var member = relation.members[i]; + if (member.id !== needleID) { + members.push(member); + } else { + members.push({id: replacement.id, type: replacement.type, role: member.role}); + } + } + return relation.update({members: members}); + } + var action = function(graph) { var relation = graph.entity(relationId); - var numAdded = 0; - // If we weren't passed a memberIndex, - // try to perform sensible inserts based on how the ways join together - if ((isNaN(memberIndex) || insertHint) && member.type === 'way') { - var members = relation.indexedMembers(); - if (!insertHint) { - members.push(member); // just push and let osmJoinWays sort it out - } - - var joined = osmJoinWays(members, graph, insertHint); - - for (var i = 0; i < joined.length; i++) { - var segment = joined[i]; - for (var j = 0; j < segment.length && segment.length >= 2; j++) { - if (segment[j] !== member) - continue; - - if (j === 0) { - memberIndex = segment[j + 1].index; - } else if (j === segment.length - 1) { - memberIndex = segment[j - 1].index + 1; - } else { - memberIndex = Math.min(segment[j - 1].index + 1, segment[j + 1].index + 1); - } - - relation = relation.addMember(member, memberIndex + (numAdded++)); - } - } + if ((isNaN(memberIndex) || insertPair) && member.type === 'way') { + // Try to perform sensible inserts based on how the ways join together + graph = addWayMember(relation, graph); + } else { + graph = graph.replace(relation.addMember(member, memberIndex)); } - // By default, add at index (or append to end if index undefined) - if (!numAdded) { - relation = relation.addMember(member, memberIndex); - } - - return graph.replace(relation); + return graph; }; + // Add a way member into the relation "wherever it makes sense". + // In this situation we were not supplied a memberIndex. + function addWayMember(relation, graph) { + var groups; + var tempWay; + var i, j; + + if (insertPair) { + // We're adding a member that must stay paired with an existing member. + // (This feature is used by `actionSplit`) + // + // This is tricky because the members may exist multiple times in the + // member list, and with different A-B/B-A ordering and different roles. + // (e.g. a bus route that loops out and back - #4589). + // + // Replace the existing member with a temporary way, + // so that `osmJoinWays` can treat the pair like a single way. + tempWay = osmWay({ id: 'wTemp', nodes: insertPair.nodes }); + graph = graph.replace(tempWay); + var tempMember = { id: tempWay.id, type: 'way', role: '' }; + var tempRelation = replaceMemberAll(relation, insertPair.originalID, tempMember); + groups = _groupBy(tempRelation.members, function(m) { return m.type; }); + groups.way = groups.way || []; + + } else { + // Add the member anywhere.. Just push and let `osmJoinWays` decide where to put it. + groups = _groupBy(relation.members, function(m) { return m.type; }); + groups.way = groups.way || []; + groups.way.push(member); + } + + var joined = osmJoinWays(groups.way, graph); + + var newWayMembers = []; + for (i = 0; i < joined.length; i++) { + var segment = joined[i]; + var nodes = segment.nodes.slice(); + + for (j = 0; j < segment.length; j++) { + var way = graph.entity(segment[j].id); + if (tempWay && segment[j].id === tempWay.id) { + if (nodes[0].id === insertPair.nodes[0]) { + newWayMembers.push({ id: insertPair.originalID, type: 'way', role: segment[j].role }); + newWayMembers.push({ id: insertPair.insertedID, type: 'way', role: segment[j].role }); + } else { + newWayMembers.push({ id: insertPair.insertedID, type: 'way', role: segment[j].role }); + newWayMembers.push({ id: insertPair.originalID, type: 'way', role: segment[j].role }); + } + } else { + newWayMembers.push(segment[j]); + } + nodes.splice(0, way.nodes.length - 1); + } + } + + if (tempWay) { + graph = graph.remove(tempWay); + } + + // Write members in the order: nodes, ways, relations + // This is reccomended for Public Transport routes: + // see https://wiki.openstreetmap.org/wiki/Public_transport#Service_routes + var newMembers = (groups.node || []).concat(newWayMembers, (groups.relation || [])); + return graph.replace(relation.update({members: newMembers})); + } + + return action; } diff --git a/modules/actions/split.js b/modules/actions/split.js index a9d0bd77f..1763274ac 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -85,6 +85,7 @@ export function actionSplit(nodeId, newWayIds) { function split(graph, wayA, newWayId) { var wayB = osmWay({id: newWayId, tags: wayA.tags}); + var origNodes = wayA.nodes.slice(); var nodesA; var nodesB; var isArea = wayA.isArea(); @@ -134,12 +135,13 @@ export function actionSplit(nodeId, newWayIds) { role: relation.memberById(wayA.id).role }; - var insertHint = { - item: member, - nextTo: wayA.id + var insertPair = { + originalID: wayA.id, + insertedID: wayB.id, + nodes: origNodes }; - graph = actionAddMember(relation.id, member, undefined, insertHint)(graph); + graph = actionAddMember(relation.id, member, undefined, insertPair)(graph); } }); diff --git a/modules/osm/multipolygon.js b/modules/osm/multipolygon.js index 0990e3e30..41e61a5ae 100644 --- a/modules/osm/multipolygon.js +++ b/modules/osm/multipolygon.js @@ -88,12 +88,7 @@ export function osmSimpleMultipolygonOuterMember(entity, graph) { // Incomplete members (those for which `graph.hasEntity(element.id)` returns // false) and non-way members are ignored. // -// `insertHint` is an optional object. -// If supplied, insert the given way/member next to an existing way/member: -// `{ item: wayOrMember, nextTo: id }` -// (This feature is used by `actionSplit`) -// -export function osmJoinWays(toJoin, graph, insertHint) { +export function osmJoinWays(toJoin, graph) { function resolve(member) { return graph.childNodes(graph.entity(member.id)); } @@ -109,6 +104,7 @@ export function osmJoinWays(toJoin, graph, insertHint) { return member.type === 'way' && graph.hasEntity(member.id); }); + var sequences = []; sequences.actions = []; @@ -118,7 +114,6 @@ export function osmJoinWays(toJoin, graph, insertHint) { var currWays = [item]; var currNodes = resolve(item).slice(); var doneSequence = false; - var isInserting = false; // add to it while (toJoin.length && !doneSequence) { @@ -129,19 +124,8 @@ export function osmJoinWays(toJoin, graph, insertHint) { var i; // Find the next way/member to join. - // If it is time to attempt an insert, try that item first. - // Otherwise, search for a next item in `toJoin` - var toCheck; - if (!isInserting && insertHint && insertHint.nextTo === currWays[currWays.length - 1].id) { - toCheck = [insertHint.item]; - isInserting = true; - } else { - toCheck = toJoin.slice(); - isInserting = false; - } - - for (i = 0; i < toCheck.length; i++) { - item = toCheck[i]; + for (i = 0; i < toJoin.length; i++) { + item = toJoin[i]; nodes = resolve(item); // Strongly prefer to generate a forward path that preserves the order @@ -181,22 +165,15 @@ export function osmJoinWays(toJoin, graph, insertHint) { } } - if (nodes) { // we found something to join - fn.apply(currWays, [item]); - fn.apply(currNodes, nodes); - - if (!isInserting) { - toJoin.splice(i, 1); - } - - } else { // couldn't find a joinable way/member - // if inserting, restart the loop and look in `toJoin` next time. - // if not inserting, there is nowhere else to look, mark as done. - if (!isInserting) { - doneSequence = true; - break; - } + if (!nodes) { // couldn't find a joinable way/member + doneSequence = true; + break; } + + fn.apply(currWays, [item]); + fn.apply(currNodes, nodes); + + toJoin.splice(i, 1); } currWays.nodes = currNodes; diff --git a/test/spec/actions/add_member.js b/test/spec/actions/add_member.js index 863045849..3f7895097 100644 --- a/test/spec/actions/add_member.js +++ b/test/spec/actions/add_member.js @@ -101,7 +101,7 @@ describe('iD.actionAddMember', function() { expect(members(graph)).to.eql(['-', '=', '~']); }); - it('inserts the member multiple times if hint provided (middle)', function() { + it('inserts the member multiple times if insertPair provided (middle)', function() { // Before: a ---> b .. c ~~~> d <~~~ c .. b <--- a // After: a ---> b ===> c ~~~> d <~~~ c <=== b <--- a var graph = iD.coreGraph([ @@ -121,21 +121,25 @@ describe('iD.actionAddMember', function() { ]); var member = { id: '=', type: 'way' }; - var hint = { item: member, nextTo: '-' }; - graph = iD.actionAddMember('r', member, undefined, hint)(graph); + var insertPair = { + originalID: '-', + insertedID: '=', + nodes: ['a','b','c'] + }; + graph = iD.actionAddMember('r', member, undefined, insertPair)(graph); expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); }); - it('inserts the member multiple times if hint provided (beginning/end)', function() { - // Before: b ===> c ~~~> d <~~~ c <=== b - // After: a ---> b ===> c ~~~> d <~~~ c <=== b <--- a + it('inserts the member multiple times if insertPair provided (beginning/end)', function() { + // Before: b <=== c ~~~> d <~~~ c ===> b + // After: a <--- b <=== c ~~~> d <~~~ c ===> b ---> a var graph = iD.coreGraph([ iD.osmNode({id: 'a', loc: [0, 0]}), iD.osmNode({id: 'b', loc: [0, 0]}), iD.osmNode({id: 'c', loc: [0, 0]}), iD.osmNode({id: 'd', loc: [0, 0]}), - iD.osmWay({id: '-', nodes: ['a', 'b']}), - iD.osmWay({id: '=', nodes: ['b', 'c']}), + iD.osmWay({id: '-', nodes: ['b', 'a']}), + iD.osmWay({id: '=', nodes: ['c', 'b']}), iD.osmWay({id: '~', nodes: ['c', 'd']}), iD.osmRelation({id: 'r', members: [ {id: '=', type: 'way'}, @@ -146,8 +150,12 @@ describe('iD.actionAddMember', function() { ]); var member = { id: '-', type: 'way' }; - var hint = { item: member, nextTo: '=' }; - graph = iD.actionAddMember('r', member, undefined, hint)(graph); + var insertPair = { + originalID: '=', + insertedID: '-', + nodes: ['c','b','a'] + }; + graph = iD.actionAddMember('r', member, undefined, insertPair)(graph); expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); });