From ecc217f5d811253a0990bbedfeeab84663f7c75f Mon Sep 17 00:00:00 2001 From: Quincy Morgan Date: Fri, 26 Apr 2019 12:04:43 -0700 Subject: [PATCH] Add validation rule to flag impossible oneway highways and waterways (close #6216) --- data/core.yaml | 19 +++ data/deprecated.json | 14 ++ data/taginfo.json | 4 + dist/locales/en.json | 28 ++++ modules/core/validator.js | 9 +- modules/operations/reverse.js | 1 + modules/osm/tags.js | 20 +++ modules/validations/almost_junction.js | 5 +- modules/validations/crossing_ways.js | 23 +-- modules/validations/impossible_oneway.js | 176 +++++++++++++++++++++++ modules/validations/index.js | 1 + 11 files changed, 274 insertions(+), 26 deletions(-) create mode 100644 modules/validations/impossible_oneway.js diff --git a/data/core.yaml b/data/core.yaml index f2c1ad8d0..1a14e20da 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -1403,6 +1403,23 @@ en: unknown_road: message: "{feature} has no classification" reference: "Roads without a specific type may not appear in maps or routing." + impossible_oneway: + title: Impossible One-Ways + tip: "Find route issues with one-way features" + waterway: + connected: + start: + message: "{feature} flows away from a connected waterway" + end: + message: "{feature} flows against a connected waterway" + reference: "Waterway segements should all flow in the same direction." + highway: + start: + message: "{feature} is unreachable" + reference: "One-way roads must be accessible via other roads." + end: + message: "{feature} has no outlet" + reference: "One-way roads must lead to other roads." unsquare_way: title: Unsquare Buildings message: "{feature} isn't quite square" @@ -1444,6 +1461,8 @@ en: title: Remove the tags reposition_features: title: Reposition the features + reverse_feature: + title: Reverse this feature select_preset: title: Select a feature type select_road_type: diff --git a/data/deprecated.json b/data/deprecated.json index 7702fcec3..0da58f2c2 100644 --- a/data/deprecated.json +++ b/data/deprecated.json @@ -467,6 +467,20 @@ "old": {"office": "real_estate"}, "replace": {"office": "estate_agent"} }, + { + "old": {"oneway": "1"}, + "replace": {"oneway": "yes"} + }, + { + "old": {"oneway": "alternate"}, + "replace": {"oneway": "alternating"} + }, + { + "old": {"oneway": "no;yes"} + }, + { + "old": {"oneway": "unknown"} + }, { "old": {"place_name": "*"}, "replace": {"name": "$1"} diff --git a/data/taginfo.json b/data/taginfo.json index 5983ef2d2..d24ea0bc3 100644 --- a/data/taginfo.json +++ b/data/taginfo.json @@ -1801,6 +1801,10 @@ {"key": "natural", "value": "marsh", "description": "🄳 ➜ natural=wetland + wetland=marsh"}, {"key": "natural", "value": "waterfall", "description": "🄳 ➜ waterway=waterfall"}, {"key": "office", "value": "real_estate", "description": "🄳 ➜ office=estate_agent"}, + {"key": "oneway", "value": "1", "description": "🄳 ➜ oneway=yes"}, + {"key": "oneway", "value": "alternate", "description": "🄳 ➜ oneway=alternating"}, + {"key": "oneway", "value": "no;yes", "description": "🄳"}, + {"key": "oneway", "value": "unknown", "description": "🄳"}, {"key": "place_name", "description": "🄳 ➜ name=*"}, {"key": "pole", "value": "transition", "description": "🄳 ➜ location:transition=yes"}, {"key": "postcode", "description": "🄳 ➜ addr:postcode=*"}, diff --git a/dist/locales/en.json b/dist/locales/en.json index ac80a779f..983f64180 100644 --- a/dist/locales/en.json +++ b/dist/locales/en.json @@ -1737,6 +1737,31 @@ "message": "{feature} has no classification", "reference": "Roads without a specific type may not appear in maps or routing." }, + "impossible_oneway": { + "title": "Impossible One-Ways", + "tip": "Find route issues with one-way features", + "waterway": { + "connected": { + "start": { + "message": "{feature} flows away from a connected waterway" + }, + "end": { + "message": "{feature} flows against a connected waterway" + }, + "reference": "Waterway segements should all flow in the same direction." + } + }, + "highway": { + "start": { + "message": "{feature} is unreachable", + "reference": "One-way roads must be accessible via other roads." + }, + "end": { + "message": "{feature} has no outlet", + "reference": "One-way roads must lead to other roads." + } + } + }, "unsquare_way": { "title": "Unsquare Buildings", "message": "{feature} isn't quite square", @@ -1795,6 +1820,9 @@ "reposition_features": { "title": "Reposition the features" }, + "reverse_feature": { + "title": "Reverse this feature" + }, "select_preset": { "title": "Select a feature type" }, diff --git a/modules/core/validator.js b/modules/core/validator.js index 382484080..093b04cc7 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -218,12 +218,13 @@ export function coreValidator(context) { if (entity.type === 'way') { runValidation('crossing_ways'); + runValidation('almost_junction'); - // only check for disconnected way if no almost junctions - if (runValidation('almost_junction')) { - runValidation('disconnected_way'); + // only check impossible_oneway if no disconnected_way issues + if (runValidation('disconnected_way')) { + runValidation('impossible_oneway'); } else { - ran.disconnected_way = true; + ran.impossible_oneway = true; } runValidation('tag_suggests_area'); diff --git a/modules/operations/reverse.js b/modules/operations/reverse.js index ba2ca84b0..0f0a0952b 100644 --- a/modules/operations/reverse.js +++ b/modules/operations/reverse.js @@ -8,6 +8,7 @@ export function operationReverse(selectedIDs, context) { var operation = function() { context.perform(actionReverse(entityID), operation.annotation()); + context.validator().validate(); }; diff --git a/modules/osm/tags.js b/modules/osm/tags.js index 5a7c946f2..8103b68bf 100644 --- a/modules/osm/tags.js +++ b/modules/osm/tags.js @@ -72,3 +72,23 @@ export var osmRightSideIsInsideTags = { 'embankment': true } }; + +// "highway" tag values for pedestrian or vehicle right-of-ways that make up the routable network +export var osmRoutableHighwayTagValues = { + motorway: true, trunk: true, primary: true, secondary: true, tertiary: true, residential: true, + motorway_link: true, trunk_link: true, primary_link: true, secondary_link: true, tertiary_link: true, + unclassified: true, road: true, service: true, track: true, living_street: true, raceway: true, bus_guideway: true, + path: true, footway: true, cycleway: true, bridleway: true, pedestrian: true, corridor: true, steps: true +}; + +// "railway" tag values representing existing railroad tracks (purposely does not include 'abandoned') +export var osmRailwayTrackTagValues = { + rail: true, light_rail: true, tram: true, subway: true, + monorail: true, funicular: true, miniature: true, narrow_gauge: true, + disused: true, preserved: true +}; + +// "waterway" tag values for line features representing water flow +export var osmFlowingWaterwayTagValues = { + canal: true, ditch: true, drain: true, river: true, stream: true +}; diff --git a/modules/validations/almost_junction.js b/modules/validations/almost_junction.js index f432f6560..161ec112e 100644 --- a/modules/validations/almost_junction.js +++ b/modules/validations/almost_junction.js @@ -9,6 +9,7 @@ import { actionChangeTags } from '../actions/change_tags'; import { actionMergeNodes } from '../actions/merge_nodes'; import { t } from '../util/locale'; import { utilDisplayLabel } from '../util'; +import { osmRoutableHighwayTagValues } from '../osm/tags'; import { validationIssue, validationIssueFix } from '../core/validation'; @@ -21,9 +22,7 @@ export function validationAlmostJunction() { function isHighway(entity) { return entity.type === 'way' && - entity.tags.highway && - entity.tags.highway !== 'no' && - entity.tags.highway !== 'proposed'; + osmRoutableHighwayTagValues[entity.tags.highway]; } function isNoexit(node) { diff --git a/modules/validations/crossing_ways.js b/modules/validations/crossing_ways.js index a94895157..112108081 100644 --- a/modules/validations/crossing_ways.js +++ b/modules/validations/crossing_ways.js @@ -3,6 +3,7 @@ import { actionChangeTags } from '../actions/change_tags'; import { actionMergeNodes } from '../actions/merge_nodes'; import { geoExtent, geoLineIntersection, geoSphericalClosestNode } from '../geo'; import { osmNode } from '../osm/node'; +import { osmFlowingWaterwayTagValues, osmRailwayTrackTagValues, osmRoutableHighwayTagValues } from '../osm/tags'; import { t } from '../util/locale'; import { utilDisplayLabel } from '../util'; import { validationIssue, validationIssueFix } from '../core/validation'; @@ -67,22 +68,6 @@ export function validationCrossingWays() { return getFeatureTypeForTags(tags); } - // whitelists - var waterways = { - canal: true, ditch: true, drain: true, river: true, stream: true - }; - var railways = { - rail: true, disused: true, tram: true, subway: true, narrow_gauge: true, - light_rail: true, preserved: true, miniature: true, monorail: true, funicular: true - }; - var highways = { - residential: true, service: true, track: true, unclassified: true, footway: true, - path: true, tertiary: true, secondary: true, primary: true, living_street: true, - cycleway: true, trunk: true, steps: true, motorway: true, motorway_link: true, - pedestrian: true, trunk_link: true, primary_link: true, secondary_link: true, - road: true, tertiary_link: true, bridleway: true, raceway: true, corridor: true, - bus_guideway: true - }; // blacklist var ignoredBuildings = { demolished: true, dismantled: true, proposed: true, razed: true @@ -95,9 +80,9 @@ export function validationCrossingWays() { // don't check non-building areas if (hasTag(tags, 'area')) return null; - if (hasTag(tags, 'highway') && highways[tags.highway]) return 'highway'; - if (hasTag(tags, 'railway') && railways[tags.railway]) return 'railway'; - if (hasTag(tags, 'waterway') && waterways[tags.waterway]) return 'waterway'; + if (hasTag(tags, 'highway') && osmRoutableHighwayTagValues[tags.highway]) return 'highway'; + if (hasTag(tags, 'railway') && osmRailwayTrackTagValues[tags.railway]) return 'railway'; + if (hasTag(tags, 'waterway') && osmFlowingWaterwayTagValues[tags.waterway]) return 'waterway'; return null; } diff --git a/modules/validations/impossible_oneway.js b/modules/validations/impossible_oneway.js new file mode 100644 index 000000000..ae0b4f6d5 --- /dev/null +++ b/modules/validations/impossible_oneway.js @@ -0,0 +1,176 @@ +import { t } from '../util/locale'; +import { modeDrawLine } from '../modes/draw_line'; +import { actionReverse } from '../actions/reverse'; +import { utilDisplayLabel } from '../util'; +import { osmFlowingWaterwayTagValues, osmOneWayTags, osmRoutableHighwayTagValues } from '../osm/tags'; +import { validationIssue, validationIssueFix } from '../core/validation'; + + +export function validationImpossibleOneway() { + var type = 'impossible_oneway'; + + function typeForWay(way) { + if (osmRoutableHighwayTagValues[way.tags.highway]) return 'highway'; + if (osmFlowingWaterwayTagValues[way.tags.waterway]) return 'waterway'; + return null; + } + + function isOneway(way) { + if (way.tags.oneway === 'yes') return true; + if (way.tags.oneway) return false; + + for (var key in way.tags) { + if (osmOneWayTags[key] && osmOneWayTags[key][way.tags[key]]) { + return true; + } + } + return false; + } + + function continueDrawing(way, vertex, context) { + // make sure the vertex is actually visible and editable + var map = context.map(); + if (!map.editable() || !map.trimmedExtent().contains(vertex.loc)) { + map.zoomToEase(vertex); + } + + context.enter( + modeDrawLine(context, way.id, context.graph(), context.graph(), 'line', way.affix(vertex.id), true) + ); + } + + function nodeOccursMoreThanOnce(way, nodeID) { + var occurences = 0; + for (var index in way.nodes) { + if (way.nodes[index] === nodeID) { + occurences += 1; + if (occurences > 1) return true; + } + } + return false; + } + + function issuesForNode(context, way, nodeID) { + + var isFirst = nodeID === way.first(); + + var wayType = typeForWay(way); + var isWaterway = wayType === 'waterway'; + + // ignore if this way is self-connected at this node + if (nodeOccursMoreThanOnce(way, nodeID)) return []; + + var osm = context.connection(); + if (!osm) return []; + + var node = context.hasEntity(nodeID); + + // ignore if this node or its tile are unloaded + if (!node || !osm.isDataLoaded(node.loc)) return []; + + var attachedWaysOfSameType = context.graph().parentWays(node).filter(function(parentWay) { + if (parentWay.id === way.id) return false; + return typeForWay(parentWay) === wayType; + }); + + // assume it's okay for waterways to start or end disconnected for now + if (isWaterway && attachedWaysOfSameType.length === 0) return []; + + var attachedOneways = attachedWaysOfSameType.filter(function(attachedWay) { + return isOneway(attachedWay); + }); + + // ignore if the way is connected to some non-oneway features + if (attachedOneways.length < attachedWaysOfSameType.length) return []; + + if (attachedOneways.length) { + var connectedEndpointsOkay = attachedOneways.some(function(attachedOneway) { + return (isFirst ? attachedOneway.first() : attachedOneway.last()) !== nodeID; + }); + if (connectedEndpointsOkay) return []; + } + + var fixes = []; + + if (attachedOneways.length) { + fixes.push(new validationIssueFix({ + icon: 'iD-operation-reverse', + title: t('issues.fix.reverse_feature.title'), + entityIds: [way.id], + onClick: function() { + var id = this.issue.entities[0].id; + context.perform(actionReverse(id), t('operations.reverse.annotation')); + } + })); + } + if (node.tags.noexit !== 'yes') { + fixes.push(new validationIssueFix({ + icon: 'iD-operation-continue' + (isFirst ? '-left' : ''), + title: t('issues.fix.continue_from_' + (isFirst ? 'start' : 'end') + '.title'), + onClick: function() { + var entityID = this.issue.entities[0].id; + var vertexID = this.issue.entities[1].id; + var way = context.entity(entityID); + var vertex = context.entity(vertexID); + continueDrawing(way, vertex, context); + } + })); + } + + var placement = isFirst ? 'start' : 'end', + messageID = wayType + '.', + referenceID = wayType + '.'; + + if (isWaterway) { + messageID += 'connected.' + placement; + referenceID += 'connected'; + } else { + messageID += placement; + referenceID += placement; + } + + return [new validationIssue({ + type: type, + subtype: wayType, + severity: 'warning', + message: t('issues.impossible_oneway.' + messageID + '.message', { + feature: utilDisplayLabel(way, context) + }), + reference: getReference(referenceID), + entities: [way, node], + fixes: fixes + })]; + + function getReference(referenceID) { + return function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .text(t('issues.impossible_oneway.' + referenceID + '.reference')); + }; + } + } + + var validation = function checkDisconnectedWay(entity, context) { + + if (entity.type !== 'way' || entity.geometry(context.graph()) !== 'line') return []; + + if (entity.isClosed()) return []; + + if (!typeForWay(entity)) return []; + + if (!isOneway(entity)) return []; + + var firstIssues = issuesForNode(context, entity, entity.first()); + var lastIssues = issuesForNode(context, entity, entity.last()); + + return firstIssues.concat(lastIssues); + }; + + + validation.type = type; + + return validation; +} diff --git a/modules/validations/index.js b/modules/validations/index.js index e2a25f822..35862a874 100644 --- a/modules/validations/index.js +++ b/modules/validations/index.js @@ -3,6 +3,7 @@ export { validationCrossingWays } from './crossing_ways'; export { validationDisconnectedWay } from './disconnected_way'; export { validationFixmeTag } from './fixme_tag'; export { validationGenericName } from './generic_name'; +export { validationImpossibleOneway } from './impossible_oneway'; export { validationIncompatibleSource } from './incompatible_source'; export { validationMaprules } from './maprules'; export { validationMissingRole } from './missing_role';