From 11dfbe804cdf15e60352dba1a30dfc9c2db8e5f0 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Sat, 2 Mar 2024 12:16:57 +0100 Subject: [PATCH 01/11] fix splitting of (route) relation member ways instead of fully re-sorting the whole relation every time a member is split, perform a local operation: This works under the assumption that the relation is already sorted properly. The new way is inserted into the relation before or after the existing member, depending on how the old/new way connect to their neighboring members. for cases where two ways form a loop, a additional look-ahead is implemented to disambiguate the order --- data/core.yaml | 1 + modules/actions/add_member.js | 77 ++---------- modules/actions/split.js | 204 +++++++++++++++++++++++++++----- test/spec/actions/add_member.js | 116 ------------------ test/spec/actions/split.js | 87 +++++++++++--- 5 files changed, 261 insertions(+), 224 deletions(-) diff --git a/data/core.yaml b/data/core.yaml index 20f91e406..68bd982f7 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -499,6 +499,7 @@ en: one: "Split a feature." other: "Split {n} features." not_eligible: Lines can't be split at their beginning or end. + parent_incomplete: This line cannot be split because it is part of a larger relation which has only been partially loaded. Make sure that all ways connected to this way are present on the map. connected_to_hidden: This can't be split because it is connected to a hidden feature. restriction: annotation: diff --git a/modules/actions/add_member.js b/modules/actions/add_member.js index 9be9dc937..f33145a3f 100644 --- a/modules/actions/add_member.js +++ b/modules/actions/add_member.js @@ -1,9 +1,8 @@ import { osmJoinWays } from '../osm/multipolygon'; -import { osmWay } from '../osm/way'; import { utilArrayGroupBy, utilObjectOmit } from '../util'; -export function actionAddMember(relationId, member, memberIndex, insertPair) { +export function actionAddMember(relationId, member, memberIndex) { return function action(graph) { var relation = graph.entity(relationId); @@ -11,7 +10,7 @@ export function actionAddMember(relationId, member, memberIndex, insertPair) { // There are some special rules for Public Transport v2 routes. var isPTv2 = /stop|platform/.test(member.role); - if ((isNaN(memberIndex) || insertPair) && member.type === 'way' && !isPTv2) { + if (member.type === 'way' && !isPTv2) { // Try to perform sensible inserts based on how the ways join together graph = addWayMember(relation, graph); } else { @@ -30,9 +29,8 @@ export function actionAddMember(relationId, member, memberIndex, insertPair) { // Add a way member into the relation "wherever it makes sense". - // In this situation we were not supplied a memberIndex. function addWayMember(relation, graph) { - var groups, tempWay, insertPairIsReversed, item, i, j, k; + var groups, item, i, j, k; // remove PTv2 stops and platforms before doing anything. var PTv2members = []; @@ -47,38 +45,10 @@ export function actionAddMember(relationId, member, memberIndex, insertPair) { } relation = relation.update({ members: members }); - - if (insertPair) { - // We're adding a member that must stay paired with an existing member. - // (This feature is used by `actionSplit`) - // - // This is tricky because the members may exist multiple times in the - // member list, and with different A-B/B-A ordering and different roles. - // (e.g. a bus route that loops out and back - #4589). - // - // Replace the existing member with a temporary way, - // so that `osmJoinWays` can treat the pair like a single way. - tempWay = osmWay({ id: 'wTemp', nodes: insertPair.nodes }); - graph = graph.replace(tempWay); - var tempMember = { id: tempWay.id, type: 'way', role: member.role }; - var tempRelation = relation.replaceMember({id: insertPair.originalID}, tempMember, true); - groups = utilArrayGroupBy(tempRelation.members, 'type'); - groups.way = groups.way || []; - - // Insert pair is reversed if the inserted way comes before the original one. - // (Except when they form a loop.) - var originalWay = graph.entity(insertPair.originalID); - var insertedWay = graph.entity(insertPair.insertedID); - insertPairIsReversed = originalWay.nodes.length > 0 && insertedWay.nodes.length > 0 && - insertedWay.nodes[insertedWay.nodes.length - 1] === originalWay.nodes[0] && - originalWay.nodes[originalWay.nodes.length - 1] !== insertedWay.nodes[0]; - - } else { - // Add the member anywhere, one time. Just push and let `osmJoinWays` decide where to put it. - groups = utilArrayGroupBy(relation.members, 'type'); - groups.way = groups.way || []; - groups.way.push(member); - } + // Add the member anywhere, one time. Just push and let `osmJoinWays` decide where to put it. + groups = utilArrayGroupBy(relation.members, 'type'); + groups.way = groups.way || []; + groups.way.push(member); members = withIndex(groups.way); var joined = osmJoinWays(members, graph); @@ -102,22 +72,6 @@ export function actionAddMember(relationId, member, memberIndex, insertPair) { item = segment[k]; var way = graph.entity(item.id); - // If this is a paired item, generate members in correct order and role - if (tempWay && item.id === tempWay.id) { - var reverse = nodes[0].id !== insertPair.nodes[0] ^ insertPairIsReversed; - if (reverse) { - item.pair = [ - { id: insertPair.insertedID, type: 'way', role: item.role }, - { id: insertPair.originalID, type: 'way', role: item.role } - ]; - } else { - item.pair = [ - { id: insertPair.originalID, type: 'way', role: item.role }, - { id: insertPair.insertedID, type: 'way', role: item.role } - ]; - } - } - // reorder `members` if necessary if (k > 0) { if (j+k >= members.length || item.index !== members[j+k].index) { @@ -129,22 +83,13 @@ export function actionAddMember(relationId, member, memberIndex, insertPair) { } } - if (tempWay) { - graph = graph.remove(tempWay); - } - - // Final pass: skip dead items, split pairs, remove index properties + // Final pass: skip dead items, remove index properties var wayMembers = []; for (i = 0; i < members.length; i++) { item = members[i]; if (item.index === -1) continue; - if (item.pair) { - wayMembers.push(item.pair[0]); - wayMembers.push(item.pair[1]); - } else { - wayMembers.push(utilObjectOmit(item, ['index'])); - } + wayMembers.push(utilObjectOmit(item, ['index'])); } // Put stops and platforms first, then nodes, ways, relations @@ -186,8 +131,8 @@ export function actionAddMember(relationId, member, memberIndex, insertPair) { } var item = Object.assign({}, arr[i]); // shallow copy - arr[i].index = -1; // mark as dead - item.index = toIndex; + arr[i].index = -1; // mark previous entry as dead + delete item.index; // inserted items must never be moved again arr.splice(toIndex, 0, item); } diff --git a/modules/actions/split.js b/modules/actions/split.js index 042b25aa0..23905622f 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -1,4 +1,3 @@ -import { actionAddMember } from './add_member'; import { geoSphericalDistance } from '../geo/geo'; import { osmRelation } from '../osm/relation'; import { osmWay } from '../osm/way'; @@ -95,7 +94,6 @@ export function actionSplit(nodeIds, newWayIds) { function split(graph, nodeId, wayA, newWayId) { var wayB = osmWay({ id: newWayId, tags: wayA.tags }); // `wayB` is the NEW way - var origNodes = wayA.nodes.slice(); var nodesA; var nodesB; var isArea = wayA.isArea(); @@ -165,8 +163,6 @@ export function actionSplit(nodeIds, newWayIds) { graph = graph.replace(wayB); graph.parentRelations(wayA).forEach(function(relation) { - var member; - // Turn restrictions - make sure: // 1. Splitting a FROM/TO way - only `wayA` OR `wayB` remains in relation // (whichever one is connected to the VIA node/ways) @@ -203,34 +199,16 @@ export function actionSplit(nodeIds, newWayIds) { } else { for (i = 0; i < v.length; i++) { if (v[i].type === 'way' && v[i].id === wayA.id) { - member = { - id: wayB.id, - type: 'way', - role: 'via' - }; - graph = actionAddMember(relation.id, member, v[i].index + 1)(graph); - break; + graph = splitWayMember(graph, relation.id, wayA, wayB); } } } // All other relations (Routes, Multipolygons, etc): // 1. Both `wayA` and `wayB` remain in the relation - // 2. But must be inserted as a pair (see `actionAddMember` for details) + // 2. But must be inserted in the correct order } else { - member = { - id: wayB.id, - type: 'way', - role: relation.memberById(wayA.id).role - }; - - var insertPair = { - originalID: wayA.id, - insertedID: wayB.id, - nodes: origNodes - }; - - graph = actionAddMember(relation.id, member, undefined, insertPair)(graph); + graph = splitWayMember(graph, relation.id, wayA, wayB); } }); @@ -253,6 +231,154 @@ export function actionSplit(nodeIds, newWayIds) { return graph; } + function splitWayMember(graph, relationId, wayA, wayB) { + let relation = graph.entity(relationId); + const insertMembers = []; + for (let i = 0; i < relation.members.length; i++) { + const member = relation.members[i]; + if (member.id === wayA.id) { + let wayAconnectsPrev = false; + let wayAconnectsNext = false; + let wayBconnectsPrev = false; + let wayBconnectsNext = false; + + function connects(way1, way2) { + if (way1.nodes.length < 2 || way2.nodes.length < 2) return false; + if (way1.nodes[0] === way2.nodes[0]) return true; + if (way1.nodes[0] === way2.nodes[way2.nodes.length - 1]) return true; + if (way1.nodes[way1.nodes.length - 1] === way2.nodes[way2.nodes.length - 1]) return true; + if (way1.nodes[way1.nodes.length - 1] === way2.nodes[0]) return true; + return false; + } + + if (i > 0 && graph.hasEntity(relation.members[i - 1].id)) { + const prevMember = relation.members[i - 1]; + const prevEntity = graph.entity(prevMember.id); + if (prevEntity.type === 'way' && prevEntity.id !== wayA.id && prevEntity.nodes.length > 0) { + wayAconnectsPrev = connects(prevEntity, wayA); + wayBconnectsPrev = connects(prevEntity, wayB); + } + } + if (i < relation.members.length - 1 && graph.hasEntity(relation.members[i + 1].id)) { + const nextMember = relation.members[i + 1]; + const nextEntity = graph.entity(nextMember.id); + if (nextEntity.type === 'way' && nextEntity.nodes.length > 0) { + wayAconnectsNext = connects(nextEntity, wayA); + wayBconnectsNext = connects(nextEntity, wayB); + } + } + + if (wayAconnectsPrev && !wayBconnectsPrev && !wayAconnectsNext && !wayBconnectsNext) { + // wayA connects to prev member -> insert B after A + insertMembers.push({at: i + 1, role: member.role}); + continue; + } + if (wayAconnectsPrev && !wayBconnectsPrev && wayAconnectsNext && wayBconnectsNext) { + // wayB only connects to next -> insert B after A + insertMembers.push({at: i + 1, role: member.role}); + continue; + } + if (!wayAconnectsPrev && !wayBconnectsPrev && !wayAconnectsNext && wayBconnectsNext) { + // wayB connects to next member -> insert B after A + insertMembers.push({at: i + 1, role: member.role}); + continue; + } + if (wayAconnectsPrev && wayBconnectsPrev && !wayAconnectsNext && wayBconnectsNext) { + // wayA only connects to prev -> insert B after A + insertMembers.push({at: i + 1, role: member.role}); + continue; + } + if (wayAconnectsPrev && !wayBconnectsPrev && !wayAconnectsNext && wayBconnectsNext) { + // wayA connects to prev, wayB connects to next -> insert B after A + insertMembers.push({at: i + 1, role: member.role}); + continue; + } + + if (!wayAconnectsPrev && wayBconnectsPrev && !wayAconnectsNext && !wayBconnectsNext) { + // wayB connects to prev member -> insert B before A + insertMembers.push({at: i, role: member.role}); + continue; + } + if (!wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && wayBconnectsNext) { + // wayA only connects to next -> insert B before A + insertMembers.push({at: i, role: member.role}); + continue; + } + if (!wayAconnectsPrev && !wayBconnectsPrev && wayAconnectsNext && !wayBconnectsNext) { + // wayA connects to next member -> insert B before A + insertMembers.push({at: i, role: member.role}); + continue; + } + if (wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && !wayBconnectsNext) { + // wayB only connects to prev -> insert B before A + insertMembers.push({at: i, role: member.role}); + continue; + } + if (!wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && !wayBconnectsNext) { + // wayB connects to prev, wayA connects to next -> insert B before A + insertMembers.push({at: i, role: member.role}); + continue; + } + + // check for loops + if (wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && wayBconnectsNext) { + // complete loop + // look one more member ahead + if (i > 2 && graph.hasEntity(relation.members[i - 2].id)) { + const prev2Entity = graph.entity(relation.members[i - 2].id); + if (connects(prev2Entity, wayA) && !connects(prev2Entity, wayB)) { + // prev-2 member connects only to A: insert B before A + insertMembers.push({at: i, role: member.role}); + continue; + } + if (connects(prev2Entity, wayB) && !connects(prev2Entity, wayA)) { + // prev-2 member connects only to B: insert B after A + insertMembers.push({at: i + 1, role: member.role}); + continue; + } + } + if (i < relation.members.length - 2 && graph.hasEntity(relation.members[i + 2].id)) { + const next2Entity = graph.entity(relation.members[i + 2].id); + if (connects(next2Entity, wayA) && !connects(next2Entity, wayB)) { + // next+2 member connects only to A: insert B after A + insertMembers.push({at: i + 1, role: member.role}); + continue; + } + if (connects(next2Entity, wayB) && !connects(next2Entity, wayA)) { + // next+2 member connects only to B: insert B before A + insertMembers.push({at: i, role: member.role}); + continue; + } + } + } + // could not determine how new member should connect (i.e. existing way was not connected to other member ways) + // just make sure before/after still connect + if (wayA.nodes[wayA.nodes.length - 1] === wayB.nodes[0]) { + insertMembers.push({at: i + 1, role: member.role}); + continue; + } else { + insertMembers.push({at: i, role: member.role}); + continue; + } + + /* + // could not determine how new member should connect (i.e. existing way was not connected to other member ways) + // -> insert new way after existing way + insertMembers.push({at: i + 1, role: member.role});*/ + } + } + // insert new member(s) + insertMembers.reverse().forEach(item => { + graph = graph.replace(relation.addMember({ + id: wayB.id, + type: 'way', + role: item.role + }, item.at)); + relation = graph.entity(relation.id); + }); + return graph; + } + var action = function(graph) { _createdWayIDs = []; var newWayIndex = 0; @@ -313,12 +439,34 @@ export function actionSplit(nodeIds, newWayIds) { action.disabled = function(graph) { - for (var i = 0; i < nodeIds.length; i++) { - var nodeId = nodeIds[i]; - var candidates = action.waysForNode(nodeId, 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 memebers must be loaded + const vias = parentRelation.membersByRole('via'); + 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) { + return 'parent_incomplete'; + } + } + } + } + } + } } }; diff --git a/test/spec/actions/add_member.js b/test/spec/actions/add_member.js index 62f6c69bc..6bc8669bc 100644 --- a/test/spec/actions/add_member.js +++ b/test/spec/actions/add_member.js @@ -119,122 +119,6 @@ describe('iD.actionAddMember', function() { expect(members(graph)).to.eql(['-', '=', '~']); }); - it('inserts the member multiple times if insertPair provided (middle)', function() { - // Before: a ---> b .. c ~~~> d <~~~ c .. b <--- a - // After: a ---> b ===> c ~~~> d <~~~ c <=== b <--- a - var graph = iD.coreGraph([ - iD.osmNode({id: 'a', loc: [0, 0]}), - iD.osmNode({id: 'b', loc: [0, 0]}), - iD.osmNode({id: 'c', loc: [0, 0]}), - iD.osmNode({id: 'd', loc: [0, 0]}), - iD.osmWay({id: '-', nodes: ['a', 'b']}), - iD.osmWay({id: '=', nodes: ['b', 'c']}), - iD.osmWay({id: '~', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r', members: [ - {id: '-', type: 'way'}, - {id: '~', type: 'way'}, - {id: '~', type: 'way'}, - {id: '-', type: 'way'} - ]}) - ]); - - var member = { id: '=', type: 'way' }; - var insertPair = { - originalID: '-', - insertedID: '=', - nodes: ['a','b','c'] - }; - graph = iD.actionAddMember('r', member, undefined, insertPair)(graph); - expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); - }); - - it('inserts the member multiple times if insertPair provided (middle) (reversed pair)', function() { - // Before: a .. b ===> c ~~~> d <~~~ c <=== b .. a - // After: a ---> b ===> c ~~~> d <~~~ c <=== b <--- a - var graph = iD.coreGraph([ - iD.osmNode({id: 'a', loc: [0, 0]}), - iD.osmNode({id: 'b', loc: [0, 0]}), - iD.osmNode({id: 'c', loc: [0, 0]}), - iD.osmNode({id: 'd', loc: [0, 0]}), - iD.osmWay({id: '-', nodes: ['a', 'b']}), - iD.osmWay({id: '=', nodes: ['b', 'c']}), - iD.osmWay({id: '~', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r', members: [ - {id: '=', type: 'way'}, - {id: '~', type: 'way'}, - {id: '~', type: 'way'}, - {id: '=', type: 'way'} - ]}) - ]); - - var member = { id: '=', type: 'way' }; - var insertPair = { - originalID: '=', - insertedID: '-', - nodes: ['a','b','c'] - }; - graph = iD.actionAddMember('r', member, undefined, insertPair)(graph); - expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); - }); - - it('inserts the member multiple times if insertPair provided (beginning/end)', function() { - // Before: b <=== c ~~~> d <~~~ c ===> b - // After: a <--- b <=== c ~~~> d <~~~ c ===> b ---> a - var graph = iD.coreGraph([ - iD.osmNode({id: 'a', loc: [0, 0]}), - iD.osmNode({id: 'b', loc: [0, 0]}), - iD.osmNode({id: 'c', loc: [0, 0]}), - iD.osmNode({id: 'd', loc: [0, 0]}), - iD.osmWay({id: '-', nodes: ['b', 'a']}), - iD.osmWay({id: '=', nodes: ['c', 'b']}), - iD.osmWay({id: '~', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r', members: [ - {id: '=', type: 'way'}, - {id: '~', type: 'way'}, - {id: '~', type: 'way'}, - {id: '=', type: 'way'} - ]}) - ]); - - var member = { id: '-', type: 'way' }; - var insertPair = { - originalID: '=', - insertedID: '-', - nodes: ['c','b','a'] - }; - graph = iD.actionAddMember('r', member, undefined, insertPair)(graph); - expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); - }); - - it('inserts the member multiple times if insertPair provided (beginning/end) (reversed pair)', function() { - // Before: a <--- b .. c ~~~> d <~~~ c .. b ---> a - // After: a <--- b <=== c ~~~> d <~~~ c ===> b ---> a - var graph = iD.coreGraph([ - iD.osmNode({id: 'a', loc: [0, 0]}), - iD.osmNode({id: 'b', loc: [0, 0]}), - iD.osmNode({id: 'c', loc: [0, 0]}), - iD.osmNode({id: 'd', loc: [0, 0]}), - iD.osmWay({id: '-', nodes: ['b', 'a']}), - iD.osmWay({id: '=', nodes: ['c', 'b']}), - iD.osmWay({id: '~', nodes: ['c', 'd']}), - iD.osmRelation({id: 'r', members: [ - {id: '-', type: 'way'}, - {id: '~', type: 'way'}, - {id: '~', type: 'way'}, - {id: '-', type: 'way'} - ]}) - ]); - - var member = { id: '-', type: 'way' }; - var insertPair = { - originalID: '-', - insertedID: '=', - nodes: ['c','b','a'] - }; - graph = iD.actionAddMember('r', member, undefined, insertPair)(graph); - expect(members(graph)).to.eql(['-', '=', '~', '~', '=', '-']); - }); - it('keeps stops and platforms ordered before node, way, relation (for PTv2 routes)', function() { var graph = iD.coreGraph([ iD.osmNode({id: 'a', loc: [0, 0]}), diff --git a/test/spec/actions/split.js b/test/spec/actions/split.js index 7a4dfd635..7202f3d8f 100644 --- a/test/spec/actions/split.js +++ b/test/spec/actions/split.js @@ -425,15 +425,14 @@ describe('iD.actionSplit', function () { } - it('handles incomplete relations', function () { + it('disables split action on too incomplete relations', function () { // // Situation: // a ---> b ---> c split at 'b' - // Relation: ['~', '-'] + // Relation: ['?', '-'] member '?' missing // // Expected result: - // a ---> b ===> c - // Relation: ['~', '-', '='] + // forbidden, because correct order of -/= cannot be determined // var graph = iD.coreGraph([ iD.osmNode({ id: 'a', loc: [0, 0] }), @@ -441,13 +440,73 @@ describe('iD.actionSplit', function () { iD.osmNode({ id: 'c', loc: [2, 0] }), iD.osmWay({ id: '-', nodes: ['a', 'b', 'c'] }), iD.osmRelation({id: 'r', members: [ - { id: '~', type: 'way' }, + { id: '?', type: 'way' }, { id: '-', type: 'way' } ]}) ]); - graph = iD.actionSplit('b', ['='])(graph); - expect(members(graph)).to.eql(['~', '-', '=']); + var action = iD.actionSplit('b', ['=']); + expect(action.disabled(graph)).to.be.ok; + }); + + it('enables split action on partially incomplete, but still sufficiently complete relations (before split)', function () { + // + // Situation: + // a ~~~> b ---> c ---> d split at 'c' + // Relation: ['~', '-', '?'] member '?' missing + // + // Expected result: + // a ~~~> b ---> c ===> d + // Relation: ['~', '-', '=', '?'] + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [2, 0] }), + iD.osmNode({ id: 'd', loc: [3, 0] }), + iD.osmWay({ id: '~', nodes: ['a', 'b'] }), + iD.osmWay({ id: '-', nodes: ['b', 'c', 'd'] }), + iD.osmRelation({id: 'r', members: [ + { id: '~', type: 'way' }, + { id: '-', type: 'way' }, + { id: '?', type: 'way' } + ]}) + ]); + + var action = iD.actionSplit('c', ['=']); + expect(action.disabled(graph)).to.be.not.ok; + graph = action(graph); + expect(members(graph)).to.eql(['~', '-', '=', '?']); + }); + + it('enables split action on partially incomplete, but still sufficiently complete relations (after split)', function () { + // + // Situation: + // a ---> b ---> c ~~~> d split at 'b' + // Relation: ['?', '-', '~'] member '?' missing + // + // Expected result: + // a ---> b ===> c ~~~> d + // Relation: ['?', '-', '=', '~'] + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [2, 0] }), + iD.osmNode({ id: 'd', loc: [3, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c'] }), + iD.osmWay({ id: '~', nodes: ['c', 'd'] }), + iD.osmRelation({id: 'r', members: [ + { id: '?', type: 'way' }, + { id: '-', type: 'way' }, + { id: '~', type: 'way' } + ]}) + ]); + + var action = iD.actionSplit('b', ['=']); + expect(action.disabled(graph)).to.be.not.ok; + graph = action(graph); + expect(members(graph)).to.eql(['?', '-', '=', '~']); }); @@ -593,28 +652,28 @@ describe('iD.actionSplit', function () { ]); }); - it('reorders members as node, way, relation (for Public Transport routing)', function () { + it('preserves other members (example: Public Transport routing)', function () { var graph = iD.coreGraph([ iD.osmNode({ id: 'a', loc: [0, 0] }), iD.osmNode({ id: 'b', loc: [1, 0] }), iD.osmNode({ id: 'c', loc: [2, 0] }), iD.osmWay({ id: '-', nodes: ['a', 'b', 'c'] }), iD.osmRelation({id: 'r', members: [ - { id: 'n1', type: 'node', role: 'forward' }, + { id: 'n1', type: 'node', role: 'stop' }, { id: '-', type: 'way', role: 'forward' }, - { id: 'r1', type: 'relation', role: 'forward' }, - { id: 'n2', type: 'node', role: 'forward' } + { id: 'r1', type: 'relation', role: '' }, + { id: 'n2', type: 'node', role: 'stop' } ]}) ]); graph = iD.actionSplit('b', ['='])(graph); expect(graph.entity('r').members).to.eql([ - { id: 'n1', type: 'node', role: 'forward' }, - { id: 'n2', type: 'node', role: 'forward' }, + { id: 'n1', type: 'node', role: 'stop' }, { id: '-', type: 'way', role: 'forward' }, { id: '=', type: 'way', role: 'forward' }, - { id: 'r1', type: 'relation', role: 'forward'} + { id: 'r1', type: 'relation', role: ''}, + { id: 'n2', type: 'node', role: 'stop' } ]); }); }); From 088db7ccc13034c1dec27ac166b3344d0fdb68f9 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Mon, 4 Mar 2024 17:25:54 +0100 Subject: [PATCH 02/11] simplify boolean conditions --- modules/actions/split.js | 76 +++++++++------------------------------- 1 file changed, 16 insertions(+), 60 deletions(-) diff --git a/modules/actions/split.js b/modules/actions/split.js index 23905622f..92fab3ebf 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -232,6 +232,15 @@ export function actionSplit(nodeIds, newWayIds) { } function splitWayMember(graph, relationId, wayA, wayB) { + function connects(way1, way2) { + if (way1.nodes.length < 2 || way2.nodes.length < 2) return false; + if (way1.nodes[0] === way2.nodes[0]) return true; + if (way1.nodes[0] === way2.nodes[way2.nodes.length - 1]) return true; + if (way1.nodes[way1.nodes.length - 1] === way2.nodes[way2.nodes.length - 1]) return true; + if (way1.nodes[way1.nodes.length - 1] === way2.nodes[0]) return true; + return false; + } + let relation = graph.entity(relationId); const insertMembers = []; for (let i = 0; i < relation.members.length; i++) { @@ -242,15 +251,6 @@ export function actionSplit(nodeIds, newWayIds) { let wayBconnectsPrev = false; let wayBconnectsNext = false; - function connects(way1, way2) { - if (way1.nodes.length < 2 || way2.nodes.length < 2) return false; - if (way1.nodes[0] === way2.nodes[0]) return true; - if (way1.nodes[0] === way2.nodes[way2.nodes.length - 1]) return true; - if (way1.nodes[way1.nodes.length - 1] === way2.nodes[way2.nodes.length - 1]) return true; - if (way1.nodes[way1.nodes.length - 1] === way2.nodes[0]) return true; - return false; - } - if (i > 0 && graph.hasEntity(relation.members[i - 1].id)) { const prevMember = relation.members[i - 1]; const prevEntity = graph.entity(prevMember.id); @@ -268,61 +268,21 @@ export function actionSplit(nodeIds, newWayIds) { } } - if (wayAconnectsPrev && !wayBconnectsPrev && !wayAconnectsNext && !wayBconnectsNext) { - // wayA connects to prev member -> insert B after A + if (wayAconnectsPrev && !wayAconnectsNext || + !wayBconnectsPrev && wayBconnectsNext && !(!wayAconnectsPrev && wayAconnectsNext) + ) { insertMembers.push({at: i + 1, role: member.role}); continue; } - if (wayAconnectsPrev && !wayBconnectsPrev && wayAconnectsNext && wayBconnectsNext) { - // wayB only connects to next -> insert B after A - insertMembers.push({at: i + 1, role: member.role}); - continue; - } - if (!wayAconnectsPrev && !wayBconnectsPrev && !wayAconnectsNext && wayBconnectsNext) { - // wayB connects to next member -> insert B after A - insertMembers.push({at: i + 1, role: member.role}); - continue; - } - if (wayAconnectsPrev && wayBconnectsPrev && !wayAconnectsNext && wayBconnectsNext) { - // wayA only connects to prev -> insert B after A - insertMembers.push({at: i + 1, role: member.role}); - continue; - } - if (wayAconnectsPrev && !wayBconnectsPrev && !wayAconnectsNext && wayBconnectsNext) { - // wayA connects to prev, wayB connects to next -> insert B after A - insertMembers.push({at: i + 1, role: member.role}); - continue; - } - - if (!wayAconnectsPrev && wayBconnectsPrev && !wayAconnectsNext && !wayBconnectsNext) { - // wayB connects to prev member -> insert B before A - insertMembers.push({at: i, role: member.role}); - continue; - } - if (!wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && wayBconnectsNext) { - // wayA only connects to next -> insert B before A - insertMembers.push({at: i, role: member.role}); - continue; - } - if (!wayAconnectsPrev && !wayBconnectsPrev && wayAconnectsNext && !wayBconnectsNext) { - // wayA connects to next member -> insert B before A - insertMembers.push({at: i, role: member.role}); - continue; - } - if (wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && !wayBconnectsNext) { - // wayB only connects to prev -> insert B before A - insertMembers.push({at: i, role: member.role}); - continue; - } - if (!wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && !wayBconnectsNext) { - // wayB connects to prev, wayA connects to next -> insert B before A + if (!wayAconnectsPrev && wayAconnectsNext || + wayBconnectsPrev && !wayBconnectsNext && !(wayAconnectsPrev && !wayAconnectsNext) + ) { insertMembers.push({at: i, role: member.role}); continue; } // check for loops if (wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && wayBconnectsNext) { - // complete loop // look one more member ahead if (i > 2 && graph.hasEntity(relation.members[i - 2].id)) { const prev2Entity = graph.entity(relation.members[i - 2].id); @@ -351,6 +311,7 @@ export function actionSplit(nodeIds, newWayIds) { } } } + // could not determine how new member should connect (i.e. existing way was not connected to other member ways) // just make sure before/after still connect if (wayA.nodes[wayA.nodes.length - 1] === wayB.nodes[0]) { @@ -360,11 +321,6 @@ export function actionSplit(nodeIds, newWayIds) { insertMembers.push({at: i, role: member.role}); continue; } - - /* - // could not determine how new member should connect (i.e. existing way was not connected to other member ways) - // -> insert new way after existing way - insertMembers.push({at: i + 1, role: member.role});*/ } } // insert new member(s) From 7268a8538c3a32d340d475908dbb41291f92913b Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Mon, 4 Mar 2024 17:58:12 +0100 Subject: [PATCH 03/11] handle closed roundabout ways like a single junction "point" --- modules/actions/split.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/modules/actions/split.js b/modules/actions/split.js index 92fab3ebf..8e3a51d90 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -234,6 +234,15 @@ export function actionSplit(nodeIds, newWayIds) { function splitWayMember(graph, relationId, wayA, wayB) { function connects(way1, way2) { if (way1.nodes.length < 2 || way2.nodes.length < 2) return false; + if (way1.tags.junction === 'roundabout' && way1.isClosed()) { + return way1.nodes.some(nodeId => + nodeId === way2.nodes[0] || + nodeId === way2.nodes[way2.nodes.length - 1]); + } else if (way2.tags.junction === 'roundabout' && way2.isClosed()) { + return way2.nodes.some(nodeId => + nodeId === way1.nodes[0] || + nodeId === way1.nodes[way1.nodes.length - 1]); + } if (way1.nodes[0] === way2.nodes[0]) return true; if (way1.nodes[0] === way2.nodes[way2.nodes.length - 1]) return true; if (way1.nodes[way1.nodes.length - 1] === way2.nodes[way2.nodes.length - 1]) return true; @@ -283,7 +292,7 @@ export function actionSplit(nodeIds, newWayIds) { // check for loops if (wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && wayBconnectsNext) { - // look one more member ahead + // try looking one more member ahead if (i > 2 && graph.hasEntity(relation.members[i - 2].id)) { const prev2Entity = graph.entity(relation.members[i - 2].id); if (connects(prev2Entity, wayA) && !connects(prev2Entity, wayB)) { @@ -316,10 +325,8 @@ export function actionSplit(nodeIds, newWayIds) { // just make sure before/after still connect if (wayA.nodes[wayA.nodes.length - 1] === wayB.nodes[0]) { insertMembers.push({at: i + 1, role: member.role}); - continue; } else { insertMembers.push({at: i, role: member.role}); - continue; } } } From 82af60115dfec446851091bc97be2a6e1b6d4a50 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Mon, 4 Mar 2024 18:16:31 +0100 Subject: [PATCH 04/11] disalow splitting of closed roundabouts when they are part of a route these cases are not covered by the splitting algorithm at the moment: * typicall, only a part of the roundabout should be part of the resulting route * when multiple routes meet at a roundabout coming from different angles, the roundabout must be split at any of those entry/exit point simutaneously, such that all routes remain correct as a workaround, a mapper can remove the route relation(s) from the roundabout, split it accordingly and then re-add the respective parts to the relation(s) again. --- data/core.yaml | 1 + modules/actions/split.js | 3 +++ 2 files changed, 4 insertions(+) diff --git a/data/core.yaml b/data/core.yaml index 68bd982f7..e02459336 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -500,6 +500,7 @@ en: other: "Split {n} features." not_eligible: Lines can't be split at their beginning or end. parent_incomplete: This line cannot be split because it is part of a larger relation which has only been partially loaded. Make sure that all ways connected to this way are present on the map. + simple_roundabout: This line cannot be split because this roundabout is part of a larger relation. You must remove it from the relation first. connected_to_hidden: This can't be split because it is connected to a hidden feature. restriction: annotation: diff --git a/modules/actions/split.js b/modules/actions/split.js index 8e3a51d90..2039bc3e9 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -428,6 +428,9 @@ export function actionSplit(nodeIds, newWayIds) { } } } + if (way.tags.junction === 'roundabout' && way.isClosed()) { + return 'simple_roundabout'; + } } } } From 13d46b05788739566a6105442c612f9fd277afa7 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Mon, 4 Mar 2024 23:37:29 +0100 Subject: [PATCH 05/11] typo --- modules/actions/split.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/actions/split.js b/modules/actions/split.js index 2039bc3e9..2e6bcb80f 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -411,7 +411,7 @@ export function actionSplit(nodeIds, newWayIds) { const parentRelations = graph.parentRelations(way); for (const parentRelation of parentRelations) { if (parentRelation.hasFromViaTo()) { - // turn restrictions: via memebers must be loaded + // turn restrictions: via members must be loaded const vias = parentRelation.membersByRole('via'); if (!vias.every(via => graph.hasEntity(via.id))) { return 'parent_incomplete'; From 296ce859cfee0ee2e5b6d7970ec4baa6ed5b85fa Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Tue, 5 Mar 2024 13:40:37 +0100 Subject: [PATCH 06/11] allow splitting of closed roundabout for certain rel types --- modules/actions/split.js | 5 +- test/spec/actions/split.js | 125 +++++++++++++++++++++++++++++-------- 2 files changed, 101 insertions(+), 29 deletions(-) diff --git a/modules/actions/split.js b/modules/actions/split.js index 2e6bcb80f..46bdbece6 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -422,13 +422,14 @@ export function actionSplit(nodeIds, newWayIds) { 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) { + if (!memberBeforePresent && !memberAfterPresent && parentRelation.members.length > 1) { return 'parent_incomplete'; } } } } - if (way.tags.junction === 'roundabout' && way.isClosed()) { + const relTypesExceptions = ['junction', 'enforcement']; // some relation types should not prehibit a member from being split + if (way.tags.junction === 'roundabout' && way.isClosed() && !relTypesExceptions.includes(parentRelation.tags.type)) { return 'simple_roundabout'; } } diff --git a/test/spec/actions/split.js b/test/spec/actions/split.js index 7202f3d8f..07794b810 100644 --- a/test/spec/actions/split.js +++ b/test/spec/actions/split.js @@ -122,6 +122,102 @@ describe('iD.actionSplit', function () { expect(iD.actionSplit('*').limitWays(['-', '=']).disabled(graph)).to.equal('not_eligible'); }); + + it('returns \'parent_incomplete\' when parent relations are too incomplete', function () { + // + // Situation: + // a ---> b ---> c split at 'b' + // Relation: ['?', '-'] member '?' missing + // + // Expected result: + // forbidden, because correct order of -/= cannot be determined + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [2, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c'] }), + iD.osmRelation({id: 'r', members: [ + { id: '?', type: 'way' }, + { id: '-', type: 'way' } + ]}) + ]); + + var action = iD.actionSplit('b', ['=']); + expect(action.disabled(graph)).to.equal('parent_incomplete'); + }); + + it('allows split operation for single-member relations', function () { + // + // Situation: + // a ---> b ---> c split at 'b' + // Relation: ['-'] + // + // Expected result: + // any order is allowed, because the way is the only member of the relation + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [2, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c'] }), + iD.osmRelation({id: 'r', members: [ + { id: '-', type: 'way' } + ]}) + ]); + + var action = iD.actionSplit('b', ['=']); + expect(action.disabled(graph)).to.be.not.ok; + }); + + it('returns \'simple_roundabout\' when a closed roundabout is part of a route relations', function () { + // + // Situation: + // x ~~~> b / a ---> b ---> c ---> a / c ===> y split at 'b' + // Relation: ['~', '-', '='], '-' tagged as 'junction=roundabout' + // + // Expected result: + // forbidden, because the split action would break the connectedness of the route relation + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [1, 1] }), + iD.osmNode({ id: 'x', loc: [-1, -1] }), + iD.osmNode({ id: 'y', loc: [2, 2] }), + iD.osmWay({ id: '~', nodes: ['x', 'b'] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c', 'a'], tags: { junction: 'roundabout' } }), + iD.osmWay({ id: '=', nodes: ['c', 'y'] }), + iD.osmRelation({id: 'r', members: [ + { id: '~', type: 'way' }, + { id: '-', type: 'way' }, + { id: '=', type: 'way' } + ], tags: { type: 'route' }}) + ]); + + var action = iD.actionSplit('b', ['*']); + expect(action.disabled(graph)).to.equal('simple_roundabout'); + }); + + it('allows splitting of a closed roundabout that is part of a junction relation', function () { + // + // Situation: + // a ---> b ---> c ---> a split at 'b' + // Relation: ['-'], '-' tagged as 'junction=roundabout' + // + var graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [1, 1] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c', 'a'], tags: { junction: 'roundabout' } }), + iD.osmRelation({id: 'r', members: [ + { id: '-', type: 'way' } + ], tags: { type: 'junction' }}) + ]); + + var action = iD.actionSplit('a', ['*']); + expect(action.disabled(graph)).to.not.be.ok; + }); }); @@ -424,32 +520,7 @@ describe('iD.actionSplit', function () { return graph.entity('r').members.map(function (m) { return m.id; }); } - - it('disables split action on too incomplete relations', function () { - // - // Situation: - // a ---> b ---> c split at 'b' - // Relation: ['?', '-'] member '?' missing - // - // Expected result: - // forbidden, because correct order of -/= cannot be determined - // - var graph = iD.coreGraph([ - iD.osmNode({ id: 'a', loc: [0, 0] }), - iD.osmNode({ id: 'b', loc: [1, 0] }), - iD.osmNode({ id: 'c', loc: [2, 0] }), - iD.osmWay({ id: '-', nodes: ['a', 'b', 'c'] }), - iD.osmRelation({id: 'r', members: [ - { id: '?', type: 'way' }, - { id: '-', type: 'way' } - ]}) - ]); - - var action = iD.actionSplit('b', ['=']); - expect(action.disabled(graph)).to.be.ok; - }); - - it('enables split action on partially incomplete, but still sufficiently complete relations (before split)', function () { + it('allows split action on partially incomplete relation, when member before split is present', function () { // // Situation: // a ~~~> b ---> c ---> d split at 'c' @@ -479,7 +550,7 @@ describe('iD.actionSplit', function () { expect(members(graph)).to.eql(['~', '-', '=', '?']); }); - it('enables split action on partially incomplete, but still sufficiently complete relations (after split)', function () { + it('allows split action on partially incomplete relation, when member after split is present', function () { // // Situation: // a ---> b ---> c ~~~> d split at 'b' From d7111e20f59d907f6ee091e44060307b144e25be Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Tue, 5 Mar 2024 14:17:41 +0100 Subject: [PATCH 07/11] add button to load a feature's relations fully (if incomplete), fixes #5420 --- CHANGELOG.md | 4 ++- css/80_app.css | 6 +++++ modules/ui/sections/raw_member_editor.js | 2 +- modules/ui/sections/raw_membership_editor.js | 26 ++++++++++++++++++++ 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 916ef3601..194da841b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ _Breaking developer changes, which may affect downstream projects or sites that # Unreleased (2.29.0-dev) #### :tada: New Features +* Add button to fully load incompletely downloaded relations ([#5420]) #### :sparkles: Usability & Accessibility #### :scissors: Operations #### :camera: Street-Level @@ -51,7 +52,8 @@ _Breaking developer changes, which may affect downstream projects or sites that #### :rocket: Presets #### :hammer: Development -[#10135]: https://github.com/openstreetmap/iD/issues/10135# +[#5420]: https://github.com/openstreetmap/iD/issues/5420 +[#10135]: https://github.com/openstreetmap/iD/issues/10135 [@Sushil642]: https://github.com/Sushil642 diff --git a/css/80_app.css b/css/80_app.css index efe5b4166..b4f79e722 100644 --- a/css/80_app.css +++ b/css/80_app.css @@ -504,6 +504,12 @@ button.disabled .icon.operation use, vertical-align: baseline; } +button.loading .icon { + background-image: url(img/mini-loader.gif); + background-position: 0 0; + background-size: auto; +} + /* Toolbar / Persistent UI Elements ------------------------------------------------------- */ diff --git a/modules/ui/sections/raw_member_editor.js b/modules/ui/sections/raw_member_editor.js index e15712f6d..867176024 100644 --- a/modules/ui/sections/raw_member_editor.js +++ b/modules/ui/sections/raw_member_editor.js @@ -46,7 +46,7 @@ export function uiSectionRawMemberEditor(context) { d3_event.preventDefault(); // display the loading indicator - d3_select(this.parentNode).classed('tag-reference-loading', true); + d3_select(this).classed('loading', true); context.loadEntity(d.id, function() { section.reRender(); }); diff --git a/modules/ui/sections/raw_membership_editor.js b/modules/ui/sections/raw_membership_editor.js index 3ef210d7e..a22790b7f 100644 --- a/modules/ui/sections/raw_membership_editor.js +++ b/modules/ui/sections/raw_membership_editor.js @@ -206,6 +206,18 @@ export function uiSectionRawMembershipEditor(context) { } + function downloadMembers(d3_event, d) { + d3_event.preventDefault(); + const button = d3_select(this); + + // display the loading indicator + button.classed('loading', true); + context.loadEntity(d.relation.id, function() { + section.reRender(); + }); + } + + function deleteMembership(d3_event, d) { this.blur(); // avoid keeping focus on the button if (d === 0) return; // called on newrow (shouldn't happen) @@ -376,6 +388,13 @@ export function uiSectionRawMembershipEditor(context) { .style('border-color', d => d.relation.tags.colour) .text(function(d) { return utilDisplayName(d.relation); }); + labelEnter + .append('button') + .attr('class', 'members-download') + .attr('title', t('icons.download')) + .call(svgIcon('#iD-icon-load')) + .on('click', downloadMembers); + labelEnter .append('button') .attr('class', 'remove member-delete') @@ -390,6 +409,13 @@ export function uiSectionRawMembershipEditor(context) { .call(svgIcon('#iD-icon-framed-dot', 'monochrome')) .on('click', zoomToRelation); + items = items.merge(itemsEnter); + items.selectAll('button.members-download') + .classed('hide', d => { + const graph = context.graph(); + return d.relation.members.every(m => graph.hasEntity(m.id)); + }); + var wrapEnter = itemsEnter .append('div') .attr('class', 'form-field-input-wrap form-field-input-member'); From b450dae2a0cbeb976a1fd714a7568fa32f558e24 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Tue, 5 Mar 2024 14:27:13 +0100 Subject: [PATCH 08/11] add linked bug reports to changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 194da841b..137ce919c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ _Breaking developer changes, which may affect downstream projects or sites that #### :white_check_mark: Validation * Drop validation which checks for [old style multipolygons](https://wiki.openstreetmap.org/wiki/Old_style_multipolygons), as these have long been [fixed](https://blog.jochentopf.com/2017-08-28-polygon-fixing-effort-concluded.html) in OSM #### :bug: Bugfixes +* Prevent (route) relations from getting corrupted while splitting their way members in certain conditions ([#7653], [#8415]) #### :earth_asia: Localization #### :hourglass: Performance #### :mortar_board: Walkthrough / Help @@ -53,6 +54,8 @@ _Breaking developer changes, which may affect downstream projects or sites that #### :hammer: Development [#5420]: https://github.com/openstreetmap/iD/issues/5420 +[#7653]: https://github.com/openstreetmap/iD/issues/7653 +[#8415]: https://github.com/openstreetmap/iD/issues/8415 [#10135]: https://github.com/openstreetmap/iD/issues/10135 [@Sushil642]: https://github.com/Sushil642 From f89beef46d08e6e3d6387e3c657e1fdc5e41ea79 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Tue, 5 Mar 2024 17:17:38 +0100 Subject: [PATCH 09/11] add a few more test cases: split carriageway route; route with alternate --- test/spec/actions/split.js | 161 +++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) diff --git a/test/spec/actions/split.js b/test/spec/actions/split.js index 07794b810..32cb888fe 100644 --- a/test/spec/actions/split.js +++ b/test/spec/actions/split.js @@ -1436,6 +1436,167 @@ describe('iD.actionSplit', function () { }); + describe('splitting Y routes', function () { + var a = iD.osmNode({ id: 'a', loc: [0, 0] }); + var b = iD.osmNode({ id: 'b', loc: [1, 0] }); + var c = iD.osmNode({ id: 'c', loc: [2, 0] }); + var d = iD.osmNode({ id: 'd', loc: [2, -1] }); + var e = iD.osmNode({ id: 'e', loc: [3, -1] }); + + it('splits excursion part of a forking route', function () { + // + // Situation: + // a ---> b ~~~> c + // # + // #####> d ###> e + // + // Relation: ['-', '~', '#'] + // + // + // Expected result: + // a ---> b ~~~> c + // # + // #####> d ***> e + // + // Relation: ['-', '~', '#', '*'] + // + var graph = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['a', 'b']}), + iD.osmWay({id: '~', nodes: ['b', 'c']}), + iD.osmWay({id: '#', nodes: ['b', 'd', 'e']}), + iD.osmRelation({id: 'r', members: [ + {id: '-', type: 'way', role: 'main'}, + {id: '~', type: 'way', role: 'main'}, + {id: '#', type: 'way', role: 'excursion'} + ]}) + ]); + graph = iD.actionSplit('d', ['*'])(graph); + + expect(graph.entity('*').nodes).to.eql(['d', 'e']); + expect(members(graph)).to.eql(['-', '~', '#', '*']); + expect(graph.entity('r').members.find(m => m.id === '*').role).to.eql('excursion'); + }); + + it('splits main part of forking route', function () { + // + // Situation: + // a ---> b ###> c + // ~ + // ~~~~~> d ~~~> e + // + // Relation: ['-', '~', '#'] + // + // + // Expected result: + // a ---> b ###> c + // ~ + // ~~~~~> d ***> e + // + // Relation: ['-', '~', '*', '#'] + // + var graph = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['a', 'b']}), + iD.osmWay({id: '~', nodes: ['b', 'd', 'e']}), + iD.osmWay({id: '#', nodes: ['b', 'c']}), + iD.osmRelation({id: 'r', members: [ + {id: '-', type: 'way', role: 'main'}, + {id: '~', type: 'way', role: 'main'}, + {id: '#', type: 'way', role: 'excursion'} + ]}) + ]); + graph = iD.actionSplit('d', ['*'])(graph); + + expect(graph.entity('*').nodes).to.eql(['d', 'e']); + expect(members(graph)).to.eql(['-', '~', '*', '#']); + expect(graph.entity('r').members.find(m => m.id === '*').role).to.eql('main'); + }); + }); + + describe('splitting dual carriageway routes', function () { + var a = iD.osmNode({ id: 'a', loc: [0, 0] }); + var b = iD.osmNode({ id: 'b', loc: [1, 0] }); + var c = iD.osmNode({ id: 'c', loc: [2, 0] }); + var d = iD.osmNode({ id: 'd', loc: [3, 0] }); + var e = iD.osmNode({ id: 'e', loc: [1.5, -1] }); + + it('splits dual-carriageway route, preserving role (forward way)', function () { + // + // Situation: + // a ---> b <~~~~~ c ===> d + // # # + // ###> e ### + // + // Relation: ['-', '~', '#', '='] + // + // + // Expected result: + // a ---> b <~~~~~ c ===> d + // # * + // ###> e *** + // + // Relation: ['-', '~', '#', '*', '='] + // + var graph = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['a', 'b']}), + iD.osmWay({id: '~', nodes: ['b', 'c']}), + iD.osmWay({id: '#', nodes: ['b', 'e', 'c']}), + iD.osmWay({id: '=', nodes: ['c', 'd']}), + iD.osmRelation({id: 'r', members: [ + {id: '-', type: 'way'}, + {id: '~', type: 'way', role: 'forward'}, + {id: '#', type: 'way', role: 'forward'}, + {id: '=', type: 'way'} + ]}) + ]); + graph = iD.actionSplit('e', ['*'])(graph); + + expect(graph.entity('*').nodes).to.eql(['e', 'c']); + expect(members(graph)).to.eql(['-', '~', '#', '*', '=']); + expect(graph.entity('r').members.find(m => m.id === '*').role).to.eql('forward'); + }); + + it('splits dual-carriageway route, preserving role (backward way)', function () { + // + // Situation: + // a ---> b <~~~~~ c ===> d + // # # + // ### e <### + // + // Relation: ['-', '~', '#', '='] + // + // + // Expected result: + // a ---> b <~~~~~ c ===> d + // * # + // **< e <### + // + // Relation: ['-', '~', '*', '#', '='] + // + var graph = iD.coreGraph([ + a, b, c, d, e, + iD.osmWay({id: '-', nodes: ['a', 'b']}), + iD.osmWay({id: '~', nodes: ['b', 'c']}), + iD.osmWay({id: '#', nodes: ['c', 'e', 'b']}), + iD.osmWay({id: '=', nodes: ['c', 'd']}), + iD.osmRelation({id: 'r', members: [ + {id: '-', type: 'way'}, + {id: '~', type: 'way', role: 'forward'}, + {id: '#', type: 'way', role: 'backward'}, + {id: '=', type: 'way'} + ]}) + ]); + graph = iD.actionSplit('e', ['*'])(graph); + + expect(graph.entity('*').nodes).to.eql(['e', 'b']); + expect(members(graph)).to.eql(['-', '~', '*', '#', '=']); + expect(graph.entity('r').members.find(m => m.id === '*').role).to.eql('backward'); + }); + + }); + describe('type = multipolygon', function () { From d7aca27f414a74eb37888e5deaaceca6caf544a2 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Tue, 5 Mar 2024 17:20:04 +0100 Subject: [PATCH 10/11] minor code readibility refactoring --- modules/actions/split.js | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/modules/actions/split.js b/modules/actions/split.js index 46bdbece6..fb67418f0 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -252,24 +252,25 @@ export function actionSplit(nodeIds, newWayIds) { let relation = graph.entity(relationId); const insertMembers = []; - for (let i = 0; i < relation.members.length; i++) { - const member = relation.members[i]; + const members = relation.members; + for (let i = 0; i < members.length; i++) { + const member = members[i]; if (member.id === wayA.id) { let wayAconnectsPrev = false; let wayAconnectsNext = false; let wayBconnectsPrev = false; let wayBconnectsNext = false; - if (i > 0 && graph.hasEntity(relation.members[i - 1].id)) { - const prevMember = relation.members[i - 1]; + if (i > 0 && graph.hasEntity(members[i - 1].id)) { + const prevMember = members[i - 1]; const prevEntity = graph.entity(prevMember.id); if (prevEntity.type === 'way' && prevEntity.id !== wayA.id && prevEntity.nodes.length > 0) { wayAconnectsPrev = connects(prevEntity, wayA); wayBconnectsPrev = connects(prevEntity, wayB); } } - if (i < relation.members.length - 1 && graph.hasEntity(relation.members[i + 1].id)) { - const nextMember = relation.members[i + 1]; + if (i < members.length - 1 && graph.hasEntity(members[i + 1].id)) { + const nextMember = members[i + 1]; const nextEntity = graph.entity(nextMember.id); if (nextEntity.type === 'way' && nextEntity.nodes.length > 0) { wayAconnectsNext = connects(nextEntity, wayA); @@ -293,8 +294,8 @@ export function actionSplit(nodeIds, newWayIds) { // check for loops if (wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && wayBconnectsNext) { // try looking one more member ahead - if (i > 2 && graph.hasEntity(relation.members[i - 2].id)) { - const prev2Entity = graph.entity(relation.members[i - 2].id); + if (i > 2 && graph.hasEntity(members[i - 2].id)) { + const prev2Entity = graph.entity(members[i - 2].id); if (connects(prev2Entity, wayA) && !connects(prev2Entity, wayB)) { // prev-2 member connects only to A: insert B before A insertMembers.push({at: i, role: member.role}); @@ -306,8 +307,8 @@ export function actionSplit(nodeIds, newWayIds) { continue; } } - if (i < relation.members.length - 2 && graph.hasEntity(relation.members[i + 2].id)) { - const next2Entity = graph.entity(relation.members[i + 2].id); + if (i < members.length - 2 && graph.hasEntity(members[i + 2].id)) { + const next2Entity = graph.entity(members[i + 2].id); if (connects(next2Entity, wayA) && !connects(next2Entity, wayB)) { // next+2 member connects only to A: insert B after A insertMembers.push({at: i + 1, role: member.role}); From 20f53e5748d6c944d0528a8c622a51ab7c2ccce8 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Wed, 6 Mar 2024 13:46:00 +0100 Subject: [PATCH 11/11] document algorithm inline, handle "junction=circular" like roundabouts --- modules/actions/split.js | 84 +++++++++++++++++++++++++++++++--------- 1 file changed, 66 insertions(+), 18 deletions(-) diff --git a/modules/actions/split.js b/modules/actions/split.js index fb67418f0..3a9e06908 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -26,6 +26,9 @@ export function actionSplit(nodeIds, newWayIds) { // the strategy for picking which way will have a new version and which way is newly created var _keepHistoryOn = 'longest'; // 'longest', 'first' + // these closed ways have to be treated in a special way when contained in a (route) relation + const circularJunctions = ['roundabout', 'circular']; + // The IDs of the ways actually created by running this action var _createdWayIDs = []; @@ -231,14 +234,32 @@ export function actionSplit(nodeIds, newWayIds) { return graph; } + // Handles (most kinds of) parent relations of a way that is split: + // We need to find the correct order to insert the newly created way + // relative to the existing way. + // + // This applies some heuristics to find the most likely correct order to + // perform the operation, working under the assumption that the members + // of the relation are already "properly" sorted and that the relevant + // member entities are loaded in graph: The new way is inserted into the + // relation before or after the existing way depending on how the old/new + // way connect to their neighboring members. + // + // As this is a local operation, it means that even if these conditions + // are not met, the order of the relation members will at most be incorrect + // between the existing and newly created way; other relation members are + // kept unmodified. function splitWayMember(graph, relationId, wayA, wayB) { + // returns true if way1 connects to way2 at either end node, or if one + // of the two ways is tagged as a "roundabout" and connects to the other + // way at any of its nodes. function connects(way1, way2) { if (way1.nodes.length < 2 || way2.nodes.length < 2) return false; - if (way1.tags.junction === 'roundabout' && way1.isClosed()) { + if (circularJunctions.includes(way1.tags.junction) && way1.isClosed()) { return way1.nodes.some(nodeId => nodeId === way2.nodes[0] || nodeId === way2.nodes[way2.nodes.length - 1]); - } else if (way2.tags.junction === 'roundabout' && way2.isClosed()) { + } else if (circularJunctions.includes(way2.tags.junction) && way2.isClosed()) { return way2.nodes.some(nodeId => nodeId === way1.nodes[0] || nodeId === way1.nodes[way1.nodes.length - 1]); @@ -251,33 +272,60 @@ export function actionSplit(nodeIds, newWayIds) { } let relation = graph.entity(relationId); + // insertMembers stores the positions where the new way (wayB) is to be inserted + // into the parent relation const insertMembers = []; const members = relation.members; for (let i = 0; i < members.length; i++) { const member = members[i]; - if (member.id === wayA.id) { + if (member.id === wayA.id) { // wayA is the existing way + // determine connection matrix of wayA, wayB and their neighboring members let wayAconnectsPrev = false; let wayAconnectsNext = false; let wayBconnectsPrev = false; let wayBconnectsNext = false; - if (i > 0 && graph.hasEntity(members[i - 1].id)) { - const prevMember = members[i - 1]; - const prevEntity = graph.entity(prevMember.id); - if (prevEntity.type === 'way' && prevEntity.id !== wayA.id && prevEntity.nodes.length > 0) { + const prevEntity = graph.entity(members[i - 1].id); + if (prevEntity.type === 'way') { wayAconnectsPrev = connects(prevEntity, wayA); wayBconnectsPrev = connects(prevEntity, wayB); } } if (i < members.length - 1 && graph.hasEntity(members[i + 1].id)) { - const nextMember = members[i + 1]; - const nextEntity = graph.entity(nextMember.id); - if (nextEntity.type === 'way' && nextEntity.nodes.length > 0) { + const nextEntity = graph.entity(members[i + 1].id); + if (nextEntity.type === 'way') { wayAconnectsNext = connects(nextEntity, wayA); wayBconnectsNext = connects(nextEntity, wayB); } } - + // possible outcomes of connection matrix + // + // ⟍ 0 0 1 1 <- wayA connects to next member + // ⟍ 0 1 0 1 <- wayB connects to next member + // +---+---+---+---+ + // 0 0 | ? | → | ← | * | → ... wayB should be inserted after wayA + // +---+---+---+---+ ← ... wayB should be inserted before wayA + // 0 1 | ← | x | ← | ← | ↺ ... members form a loop + // +---+---+---+---+ ? ... wayA/B do not connect to their neighbor members + // 1 0 | → | → | x | → | x ... undefined state + // +---+---+---+---+ * ... undefined state (any order results in a connection) + // 1 1 | * | → | ← | ↺ | + // +---+---+---+---+ + // ^ ^ + // | | + // | +-- wayB connects to previous member + // +---- wayA connects to previous member + // + // These boolean conditions can be simplified to the following conditions + // (considering the outcome as arbitrary for the undefined "*" cases), + // i.e. wayB should be inserted after wayA if: + // * wayA connects the the previous member but not the next one, or + // * wayB connects to the next member but not the previous one, and wayA's connectivity does not contradict that + // (and vice versa) + // the remaining cases to be handles specifically are: + // * unconnected ways + // * members for a loop + // * a few invalid/undefined cases (e.g. forks with no proper solution) if (wayAconnectsPrev && !wayAconnectsNext || !wayBconnectsPrev && wayBconnectsNext && !(!wayAconnectsPrev && wayAconnectsNext) ) { @@ -290,10 +338,9 @@ export function actionSplit(nodeIds, newWayIds) { insertMembers.push({at: i, role: member.role}); continue; } - - // check for loops + // loops: try to look one further member ahead/behind to resolve the connectivity if (wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && wayBconnectsNext) { - // try looking one more member ahead + // look one further member ahead if (i > 2 && graph.hasEntity(members[i - 2].id)) { const prev2Entity = graph.entity(members[i - 2].id); if (connects(prev2Entity, wayA) && !connects(prev2Entity, wayB)) { @@ -307,6 +354,7 @@ export function actionSplit(nodeIds, newWayIds) { continue; } } + // look one further member behind if (i < members.length - 2 && graph.hasEntity(members[i + 2].id)) { const next2Entity = graph.entity(members[i + 2].id); if (connects(next2Entity, wayA) && !connects(next2Entity, wayB)) { @@ -322,8 +370,8 @@ export function actionSplit(nodeIds, newWayIds) { } } - // could not determine how new member should connect (i.e. existing way was not connected to other member ways) - // just make sure before/after still connect + // could not determine how new member should connect (e.g. existing way was not + // connected to other member ways): insert them in the original orientation of wayA if (wayA.nodes[wayA.nodes.length - 1] === wayB.nodes[0]) { insertMembers.push({at: i + 1, role: member.role}); } else { @@ -331,7 +379,7 @@ export function actionSplit(nodeIds, newWayIds) { } } } - // insert new member(s) + // insert new member(s) at the determined indices insertMembers.reverse().forEach(item => { graph = graph.replace(relation.addMember({ id: wayB.id, @@ -430,7 +478,7 @@ export function actionSplit(nodeIds, newWayIds) { } } const relTypesExceptions = ['junction', 'enforcement']; // some relation types should not prehibit a member from being split - if (way.tags.junction === 'roundabout' && way.isClosed() && !relTypesExceptions.includes(parentRelation.tags.type)) { + if (circularJunctions.includes(way.tags.junction) && way.isClosed() && !relTypesExceptions.includes(parentRelation.tags.type)) { return 'simple_roundabout'; } }