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'; } } }