From ce721bc42f49a7ff309864378ad9a8b223571f26 Mon Sep 17 00:00:00 2001 From: Frederic Junod Date: Mon, 3 Feb 2014 14:57:28 +0100 Subject: [PATCH 1/5] Assert that an element is removed in ol.structs.RBush#remove_ --- src/ol/structs/rbush.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ol/structs/rbush.js b/src/ol/structs/rbush.js index 349631748f..06367bb75c 100644 --- a/src/ol/structs/rbush.js +++ b/src/ol/structs/rbush.js @@ -660,6 +660,7 @@ ol.structs.RBush.prototype.remove_ = function(extent, value) { } } } + goog.asserts.fail(); }; From c94b78144f6aa70c3c6ce79a1bf9b2cfc055ba5e Mon Sep 17 00:00:00 2001 From: Frederic Junod Date: Mon, 3 Feb 2014 15:40:37 +0100 Subject: [PATCH 2/5] Fix test description --- test/spec/ol/structs/rbush.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/spec/ol/structs/rbush.test.js b/test/spec/ol/structs/rbush.test.js index bb2f78c62f..dc5c20f7c8 100644 --- a/test/spec/ol/structs/rbush.test.js +++ b/test/spec/ol/structs/rbush.test.js @@ -319,7 +319,7 @@ describe('ol.structs.RBush', function() { describe('#remove', function() { - it('can remove all 2000 objects', function() { + it('can remove all 1000 objects', function() { var objs = rBush.getAll(); var i, value; for (i = objs.length - 1; i >= 0; --i) { From 09326519d306dc84bf18c91859e7ebadd8dde174 Mon Sep 17 00:00:00 2001 From: Tom Payne Date: Thu, 6 Feb 2014 01:06:15 +0100 Subject: [PATCH 3/5] Replace faulty iterative ol.structs.RBush#remove with less faulty recursive version --- src/ol/structs/rbush.js | 73 ++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 38 deletions(-) diff --git a/src/ol/structs/rbush.js b/src/ol/structs/rbush.js index 06367bb75c..9c4497e616 100644 --- a/src/ol/structs/rbush.js +++ b/src/ol/structs/rbush.js @@ -616,51 +616,48 @@ ol.structs.RBush.prototype.remove = function(value) { * @private */ ol.structs.RBush.prototype.remove_ = function(extent, value) { - var node = this.root_; - var index = 0; - /** @type {Array.>} */ - var path = [node]; - /** @type {Array.} */ - var indexes = [0]; - var childrenDone, child, children, i, ii; - while (path.length > 0) { - childrenDone = false; - goog.asserts.assert(node.height > 0); - if (node.height == 1) { - children = node.children; - for (i = 0, ii = children.length; i < ii; ++i) { - child = children[i]; - if (child.value === value) { - goog.array.removeAt(children, i); - this.condense_(path); - return; - } + var path = [this.root_]; + var removed = this.removeRecursive_(this.root_, extent, value, path); + goog.asserts.assert(removed); + this.condense_(path); +}; + + +/** + * @param {ol.structs.RBushNode.} node Node. + * @param {ol.Extent} extent Extent. + * @param {T} value Value. + * @param {Array.>} path Path. + * @private + * @return {boolean} Removed. + */ +ol.structs.RBush.prototype.removeRecursive_ = + function(node, extent, value, path) { + var children = node.children; + var ii = children.length; + var child, i; + if (node.height == 1) { + for (i = 0; i < ii; ++i) { + child = children[i]; + if (child.value === value) { + goog.array.removeAt(children, i); + return true; } - childrenDone = true; - } else if (index < node.children.length) { - child = node.children[index]; + } + } else { + goog.asserts.assert(node.height > 1); + for (i = 0; i < ii; ++i) { + child = children[i]; if (ol.extent.containsExtent(child.extent, extent)) { path.push(child); - indexes.push(index + 1); - node = child; - index = 0; - } else { - ++index; - } - } else { - childrenDone = true; - } - if (childrenDone) { - var lastPathIndex = path.length - 1; - node = path[lastPathIndex]; - index = ++indexes[lastPathIndex]; - if (index > node.children.length) { + if (this.removeRecursive_(child, extent, value, path)) { + return true; + } path.pop(); - indexes.pop(); } } } - goog.asserts.fail(); + return false; }; From 7f64a09e3c785384f995a42b21af980e7a35014f Mon Sep 17 00:00:00 2001 From: Tom Payne Date: Thu, 6 Feb 2014 01:14:19 +0100 Subject: [PATCH 4/5] Return, and check, boolean value from ol.structs.RBush#remove indicating whether value was removed --- src/ol/structs/rbush.js | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/ol/structs/rbush.js b/src/ol/structs/rbush.js index 9c4497e616..0c805e7327 100644 --- a/src/ol/structs/rbush.js +++ b/src/ol/structs/rbush.js @@ -597,6 +597,7 @@ ol.structs.RBush.prototype.isEmpty = function() { /** * @param {T} value Value. + * @return {boolean} Removed. */ ol.structs.RBush.prototype.remove = function(value) { if (goog.DEBUG && this.readers_) { @@ -606,7 +607,7 @@ ol.structs.RBush.prototype.remove = function(value) { goog.asserts.assert(this.valueExtent_.hasOwnProperty(key)); var extent = this.valueExtent_[key]; delete this.valueExtent_[key]; - this.remove_(extent, value); + return this.remove_(extent, value); }; @@ -614,12 +615,18 @@ ol.structs.RBush.prototype.remove = function(value) { * @param {ol.Extent} extent Extent. * @param {T} value Value. * @private + * @return {boolean} Removed. */ ol.structs.RBush.prototype.remove_ = function(extent, value) { var path = [this.root_]; var removed = this.removeRecursive_(this.root_, extent, value, path); - goog.asserts.assert(removed); - this.condense_(path); + if (removed) { + this.condense_(path); + } else { + goog.asserts.assert(path.length == 1); + goog.asserts.assert(path[0] === this.root_); + } + return removed; }; @@ -710,7 +717,8 @@ ol.structs.RBush.prototype.update = function(extent, value) { var currentExtent = this.valueExtent_[key]; goog.asserts.assert(goog.isDef(currentExtent)); if (!ol.extent.equals(currentExtent, extent)) { - this.remove_(currentExtent, value); + var removed = this.remove_(currentExtent, value); + goog.asserts.assert(removed); this.insert_(extent, value, this.root_.height - 1); this.valueExtent_[key] = ol.extent.clone(extent, currentExtent); } From 286284b0c4a8af8529f6796969eb1b81f7e4a2a1 Mon Sep 17 00:00:00 2001 From: Tom Payne Date: Thu, 6 Feb 2014 01:21:22 +0100 Subject: [PATCH 5/5] Move remove from ol.structs.RBush to ol.structs.RBushNode --- src/ol/structs/rbush.js | 80 ++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 41 deletions(-) diff --git a/src/ol/structs/rbush.js b/src/ol/structs/rbush.js index 0c805e7327..aa01db816b 100644 --- a/src/ol/structs/rbush.js +++ b/src/ol/structs/rbush.js @@ -135,6 +135,41 @@ ol.structs.RBushNode.prototype.getChildrenExtent = }; +/** + * @param {ol.Extent} extent Extent. + * @param {T} value Value. + * @param {Array.>} path Path. + * @return {boolean} Removed. + */ +ol.structs.RBushNode.prototype.remove = function(extent, value, path) { + var children = this.children; + var ii = children.length; + var child, i; + if (this.height == 1) { + for (i = 0; i < ii; ++i) { + child = children[i]; + if (child.value === value) { + goog.array.removeAt(children, i); + return true; + } + } + } else { + goog.asserts.assert(this.height > 1); + for (i = 0; i < ii; ++i) { + child = children[i]; + if (ol.extent.containsExtent(child.extent, extent)) { + path.push(child); + if (child.remove(extent, value, path)) { + return true; + } + path.pop(); + } + } + } + return false; +}; + + /** * FIXME empty description for jsdoc */ @@ -618,56 +653,19 @@ ol.structs.RBush.prototype.remove = function(value) { * @return {boolean} Removed. */ ol.structs.RBush.prototype.remove_ = function(extent, value) { - var path = [this.root_]; - var removed = this.removeRecursive_(this.root_, extent, value, path); + var root = this.root_; + var path = [root]; + var removed = root.remove(extent, value, path); if (removed) { this.condense_(path); } else { goog.asserts.assert(path.length == 1); - goog.asserts.assert(path[0] === this.root_); + goog.asserts.assert(path[0] === root); } return removed; }; -/** - * @param {ol.structs.RBushNode.} node Node. - * @param {ol.Extent} extent Extent. - * @param {T} value Value. - * @param {Array.>} path Path. - * @private - * @return {boolean} Removed. - */ -ol.structs.RBush.prototype.removeRecursive_ = - function(node, extent, value, path) { - var children = node.children; - var ii = children.length; - var child, i; - if (node.height == 1) { - for (i = 0; i < ii; ++i) { - child = children[i]; - if (child.value === value) { - goog.array.removeAt(children, i); - return true; - } - } - } else { - goog.asserts.assert(node.height > 1); - for (i = 0; i < ii; ++i) { - child = children[i]; - if (ol.extent.containsExtent(child.extent, extent)) { - path.push(child); - if (this.removeRecursive_(child, extent, value, path)) { - return true; - } - path.pop(); - } - } - } - return false; -}; - - /** * @param {Array.>} path Path. * @param {number} level Level.