diff --git a/js/id/actions/add_member.js b/js/id/actions/add_member.js index f3eac3d9c..304507a2b 100644 --- a/js/id/actions/add_member.js +++ b/js/id/actions/add_member.js @@ -6,7 +6,7 @@ iD.actions.AddMember = function(relationId, member, memberIndex) { var members = relation.indexedMembers(); members.push(member); - var joined = iD.geo.joinMemberWays(members, graph); + var joined = iD.geo.joinWays(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++) { diff --git a/js/id/actions/join.js b/js/id/actions/join.js index 06f818189..e0d7135cd 100644 --- a/js/id/actions/join.js +++ b/js/id/actions/join.js @@ -14,99 +14,47 @@ iD.actions.Join = function(ids) { } var action = function(graph) { - var existing = 0, - nodes, - a, b, - i, j; - - function replaceWithA(parent) { - graph = graph.replace(parent.replaceMember(b, a)); - } + var ways = ids.map(graph.entity, graph), + survivor = ways[0]; // Prefer to keep an existing way. - for (i = 0; i < ids.length; i++) { - if (!graph.entity(ids[i]).isNew()) { - existing = i; + for (var i = 0; i < ways.length; i++) { + if (!ways[i].isNew()) { + survivor = ways[i]; break; } } - // Join ways to 'a' in the following order: a-1, a-2, ..., 0, a+1, a+2, ..., ids.length-1 - for (i = 0; i < ids.length; i++) { - j = (i <= existing) ? (existing - i) : i; - if (j === existing) { - continue; - } + var joined = iD.geo.joinWays(ways, graph)[0]; - a = graph.entity(ids[existing]); - b = graph.entity(ids[j]); + survivor = survivor.update({nodes: _.pluck(joined.nodes, 'id')}); + graph = graph.replace(survivor); - if (a.first() === b.first()) { - // a <-- b ==> c - // Expected result: - // a <-- b <-- c - b = iD.actions.Reverse(ids[j])(graph).entity(ids[j]); - nodes = b.nodes.slice().concat(a.nodes.slice(1)); + joined.forEach(function(way) { + if (way.id === survivor.id) + return; - } else if (a.first() === b.last()) { - // a <-- b <== c - // Expected result: - // a <-- b <-- c - nodes = b.nodes.concat(a.nodes.slice(1)); + graph.parentRelations(way).forEach(function(parent) { + graph = graph.replace(parent.replaceMember(way, survivor)); + }); - } else if (a.last() === b.first()) { - // a --> b ==> c - // Expected result: - // a --> b --> c - nodes = a.nodes.concat(b.nodes.slice(1)); + survivor = survivor.mergeTags(way.tags); - } else if (a.last() === b.last()) { - // a --> b <== c - // Expected result: - // a --> b --> c - b = iD.actions.Reverse(ids[j])(graph).entity(ids[j]); - nodes = a.nodes.concat(b.nodes.slice().slice(1)); - } - - graph.parentRelations(b).forEach(replaceWithA); - - graph = graph.replace(a.mergeTags(b.tags).update({ nodes: nodes })); - graph = iD.actions.DeleteWay(ids[j])(graph); - } + graph = graph.replace(survivor); + graph = iD.actions.DeleteWay(way.id)(graph); + }); return graph; }; action.disabled = function(graph) { - var geometries = groupEntitiesByGeometry(graph), - i; - - // direction of the previous way -- the next way can join only on the opposite side than the previous joint - var prev_direction = 0; - + var geometries = groupEntitiesByGeometry(graph); if (ids.length < 2 || ids.length !== geometries.line.length) return 'not_eligible'; - for (i = 0; i+1 < ids.length; i++) { - var a = graph.entity(ids[i+0]), - b = graph.entity(ids[i+1]); - - if (a.first() === b.first() && prev_direction <= 0) { - prev_direction = 1; - continue; - } else if (a.first() === b.last() && prev_direction <= 0) { - prev_direction = -1; - continue; - } else if (a.last() === b.first() && prev_direction >= 0) { - prev_direction = 1; - continue; - } else if (a.last() === b.last() && prev_direction >= 0) { - prev_direction = -1; - continue; - } - + var joined = iD.geo.joinWays(ids.map(graph.entity, graph), graph); + if (joined.length > 1) return 'not_adjacent'; - } }; return action; diff --git a/js/id/actions/merge_polygon.js b/js/id/actions/merge_polygon.js index 6f7847ce3..a6c04c09d 100644 --- a/js/id/actions/merge_polygon.js +++ b/js/id/actions/merge_polygon.js @@ -25,10 +25,10 @@ iD.actions.MergePolygon = function(ids, newRelationId) { // 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(iD.geo.joinMemberWays(m.members, graph)); + return polygons.concat(iD.geo.joinWays(m.members, graph)); }, []).concat(entities.closedWay.map(function(d) { var member = [{id: d.id}]; - member.locs = graph.childNodes(d).map(function(n) { return n.loc; }); + member.nodes = graph.childNodes(d); return member; })); @@ -38,7 +38,9 @@ iD.actions.MergePolygon = function(ids, newRelationId) { var contained = polygons.map(function(w, i) { return polygons.map(function(d, n) { if (i === n) return null; - return iD.geo.polygonContainsPolygon(d.locs, w.locs); + return iD.geo.polygonContainsPolygon( + _.pluck(d.nodes, 'loc'), + _.pluck(w.nodes, 'loc')); }); }); diff --git a/js/id/core/relation.js b/js/id/core/relation.js index 4f527314c..a1002d65e 100644 --- a/js/id/core/relation.js +++ b/js/id/core/relation.js @@ -185,11 +185,11 @@ _.extend(iD.Relation.prototype, { 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 = iD.geo.joinWays(outers, resolver); + inners = iD.geo.joinWays(inners, resolver); - outers = _.pluck(outers, 'locs'); - inners = _.pluck(inners, 'locs'); + outers = outers.map(function(outer) { return _.pluck(outer.nodes, 'loc'); }); + inners = inners.map(function(inner) { return _.pluck(inner.nodes, 'loc'); }); var result = outers.map(function(o) { return [o]; }); diff --git a/js/id/geo/multipolygon.js b/js/id/geo/multipolygon.js index 878926f86..761831ec2 100644 --- a/js/id/geo/multipolygon.js +++ b/js/id/geo/multipolygon.js @@ -49,58 +49,72 @@ iD.geo.simpleMultipolygonOuterMember = function(entity, graph) { return outerMember && graph.hasEntity(outerMember.id); }; -// Join an array of relation `members` into sequences of connecting segments. +// Join `array` into sequences of connecting ways. // // 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, +// of `array` which have been determined to connect. Each consitituent array +// also has a `nodes` property whose value is an ordered array of member nodes, // with appropriate order reversal and start/end coordinate de-duplication. // -// Incomplete members are ignored. +// Members of `array` must have, at minimum, `type` and `id` properties. +// Thus either an array of `iD.Way`s or a relation member array may be +// used. // -iD.geo.joinMemberWays = function(members, graph) { - var joined = [], member, current, locs, first, last, i, how, what; +// If an member has a `tags` property, its tags will be reversed via +// `iD.actions.Reverse` in the output. +// +// Incomplete members (those for which `graph.hasEntity(element.id)` returns +// false) and non-way members are ignored. +// +iD.geo.joinWays = function(array, graph) { + var joined = [], member, current, nodes, first, last, i, how, what; - members = members.filter(function(member) { + array = array.filter(function(member) { return member.type === 'way' && graph.hasEntity(member.id); }); function resolve(member) { - return _.pluck(graph.childNodes(graph.entity(member.id)), 'loc'); + return graph.childNodes(graph.entity(member.id)); } - while (members.length) { - member = members.shift(); + function reverse(member) { + return member.tags ? iD.actions.Reverse(member.id)(graph).entity(member.id) : member; + } + + while (array.length) { + member = array.shift(); current = [member]; - current.locs = locs = resolve(member); + current.nodes = nodes = resolve(member); joined.push(current); - while (members.length && _.first(locs) !== _.last(locs)) { - first = _.first(locs); - last = _.last(locs); + while (array.length && _.first(nodes) !== _.last(nodes)) { + first = _.first(nodes); + last = _.last(nodes); - for (i = 0; i < members.length; i++) { - member = members[i]; + for (i = 0; i < array.length; i++) { + member = array[i]; what = resolve(member); if (last === _.first(what)) { - how = locs.push; + how = nodes.push; what = what.slice(1); break; } else if (last === _.last(what)) { - how = locs.push; + how = nodes.push; what = what.slice(0, -1).reverse(); + member = reverse(member); break; } else if (first === _.last(what)) { - how = locs.unshift; + how = nodes.unshift; what = what.slice(0, -1); break; } else if (first === _.first(what)) { - how = locs.unshift; + how = nodes.unshift; what = what.slice(1).reverse(); + member = reverse(member); break; } else { what = how = null; @@ -111,9 +125,9 @@ iD.geo.joinMemberWays = function(members, graph) { break; // No more joinable ways. how.apply(current, [member]); - how.apply(locs, what); + how.apply(nodes, what); - members.splice(i, 1); + array.splice(i, 1); } } diff --git a/test/spec/actions/join.js b/test/spec/actions/join.js index 02eefdbb2..00c03fde9 100644 --- a/test/spec/actions/join.js +++ b/test/spec/actions/join.js @@ -52,6 +52,26 @@ describe("iD.actions.Join", function () { expect(iD.actions.Join(['-', '=']).disabled(graph)).not.to.be.ok; }); + it("returns falsy for more than two ways when connected, regardless of order", function () { + // a --> b ==> c ~~> d + var graph = iD.Graph({ + 'a': iD.Node({id: 'a'}), + 'b': iD.Node({id: 'b'}), + 'c': iD.Node({id: 'c'}), + 'd': iD.Node({id: 'd'}), + '-': iD.Way({id: '-', nodes: ['a', 'b']}), + '=': iD.Way({id: '=', nodes: ['b', 'c']}), + '~': iD.Way({id: '~', nodes: ['c', 'd']}) + }); + + expect(iD.actions.Join(['-', '=', '~']).disabled(graph)).not.to.be.ok; + expect(iD.actions.Join(['-', '~', '=']).disabled(graph)).not.to.be.ok; + expect(iD.actions.Join(['=', '-', '~']).disabled(graph)).not.to.be.ok; + expect(iD.actions.Join(['=', '~', '-']).disabled(graph)).not.to.be.ok; + expect(iD.actions.Join(['~', '=', '-']).disabled(graph)).not.to.be.ok; + expect(iD.actions.Join(['~', '-', '=']).disabled(graph)).not.to.be.ok; + }); + it("returns 'not_eligible' for non-line geometries", function () { var graph = iD.Graph({ 'a': iD.Node({id: 'a'}) diff --git a/test/spec/geo/multipolygon.js b/test/spec/geo/multipolygon.js index 4db4839e8..a68a2c553 100644 --- a/test/spec/geo/multipolygon.js +++ b/test/spec/geo/multipolygon.js @@ -40,17 +40,17 @@ describe("iD.geo.simpleMultipolygonOuterMember", function() { }); }); -describe("iD.geo.joinMemberWays", function() { - it("returns an array of members with locs properties", function() { +describe("iD.geo.joinWays", function() { + it("returns an array of members with nodes 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); + result = iD.geo.joinWays([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].nodes.length).to.equal(1); + expect(result[0].nodes[0]).to.equal(node); expect(result[0].length).to.equal(1); expect(result[0][0]).to.equal(member); }); @@ -72,20 +72,37 @@ describe("iD.geo.joinMemberWays", function() { ]}) }); - var result = iD.geo.joinMemberWays(graph.entity('r').members, graph); + var result = iD.geo.joinWays(graph.entity('r').members, graph); expect(_.pluck(result[0], 'id')).to.eql(['=', '-', '~']); }); + it("reverses member tags of reversed segements", function() { + // a --> b <== c + // Expected result: + // a --> b --> c + // tags on === reversed + var graph = iD.Graph({ + 'a': iD.Node({id: 'a'}), + 'b': iD.Node({id: 'b'}), + 'c': iD.Node({id: 'c'}), + '-': iD.Way({id: '-', nodes: ['a', 'b']}), + '=': iD.Way({id: '=', nodes: ['c', 'b'], tags: {'lanes:forward': 2}}) + }); + + var result = iD.geo.joinWays([graph.entity('-'), graph.entity('=')], graph); + expect(result[0][1].tags).to.eql({'lanes:backward': 2}); + }); + 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([]); + expect(iD.geo.joinWays([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([]); + expect(iD.geo.joinWays([member], graph)).to.eql([]); }); });