Extract and refactor iD.geo.joinMemberWays

js/id/actions/merge_polygon.js

@@ -20,18 +20,16 @@ iD.actions.MergePolygon = function(ids, newRelationId) {
     var action = function(graph) {
         var entities = groupEntities(graph);
 
-        // An array of objects representing all the polygons that are part of the multipolygon.
+        // An array representing all the polygons that are part of the multipolygon.
         //
-        // Each object has two properties:
-        //     ids - an array of ids of entities that are part of that polygon
-        //     locs - an array of the locations forming the polygon
+        // Each element is itself an array of objects with an id property, and has a
+        // locs property which is an array of the locations forming the polygon.
         var polygons = entities.multipolygon.reduce(function(polygons, m) {
-            return polygons.concat(m.joinMemberWays(null, graph));
+            return polygons.concat(iD.geo.joinMemberWays(m.members, graph));
         }, []).concat(entities.closedWay.map(function(d) {
-            return {
-                ids: [d.id],
-                locs: graph.childNodes(d).map(function(n) { return n.loc; })
-            };
+            var member = [{id: d.id}];
+            member.locs = graph.childNodes(d).map(function(n) { return n.loc; });
+            return member;
         }));
 
         // contained is an array of arrays of boolean values,
@@ -65,10 +63,10 @@ iD.actions.MergePolygon = function(ids, newRelationId) {
         function extractUncontained(polygons) {
             polygons.forEach(function(d, i) {
                 if (!isContained(d, i)) {
-                    d.ids.forEach(function(id) {
+                    d.forEach(function(member) {
                         members.push({
                             type: 'way',
-                            id: id,
+                            id: member.id,
                             role: outer ? 'outer' : 'inner'
                         });
                     });

js/id/core/relation.js

@@ -172,9 +172,16 @@ _.extend(iD.Relation.prototype, {
     // rings not matched with the intended outer ring.
     //
     multipolygon: function(resolver) {
-        var members = this.members
-            .filter(function(m) { return m.type === 'way' && resolver.hasEntity(m.id); })
-            .map(function(m) { return { role: m.role || 'outer', id: m.id, nodes: resolver.childNodes(resolver.entity(m.id)) }; });
+        var outers = this.members.filter(function(m) { return 'outer' === (m.role || 'outer'); }),
+            inners = this.members.filter(function(m) { return 'inner' === m.role; });
+
+        outers = iD.geo.joinMemberWays(outers, resolver);
+        inners = iD.geo.joinMemberWays(inners, resolver);
+
+        outers = _.pluck(outers, 'locs');
+        inners = _.pluck(inners, 'locs');
+
+        var result = outers.map(function(o) { return [o]; });
 
         function findOuter(inner) {
             var o, outer;
@@ -192,10 +199,6 @@ _.extend(iD.Relation.prototype, {
             }
         }
 
-        var outers = _.pluck(this.joinMemberWays(members.filter(function(m) { return m.role === 'outer'; })), 'locs'),
-            inners = _.pluck(this.joinMemberWays(members.filter(function(m) { return m.role === 'inner'; })), 'locs'),
-            result = outers.map(function(o) { return [o]; });
-
         for (var i = 0; i < inners.length; i++) {
             var o = findOuter(inners[i]);
             if (o !== undefined)
@@ -205,68 +208,5 @@ _.extend(iD.Relation.prototype, {
         }
 
         return result;
-    },
-
-    joinMemberWays: function(ways, resolver) {
-        var joined = [], way, current, first, last, i, how, what;
-
-        ways = ways || this.members.filter(function(m) {
-            return m.type === 'way';
-        }).map(function(m) {
-            return {
-                id: m.id,
-                nodes: resolver.childNodes(resolver.entity(m.id))
-            };
-        });
-
-        while (ways.length) {
-            way = ways.pop();
-            current = way.nodes.slice();
-            current.ids = [way.id];
-            joined.push(current);
-
-            while (ways.length && _.first(current) !== _.last(current)) {
-                first = _.first(current);
-                last  = _.last(current);
-
-                for (i = 0; i < ways.length; i++) {
-                    what = ways[i].nodes;
-
-                    if (last === _.first(what)) {
-                        how  = current.push;
-                        what = what.slice(1);
-                        break;
-                    } else if (last === _.last(what)) {
-                        how  = current.push;
-                        what = what.slice(0, -1).reverse();
-                        break;
-                    } else if (first == _.last(what)) {
-                        how  = current.unshift;
-                        what = what.slice(0, -1);
-                        break;
-                    } else if (first == _.first(what)) {
-                        how  = current.unshift;
-                        what = what.slice(1).reverse();
-                        break;
-                    } else {
-                        what = how = null;
-                    }
-                }
-
-                if (!what)
-                    break; // Invalid geometry (unclosed ring)
-
-                current.ids.push(ways[i].id);
-                ways.splice(i, 1);
-                how.apply(current, what);
-            }
-        }
-        return joined.map(function(nodes) {
-            return {
-                ids: nodes.ids,
-                locs: _.pluck(nodes, 'loc')
-            };
-        });
     }
-
 });

js/id/geo/multipolygon.js

@@ -48,3 +48,74 @@ iD.geo.simpleMultipolygonOuterMember = function(entity, graph) {
 
     return outerMember && graph.hasEntity(outerMember.id);
 };
+
+// Join an array of relation `members` into sequences of connecting segments.
+//
+// Segments which share identical start/end nodes will, as much as possible,
+// be connected with each other.
+//
+// The return value is a nested array. Each constituent array contains elements
+// of `members` which have been determined to connect. Each consitituent array
+// also has a `locs` property whose value is an ordered array of member coordinates,
+// with appropriate order reversal and start/end coordinate de-duplication.
+//
+// Incomplete members are ignored.
+//
+iD.geo.joinMemberWays = function(members, graph) {
+    var joined = [], member, current, locs, first, last, i, how, what;
+
+    members = members.filter(function(member) {
+        return member.type === 'way' && graph.hasEntity(member.id);
+    });
+
+    function resolve(member) {
+        return _.pluck(graph.childNodes(graph.entity(member.id)), 'loc');
+    }
+
+    while (members.length) {
+        member = members.pop();
+        current = [member];
+        current.locs = locs = resolve(member);
+        joined.push(current);
+
+        while (members.length && _.first(locs) !== _.last(locs)) {
+            first = _.first(locs);
+            last  = _.last(locs);
+
+            for (i = 0; i < members.length; i++) {
+                member = members[i];
+                what = resolve(member);
+
+                if (last === _.first(what)) {
+                    how  = locs.push;
+                    what = what.slice(1);
+                    break;
+                } else if (last === _.last(what)) {
+                    how  = locs.push;
+                    what = what.slice(0, -1).reverse();
+                    break;
+                } else if (first === _.last(what)) {
+                    how  = locs.unshift;
+                    what = what.slice(0, -1);
+                    break;
+                } else if (first === _.first(what)) {
+                    how  = locs.unshift;
+                    what = what.slice(1).reverse();
+                    break;
+                } else {
+                    what = how = null;
+                }
+            }
+
+            if (!what)
+                break; // No more joinable ways.
+
+            how.apply(current, [member]);
+            how.apply(locs, what);
+
+            members.splice(i, 1);
+        }
+    }
+
+    return joined;
+};

test/spec/core/relation.js

@@ -318,10 +318,10 @@ describe('iD.Relation', function () {
         });
 
         specify("invalid geometry: unclosed ring consisting of multiple ways, one needing reversal, alternate order", function () {
-            var a  = iD.Node(),
-                b  = iD.Node(),
-                c  = iD.Node(),
-                d  = iD.Node(),
+            var a  = iD.Node({loc: [1, 1]}),
+                b  = iD.Node({loc: [2, 2]}),
+                c  = iD.Node({loc: [3, 3]}),
+                d  = iD.Node({loc: [4, 4]}),
                 w1 = iD.Way({nodes: [c.id, d.id]}),
                 w2 = iD.Way({nodes: [c.id, b.id, a.id]}),
                 r  = iD.Relation({members: [{id: w2.id, type: 'way'}, {id: w1.id, type: 'way'}]}),

test/spec/geo/multipolygon.js

@@ -39,3 +39,53 @@ describe("iD.geo.simpleMultipolygonOuterMember", function() {
         expect(iD.geo.simpleMultipolygonOuterMember(way, graph)).to.be.undefined;
     });
 });
+
+describe("iD.geo.joinMemberWays", function() {
+    it("returns an array of members with locs properties", function() {
+        var node = iD.Node({loc: [0, 0]}),
+            way  = iD.Way({nodes: [node.id]}),
+            member = {id: way.id, type: 'way'},
+            graph = iD.Graph([node, way]),
+            result = iD.geo.joinMemberWays([member], graph);
+
+        expect(result.length).to.equal(1);
+        expect(result[0].locs.length).to.equal(1);
+        expect(result[0].locs[0]).to.equal(node.loc);
+        expect(result[0].length).to.equal(1);
+        expect(result[0][0]).to.equal(member);
+    });
+
+    it("returns the members in the correct order", function() {
+        // a<===b--->c~~~>d
+        var graph = iD.Graph({
+            'a': iD.Node({id: 'a', loc: [0, 0]}),
+            'b': iD.Node({id: 'b', loc: [0, 0]}),
+            'c': iD.Node({id: 'c', loc: [0, 0]}),
+            'd': iD.Node({id: 'd', loc: [0, 0]}),
+            '=': iD.Way({id: '=', nodes: ['b', 'a']}),
+            '-': iD.Way({id: '-', nodes: ['b', 'c']}),
+            '~': iD.Way({id: '~', nodes: ['c', 'd']}),
+            'r': iD.Relation({id: 'r', members: [
+                {id: '-', type: 'way'},
+                {id: '~', type: 'way'},
+                {id: '=', type: 'way'}
+            ]})
+        });
+
+        var result = iD.geo.joinMemberWays(graph.entity('r').members, graph);
+        expect(_.pluck(result[0], 'id')).to.eql(['=', '-', '~']);
+    });
+
+    it("ignores non-way members", function() {
+        var node = iD.Node({loc: [0, 0]}),
+            member = {id: 'n', type: 'node'},
+            graph = iD.Graph([node]);
+        expect(iD.geo.joinMemberWays([member], graph)).to.eql([]);
+    });
+
+    it("ignores incomplete members", function() {
+        var member = {id: 'w', type: 'way'},
+            graph = iD.Graph();
+        expect(iD.geo.joinMemberWays([member], graph)).to.eql([]);
+    });
+});