From f0a27bc1ec9079f5349deca81b41653d80fd914b Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Tue, 9 Jan 2018 10:07:21 -0500 Subject: [PATCH] Simplify way segmentation and fix bug with adjacent segment type (closes #4669) Now instead of creating MultiLineString targets, we just create a bunch of LineString targets. This makes the code simpler, and anyway the entity is still there in `properties` for drawing code to decide what to do with the target. Incidentally, this change allows iD to support an extrusion operation. (Because each way segment has its own unique GeoJSON target now) --- modules/behavior/draw.js | 8 +- modules/behavior/draw_way.js | 29 +++---- modules/modes/drag_node.js | 21 ++---- modules/modes/draw_area.js | 4 +- modules/modes/draw_line.js | 4 +- modules/svg/helpers.js | 141 +++++++++++++---------------------- modules/svg/vertices.js | 11 ++- 7 files changed, 85 insertions(+), 133 deletions(-) diff --git a/modules/behavior/draw.js b/modules/behavior/draw.js index 5866d3c26..d372a6652 100644 --- a/modules/behavior/draw.js +++ b/modules/behavior/draw.js @@ -128,12 +128,12 @@ export function behaviorDraw(context) { // - `behavior/draw_way.js` `move()` function click() { var d = datum(); - var target = d && d.id && context.hasEntity(d.id); - + var target = d && d.properties && d.properties.entity; var trySnap = geoViewportEdge(context.mouse(), context.map().dimensions()) === null; + if (trySnap) { if (target && target.type === 'node') { // Snap to a node - dispatch.call('clickNode', this, target); + dispatch.call('clickNode', this, target, d); return; } else if (target && target.type === 'way') { // Snap to a way @@ -142,7 +142,7 @@ export function behaviorDraw(context) { ); if (choice) { var edge = [target.nodes[choice.index - 1], target.nodes[choice.index]]; - dispatch.call('clickWay', this, choice.loc, edge); + dispatch.call('clickWay', this, choice.loc, edge, d); return; } } diff --git a/modules/behavior/draw_way.js b/modules/behavior/draw_way.js index 3a684f9f6..1444584e1 100644 --- a/modules/behavior/draw_way.js +++ b/modules/behavior/draw_way.js @@ -75,22 +75,17 @@ export function behaviorDrawWay(context, wayId, index, mode, startGraph) { function move(datum) { context.surface().classed('nope-disabled', d3_event.altKey); - var nodeLoc = datum && datum.properties && datum.properties.entity && datum.properties.entity.loc; - var nodeGroups = datum && datum.properties && datum.properties.nodes; + var targetLoc = datum && datum.properties && datum.properties.entity && datum.properties.entity.loc; + var targetNodes = datum && datum.properties && datum.properties.nodes; var loc = context.map().mouseCoordinates(); - if (nodeLoc) { // snap to node/vertex - a point target with `.loc` - loc = nodeLoc; + if (targetLoc) { // snap to node/vertex - a point target with `.loc` + loc = targetLoc; - } else if (nodeGroups) { // snap to way - a line target with `.nodes` - var best = Infinity; - for (var i = 0; i < nodeGroups.length; i++) { - var childNodes = nodeGroups[i].map(function(id) { return context.entity(id); }); - var choice = geoChooseEdge(childNodes, context.mouse(), context.projection, end.id); - if (choice && choice.distance < best) { - best = choice.distance; - loc = choice.loc; - } + } else if (targetNodes) { // snap to way - a line target with `.nodes` + var choice = geoChooseEdge(targetNodes, context.mouse(), context.projection, end.id); + if (choice) { + loc = choice.loc; } } @@ -252,8 +247,8 @@ export function behaviorDrawWay(context, wayId, index, mode, startGraph) { // Connect the way to an existing way. - drawWay.addWay = function(loc, edge) { - if (context.surface().classed('nope')) { + drawWay.addWay = function(loc, edge, d) { + if ((d && d.properties && d.properties.nope) || context.surface().classed('nope')) { return; // can't click here } @@ -272,8 +267,8 @@ export function behaviorDrawWay(context, wayId, index, mode, startGraph) { // Connect the way to an existing node and continue drawing. - drawWay.addNode = function(node) { - if (context.surface().classed('nope')) { + drawWay.addNode = function(node, d) { + if ((d && d.properties && d.properties.nope) || context.surface().classed('nope')) { return; // can't click here } diff --git a/modules/modes/drag_node.js b/modules/modes/drag_node.js index 5a93ba526..fc4b77276 100644 --- a/modules/modes/drag_node.js +++ b/modules/modes/drag_node.js @@ -175,21 +175,16 @@ export function modeDragNode(context) { // - `behavior/draw.js` `click()` // - `behavior/draw_way.js` `move()` var d = datum(); - var nodeLoc = d && d.properties && d.properties.entity && d.properties.entity.loc; - var nodeGroups = d && d.properties && d.properties.nodes; + var targetLoc = d && d.properties && d.properties.entity && d.properties.entity.loc; + var targetNodes = d && d.properties && d.properties.nodes; - if (nodeLoc) { // snap to node/vertex - a point target with `.loc` - loc = nodeLoc; + if (targetLoc) { // snap to node/vertex - a point target with `.loc` + loc = targetLoc; - } else if (nodeGroups) { // snap to way - a line target with `.nodes` - var best = Infinity; - for (var i = 0; i < nodeGroups.length; i++) { - var childNodes = nodeGroups[i].map(function(id) { return context.entity(id); }); - var choice = geoChooseEdge(childNodes, context.mouse(), context.projection, entity.id); - if (choice && choice.distance < best) { - best = choice.distance; - loc = choice.loc; - } + } else if (targetNodes) { // snap to way - a line target with `.nodes` + var choice = geoChooseEdge(targetNodes, context.mouse(), context.projection, end.id); + if (choice) { + loc = choice.loc; } } } diff --git a/modules/modes/draw_area.js b/modules/modes/draw_area.js index 0890e3592..a06908faa 100644 --- a/modules/modes/draw_area.js +++ b/modules/modes/draw_area.js @@ -19,14 +19,14 @@ export function modeDrawArea(context, wayId, startGraph) { var addNode = behavior.addNode; - behavior.addNode = function(node) { + behavior.addNode = function(node, d) { var length = way.nodes.length; var penultimate = length > 2 ? way.nodes[length - 2] : null; if (node.id === way.first() || node.id === penultimate) { behavior.finish(); } else { - addNode(node); + addNode(node, d); } }; diff --git a/modules/modes/draw_line.js b/modules/modes/draw_line.js index ca567601e..83c2a9ae0 100644 --- a/modules/modes/draw_line.js +++ b/modules/modes/draw_line.js @@ -20,11 +20,11 @@ export function modeDrawLine(context, wayId, startGraph, affix) { .tail(t('modes.draw_line.tail')); var addNode = behavior.addNode; - behavior.addNode = function(node) { + behavior.addNode = function(node, d) { if (node.id === headId) { behavior.finish(); } else { - addNode(node); + addNode(node, d); } }; diff --git a/modules/svg/helpers.js b/modules/svg/helpers.js index 4cee3047b..bf6647e66 100644 --- a/modules/svg/helpers.js +++ b/modules/svg/helpers.js @@ -205,102 +205,65 @@ export function svgRelationMemberTags(graph) { export function svgSegmentWay(way, graph, activeID) { var features = { passive: [], active: [] }; - var coordGroups = { passive: [], active: [] }; - var nodeGroups = { passive: [], active: [] }; - var coords = []; - var nodes = []; - var startType = null; // 0 = active, 1 = passive, 2 = adjacent - var currType = null; // 0 = active, 1 = passive, 2 = adjacent - var node; + var start = {}; + var end = {}; + var node, type; for (var i = 0; i < way.nodes.length; i++) { - if (way.nodes[i] === activeID) { // vertex is the activeID - coords = []; // draw no segment here - nodes = []; - startType = null; - continue; - } - node = graph.entity(way.nodes[i]); - currType = svgPassiveVertex(node, graph, activeID); + type = svgPassiveVertex(node, graph, activeID); + end = { node: node, type: type }; - if (startType === null) { - startType = currType; + if (start.type !== undefined) { + if (start.node.id === activeID || end.node.id === activeID) { + // push nothing + } else if (start.type === 2 || end.type === 2) { // one adjacent vertex + pushActive(start, end, i); + } else if (start.type === 0 && end.type === 0) { // both active vertices + pushActive(start, end, i); + } else { + pushPassive(start, end, i); + } } - if (currType !== startType) { // line changes here - try to save a segment - - if (coords.length > 0) { // finish previous segment - coords.push(node.loc); - nodes.push(node.id); - if (startType === 2 || currType === 2) { // one adjacent vertex - coordGroups.active.push(coords); - nodeGroups.active.push(nodes); - } else if (startType === 0 && currType === 0) { // both active vertices - coordGroups.active.push(coords); - nodeGroups.active.push(nodes); - } else { - coordGroups.passive.push(coords); - nodeGroups.passive.push(nodes); - } - } - - coords = []; - nodes = []; - startType = currType; - } - - coords.push(node.loc); - nodes.push(node.id); - } - - // complete whatever segment we ended on - if (coords.length > 1) { - if (startType === 2 || currType === 2) { // one adjacent vertex - coordGroups.active.push(coords); - nodeGroups.active.push(nodes); - } else if (startType === 0 && currType === 0) { // both active vertices - coordGroups.active.push(coords); - nodeGroups.active.push(nodes); - } else { - coordGroups.passive.push(coords); - nodeGroups.passive.push(nodes); - } - } - - if (coordGroups.passive.length) { - features.passive.push({ - type: 'Feature', - id: way.id, - properties: { - target: true, - entity: way, - nodes: nodeGroups.passive - }, - geometry: { - type: 'MultiLineString', - coordinates: coordGroups.passive - } - }); - } - - if (coordGroups.active.length) { - features.active.push({ - type: 'Feature', - id: way.id + '-nope', // break the ids on purpose - properties: { - target: true, - entity: way, - nodes: nodeGroups.active, - nope: true, - originalID: way.id - }, - geometry: { - type: 'MultiLineString', - coordinates: coordGroups.active - } - }); + start = end; } return features; + + + function pushActive(start, end, index) { + features.active.push({ + type: 'Feature', + id: way.id + '-' + index + '-nope', + properties: { + nope: true, + target: true, + entity: way, + nodes: [start.node, end.node], + index: index + }, + geometry: { + type: 'LineString', + coordinates: [start.node.loc, end.node.loc] + } + }); + } + + function pushPassive(start, end, index) { + features.passive.push({ + type: 'Feature', + id: way.id + '-' + index, + properties: { + target: true, + entity: way, + nodes: [start.node, end.node], + index: index + }, + geometry: { + type: 'LineString', + coordinates: [start.node.loc, end.node.loc] + } + }); + } } diff --git a/modules/svg/vertices.js b/modules/svg/vertices.js index c242decf9..22b4477e8 100644 --- a/modules/svg/vertices.js +++ b/modules/svg/vertices.js @@ -192,7 +192,7 @@ export function svgVertices(projection, context) { if (activeID === node.id) return; // draw no target on the activeID var vertexType = svgPassiveVertex(node, graph, activeID); - if (vertexType !== 0) { // passive or adjacent - allow to connect + if (vertexType !== 0) { // passive or adjacent - allow to connect data.targets.push({ type: 'Feature', id: node.id, @@ -205,12 +205,11 @@ export function svgVertices(projection, context) { } else { data.nopes.push({ type: 'Feature', - id: node.id + '-nope', // break the ids on purpose + id: node.id + '-nope', properties: { - target: true, - entity: node, nope: true, - originalID: node.id + target: true, + entity: node }, geometry: node.asGeoJSON() }); @@ -248,7 +247,7 @@ export function svgVertices(projection, context) { // enter/update nopes.enter() .append('circle') - .attr('r', function(d) { return (_radii[d.properties.originalID] || radiuses.shadow[3]); }) + .attr('r', function(d) { return (_radii[d.properties.entity.id] || radiuses.shadow[3]); }) .merge(nopes) .attr('class', function(d) { return 'node vertex target target-nope ' + nopeClass + d.id; }) .attr('transform', getTransform);