Fixes for joining and splitting bugs

modules/actions/add_member.js

@@ -1,32 +1,175 @@
-import { osmJoinWays } from '../osm';
+import _clone from 'lodash-es/clone';
+import _groupBy from 'lodash-es/groupBy';
+import _omit from 'lodash-es/omit';
 
+import { osmJoinWays, osmWay } from '../osm';
 
-export function actionAddMember(relationId, member, memberIndex) {
-    return function(graph) {
+
+export function actionAddMember(relationId, member, memberIndex, insertPair) {
+
+    return function action(graph) {
         var relation = graph.entity(relationId);
 
-        if (isNaN(memberIndex) && member.type === 'way') {
-            var members = relation.indexedMembers();
-            members.push(member);
-
-            var joined = osmJoinWays(members, graph);
-            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;
+        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));
+        }
+
+        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, tempWay, item, i, j, k;
+
+        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: member.role };
+            var tempRelation = relation.replaceMember({id: insertPair.originalID}, tempMember, true);
+            groups = _groupBy(tempRelation.members, function(m) { return m.type; });
+            groups.way = groups.way || [];
+
+        } else {
+            // Add the member anywhere, one time. 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 members = withIndex(groups.way);
+        var joined = osmJoinWays(members, graph);
+
+        // `joined` might not contain all of the way members,
+        // But will contain only the completed (downloaded) members
+        for (i = 0; i < joined.length; i++) {
+            var segment = joined[i];
+            var nodes = segment.nodes.slice();
+            var startIndex = segment[0].index;
+
+            // j = array index in `members` where this segment starts
+            for (j = 0; j < members.length; j++) {
+                if (members[j].index === startIndex) {
+                    break;
+                }
+            }
+
+            // k = each member in segment
+            for (k = 0; k < segment.length; k++) {
+                item = segment[k];
+                var way = graph.entity(item.id);
+
+                // If this is a paired item, generate members in correct order and role
+                if (tempWay && item.id === tempWay.id) {
+                    if (nodes[0].id === insertPair.nodes[0]) {
+                        item.pair = [
+                            { id: insertPair.originalID, type: 'way', role: item.role },
+                            { id: insertPair.insertedID, type: 'way', role: item.role }
+                        ];
                     } else {
-                        memberIndex = Math.min(segment[j - 1].index + 1, segment[j + 1].index + 1);
+                        item.pair = [
+                            { id: insertPair.insertedID, type: 'way', role: item.role },
+                            { id: insertPair.originalID, type: 'way', role: item.role }
+                        ];
+                    }
+                }
+
+                // reorder `members` if necessary
+                if (k > 0) {
+                    if (j+k >= members.length || item.index !== members[j+k].index) {
+                        moveMember(members, item.index, j+k);
                     }
                 }
+
+                nodes.splice(0, way.nodes.length - 1);
             }
         }
 
-        return graph.replace(relation.addMember(member, memberIndex));
-    };
+        if (tempWay) {
+            graph = graph.remove(tempWay);
+        }
+
+        // Final pass: skip dead items, split pairs, remove index properties
+        var wayMembers = [];
+        for (i = 0; i < members.length; i++) {
+            item = members[i];
+            if (item.index === -1) continue;
+
+            if (item.pair) {
+                wayMembers.push(item.pair[0]);
+                wayMembers.push(item.pair[1]);
+            } else {
+                wayMembers.push(_omit(item, 'index'));
+            }
+        }
+
+        // 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(wayMembers, (groups.relation || []));
+
+        return graph.replace(relation.update({members: newMembers}));
+
+
+        // `moveMember()` changes the `members` array in place by splicing
+        // the item with `.index = findIndex` to where it belongs,
+        // and marking the old position as "dead" with `.index = -1`
+        //
+        // j=5, k=0                jk
+        // segment                 5 4 7 6
+        // members       0 1 2 3 4 5 6 7 8 9        keep 5 in j+k
+        //
+        // j=5, k=1                j k
+        // segment                 5 4 7 6
+        // members       0 1 2 3 4 5 6 7 8 9        move 4 to j+k
+        // members       0 1 2 3 x 5 4 6 7 8 9      moved
+        //
+        // j=5, k=2                j   k
+        // segment                 5 4 7 6
+        // members       0 1 2 3 x 5 4 6 7 8 9      move 7 to j+k
+        // members       0 1 2 3 x 5 4 7 6 x 8 9    moved
+        //
+        // j=5, k=3                j     k
+        // segment                 5 4 7 6
+        // members       0 1 2 3 x 5 4 7 6 x 8 9    keep 6 in j+k
+        //
+        function moveMember(arr, findIndex, toIndex) {
+            for (var i = 0; i < arr.length; i++) {
+                if (arr[i].index === findIndex) {
+                    break;
+                }
+            }
+
+            var item = _clone(arr[i]);
+            arr[i].index = -1;   // mark as dead
+            item.index = toIndex;
+            arr.splice(toIndex, 0, item);
+        }
+
+
+        // This is the same as `Relation.indexedMembers`,
+        // Except we don't want to index all the members, only the ways
+        function withIndex(arr) {
+            var result = new Array(arr.length);
+            for (var i = 0; i < arr.length; i++) {
+                result[i] = arr[i];
+                result[i].index = i;
+            }
+            return result;
+        }
+    }
+
 }

