From 3c57f7147fc4b115eeaa5bcbc4426c91b1e19ff5 Mon Sep 17 00:00:00 2001 From: Jon D Date: Wed, 24 Aug 2016 23:36:23 +0100 Subject: [PATCH 1/5] Add tests for reverse way-node behaviour. No implementation yet. --- test/spec/actions/reverse.js | 92 ++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/test/spec/actions/reverse.js b/test/spec/actions/reverse.js index 1412f932f..cc92959d5 100644 --- a/test/spec/actions/reverse.js +++ b/test/spec/actions/reverse.js @@ -156,4 +156,96 @@ describe('iD.actions.Reverse', function () { graph = iD.actions.Reverse(way.id)(graph); expect(graph.entity(relation.id).members[0].role).to.eql('east'); }); + + // For issue #3076 + it('reverses the direction of a forward facing stop sign on the way', function () { + var node1 = iD.Node(); + var node2 = iD.Node(); + var node3 = iD.Node(); + // Attach a forward facing stop sign to node 2 + node2.tags = { 'direction': 'forward', 'highway': 'stop' }; + // Create our way + var way = iD.Way({nodes: [node1.id, node2.id, node3.id]}); + // Act - reverse the way + var graph = iD.actions.Reverse(way.id)(iD.Graph([node1, node2, node3, way])); + // Assert - confirm that the stop sign on node 2 has changed direction + var target = graph.entity(node2.id); + expect(target.tags.direction).to.eql('backward'); + }); + + it('reverses the direction of a backward facing stop sign on the way', function () { + var node1 = iD.Node(); + var node2 = iD.Node(); + var node3 = iD.Node(); + // Attach a backward facing stop sign to node 2 + node2.tags = { 'direction': 'backward', 'highway': 'stop' }; + // Create our way + var way = iD.Way({nodes: [node1.id, node2.id, node3.id]}); + // Act - reverse the way + var graph = iD.actions.Reverse(way.id)(iD.Graph([node1, node2, node3, way])); + // Assert - confirm that the stop sign on node 2 has changed direction + var target = graph.entity(node2.id); + expect(target.tags.direction).to.eql('forward'); + }); + + it('does not assign a direction to a directionless stop sign on the way during a reverse', function () { + var node1 = iD.Node(); + var node2 = iD.Node(); + var node3 = iD.Node(); + // Attach a stop sign to node 2 with no direction specified + node2.tags = { 'highway': 'stop' }; + // Create our way + var way = iD.Way({nodes: [node1.id, node2.id, node3.id]}); + // Act - reverse the way + var graph = iD.actions.Reverse(way.id)(iD.Graph([node1, node2, node3, way])); + // Assert - confirm that the stop sign on node 2 has not gained a direction tag + var target = graph.entity(node2.id); + expect(target.tags.direction).to.be.undefined; + }); + + it('ignores directions other than forward or backward on attached stop sign during a reverse', function () { + var node1 = iD.Node(); + var node2 = iD.Node(); + var node3 = iD.Node(); + // Attach a stop sign to node 2 with a non-standard direction + node2.tags = { 'direction': 'empty', 'highway': 'stop' }; + // Create our way + var way = iD.Way({nodes: [node1.id, node2.id, node3.id]}); + // Act - reverse the way + var graph = iD.actions.Reverse(way.id)(iD.Graph([node1, node2, node3, way])); + // Assert - confirm that the stop sign on node 2 has not had its direction tag altered + var target = graph.entity(node2.id); + expect(target.tags.direction).to.eql('empty'); + }); + + it('reverses the direction of a forward facing traffic sign on the way', function () { + var node1 = iD.Node(); + var node2 = iD.Node(); + var node3 = iD.Node(); + // Attach a forward facing stop sign to node 2 using the traffic_sign approach + node2.tags = { 'traffic_sign:forward': 'stop' }; + // Create our way + var way = iD.Way({nodes: [node1.id, node2.id, node3.id]}); + // Act - reverse the way + var graph = iD.actions.Reverse(way.id)(iD.Graph([node1, node2, node3, way])); + // Assert - confirm that the stop sign on node 2 has changed direction + var target = graph.entity(node2.id); + expect(target.tags['traffic_sign:backward']).to.eql('stop'); + }); + + it('reverses the direction of a backward facing stop sign on the way', function () { + var node1 = iD.Node(); + var node2 = iD.Node(); + var node3 = iD.Node(); + // Attach a backward facing stop sign to node 2 using the traffic_sign approach + node2.tags = { 'traffic_sign:backward': 'stop' }; + // Create our way + var way = iD.Way({nodes: [node1.id, node2.id, node3.id]}); + // Act - reverse the way + var graph = iD.actions.Reverse(way.id)(iD.Graph([node1, node2, node3, way])); + // Assert - confirm that the stop sign on node 2 has changed direction + var target = graph.entity(node2.id); + expect(target.tags['traffic_sign:forward']).to.eql('stop'); + }); + }); From 08ca3e6fa3291ae6b1bd6ef83863e261ccefc675 Mon Sep 17 00:00:00 2001 From: Jon D Date: Thu, 25 Aug 2016 00:10:30 +0100 Subject: [PATCH 2/5] Add implementation for directional tag reversal for nodes on a way. --- modules/actions/reverse.js | 43 +++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/modules/actions/reverse.js b/modules/actions/reverse.js index 1fb18ad85..8d896047f 100644 --- a/modules/actions/reverse.js +++ b/modules/actions/reverse.js @@ -1,3 +1,4 @@ +import _ from 'lodash'; /* Order the nodes of a way in reverse order and reverse any direction dependent tags other than `oneway`. (We assume that correcting a backwards oneway is the primary @@ -22,12 +23,21 @@ The JOSM implementation was used as a guide, but transformations that were of unclear benefit or adjusted tags that don't seem to be used in practice were omitted. + Also, each node on the way is examined for its own tags and the following transformations are performed + in order to ensure associated nodes (eg a Stop Sign) is also reversed + + Node Keys: + direction=forward ⟺ direction=backward + *:forward=* ⟺ *:backward=* + References: http://wiki.openstreetmap.org/wiki/Forward_%26_backward,_left_%26_right http://wiki.openstreetmap.org/wiki/Key:direction#Steps http://wiki.openstreetmap.org/wiki/Key:incline http://wiki.openstreetmap.org/wiki/Route#Members http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/corrector/ReverseWayTagCorrector.java + http://wiki.openstreetmap.org/wiki/Tag:highway%3Dstop + http://wiki.openstreetmap.org/wiki/Key:traffic_sign#On_a_way_or_area */ export function Reverse(wayId, options) { var replacements = [ @@ -66,6 +76,35 @@ export function Reverse(wayId, options) { } } + function reverseDirectionTags(node) { + // Update the direction based tags as appropriate then return an updated node + return node.update({tags: _.transform(node.tags, function(acc, tagValue, tagKey) { + // See if this is a direction tag and reverse (or use existing value if not forward/backward) + if (tagKey === 'direction') { + acc[tagKey] = tagValue === 'forward' ? 'backward' + : tagValue === 'backward' ? 'forward' : tagValue; + } else { + // Use the reverseKey method to cater for situations such as traffic_sign:forward=stop + // This will pass through other tags unchanged + acc[reverseKey(tagKey)] = tagValue; + } + return acc; + }, {})}); + } + + function reverseTagsOnNodes(graph, nodeIds) { + // Reverse the direction of appropriate tags attached to the nodes (#3076) + return _(nodeIds) + // Get each node from the graph + .map(function(nodeId) { return graph.entity(nodeId);}) + // Check tags on the node, if there aren't any, we can skip + .filter(function(existingNode) { return existingNode.tags !== undefined;}) + // Get a new version of each node with the appropriate tags reversed + .map(function(existingNode) { return reverseDirectionTags(existingNode);}) + // Chain together consecutive updates to the graph for each updated node and return + .reduce(function (accGraph, value) { return accGraph.replace(value); }, graph); + } + return function(graph) { var way = graph.entity(wayId), nodes = way.nodes.slice().reverse(), @@ -84,6 +123,8 @@ export function Reverse(wayId, options) { }); }); - return graph.replace(way.update({nodes: nodes, tags: tags})); + // Reverse any associated directions on nodes on the way and then replace + // the way itself with the reversed node ids and updated way tags + return reverseTagsOnNodes(graph, nodes).replace(way.update({nodes: nodes, tags: tags})); }; } From 95ac00ffbf9824709178bbd4743b252e02e4aba2 Mon Sep 17 00:00:00 2001 From: Jon D Date: Thu, 25 Aug 2016 06:54:54 +0100 Subject: [PATCH 3/5] Add tests for left/right reversal. --- test/spec/actions/reverse.js | 62 ++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/test/spec/actions/reverse.js b/test/spec/actions/reverse.js index cc92959d5..2e2ce7aef 100644 --- a/test/spec/actions/reverse.js +++ b/test/spec/actions/reverse.js @@ -188,6 +188,38 @@ describe('iD.actions.Reverse', function () { expect(target.tags.direction).to.eql('forward'); }); + it('reverses the direction of a left facing stop sign on the way', function () { + var node1 = iD.Node(); + var node2 = iD.Node(); + var node3 = iD.Node(); + // Attach a left facing stop sign to node 2 (not sure this is a real situation, + // but allows us to test) + node2.tags = { 'direction': 'left', 'highway': 'stop' }; + // Create our way + var way = iD.Way({nodes: [node1.id, node2.id, node3.id]}); + // Act - reverse the way + var graph = iD.actions.Reverse(way.id)(iD.Graph([node1, node2, node3, way])); + // Assert - confirm that the stop sign on node 2 has changed direction + var target = graph.entity(node2.id); + expect(target.tags.direction).to.eql('right'); + }); + + it('reverses the direction of a right facing stop sign on the way', function () { + var node1 = iD.Node(); + var node2 = iD.Node(); + var node3 = iD.Node(); + // Attach a right facing stop sign to node 2 (not sure this is a real situation, + // but allows us to test) + node2.tags = { 'direction': 'right', 'highway': 'stop' }; + // Create our way + var way = iD.Way({nodes: [node1.id, node2.id, node3.id]}); + // Act - reverse the way + var graph = iD.actions.Reverse(way.id)(iD.Graph([node1, node2, node3, way])); + // Assert - confirm that the stop sign on node 2 has changed direction + var target = graph.entity(node2.id); + expect(target.tags.direction).to.eql('left'); + }); + it('does not assign a direction to a directionless stop sign on the way during a reverse', function () { var node1 = iD.Node(); var node2 = iD.Node(); @@ -248,4 +280,34 @@ describe('iD.actions.Reverse', function () { expect(target.tags['traffic_sign:forward']).to.eql('stop'); }); + it('reverses the direction of a left facing traffic sign on the way', function () { + var node1 = iD.Node(); + var node2 = iD.Node(); + var node3 = iD.Node(); + // Attach a left facing stop sign to node 2 using the traffic_sign approach + node2.tags = { 'traffic_sign:left': 'stop' }; + // Create our way + var way = iD.Way({nodes: [node1.id, node2.id, node3.id]}); + // Act - reverse the way + var graph = iD.actions.Reverse(way.id)(iD.Graph([node1, node2, node3, way])); + // Assert - confirm that the stop sign on node 2 has changed direction + var target = graph.entity(node2.id); + expect(target.tags['traffic_sign:right']).to.eql('stop'); + }); + + it('reverses the direction of a right facing stop sign on the way', function () { + var node1 = iD.Node(); + var node2 = iD.Node(); + var node3 = iD.Node(); + // Attach a right facing stop sign to node 2 using the traffic_sign approach + node2.tags = { 'traffic_sign:right': 'stop' }; + // Create our way + var way = iD.Way({nodes: [node1.id, node2.id, node3.id]}); + // Act - reverse the way + var graph = iD.actions.Reverse(way.id)(iD.Graph([node1, node2, node3, way])); + // Assert - confirm that the stop sign on node 2 has changed direction + var target = graph.entity(node2.id); + expect(target.tags['traffic_sign:left']).to.eql('stop'); + }); + }); From 1b09b0dd60ea5b12c849a40c5d4858096f6a1383 Mon Sep 17 00:00:00 2001 From: Jon D Date: Thu, 25 Aug 2016 07:03:35 +0100 Subject: [PATCH 4/5] Add implementation for left/right exchange on direction tags. --- modules/actions/reverse.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/modules/actions/reverse.js b/modules/actions/reverse.js index 8d896047f..cb4697e79 100644 --- a/modules/actions/reverse.js +++ b/modules/actions/reverse.js @@ -28,7 +28,9 @@ import _ from 'lodash'; Node Keys: direction=forward ⟺ direction=backward - *:forward=* ⟺ *:backward=* + direction=left ⟺ direction=right + *:forward=* ⟺ *:backward=* + *:left=* ⟺ *:right=* References: http://wiki.openstreetmap.org/wiki/Forward_%26_backward,_left_%26_right @@ -81,8 +83,7 @@ export function Reverse(wayId, options) { return node.update({tags: _.transform(node.tags, function(acc, tagValue, tagKey) { // See if this is a direction tag and reverse (or use existing value if not forward/backward) if (tagKey === 'direction') { - acc[tagKey] = tagValue === 'forward' ? 'backward' - : tagValue === 'backward' ? 'forward' : tagValue; + acc[tagKey] = {forward: 'backward', backward: 'forward', left: 'right', right: 'left'}[tagValue] || tagValue; } else { // Use the reverseKey method to cater for situations such as traffic_sign:forward=stop // This will pass through other tags unchanged From 1dfa5aa77d428f5c4dd586eea984054ce4cdc820 Mon Sep 17 00:00:00 2001 From: Jon D Date: Thu, 25 Aug 2016 07:07:10 +0100 Subject: [PATCH 5/5] Fix comment --- modules/actions/reverse.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/actions/reverse.js b/modules/actions/reverse.js index cb4697e79..b6582b18f 100644 --- a/modules/actions/reverse.js +++ b/modules/actions/reverse.js @@ -81,7 +81,7 @@ export function Reverse(wayId, options) { function reverseDirectionTags(node) { // Update the direction based tags as appropriate then return an updated node return node.update({tags: _.transform(node.tags, function(acc, tagValue, tagKey) { - // See if this is a direction tag and reverse (or use existing value if not forward/backward) + // See if this is a direction tag and reverse (or use existing value if not recognised) if (tagKey === 'direction') { acc[tagKey] = {forward: 'backward', backward: 'forward', left: 'right', right: 'left'}[tagValue] || tagValue; } else {