diff --git a/modules/osm/multipolygon.js b/modules/osm/multipolygon.js index 41e61a5ae..01587c321 100644 --- a/modules/osm/multipolygon.js +++ b/modules/osm/multipolygon.js @@ -104,6 +104,16 @@ export function osmJoinWays(toJoin, graph) { return member.type === 'way' && graph.hasEntity(member.id); }); + // Are the things we are joining relation members or `osmWays`? + // If `osmWays`, skip the "prefer a forward path" code below (see #4872) + var i; + var joinAsMembers = true; + for (i = 0; i < toJoin.length; i++) { + if (toJoin[i] instanceof osmWay) { + joinAsMembers = false; + break; + } + } var sequences = []; sequences.actions = []; @@ -121,19 +131,19 @@ export function osmJoinWays(toJoin, graph) { var end = currNodes[currNodes.length - 1]; var fn = null; var nodes = null; - var i; // Find the next way/member to join. for (i = 0; i < toJoin.length; i++) { item = toJoin[i]; nodes = resolve(item); + // (for member ordering only, not way ordering - see #4872) // Strongly prefer to generate a forward path that preserves the order // of the members array. For multipolygons and most relations, member // order does not matter - but for routes, it does. If we started this // sequence backwards (i.e. next member way attaches to the start node // and not the end node), reverse the initial way before continuing. - if (currWays.length === 1 && nodes[0] !== end && nodes[nodes.length - 1] !== end && + if (joinAsMembers && currWays.length === 1 && nodes[0] !== end && nodes[nodes.length - 1] !== end && (nodes[nodes.length - 1] === start || nodes[0] === start) ) { currWays[0] = reverse(currWays[0]); diff --git a/test/spec/actions/join.js b/test/spec/actions/join.js index e7de473f3..ba37ae329 100644 --- a/test/spec/actions/join.js +++ b/test/spec/actions/join.js @@ -273,15 +273,13 @@ describe('iD.actionJoin', function () { ]); graph = iD.actionJoin(['-', '='])(graph); - - expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']); + expect(graph.entity('-').nodes).to.eql(['c', 'b', 'a']); expect(graph.hasEntity('=')).to.be.undefined; }); it('joins a <-- b ==> c', function () { // Expected result: // a --> b --> c - // tags on --- reversed var graph = iD.coreGraph([ iD.osmNode({id: 'a'}), iD.osmNode({id: 'b'}), @@ -292,9 +290,9 @@ describe('iD.actionJoin', function () { graph = iD.actionJoin(['-', '='])(graph); - expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']); + expect(graph.entity('-').nodes).to.eql(['c', 'b', 'a']); expect(graph.hasEntity('=')).to.be.undefined; - expect(graph.entity('-').tags).to.eql({'lanes:backward': 2}); + expect(graph.entity('-').tags).to.eql({'lanes:forward': 2}); }); it('joins a --> b <== c', function () { diff --git a/test/spec/osm/multipolygon.js b/test/spec/osm/multipolygon.js index f3bba303b..b5f4b78c2 100644 --- a/test/spec/osm/multipolygon.js +++ b/test/spec/osm/multipolygon.js @@ -164,7 +164,7 @@ describe('iD.osmJoinWays', function() { expect(result[0][0]).to.eql(member); }); - it('joins ways', function() { + it('joins ways (ordered - w1, w2)', function() { // // a ---> b ===> c // @@ -184,7 +184,27 @@ describe('iD.osmJoinWays', function() { expect(result[0][1]).to.eql(w2); }); - it('joins relation members', function() { + it('joins ways (unordered - w2, w1)', function() { + // + // a ---> b ===> c + // + var a = iD.osmNode({id: 'a', loc: [0, 0]}); + var b = iD.osmNode({id: 'b', loc: [1, 0]}); + var c = iD.osmNode({id: 'c', loc: [2, 0]}); + var w1 = iD.osmWay({id: '-', nodes: ['a', 'b']}); + var w2 = iD.osmWay({id: '=', nodes: ['b', 'c']}); + var graph = iD.coreGraph([a, b, c, w1, w2]); + + var result = iD.osmJoinWays([w2, w1], graph); + expect(result.length).to.equal(1); + expect(result.actions).to.eql([]); + expect(getIDs(result[0].nodes)).to.eql(['a', 'b', 'c']); + expect(result[0].length).to.equal(2); + expect(result[0][0]).to.eql(w1); + expect(result[0][1]).to.eql(w2); + }); + + it('joins relation members (ordered -, =)', function() { // // a ---> b ===> c // r: ['-', '='] @@ -209,6 +229,31 @@ describe('iD.osmJoinWays', function() { expect(result[0][1]).to.eql({id: '=', type: 'way'}); }); + it('joins relation members (ordered =, -)', function() { + // + // a ---> b ===> c + // r: ['=', '-'] + // + var a = iD.osmNode({id: 'a', loc: [0, 0]}); + var b = iD.osmNode({id: 'b', loc: [1, 0]}); + var c = iD.osmNode({id: 'c', loc: [2, 0]}); + var w1 = iD.osmWay({id: '-', nodes: ['a', 'b']}); + var w2 = iD.osmWay({id: '=', nodes: ['b', 'c']}); + var r = iD.osmRelation({id: 'r', members: [ + {id: '=', type: 'way'}, + {id: '-', type: 'way'} + ]}); + var graph = iD.coreGraph([a, b, c, w1, w2, r]); + + var result = iD.osmJoinWays(r.members, graph); + expect(result.length).to.equal(1); + expect(result.actions.length).to.equal(2); + expect(getIDs(result[0].nodes)).to.eql(['c', 'b', 'a']); + expect(result[0].length).to.equal(2); + expect(result[0][0]).to.eql({id: '=', type: 'way'}); + expect(result[0][1]).to.eql({id: '-', type: 'way'}); + }); + it('returns joined members in the correct order', function() { // // a <=== b ---> c ~~~> d @@ -243,7 +288,7 @@ describe('iD.osmJoinWays', function() { // Source: // a ---> b <=== c // Result: - // a ---> b ===> c (and === reversed) + // a ---> b ===> c (and tags on === reversed) // var a = iD.osmNode({id: 'a', loc: [0, 0]}); var b = iD.osmNode({id: 'b', loc: [1, 0]}); @@ -257,13 +302,14 @@ describe('iD.osmJoinWays', function() { expect(result.actions.length).to.equal(1); expect(getIDs(result[0].nodes)).to.eql(['a', 'b', 'c']); expect(result[0].length).to.equal(2); - expect(result[0][0]).to.eql(w1); + expect(result[0][0]).to.be.an.instanceof(iD.osmWay); + expect(result[0][0].nodes).to.eql(['a', 'b']); expect(result[0][1]).to.be.an.instanceof(iD.osmWay); expect(result[0][1].nodes).to.eql(['b', 'c']); expect(result[0][1].tags).to.eql({'oneway': '-1', 'lanes:backward': 2}); }); - it('reverses the initial segment to preserve member order', function() { + it('reverses the initial segment to preserve member order when joining relation members', function() { // // Source: // a <--- b ===> c @@ -275,20 +321,21 @@ describe('iD.osmJoinWays', function() { var c = iD.osmNode({id: 'c', loc: [2, 0]}); var w1 = iD.osmWay({id: '-', nodes: ['b', 'a'], tags: {'oneway': 'yes', 'lanes:forward': 2}}); var w2 = iD.osmWay({id: '=', nodes: ['b', 'c']}); - var graph = iD.coreGraph([a, b, c, w1, w2]); + var r = iD.osmRelation({id: 'r', members: [ + {id: '-', type: 'way'}, + {id: '=', type: 'way'} + ]}); + var graph = iD.coreGraph([a, b, c, w1, w2, r]); - var result = iD.osmJoinWays([w1, w2], graph); + var result = iD.osmJoinWays(r.members, graph); expect(result.length).to.equal(1); expect(result.actions.length).to.equal(1); expect(getIDs(result[0].nodes)).to.eql(['a', 'b', 'c']); expect(result[0].length).to.equal(2); - expect(result[0][0]).to.be.an.instanceof(iD.osmWay); - expect(result[0][0].nodes).to.eql(['a', 'b']); - expect(result[0][0].tags).to.eql({'oneway': '-1', 'lanes:backward': 2}); - expect(result[0][1]).to.eql(w2); + expect(result[0][0]).to.eql({id: '-', type: 'way'}); + expect(result[0][1]).to.eql({id: '=', type: 'way'}); }); - it('ignores non-way members', function() { var node = iD.osmNode({loc: [0, 0]}); var member = {id: 'n', type: 'node'};