From ffdeef398de502646cdc7d792d18f307f340d4e0 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 28 Oct 2013 16:33:53 -0700 Subject: [PATCH] Rewrite tree logic The main problem with the existing logic was that it did not update the extents of relations when previously incomplete members were loaded. It was also overly stateful; the various queues and flags are no longer required. Fixes #1928. Fixes #1540. --- js/id/core/difference.js | 8 --- js/id/core/graph.js | 18 ----- js/id/core/tree.js | 146 ++++++++++++++++----------------------- js/id/core/way.js | 7 +- test/spec/core/tree.js | 36 ++++++++++ 5 files changed, 103 insertions(+), 112 deletions(-) diff --git a/js/id/core/difference.js b/js/id/core/difference.js index 9be2ad124..6e46feb78 100644 --- a/js/id/core/difference.js +++ b/js/id/core/difference.js @@ -83,14 +83,6 @@ iD.Difference = function(base, head) { return result; }; - difference.addParents = function(entities) { - for (var i in entities) { - addParents(head.parentWays(entities[i]), entities); - addParents(head.parentRelations(entities[i]), entities); - } - return entities; - }; - difference.summary = function() { var relevant = {}; diff --git a/js/id/core/graph.js b/js/id/core/graph.js index 6837c3fdf..029cbdca6 100644 --- a/js/id/core/graph.js +++ b/js/id/core/graph.js @@ -242,24 +242,6 @@ iD.Graph.prototype = { return this; }, - hasAllChildren: function(entity) { - // we're only checking changed entities, since we assume fetched data - // must have all children present - var i; - if (this.entities.hasOwnProperty(entity.id)) { - if (entity.type === 'way') { - for (i = 0; i < entity.nodes.length; i++) { - if (!this.entities[entity.nodes[i]]) return false; - } - } else if (entity.type === 'relation') { - for (i = 0; i < entity.members.length; i++) { - if (!this.entities[entity.members[i].id]) return false; - } - } - } - return true; - }, - // Obliterates any existing entities load: function(entities) { var base = this.base(); diff --git a/js/id/core/tree.js b/js/id/core/tree.js index b31e48a9c..ee93895a9 100644 --- a/js/id/core/tree.js +++ b/js/id/core/tree.js @@ -1,11 +1,6 @@ -iD.Tree = function(graph) { - +iD.Tree = function(head) { var rtree = rbush(), - head = graph, - queuedCreated = [], - queuedModified = [], - rectangles = {}, - rebased; + rectangles = {}; function extentRectangle(extent) { return [ @@ -23,88 +18,69 @@ iD.Tree = function(graph) { return rect; } - function remove(entity) { - rtree.remove(rectangles[entity.id]); - delete rectangles[entity.id]; - } - - function bulkInsert(entities) { - for (var i = 0, rects = []; i < entities.length; i++) { - rects.push(entityRectangle(entities[i])); - } - rtree.load(rects); - } - - function bulkReinsert(entities) { - entities.forEach(remove); - bulkInsert(entities); - } - - var tree = { - - rebase: function(entities) { - var inserted = []; - - entities.forEach(function(entity) { - if (!graph.entities.hasOwnProperty(entity.id) && !rectangles.hasOwnProperty(entity.id)) { - inserted.push(entity); - } - }); - - bulkInsert(inserted); - rebased = true; - return tree; - }, - - intersects: function(extent, g) { - - head = g; - - if (graph !== head || rebased) { - var diff = iD.Difference(graph, head), - modified = {}; - - diff.modified().forEach(function(d) { - var loc = graph.entities[d.id].loc; - if (!loc || loc[0] !== d.loc[0] || loc[1] !== d.loc[1]) { - modified[d.id] = d; - } - }); - - var created = diff.created().concat(queuedCreated); - modified = d3.values(diff.addParents(modified)) - // some parents might be created, not modified - .filter(function(d) { return !!graph.hasEntity(d.id); }) - .concat(queuedModified); - queuedCreated = []; - queuedModified = []; - - var reinserted = [], - inserted = []; - - modified.forEach(function(d) { - if (head.hasAllChildren(d)) reinserted.push(d); - else queuedModified.push(d); - }); - - created.forEach(function(d) { - if (head.hasAllChildren(d)) inserted.push(d); - else queuedCreated.push(d); - }); - - bulkReinsert(reinserted); - bulkInsert(inserted); - - diff.deleted().forEach(remove); - - graph = head; - rebased = false; + function updateParents(entity, insertions) { + head.parentWays(entity).forEach(function(parent) { + if (rectangles[parent.id]) { + rtree.remove(rectangles[parent.id]); + insertions.push(entityRectangle(parent)); } + }); - return rtree.search(extentRectangle(extent)).map(function (rect) { - return graph.entities[rect.id]; + head.parentRelations(entity).forEach(function(parent) { + if (rectangles[parent.id]) { + rtree.remove(rectangles[parent.id]); + insertions.push(entityRectangle(parent)); + } + updateParents(parent, insertions); + }); + } + + var tree = {}; + + tree.rebase = function(entities) { + var insertions = []; + + entities.forEach(function(entity) { + if (head.entities.hasOwnProperty(entity.id) || rectangles[entity.id]) + return; + + insertions.push(entityRectangle(entity)); + updateParents(entity, insertions); + }); + + rtree.load(insertions); + + return tree; + }; + + tree.intersects = function(extent, graph) { + if (graph !== head) { + var diff = iD.Difference(head, graph), + insertions = []; + + head = graph; + + diff.deleted().forEach(function(entity) { + rtree.remove(rectangles[entity.id]); + delete rectangles[entity.id]; }); + + diff.modified().forEach(function(entity) { + rtree.remove(rectangles[entity.id]); + insertions.push(entityRectangle(entity)); + updateParents(entity, insertions); + }); + + diff.created().forEach(function(entity) { + insertions.push(entityRectangle(entity)); + }); + + rtree.load(insertions); } + + return rtree.search(extentRectangle(extent)).map(function(rect) { + return head.entity(rect.id); + }); }; return tree; diff --git a/js/id/core/way.js b/js/id/core/way.js index d825ad7d5..966da007e 100644 --- a/js/id/core/way.js +++ b/js/id/core/way.js @@ -15,7 +15,12 @@ _.extend(iD.Way.prototype, { extent: function(resolver) { return resolver.transient(this, 'extent', function() { return this.nodes.reduce(function(extent, id) { - return extent.extend(resolver.entity(id).extent(resolver)); + var node = resolver.hasEntity(id); + if (node) { + return extent.extend(node.extent()); + } else { + return extent; + } }, iD.geo.Extent()); }); }, diff --git a/test/spec/core/tree.js b/test/spec/core/tree.js index 532c9afd7..3c615fb2d 100644 --- a/test/spec/core/tree.js +++ b/test/spec/core/tree.js @@ -55,6 +55,24 @@ describe("iD.Tree", function() { expect(tree.intersects(extent, graph)).to.eql([n1]); }); + it("includes intersecting relations after incomplete members are loaded", function() { + var graph = iD.Graph(), + tree = iD.Tree(graph), + n1 = iD.Node({id: 'n1', loc: [0, 0]}), + n2 = iD.Node({id: 'n2', loc: [1, 1]}), + relation = iD.Relation({id: 'r', members: [{id: 'n1'}, {id: 'n2'}]}), + extent = iD.geo.Extent([0.5, 0.5], [1.5, 1.5]); + + graph.rebase({r: relation, n1: n1}); + tree.rebase([relation, n1]); + expect(tree.intersects(extent, graph)).to.eql([]); + + graph.rebase({n2: n2}); + tree.rebase([n2]); + expect(tree.intersects(extent, graph)).to.eql([n2, relation]); + }); + + // This happens when local storage includes a changed way but not its nodes. it("includes intersecting ways after missing nodes are loaded", function() { var base = iD.Graph(), tree = iD.Tree(base), @@ -66,6 +84,7 @@ describe("iD.Tree", function() { expect(tree.intersects(extent, graph)).to.eql([]); base.rebase({n: node}); + graph.rebase({n: node}); tree.rebase([node]); expect(tree.intersects(extent, graph)).to.eql([node, way]); }); @@ -125,5 +144,22 @@ describe("iD.Tree", function() { tree.rebase([node]); expect(tree.intersects(extent, graph)).to.eql([]); }); + + it("handles recursive relations", function() { + var base = iD.Graph(), + tree = iD.Tree(base), + node = iD.Node({id: 'n', loc: [1, 1]}), + r1 = iD.Relation({id: 'r1', members: [{id: 'n'}]}), + r2 = iD.Relation({id: 'r2', members: [{id: 'r1'}]}), + extent = iD.geo.Extent([0, 0], [2, 2]); + + var graph = base.replace(r1).replace(r2); + expect(tree.intersects(extent, graph)).to.eql([]); + + base.rebase({n: node}); + graph.rebase({n: node}); + tree.rebase([node]); + expect(tree.intersects(extent, graph)).to.eql([node, r1, r2]); + }); }); });