modules/actions/join.js

@@ -1,13 +1,8 @@
 import _extend from 'lodash-es/extend';
 import _groupBy from 'lodash-es/groupBy';
-import _map from 'lodash-es/map';
 
 import { actionDeleteWay } from './delete_way';
-
-import {
-    osmIsInterestingTag,
-    osmJoinWays
-} from '../osm';
+import { osmIsInterestingTag, osmJoinWays } from '../osm';
 
 
 // Join ways at the end node they share.
@@ -27,25 +22,30 @@ export function actionJoin(ids) {
 
 
     var action = function(graph) {
-        var ways = ids.map(graph.entity, graph),
-            survivor = ways[0];
+        var ways = ids.map(graph.entity, graph);
+        var survivorID = ways[0].id;
 
         // Prefer to keep an existing way.
         for (var i = 0; i < ways.length; i++) {
             if (!ways[i].isNew()) {
-                survivor = ways[i];
+                survivorID = ways[i].id;
                 break;
             }
         }
 
-        var joined = osmJoinWays(ways, graph)[0];
+        var sequences = osmJoinWays(ways, graph);
+        var joined = sequences[0];
+
+        // We might need to reverse some of these ways before joining them.  #4688
+        // `joined.actions` property will contain any actions we need to apply.
+        graph = sequences.actions.reduce(function(g, action) { return action(g); }, graph);
 
-        survivor = survivor.update({nodes: _map(joined.nodes, 'id')});
+        var survivor = graph.entity(survivorID);
+        survivor = survivor.update({ nodes: joined.nodes.map(function(n) { return n.id; }) });
         graph = graph.replace(survivor);
 
         joined.forEach(function(way) {
-            if (way.id === survivor.id)
-                return;
+            if (way.id === survivorID) return;
 
             graph.parentRelations(way).forEach(function(parent) {
                 graph = graph.replace(parent.replaceMember(way, survivor));
@@ -70,10 +70,10 @@ export function actionJoin(ids) {
         if (joined.length > 1)
             return 'not_adjacent';
 
-        var nodeIds = _map(joined[0].nodes, 'id').slice(1, -1),
-            relation,
-            tags = {},
-            conflicting = false;
+        var nodeIds = joined[0].nodes.map(function(n) { return n.id; }).slice(1, -1);
+        var relation;
+        var tags = {};
+        var conflicting = false;
 
         joined[0].forEach(function(way) {
             var parents = graph.parentRelations(way);

modules/actions/split.js

@@ -29,7 +29,7 @@ import { utilWrap } from '../util';
 //   https://github.com/systemed/potlatch2/blob/master/net/systemeD/halcyon/connection/actions/SplitWayAction.as
 //
 export function actionSplit(nodeId, newWayIds) {
-    var wayIds;
+    var _wayIDs;
 
     // if the way is closed, we need to search for a partner node
     // to split the way at.
@@ -42,11 +42,11 @@ export function actionSplit(nodeId, newWayIds) {
     // For example: bone-shaped areas get split across their waist
     // line, circles across the diameter.
     function splitArea(nodes, idxA, graph) {
-        var lengths = new Array(nodes.length),
-            length,
-            i,
-            best = 0,
-            idxB;
+        var lengths = new Array(nodes.length);
+        var length;
+        var i;
+        var best = 0;
+        var idxB;
 
         function wrap(index) {
             return utilWrap(index, nodes.length);
@@ -84,16 +84,17 @@ export function actionSplit(nodeId, newWayIds) {
 
 
     function split(graph, wayA, newWayId) {
-        var wayB = osmWay({id: newWayId, tags: wayA.tags}),
-            nodesA,
-            nodesB,
-            isArea = wayA.isArea(),
-            isOuter = osmIsSimpleMultipolygonOuterMember(wayA, graph);
+        var wayB = osmWay({id: newWayId, tags: wayA.tags});
+        var origNodes = wayA.nodes.slice();
+        var nodesA;
+        var nodesB;
+        var isArea = wayA.isArea();
+        var isOuter = osmIsSimpleMultipolygonOuterMember(wayA, graph);
 
         if (wayA.isClosed()) {
-            var nodes = wayA.nodes.slice(0, -1),
-                idxA = _indexOf(nodes, nodeId),
-                idxB = splitArea(nodes, idxA, graph);
+            var nodes = wayA.nodes.slice(0, -1);
+            var idxA = _indexOf(nodes, nodeId);
+            var idxB = splitArea(nodes, idxA, graph);
 
             if (idxB < idxA) {
                 nodesA = nodes.slice(idxA).concat(nodes.slice(0, idxB + 1));
@@ -134,7 +135,13 @@ export function actionSplit(nodeId, newWayIds) {
                     role: relation.memberById(wayA.id).role
                 };
 
-                graph = actionAddMember(relation.id, member)(graph);
+                var insertPair = {
+                    originalID: wayA.id,
+                    insertedID: wayB.id,
+                    nodes: origNodes
+                };
+
+                graph = actionAddMember(relation.id, member, undefined, insertPair)(graph);
             }
         });
 
@@ -144,7 +151,8 @@ export function actionSplit(nodeId, newWayIds) {
                 members: [
                     {id: wayA.id, role: 'outer', type: 'way'},
                     {id: wayB.id, role: 'outer', type: 'way'}
-                ]});
+                ]
+            });
 
             graph = graph.replace(multipolygon);
             graph = graph.replace(wayA.update({tags: {}}));
@@ -165,15 +173,15 @@ export function actionSplit(nodeId, newWayIds) {
 
 
     action.ways = function(graph) {
-        var node = graph.entity(nodeId),
-            parents = graph.parentWays(node),
-            hasLines = _some(parents, function(parent) { return parent.geometry(graph) === 'line'; });
+        var node = graph.entity(nodeId);
+        var parents = graph.parentWays(node);
+        var hasLines = _some(parents, function(parent) { return parent.geometry(graph) === 'line'; });
 
         return parents.filter(function(parent) {
-            if (wayIds && wayIds.indexOf(parent.id) === -1)
+            if (_wayIDs && _wayIDs.indexOf(parent.id) === -1)
                 return false;
 
-            if (!wayIds && hasLines && parent.geometry(graph) !== 'line')
+            if (!_wayIDs && hasLines && parent.geometry(graph) !== 'line')
                 return false;
 
             if (parent.isClosed()) {
@@ -193,14 +201,14 @@ export function actionSplit(nodeId, newWayIds) {
 
     action.disabled = function(graph) {
         var candidates = action.ways(graph);
-        if (candidates.length === 0 || (wayIds && wayIds.length !== candidates.length))
+        if (candidates.length === 0 || (_wayIDs && _wayIDs.length !== candidates.length))
             return 'not_eligible';
     };
 
 
     action.limitWays = function(_) {
-        if (!arguments.length) return wayIds;
-        wayIds = _;
+        if (!arguments.length) return _wayIDs;
+        _wayIDs = _;
         return action;
     };