From dee005e9aa9a3070db5467099296503d7750ad00 Mon Sep 17 00:00:00 2001 From: Xiaoming Gao Date: Thu, 20 Dec 2018 15:42:21 -0500 Subject: [PATCH] Improve cross way check 1. Add more way types and relation types to check 2. Add more legit crossing cases 3. Add tests for 1 and 2 --- modules/validations/crossing_ways.js | 112 ++++++++++++-- test/spec/validations/crossing_ways.js | 197 +++++++++++++++++++++++-- 2 files changed, 288 insertions(+), 21 deletions(-) diff --git a/modules/validations/crossing_ways.js b/modules/validations/crossing_ways.js index 8c23b62e1..1b47bdf83 100644 --- a/modules/validations/crossing_ways.js +++ b/modules/validations/crossing_ways.js @@ -1,3 +1,4 @@ +import _clone from 'lodash-es/clone'; import { geoExtent, geoLineIntersection } from '../geo'; import { set as d3_set } from 'd3-collection'; import { t } from '../util/locale'; @@ -39,10 +40,104 @@ export function validationHighwayCrossingOtherWays() { return n1 > nA ? n1 + n2 + nA + nB : nA + nB + n1 + n2; } + function getWayAndParentRelationTags(way, graph) { + var tags = _clone(way.tags); + var parentRels = graph.parentRelations(way); + for (var i = 0; i < parentRels.length; i++) { + var rel = parentRels[i]; + for (k in rel.tags) { + if (!tags[k]) tags[k] = rel.tags[k]; + } + } + return tags; + } + + function hasTag(tags, key) { + return tags[key] !== undefined && tags[key] !== 'no'; + } + + function getFeatureTypeForCrossingCheck(way, graph) { + var tags = getWayAndParentRelationTags(way, graph); + if (hasTag(tags, 'highway')) return 'highway'; + if (hasTag(tags, 'building')) return 'building'; + if (hasTag(tags, 'railway')) return 'railway'; + if (hasTag(tags, 'waterway') || tags.natural === 'water') return 'water'; + + return null; + } + + function extendTagsByInferredLayer(tags, way) { + if (!hasTag(tags, 'layer')) { + tags.layer = way.layer().toString(); + } + return tags; + } + + function isLegitCrossing(way1, featureType1, way2, featureType2, graph) { + var tags1 = getWayAndParentRelationTags(way1, graph), + tags2 = getWayAndParentRelationTags(way2, graph); + tags1 = extendTagsByInferredLayer(tags1, way1); + tags2 = extendTagsByInferredLayer(tags2, way2); + + // For better readability, not chaining all the true conditions into one if statement. + if ((featureType1 === 'highway' && featureType2 === 'highway') || + (featureType1 === 'highway' && featureType2 === 'railway') || + (featureType1 === 'railway' && featureType2 === 'railway')) { + // Legit cases: + // (1) they're on different layers + // (2) only one of the two ways is on a bridge + // (3) only one of the two ways is in a tunnel + if (tags1.layer !== tags2.layer) return true; + if (hasTag(tags1, 'bridge') && !hasTag(tags2, 'bridge')) return true; + if (!hasTag(tags1, 'bridge') && hasTag(tags2, 'bridge')) return true; + if (hasTag(tags1, 'tunnel') && !hasTag(tags2, 'tunnel')) return true; + if (!hasTag(tags1, 'tunnel') && hasTag(tags2, 'tunnel')) return true; + } + if ((featureType1 === 'highway' && featureType2 === 'water') || + (featureType1 === 'railway' && featureType2 === 'water')) { + // Legit cases: + // (1) highway/railway is on a bridge + // (2) only one of the two ways is in a tunnel + // (3) both are in tunnels but on different layers + if (hasTag(tags1, 'bridge')) return true; + if (hasTag(tags1, 'tunnel') && !hasTag(tags2, 'tunnel')) return true; + if (!hasTag(tags1, 'tunnel') && hasTag(tags2, 'tunnel')) return true; + if (hasTag(tags1, 'tunnel') && hasTag(tags2, 'tunnel') && tags1.layer !== tags2.layer) return true; + } + if ((featureType1 === 'highway' && featureType2 === 'building') || + (featureType1 === 'railway' && featureType2 === 'building')) { + // Legit cases: + // (1) highway/railway has a bridge or tunnel tag + // (2) highway/railway has a covered tag + if (hasTag(tags1, 'bridge') || hasTag(tags1, 'tunnel') || hasTag(tags1, 'covered')) return true; + } + if (featureType1 === 'water' && featureType2 === 'water') { + // Legit cases: + // (1) only one of the water is in a tunnel + // (2) both are in tunnels but on differnt layers + if (hasTag(tags1, 'tunnel') && !hasTag(tags2, 'tunnel')) return true; + if (!hasTag(tags1, 'tunnel') && hasTag(tags2, 'tunnel')) return true; + if (hasTag(tags1, 'tunnel') && hasTag(tags2, 'tunnel') && tags1.layer !== tags2.layer) return true; + } + if (featureType1 === 'water' && featureType2 === 'building') { + // Legit cases: + // (1) water is in a tunnel + // (2) water has a covered tag + if (hasTag(tags1, 'tunnel') || hasTag(tags1, 'covered')) return true; + } + if (featureType1 === 'building' && featureType2 === 'building') { + // Legit case: they're on different layers + if (tags1.layer !== tags2.layer) return true; + } + return false; + } + function findCrossingsByWay(entity, graph, tree, edgePairsVisited) { var edgeCrossInfos = []; - if (entity.type !== 'way' || !entity.tags.highway) return edgeCrossInfos; - if (entity.geometry(graph) !== 'line') return edgeCrossInfos; + if (entity.geometry(graph) !== 'line' || entity.type !== 'way') return edgeCrossInfos; + + var entFeatureType = getFeatureTypeForCrossingCheck(entity, graph); + if (entFeatureType === null) return edgeCrossInfos; for (var i = 0; i < entity.nodes.length - 1; i++) { var nid1 = entity.nodes[i], @@ -64,14 +159,14 @@ export function validationHighwayCrossingOtherWays() { if (intersected[j].type !== 'way') continue; // only check crossing highway, waterway, building, and railway - var way = intersected[j]; - if (!(way.tags.highway || way.tags.building || - way.tags.railway || way.tags.waterway)) { - continue; - } - if (way.tags.waterway && entity.tags.bridge === 'yes') { + var way = intersected[j], + wayFeatureType = getFeatureTypeForCrossingCheck(way, graph); + if (wayFeatureType === null || + isLegitCrossing(entity, entFeatureType, way, wayFeatureType, graph) || + isLegitCrossing(way, wayFeatureType, entity, entFeatureType, graph)) { continue; } + var crossCoords = findEdgeToWayCrossCoords(n1, n2, way, graph, edgePairsVisited); for (var k = 0; k < crossCoords.length; k++) { edgeCrossInfos.push({ @@ -84,7 +179,6 @@ export function validationHighwayCrossingOtherWays() { return edgeCrossInfos; } - var validation = function(changes, graph, tree) { // create one issue per crossing point var edited = changes.created.concat(changes.modified), diff --git a/test/spec/validations/crossing_ways.js b/test/spec/validations/crossing_ways.js index f90a59983..4b1985b15 100644 --- a/test/spec/validations/crossing_ways.js +++ b/test/spec/validations/crossing_ways.js @@ -5,10 +5,10 @@ describe('iD.validations.crossing_ways', function () { context = iD.Context(); }); - function createWaysWithOneCrossingPoint() { + function createWaysWithOneCrossingPoint(tags1, tags2) { var n1 = iD.Node({id: 'n-1', loc: [1,1]}); var n2 = iD.Node({id: 'n-2', loc: [2,2]}); - var w1 = iD.Way({id: 'w-1', nodes: ['n-1', 'n-2'], tags: { highway: 'residential' }}); + var w1 = iD.Way({id: 'w-1', nodes: ['n-1', 'n-2'], tags: tags1}); context.perform( iD.actionAddEntity(n1), @@ -18,7 +18,7 @@ describe('iD.validations.crossing_ways', function () { var n3 = iD.Node({id: 'n-3', loc: [1,2]}); var n4 = iD.Node({id: 'n-4', loc: [2,1]}); - var w2 = iD.Way({id: 'w-2', nodes: ['n-3', 'n-4'], tags: { highway: 'residential' }}); + var w2 = iD.Way({id: 'w-2', nodes: ['n-3', 'n-4'], tags: tags2}); context.perform( iD.actionAddEntity(n3), @@ -53,20 +53,35 @@ describe('iD.validations.crossing_ways', function () { ); } + function createHighwayCrossingRailway() { + var n1 = iD.Node({id: 'n-1', loc: [1,1]}); + var n2 = iD.Node({id: 'n-2', loc: [2,2]}); + var w1 = iD.Way({id: 'w-1', nodes: ['n-1', 'n-2'], tags: { highway: 'residential' }}); + + context.perform( + iD.actionAddEntity(n1), + iD.actionAddEntity(n2), + iD.actionAddEntity(w1) + ); + + var n3 = iD.Node({id: 'n-3', loc: [1,2]}); + var n4 = iD.Node({id: 'n-4', loc: [2,1]}); + var w2 = iD.Way({id: 'w-2', nodes: ['n-3', 'n-4'], tags: { railway: 'rail' }}); + + context.perform( + iD.actionAddEntity(n3), + iD.actionAddEntity(n4), + iD.actionAddEntity(w2) + ); + } + function validate() { var validator = iD.validationHighwayCrossingOtherWays(); var changes = context.history().changes(); return validator(changes, context.graph(), context.history().tree()); } - it('has no errors on init', function() { - var issues = validate(); - expect(issues).to.have.lengthOf(0); - }); - - it('one cross point between two ways', function() { - createWaysWithOneCrossingPoint(); - var issues = validate(); + function verifySingleCrossingIssue(issues) { expect(issues).to.have.lengthOf(1); var issue = issues[0]; expect(issue.type).to.eql(iD.ValidationIssueType.crossing_ways); @@ -77,9 +92,126 @@ describe('iD.validations.crossing_ways', function () { expect(issue.coordinates).to.have.lengthOf(2); expect(issue.coordinates[0]).to.eql(1.5); expect(issue.coordinates[1]).to.eql(1.5); + } + + it('has no errors on init', function() { + var issues = validate(); + expect(issues).to.have.lengthOf(0); }); - it('two cross points between two ways', function() { + // legit crossing cases + it('legit crossing between highway and highway', function() { + createWaysWithOneCrossingPoint({ highway: 'residential', tunnel: 'yes', layer: '-1' }, { highway: 'residential' }); + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('legit crossing between highway and railway', function() { + createWaysWithOneCrossingPoint({ highway: 'residential' }, { railway: 'rail', bridge: 'yes' }); + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('legit crossing between highway and waterway', function() { + createWaysWithOneCrossingPoint({ highway: 'residential', bridge: 'yes' }, { waterway: 'river' }); + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('legit crossing between highway and building', function() { + createWaysWithOneCrossingPoint({ highway: 'residential', covered: 'yes' }, { building: 'yes' }); + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('legit crossing between railway and railway', function() { + createWaysWithOneCrossingPoint({ railway: 'rail', layer: '1' }, { railway: 'rail' }); + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('legit crossing between railway and waterway', function() { + createWaysWithOneCrossingPoint({ railway: 'rail' }, { waterway: 'river', tunnel: 'yes' }); + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('legit crossing between railway and building', function() { + createWaysWithOneCrossingPoint({ railway: 'rail', covered: 'yes' }, { building: 'yes' }); + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('legit crossing between waterway and waterway', function() { + createWaysWithOneCrossingPoint({ waterway: 'canal', tunnel: 'yes' }, { waterway: 'river' }); + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('legit crossing between waterway and building', function() { + createWaysWithOneCrossingPoint({ waterway: 'river', covered: 'yes' }, { building: 'yes' }); + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('legit crossing between building and building', function() { + createWaysWithOneCrossingPoint({ building: 'yes' }, { building: 'yes', covered: 'yes' }); + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + // warning crossing cases between ways + it('one cross point between highway and highway', function() { + createWaysWithOneCrossingPoint({ highway: 'residential' }, { highway: 'residential' }); + verifySingleCrossingIssue(validate()); + }); + + it('one cross point between highway and railway', function() { + createWaysWithOneCrossingPoint({ highway: 'residential' }, { railway: 'rail' }); + verifySingleCrossingIssue(validate()); + }); + + it('one cross point between highway and waterway', function() { + createWaysWithOneCrossingPoint({ highway: 'residential' }, { waterway: 'river' }); + verifySingleCrossingIssue(validate()); + }); + + it('one cross point between highway and building', function() { + createWaysWithOneCrossingPoint({ highway: 'residential' }, { building: 'yes' }); + verifySingleCrossingIssue(validate()); + }); + + it('one cross point between railway and railway', function() { + createWaysWithOneCrossingPoint({ railway: 'rail' }, { railway: 'rail' }); + verifySingleCrossingIssue(validate()); + }); + + it('one cross point between railway and waterway', function() { + createWaysWithOneCrossingPoint({ railway: 'rail' }, { waterway: 'river' }); + verifySingleCrossingIssue(validate()); + }); + + it('one cross point between railway and building', function() { + createWaysWithOneCrossingPoint({ railway: 'rail' }, { building: 'yes' }); + verifySingleCrossingIssue(validate()); + }); + + it('one cross point between waterway and waterway', function() { + createWaysWithOneCrossingPoint({ waterway: 'canal' }, { waterway: 'river' }); + verifySingleCrossingIssue(validate()); + }); + + it('one cross point between waterway and building', function() { + createWaysWithOneCrossingPoint({ waterway: 'river' }, { building: 'yes' }); + verifySingleCrossingIssue(validate()); + }); + + it('one cross point between building and building', function() { + createWaysWithOneCrossingPoint({ building: 'yes' }, { building: 'yes' }); + verifySingleCrossingIssue(validate()); + }); + + it('two cross points between two highways', function() { createWaysWithTwoCrossingPoint(); var issues = validate(); expect(issues).to.have.lengthOf(2); @@ -103,4 +235,45 @@ describe('iD.validations.crossing_ways', function () { expect(issue.coordinates[0]).to.eql(2.5); expect(issue.coordinates[1]).to.eql(2.5); }); + + function createWayAndRelationWithOneCrossingPoint(wayTags, relTags) { + var n1 = iD.Node({id: 'n-1', loc: [1,1]}); + var n2 = iD.Node({id: 'n-2', loc: [2,2]}); + var w1 = iD.Way({id: 'w-1', nodes: ['n-1', 'n-2'], tags: wayTags}); + + context.perform( + iD.actionAddEntity(n1), + iD.actionAddEntity(n2), + iD.actionAddEntity(w1) + ); + + var n3 = iD.Node({id: 'n-3', loc: [1,2]}); + var n4 = iD.Node({id: 'n-4', loc: [2,1]}); + var n5 = iD.Node({id: 'n-5', loc: [3,2]}); + var n6 = iD.Node({id: 'n-6', loc: [2,3]}); + var w2 = iD.Way({id: 'w-2', nodes: ['n-3', 'n-4', 'n-5'], tags: {}}); + var w3 = iD.Way({id: 'w-3', nodes: ['n-5', 'n-6', 'n-3'], tags: {}}); + var r1 = iD.Relation({id: 'r-1', members: [{id: 'w-2'}, {id: 'w-3'}], tags: relTags}); + + context.perform( + iD.actionAddEntity(n3), + iD.actionAddEntity(n4), + iD.actionAddEntity(n5), + iD.actionAddEntity(n6), + iD.actionAddEntity(w2), + iD.actionAddEntity(w3), + iD.actionAddEntity(r1), + ); + } + + // warning crossing cases between way and relation + it('one cross point between highway and water relation', function() { + createWayAndRelationWithOneCrossingPoint({ highway: 'residential' }, { natural: 'water' }); + verifySingleCrossingIssue(validate()); + }); + + it('one cross point between railway and building relation', function() { + createWayAndRelationWithOneCrossingPoint({ highway: 'residential' }, { building: 'yes' }); + verifySingleCrossingIssue(validate()); + }); });