From 4dc4e7dbdc87a847589c3584f19860bef9ce9aad Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Mon, 28 Apr 2025 12:05:44 +0200 Subject: [PATCH] fix split operation from creating 1-node ways when 2+ nodes selected fixes #10997 Also change the split operation to only split the ways which contain all selected nodes (when thare are more than one node selected). This is more likely what the person performing the splitting intends to do --- CHANGELOG.md | 3 ++ modules/actions/split.js | 114 +++++++++++++++++++-------------------- 2 files changed, 60 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e1bcb5132..34e1de80d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ _Breaking developer changes, which may affect downstream projects or sites that #### :scissors: Operations * Preserve the sum of certain tags (`step_count`, `parking:*:capacity`) during _join_ operation ([#10492], thanks [@ChaitanyaKadu03]) * Preserve total value of `parking:*:capacity` tags during _split_ operation by distributing it proportionally to the resulting ways ([#10492]) +* A _split_ operation will now only split the ways that contain all selected nodes when more than one node is selected ([#10997]) #### :camera: Street-Level * Keep photo viewer open when disabling Panoramax overlay ([#10966]) * Don't de-select map feature when clicking on a street level photo ([#10959]) @@ -52,6 +53,7 @@ _Breaking developer changes, which may affect downstream projects or sites that * Fix removed tooltips from re-appearing when using keyboard navigation ([#9873]) * Only consider feature with proper lifecycle tags in "past/futures" layer ([#10943]) * Fix zoom level from resetting to the starting value when switching background imagery layer during the zoom transition +* Fix invalid single-noded ways from being created by a _split_ operation under certain conditions when multiple nodes are selected ([#10997]) #### :earth_asia: Localization #### :hourglass: Performance #### :mortar_board: Walkthrough / Help @@ -67,6 +69,7 @@ _Breaking developer changes, which may affect downstream projects or sites that [#10946]: https://github.com/openstreetmap/iD/issues/10946 [#10959]: https://github.com/openstreetmap/iD/issues/10959 [#10966]: https://github.com/openstreetmap/iD/issues/10966 +[#10997]: https://github.com/openstreetmap/iD/issues/10997 [@ChaitanyaKadu03]: https://github.com/ChaitanyaKadu03 diff --git a/modules/actions/split.js b/modules/actions/split.js index aa70ec0a4..ae07b4c41 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -1,7 +1,7 @@ import { geoSphericalDistance } from '../geo/geo'; import { osmRelation } from '../osm/relation'; import { osmWay } from '../osm/way'; -import { utilArrayIntersection, utilWrap, utilArrayUniq } from '../util'; +import { utilArrayIntersection, utilWrap } from '../util'; import { osmSummableTags } from '../osm/tags'; @@ -394,14 +394,14 @@ export function actionSplit(nodeIds, newWayIds) { return graph; } - var action = function(graph) { + const action = function(graph) { _createdWayIDs = []; - var newWayIndex = 0; - for (var i = 0; i < nodeIds.length; i++) { - 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], nodeIds.slice(j + 1)); + let newWayIndex = 0; + for (const i in nodeIds) { + const nodeId = nodeIds[i]; + const candidates = waysForNodes(nodeIds.slice(i), graph); + for (const candidate of candidates) { + graph = split(graph, nodeId, candidate, newWayIds && newWayIds[newWayIndex], nodeIds.slice(i + 1)); newWayIndex += 1; } } @@ -412,81 +412,81 @@ export function actionSplit(nodeIds, newWayIds) { return _createdWayIDs; }; - action.waysForNode = function(nodeId, graph) { - var node = graph.entity(nodeId); - var splittableParents = graph.parentWays(node).filter(isSplittable); + function waysForNodes(nodeIds, graph) { + const splittableWays = nodeIds + .map(nodeId => waysForNode(nodeId, graph)) + .reduce((cur, acc) => utilArrayIntersection(cur, acc)); if (!_wayIDs) { // If the ways to split aren't specified, only split the lines. // If there are no lines to split, split the areas. - - var hasLine = splittableParents.some(function(parent) { - return parent.geometry(graph) === 'line'; - }); + const hasLine = splittableWays.some(way => way.geometry(graph) === 'line'); if (hasLine) { - return splittableParents.filter(function(parent) { - return parent.geometry(graph) === 'line'; - }); + return splittableWays.filter(way => way.geometry(graph) === 'line'); } } - return splittableParents; - function isSplittable(parent) { + return splittableWays; + } + + function waysForNode(nodeId, graph) { + const node = graph.entity(nodeId); + return graph.parentWays(node).filter(isSplittable); + + function isSplittable(way) { // If the ways to split are specified, ignore everything else. - if (_wayIDs && _wayIDs.indexOf(parent.id) === -1) return false; + if (_wayIDs && _wayIDs.indexOf(way.id) === -1) return false; // We can fake splitting closed ways at their endpoints... - if (parent.isClosed()) return true; + if (way.isClosed()) return true; // otherwise, we can't split nodes at their endpoints. - for (var i = 1; i < parent.nodes.length - 1; i++) { - if (parent.nodes[i] === nodeId) return true; + for (let i = 1; i < way.nodes.length - 1; i++) { + if (way.nodes[i] === nodeId) return true; } return false; } }; action.ways = function(graph) { - return utilArrayUniq([].concat.apply([], nodeIds.map(function(nodeId) { - return action.waysForNode(nodeId, graph); - }))); + return waysForNodes(nodeIds, graph); }; action.disabled = function(graph) { - for (const nodeId of nodeIds) { - const candidates = action.waysForNode(nodeId, graph); - if (candidates.length === 0 || (_wayIDs && _wayIDs.length !== candidates.length)) { - return 'not_eligible'; - } - for (const way of candidates) { - const parentRelations = graph.parentRelations(way); - for (const parentRelation of parentRelations) { - if (parentRelation.hasFromViaTo()) { - // turn restrictions: via members must be loaded - const vias = [ - ...parentRelation.membersByRole('via'), - ...parentRelation.membersByRole('intersection'), - ]; - if (!vias.every(via => graph.hasEntity(via.id))) { - return 'parent_incomplete'; - } - } else { - // other relations (e.g. route relations): at least one members before or after way must be present - for (let i = 0; i < parentRelation.members.length; i++) { - if (parentRelation.members[i].id === way.id) { - const memberBeforePresent = i > 0 && graph.hasEntity(parentRelation.members[i - 1].id); - const memberAfterPresent = i < parentRelation.members.length - 1 && graph.hasEntity(parentRelation.members[i + 1].id); - if (!memberBeforePresent && !memberAfterPresent && parentRelation.members.length > 1) { - return 'parent_incomplete'; - } + const candidates = waysForNodes(nodeIds, graph); + if (candidates.length === 0 || (_wayIDs && _wayIDs.length !== candidates.length)) { + return 'not_eligible'; + } + for (const way of candidates) { + const parentRelations = graph.parentRelations(way); + for (const parentRelation of parentRelations) { + if (parentRelation.hasFromViaTo()) { + // turn restrictions: via members must be loaded + const vias = [ + ...parentRelation.membersByRole('via'), + ...parentRelation.membersByRole('intersection'), + ]; + if (!vias.every(via => graph.hasEntity(via.id))) { + return 'parent_incomplete'; + } + } else { + // other relations (e.g. route relations): at least one members before or after way must be present + for (let i = 0; i < parentRelation.members.length; i++) { + if (parentRelation.members[i].id === way.id) { + const memberBeforePresent = i > 0 && graph.hasEntity(parentRelation.members[i - 1].id); + const memberAfterPresent = i < parentRelation.members.length - 1 && graph.hasEntity(parentRelation.members[i + 1].id); + if (!memberBeforePresent && !memberAfterPresent && parentRelation.members.length > 1) { + return 'parent_incomplete'; } } } - const relTypesExceptions = ['junction', 'enforcement']; // some relation types should not prehibit a member from being split - if (circularJunctions.includes(way.tags.junction) && way.isClosed() && !relTypesExceptions.includes(parentRelation.tags.type)) { - return 'simple_roundabout'; - } + } + const relTypesExceptions = ['junction', 'enforcement']; // some relation types should not prehibit a member from being split + if (circularJunctions.includes(way.tags.junction) && + way.isClosed() && + !relTypesExceptions.includes(parentRelation.tags.type)) { + return 'simple_roundabout'; } } }