From f3a985f78b9a70181fbc1563865d788cee43c315 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Tue, 11 Feb 2025 20:08:38 +0100 Subject: [PATCH] ways with more than 2000 nodes: prevent lockup and provide validator-fix (#8808) * add validation & fix for way vertices limit imposed by OSM API (try to choose splitting locations at existing intersection vertices if possible) * fix splitting of closed ways at two or more nodes: this bug resulted sometimes in one extra split point in the result, or one of the split vertices to be left unsplit --- ARCHITECTURE.md | 4 + CHANGELOG.md | 3 + data/core.yaml | 8 ++ modules/actions/split.js | 6 +- modules/validations/index.js | 1 + modules/validations/osm_api_limits.js | 98 +++++++++++++++++++++++++ test/spec/actions/split.js | 20 +++++ test/spec/validations/osm_api_limits.js | 96 ++++++++++++++++++++++++ 8 files changed, 233 insertions(+), 3 deletions(-) create mode 100644 modules/validations/osm_api_limits.js create mode 100644 test/spec/validations/osm_api_limits.js diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 8acbce810..b15b8ba71 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -483,6 +483,10 @@ A feature does not have enough tags to define what it is. * `relation_type`: the OSM entity type is `relation` but there is no `type` tag * `highway_classification`: the OSM entity type is `way` and the feature is tagged as `highway=road` +##### `osm_api_limits` + +A feature does not conform to the limits and rules imposed by the OSM API, such as a way with too many nodes for example. + ##### `outdated_tags` A feature has nonstandard tags. diff --git a/CHANGELOG.md b/CHANGELOG.md index 141afe1d7..212e247e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,9 +40,11 @@ _Breaking developer changes, which may affect downstream projects or sites that #### :sparkles: Usability & Accessibility * Also show search result for coordinates in `lon/lat` order in search results ([#10720], thanks [@Deeptanshu-sankhwar]) #### :scissors: Operations +* Fix splitting of closed ways (or areas) when two or more split-points are selected #### :camera: Street-Level #### :white_check_mark: Validation * Add warning if aeroways cross each other, buildings or highways ([#9315], thanks [@k-yle]) +* Warn when a way with more than the maximum allowed number of nodes is to be uploaded and provide a way to fix it ([#7381]) #### :bug: Bugfixes * Prevent degenerate ways caused by deleting a corner of a triangle ([#10003], thanks [@k-yle]) * Fix briefly disappearing data layer during background layer tile layer switching transition ([#10748]) @@ -55,6 +57,7 @@ _Breaking developer changes, which may affect downstream projects or sites that #### :rocket: Presets #### :hammer: Development +[#7381]: https://github.com/openstreetmap/iD/issues/7381 [#10003]: https://github.com/openstreetmap/iD/pull/10003 [#10720]: https://github.com/openstreetmap/iD/issues/10720 [#10747]: https://github.com/openstreetmap/iD/issues/10747 diff --git a/data/core.yaml b/data/core.yaml index 997fda11d..8605daa88 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -1954,6 +1954,12 @@ en: vertex_as_point: message: '{feature} should be attached to a line or area based on its tags' reference: "Some features shouldn't be standalone points." + osm_api_limits: + title: OSM Data Limitations + tip: Limits and rules imposed by the OSM API on uploaded features + max_way_nodes: + message: Way has too many vertices + reference: "The maximum number of vertices of a way is {maxWayNodes}. Split the feature into smaller parts or otherwise reduce the number of nodes of the affected way." fix: add_a_bridge: title: Add a bridge @@ -2032,6 +2038,8 @@ en: title: Set as inner set_as_outer: title: Set as outer + split_way: + title: Split the way into smaller pieces. square_feature: title: Square this feature tag_as_disconnected: diff --git a/modules/actions/split.js b/modules/actions/split.js index 3a9e06908..6c39ae441 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -95,7 +95,7 @@ export function actionSplit(nodeIds, newWayIds) { return totalLength; } - function split(graph, nodeId, wayA, newWayId) { + function split(graph, nodeId, wayA, newWayId, otherNodeIds) { var wayB = osmWay({ id: newWayId, tags: wayA.tags }); // `wayB` is the NEW way var nodesA; var nodesB; @@ -104,7 +104,7 @@ export function actionSplit(nodeIds, newWayIds) { if (wayA.isClosed()) { var nodes = wayA.nodes.slice(0, -1); var idxA = nodes.indexOf(nodeId); - var idxB = splitArea(nodes, idxA, graph); + var idxB = otherNodeIds.length > 0 ? nodes.indexOf(otherNodeIds[0]) : splitArea(nodes, idxA, graph); if (idxB < idxA) { nodesA = nodes.slice(idxA).concat(nodes.slice(0, idxB + 1)); @@ -398,7 +398,7 @@ export function actionSplit(nodeIds, newWayIds) { var nodeId = nodeIds[i]; var candidates = action.waysForNode(nodeId, graph); for (var j = 0; j < candidates.length; j++) { - graph = split(graph, nodeId, candidates[j], newWayIds && newWayIds[newWayIndex]); + graph = split(graph, nodeId, candidates[j], newWayIds && newWayIds[newWayIndex], nodeIds.slice(j + 1)); newWayIndex += 1; } } diff --git a/modules/validations/index.js b/modules/validations/index.js index 0cc27ec55..72c49b75e 100644 --- a/modules/validations/index.js +++ b/modules/validations/index.js @@ -11,6 +11,7 @@ export { validationMismatchedGeometry } from './mismatched_geometry'; export { validationMissingRole } from './missing_role'; export { validationMissingTag } from './missing_tag'; export { validationMutuallyExclusiveTags } from './mutually_exclusive_tags'; +export { validationOsmApiLimits } from './osm_api_limits'; export { validationOutdatedTags } from './outdated_tags'; export { validationPrivateData } from './private_data'; export { validationSuspiciousName } from './suspicious_name'; diff --git a/modules/validations/osm_api_limits.js b/modules/validations/osm_api_limits.js new file mode 100644 index 000000000..96a7467b4 --- /dev/null +++ b/modules/validations/osm_api_limits.js @@ -0,0 +1,98 @@ +import { t } from '../core/localizer'; +import { validationIssue, validationIssueFix } from '../core/validation'; +import { operationSplit } from '../operations/split'; + +export function validationOsmApiLimits(context) { + const type = 'osm_api_limits'; + + const validation = function checkOsmApiLimits(entity) { + const issues = []; + const osm = context.connection(); + if (!osm) return issues; // cannot check if there is no connection to the osm api, e.g. during unit tests + const maxWayNodes = osm.maxWayNodes(); + + if (entity.type === 'way') { + if (entity.nodes.length > maxWayNodes) { + issues.push(new validationIssue({ + type: type, + subtype: 'exceededMaxWayNodes', + severity: 'error', + message: function() { + return t.html('issues.osm_api_limits.max_way_nodes.message'); + }, + reference: function(selection) { + selection.selectAll('.issue-reference') + .data([0]) + .enter() + .append('div') + .attr('class', 'issue-reference') + .html(t.html('issues.osm_api_limits.max_way_nodes.reference', { maxWayNodes })); + }, + entityIds: [entity.id], + dynamicFixes: splitWayIntoSmallChunks + })); + } + } + + return issues; + }; + + function splitWayIntoSmallChunks() { + const fix = new validationIssueFix({ + icon: 'iD-operation-split', + title: t.html('issues.fix.split_way.title'), + entityIds: this.entityIds, + onClick: function(context) { + const maxWayNodes = context.connection().maxWayNodes(); + const g = context.graph(); + + const entityId = this.entityIds[0]; + const entity = context.graph().entities[entityId]; + const numberOfParts = Math.ceil(entity.nodes.length / maxWayNodes); + let splitVertices; + + if (numberOfParts === 2) { + // simple case: try to split at the an intersection vertex + const splitIntersections = entity.nodes + .map(nid => g.entity(nid)) + .filter(n => g.parentWays(n).length > 1) + .map(n => n.id) + .filter(nid => { + const splitIndex = entity.nodes.indexOf(nid); + return splitIndex < maxWayNodes && + entity.nodes.length - splitIndex < maxWayNodes; + }); + if (splitIntersections.length > 0) { + splitVertices = [ + splitIntersections[Math.floor(splitIntersections.length / 2)] + ]; + } + } + + if (splitVertices === undefined) { + // general case: either more than one split is needed or no possible + // intersection split point was found -> just split at regular intervals + splitVertices = [...Array(numberOfParts - 1)].map((_, i) => + entity.nodes[Math.floor(entity.nodes.length * (i + 1) / numberOfParts)]); + } + + if (entity.isClosed()) { + // add extra split for closed ways at start of way + splitVertices.push(entity.nodes[0]); + } + + const operation = operationSplit(context, splitVertices.concat(entityId)); + if (!operation.disabled()) { + operation(); + } + } + }); + + return [fix]; + } + + + validation.type = type; + + return validation; +} diff --git a/test/spec/actions/split.js b/test/spec/actions/split.js index 32cb888fe..10597209d 100644 --- a/test/spec/actions/split.js +++ b/test/spec/actions/split.js @@ -511,6 +511,26 @@ describe('iD.actionSplit', function () { expect(g4.entity('-').nodes).to.eql(['b', 'c', 'd']); expect(g4.entity('=').nodes).to.eql(['d', 'a', 'b']); }); + + it('splits a closed way at the given points', function () { + // + // Situation: + // a ---- b + // | | + // d ---- c + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 1] }), + iD.osmNode({ id: 'b', loc: [1, 1] }), + iD.osmNode({ id: 'c', loc: [1, 0] }), + iD.osmNode({ id: 'd', loc: [0, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c', 'd', 'a']}) + ]); + + var g1 = iD.actionSplit(['a', 'b'], ['='])(graph); + expect(g1.entity('-').nodes).to.eql(['b', 'c', 'd', 'a']); + expect(g1.entity('=').nodes).to.eql(['a', 'b']); + }); }); diff --git a/test/spec/validations/osm_api_limits.js b/test/spec/validations/osm_api_limits.js new file mode 100644 index 000000000..87ad6e3a1 --- /dev/null +++ b/test/spec/validations/osm_api_limits.js @@ -0,0 +1,96 @@ +describe('iD.validations.osm_api_limits', function () { + var context; + + beforeEach(function() { + iD.services.osm = { maxWayNodes: function() { return 10; } }; + context = iD.coreContext().assetPath('../dist/').init(); + context.surface = () => d3.select('#nop'); // mock with NOP + }); + + function createWay(numNodes) { + var nodes = []; + for (var i = 0; i < numNodes; i++) { + nodes.push(iD.osmNode({ id: 'n-' + i, loc: [i, i]})); + } + var w = iD.osmWay({id: 'w-1', tags: {}, + nodes: nodes.map(function(n) { return n.id; }) }); + + context.perform.apply(null, nodes + .map(function(n) { return iD.actionAddEntity(n); }) + .concat(iD.actionAddEntity(w)) + ); + } + + function validate() { + var validator = iD.validationOsmApiLimits(context); + var changes = context.history().changes(); + var entities = changes.modified.concat(changes.created); + var issues = []; + entities.forEach(function(entity) { + issues = issues.concat(validator(entity, context.graph())); + }); + return issues; + } + + it('has no errors on init', function() { + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('flags way with more than the maximum number of allowed nodes', function() { + createWay(12); + var issues = validate(); + expect(issues).to.have.lengthOf(1); + var issue = issues[0]; + expect(issue.type).to.eql('osm_api_limits'); + expect(issue.subtype).to.eql('exceededMaxWayNodes'); + expect(issue.severity).to.eql('error'); + expect(issue.entityIds).to.have.lengthOf(1); + expect(issue.entityIds[0]).to.eql('w-1'); + + var fixes = issue.fixes(context); + expect(fixes).to.have.lengthOf(1); + fixes[0].onClick(context); + issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('can fix an extreme case', function() { + createWay(33); + var issues = validate(); + expect(issues).to.have.lengthOf(1); + var issue = issues[0]; + + var fixes = issue.fixes(context); + expect(fixes).to.have.lengthOf(1); + fixes[0].onClick(context); + issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('fix a simple case at an intersection vertex', function() { + createWay(12); + + var n2 = iD.osmNode({id: 'n-0', loc: [0,0]}); + var n1 = iD.osmNode({id: 'n-8', loc: [8,8]}); + var w = iD.osmWay({id: 'w-2', nodes: ['n-0', 'n-8'], tags: {}}); + + context.perform( + iD.actionAddEntity(n1), + iD.actionAddEntity(n2), + iD.actionAddEntity(w) + ); + + var issues = validate(); + expect(issues).to.have.lengthOf(1); + var issue = issues[0]; + + var fixes = issue.fixes(context); + expect(fixes).to.have.lengthOf(1); + fixes[0].onClick(context); + issues = validate(); + expect(issues).to.have.lengthOf(0); + + context.graph().entity('w-1').nodes.length === 8; + }); +});