diff --git a/CHANGELOG.md b/CHANGELOG.md index 916ef3601..137ce919c 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 @@ -45,13 +46,17 @@ _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 #### :rocket: Presets #### :hammer: Development -[#10135]: https://github.com/openstreetmap/iD/issues/10135# +[#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 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/data/core.yaml b/data/core.yaml index 20f91e406..e02459336 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -499,6 +499,8 @@ 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. + 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/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..3a9e06908 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'; @@ -27,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 = []; @@ -95,7 +97,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 +166,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 +202,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 +234,163 @@ 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 (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 (circularJunctions.includes(way2.tags.junction) && 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; + if (way1.nodes[way1.nodes.length - 1] === way2.nodes[0]) return true; + return false; + } + + 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) { // 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 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 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) + ) { + insertMembers.push({at: i + 1, role: member.role}); + continue; + } + if (!wayAconnectsPrev && wayAconnectsNext || + wayBconnectsPrev && !wayBconnectsNext && !(wayAconnectsPrev && !wayAconnectsNext) + ) { + insertMembers.push({at: i, role: member.role}); + continue; + } + // loops: try to look one further member ahead/behind to resolve the connectivity + if (wayAconnectsPrev && wayBconnectsPrev && wayAconnectsNext && wayBconnectsNext) { + // 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)) { + // 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; + } + } + // 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)) { + // 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 (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 { + insertMembers.push({at: i, role: member.role}); + } + } + } + // insert new member(s) at the determined indices + 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 +451,38 @@ 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 members 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 && 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'; + } + } + } } }; 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'); 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..32cb888fe 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,30 +520,64 @@ describe('iD.actionSplit', function () { return graph.entity('r').members.map(function (m) { return m.id; }); } - - it('handles incomplete relations', function () { + it('allows split action on partially incomplete relation, when member before split is present', function () { // // Situation: - // a ---> b ---> c split at 'b' - // Relation: ['~', '-'] + // a ~~~> b ---> c ---> d split at 'c' + // Relation: ['~', '-', '?'] member '?' missing // // Expected result: - // a ---> b ===> c - // Relation: ['~', '-', '='] + // 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.osmWay({ id: '-', nodes: ['a', 'b', 'c'] }), + 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' }, + { id: '?', type: 'way' } ]}) ]); - graph = iD.actionSplit('b', ['='])(graph); - expect(members(graph)).to.eql(['~', '-', '=']); + var action = iD.actionSplit('c', ['=']); + expect(action.disabled(graph)).to.be.not.ok; + graph = action(graph); + expect(members(graph)).to.eql(['~', '-', '=', '?']); + }); + + it('allows split action on partially incomplete relation, when member after split is present', 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 +723,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' } ]); }); }); @@ -1306,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 () {