From a7b3985237b4c9ac966e10a8b67347a7a3ff4eed Mon Sep 17 00:00:00 2001 From: Quincy Morgan <2046746+quincylvania@users.noreply.github.com> Date: Fri, 4 Dec 2020 16:22:36 -0500 Subject: [PATCH] Flag points as areas or lines, areas or lines as points, and lines as areas (close #8231) --- ARCHITECTURE.md | 11 +- data/core.yaml | 11 ++ dist/locales/en.json | 18 ++- modules/validations/mismatched_geometry.js | 144 ++++++++++++++++----- 4 files changed, 147 insertions(+), 37 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 3e0ac166c..83c0aadee 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -457,8 +457,17 @@ An issue with the active [MapRules](https://github.com/radiant-maxar/maprules) v A feature's tags indicate it should have a different geometry than it currently does. * `area_as_line`: an unclosed way has tags implying it should be a closed area (e.g. `area=yes` or `building=yes`) -* `vertex_as_point`: a detached node has tags implying it should be part of a way (e.g. `highway=stop`) +* `area_as_point` +* `area_as_vertex` +* `line_as_area` +* `line_as_point` +* `line_as_vertex`: a detached node has tags implying it should be a line (e.g. `highway=motorway`) +* `point_as_area` +* `point_as_line` * `point_as_vertex`: a vertex node has tags implying it should be detached from ways (e.g. `amenity=cafe`) +* `vertex_as_area` +* `vertex_as_line` +* `vertex_as_point`: a detached node has tags implying it should be part of a way (e.g. `highway=stop`) * `unclosed_multipolygon_part`: a relation is tagged as a multipolygon but not all of its member ways form closed rings ##### `missing_role` diff --git a/data/core.yaml b/data/core.yaml index 33595c94e..5eb5a0349 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -1650,6 +1650,8 @@ en: message: "{feature} ends very close to itself but does not reconnect" highway-highway: reference: Intersecting highways should share a junction vertex. + area_as_point: + message: '{feature} should be an area, not a point' close_nodes: title: "Very Close Points" tip: "Find redundant and crowded points" @@ -1729,9 +1731,14 @@ en: message: '{feature} has an invalid email address' message_multi: '{feature} has multiple invalid email addresses' reference: 'Email addresses must look like "user@example.com".' + line_as_area: + message: '{feature} should be a line, not an area' + line_as_point: + message: '{feature} should be a line, not a point' mismatched_geometry: title: Mismatched Geometry tip: "Find features with conflicting tags and geometry" + reference: Most features are limited to certain geometry types. missing_role: title: Missing Roles message: "{member} has no role within {relation}" @@ -1763,6 +1770,10 @@ en: message: "{feature} looks like a brand with nonstandard tags" message_incomplete: "{feature} looks like a brand with incomplete tags" reference: "All features of the same brand should be tagged the same way." + point_as_area: + message: '{feature} should be a point, not an area' + point_as_line: + message: '{feature} should be a point, not a line' point_as_vertex: message: '{feature} should be a standalone point based on its tags' reference: "Some features shouldn't be part of lines or areas." diff --git a/dist/locales/en.json b/dist/locales/en.json index 8896251bb..771b017c1 100644 --- a/dist/locales/en.json +++ b/dist/locales/en.json @@ -2067,6 +2067,9 @@ "reference": "Intersecting highways should share a junction vertex." } }, + "area_as_point": { + "message": "{feature} should be an area, not a point" + }, "close_nodes": { "title": "Very Close Points", "tip": "Find redundant and crowded points", @@ -2177,9 +2180,16 @@ "reference": "Email addresses must look like \"user@example.com\"." } }, + "line_as_area": { + "message": "{feature} should be a line, not an area" + }, + "line_as_point": { + "message": "{feature} should be a line, not a point" + }, "mismatched_geometry": { "title": "Mismatched Geometry", - "tip": "Find features with conflicting tags and geometry" + "tip": "Find features with conflicting tags and geometry", + "reference": "Most features are limited to certain geometry types." }, "missing_role": { "title": "Missing Roles", @@ -2222,6 +2232,12 @@ "reference": "All features of the same brand should be tagged the same way." } }, + "point_as_area": { + "message": "{feature} should be a point, not an area" + }, + "point_as_line": { + "message": "{feature} should be a point, not a line" + }, "point_as_vertex": { "message": "{feature} should be a standalone point based on its tags", "reference": "Some features shouldn't be part of lines or areas." diff --git a/modules/validations/mismatched_geometry.js b/modules/validations/mismatched_geometry.js index d2945d602..5f7c5a5f7 100644 --- a/modules/validations/mismatched_geometry.js +++ b/modules/validations/mismatched_geometry.js @@ -1,3 +1,4 @@ +import deepEqual from 'fast-deep-equal'; import { actionAddVertex } from '../actions/add_vertex'; import { actionChangeTags } from '../actions/change_tags'; import { actionMergeNodes } from '../actions/merge_nodes'; @@ -139,7 +140,7 @@ export function validationMismatchedGeometry() { } } - function vertexTaggedAsPointIssue(entity, graph) { + function vertexPointIssue(entity, graph) { // we only care about nodes if (entity.type !== 'node') return null; @@ -196,46 +197,115 @@ export function validationMismatchedGeometry() { .html(t.html('issues.point_as_vertex.reference')); }, entityIds: [entity.id], - dynamicFixes: function(context) { - - var entityId = this.entityIds[0]; - - var extractOnClick = null; - if (!context.hasHiddenConnections(entityId)) { - - extractOnClick = function(context) { - var entityId = this.issue.entityIds[0]; - var action = actionExtract(entityId); - context.perform( - action, - t('operations.extract.annotation', { n: 1 }) - ); - // re-enter mode to trigger updates - context.enter(modeSelect(context, [action.getExtractedNodeID()])); - }; - } - - return [ - new validationIssueFix({ - icon: 'iD-operation-extract', - title: t.html('issues.fix.extract_point.title'), - onClick: extractOnClick - }) - ]; - } + dynamicFixes: dynamicExtractFixes }); } return null; } + + function otherMismatchIssue(entity, graph) { + // ignore boring features + if (!entity.hasInterestingTags()) return null; + + if (entity.type !== 'node' && entity.type !== 'way') return null; + + var sourceGeom = entity.geometry(graph); + + var targetGeoms = entity.type === 'way' ? ['point', 'vertex'] : ['line', 'area']; + + if (sourceGeom === 'area') targetGeoms.unshift('line'); + + var targetGeom = targetGeoms.find(nodeGeom => { + var asSource = presetManager.matchTags(entity.tags, sourceGeom); + var asTarget = presetManager.matchTags(entity.tags, nodeGeom); + if (!asSource || !asTarget || + asSource === asTarget || + // sometimes there are two presets with the same tags for different geometries + deepEqual(asSource.tags, asTarget.tags)) return false; + + if (asTarget.isFallback()) return false; + + var primaryKey = Object.keys(asTarget.tags)[0]; + + // special case: buildings-as-points are discouraged by iD, but common in OSM, so ignore them + if (primaryKey === 'building') return false; + + if (asTarget.tags[primaryKey] === '*') return false; + + return asSource.isFallback() || asSource.tags[primaryKey] === '*'; + }); + + if (!targetGeom) return null; + + var subtype = targetGeom + '_as_' + sourceGeom; + + if (targetGeom === 'vertex') targetGeom = 'point'; + if (sourceGeom === 'vertex') sourceGeom = 'point'; + + var referenceId = targetGeom + '_as_' + sourceGeom; + + var dynamicFixes = targetGeom === 'point' ? dynamicExtractFixes : null; + + return new validationIssue({ + type: type, + subtype: subtype, + severity: 'warning', + message: function(context) { + var entity = context.hasEntity(this.entityIds[0]); + return entity ? t.html('issues.' + referenceId + '.message', { + feature: utilDisplayLabel(entity, targetGeom) + }) : ''; + }, + reference: function showReference(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .html(t.html('issues.mismatched_geometry.reference')); + }, + entityIds: [entity.id], + dynamicFixes: dynamicFixes + }); + } + + function dynamicExtractFixes(context) { + + var entityId = this.entityIds[0]; + + var extractOnClick = null; + if (!context.hasHiddenConnections(entityId)) { + + extractOnClick = function(context) { + var entityId = this.issue.entityIds[0]; + var action = actionExtract(entityId); + context.perform( + action, + t('operations.extract.annotation', { n: 1 }) + ); + // re-enter mode to trigger updates + context.enter(modeSelect(context, [action.getExtractedNodeID()])); + }; + } + + return [ + new validationIssueFix({ + icon: 'iD-operation-extract', + title: t.html('issues.fix.extract_point.title'), + onClick: extractOnClick + }) + ]; + } + function unclosedMultipolygonPartIssues(entity, graph) { if (entity.type !== 'relation' || !entity.isMultipolygon() || entity.isDegenerate() || // cannot determine issues for incompletely-downloaded relations - !entity.isComplete(graph)) return null; + !entity.isComplete(graph)) return []; var sequences = osmJoinWays(entity.members, graph); @@ -285,12 +355,16 @@ export function validationMismatchedGeometry() { } var validation = function checkMismatchedGeometry(entity, graph) { - var issues = [ - vertexTaggedAsPointIssue(entity, graph), - lineTaggedAsAreaIssue(entity) - ]; - issues = issues.concat(unclosedMultipolygonPartIssues(entity, graph)); - return issues.filter(Boolean); + var vertexPoint = vertexPointIssue(entity, graph); + if (vertexPoint) return [vertexPoint]; + + var lineAsArea = lineTaggedAsAreaIssue(entity); + if (lineAsArea) return [lineAsArea]; + + var mismatch = otherMismatchIssue(entity, graph); + if (mismatch) return [mismatch]; + + return unclosedMultipolygonPartIssues(entity, graph); }; validation.type = type;