From 665781ee03af9dfd32a85f5b2dea27cebfa3f8bd Mon Sep 17 00:00:00 2001 From: Tom Payne Date: Wed, 27 Nov 2013 14:44:05 +0100 Subject: [PATCH 1/6] Throw an exception if an ol.structs.RBush is modified while reading --- src/ol/structs/rbush.js | 41 +++++++++++++++++++++++- test/spec/ol/structs/rbush.js | 60 +++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/ol/structs/rbush.js b/src/ol/structs/rbush.js index dc6d3dfb97..36d86329b7 100644 --- a/src/ol/structs/rbush.js +++ b/src/ol/structs/rbush.js @@ -189,6 +189,12 @@ ol.structs.RBush = function(opt_maxEntries) { */ this.valueExtent_ = {}; + /** + * @private + * @type {number} + */ + this.readers_ = 0; + }; @@ -387,7 +393,12 @@ ol.structs.RBush.prototype.condense_ = function(path) { * @template S */ ol.structs.RBush.prototype.forEach = function(callback, opt_obj) { - return this.forEach_(this.root_, callback, opt_obj); + ++this.readers_; + try { + return this.forEach_(this.root_, callback, opt_obj); + } finally { + --this.readers_; + } }; @@ -432,6 +443,25 @@ ol.structs.RBush.prototype.forEach_ = function(node, callback, opt_obj) { */ ol.structs.RBush.prototype.forEachInExtent = function(extent, callback, opt_obj) { + ++this.readers_; + try { + return this.forEachInExtent_(extent, callback, opt_obj); + } finally { + --this.readers_; + } +}; + + +/** + * @param {ol.Extent} extent Extent. + * @param {function(this: S, T): *} callback Callback. + * @param {S=} opt_obj Scope. + * @private + * @return {*} Callback return value. + * @template S + */ +ol.structs.RBush.prototype.forEachInExtent_ = + function(extent, callback, opt_obj) { /** @type {Array.>} */ var toVisit = [this.root_]; var result; @@ -496,6 +526,9 @@ ol.structs.RBush.prototype.getKey_ = function(value) { * @param {T} value Value. */ ol.structs.RBush.prototype.insert = function(extent, value) { + if (this.readers_) { + throw Error('cannot insert value while reading'); + } var key = this.getKey_(value); goog.asserts.assert(!this.valueExtent_.hasOwnProperty(key)); this.insert_(extent, value, this.root_.height - 1); @@ -535,6 +568,9 @@ ol.structs.RBush.prototype.insert_ = function(extent, value, level) { * @param {T} value Value. */ ol.structs.RBush.prototype.remove = function(value) { + if (this.readers_) { + throw Error('cannot remove value while reading'); + } var key = this.getKey_(value); goog.asserts.assert(this.valueExtent_.hasOwnProperty(key)); var extent = this.valueExtent_[key]; @@ -629,6 +665,9 @@ ol.structs.RBush.prototype.splitRoot_ = function(node1, node2) { * @param {T} value Value. */ ol.structs.RBush.prototype.update = function(extent, value) { + if (this.readers_) { + throw Error('cannot update value while reading'); + } var key = this.getKey_(value); var currentExtent = this.valueExtent_[key]; goog.asserts.assert(goog.isDef(currentExtent)); diff --git a/test/spec/ol/structs/rbush.js b/test/spec/ol/structs/rbush.js index 7549662134..473126ac09 100644 --- a/test/spec/ol/structs/rbush.js +++ b/test/spec/ol/structs/rbush.js @@ -55,6 +55,27 @@ describe('ol.structs.RBush', function() { }); + describe('#insert', function() { + + it('throws an exception if called while iterating over all values', + function() { + expect(function() { + rBush.forEach(function(value) { + rBush.insert([0, 0, 1, 1], {}); + }); + }).to.throwException(); + }); + + it('throws an exception if called while iterating over an extent', + function() { + expect(function() { + rBush.forEachInExtent([-10, -10, 10, 10], function(value) { + rBush.insert([0, 0, 1, 1], {}); + }); + }).to.throwException(); + }); + }); + describe('#remove', function() { it('can remove each object', function() { @@ -66,6 +87,45 @@ describe('ol.structs.RBush', function() { } }); + it('throws an exception if called while iterating over all values', + function() { + expect(function() { + rBush.forEach(function(value) { + rBush.remove(value); + }); + }).to.throwException(); + }); + + it('throws an exception if called while iterating over an extent', + function() { + expect(function() { + rBush.forEachInExtent([-10, -10, 10, 10], function(value) { + rBush.remove(value); + }); + }).to.throwException(); + }); + + }); + + describe('#update', function() { + + it('throws an exception if called while iterating over all values', + function() { + expect(function() { + rBush.forEach(function(value) { + rBush.update([0, 0, 1, 1], objs[1]); + }); + }).to.throwException(); + }); + + it('throws an exception if called while iterating over an extent', + function() { + expect(function() { + rBush.forEachInExtent([-10, -10, 10, 10], function(value) { + rBush.update([0, 0, 1, 1], objs[1]); + }); + }).to.throwException(); + }); }); }); From 978041b68c89e63f73a907d8333b4d6e3e083e48 Mon Sep 17 00:00:00 2001 From: Tom Payne Date: Wed, 27 Nov 2013 14:54:14 +0100 Subject: [PATCH 2/6] Only activate ol.structs.RBush conflict checks when goog.DEBUG is true --- src/ol/structs/rbush.js | 42 +++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/ol/structs/rbush.js b/src/ol/structs/rbush.js index 36d86329b7..63bba6bfdd 100644 --- a/src/ol/structs/rbush.js +++ b/src/ol/structs/rbush.js @@ -189,11 +189,13 @@ ol.structs.RBush = function(opt_maxEntries) { */ this.valueExtent_ = {}; - /** - * @private - * @type {number} - */ - this.readers_ = 0; + if (goog.DEBUG) { + /** + * @private + * @type {number} + */ + this.readers_ = 0; + } }; @@ -393,11 +395,15 @@ ol.structs.RBush.prototype.condense_ = function(path) { * @template S */ ol.structs.RBush.prototype.forEach = function(callback, opt_obj) { - ++this.readers_; - try { + if (goog.DEBUG) { + ++this.readers_; + try { + return this.forEach_(this.root_, callback, opt_obj); + } finally { + --this.readers_; + } + } else { return this.forEach_(this.root_, callback, opt_obj); - } finally { - --this.readers_; } }; @@ -443,11 +449,15 @@ ol.structs.RBush.prototype.forEach_ = function(node, callback, opt_obj) { */ ol.structs.RBush.prototype.forEachInExtent = function(extent, callback, opt_obj) { - ++this.readers_; - try { + if (goog.DEBUG) { + ++this.readers_; + try { + return this.forEachInExtent_(extent, callback, opt_obj); + } finally { + --this.readers_; + } + } else { return this.forEachInExtent_(extent, callback, opt_obj); - } finally { - --this.readers_; } }; @@ -526,7 +536,7 @@ ol.structs.RBush.prototype.getKey_ = function(value) { * @param {T} value Value. */ ol.structs.RBush.prototype.insert = function(extent, value) { - if (this.readers_) { + if (goog.DEBUG && this.readers_) { throw Error('cannot insert value while reading'); } var key = this.getKey_(value); @@ -568,7 +578,7 @@ ol.structs.RBush.prototype.insert_ = function(extent, value, level) { * @param {T} value Value. */ ol.structs.RBush.prototype.remove = function(value) { - if (this.readers_) { + if (goog.DEBUG && this.readers_) { throw Error('cannot remove value while reading'); } var key = this.getKey_(value); @@ -665,7 +675,7 @@ ol.structs.RBush.prototype.splitRoot_ = function(node1, node2) { * @param {T} value Value. */ ol.structs.RBush.prototype.update = function(extent, value) { - if (this.readers_) { + if (goog.DEBUG && this.readers_) { throw Error('cannot update value while reading'); } var key = this.getKey_(value); From 828456d18e86130a2ef74e62c79c7bda43a8a521 Mon Sep 17 00:00:00 2001 From: Tom Payne Date: Wed, 27 Nov 2013 14:59:24 +0100 Subject: [PATCH 3/6] Rename ol.structs.RBush#all to getAll --- src/ol/structs/rbush.js | 32 ++++++++++++++++---------------- test/spec/ol/structs/rbush.js | 16 ++++++++-------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/ol/structs/rbush.js b/src/ol/structs/rbush.js index 63bba6bfdd..2ec4a192da 100644 --- a/src/ol/structs/rbush.js +++ b/src/ol/structs/rbush.js @@ -200,22 +200,6 @@ ol.structs.RBush = function(opt_maxEntries) { }; -/** - * @return {Array.} All. - */ -ol.structs.RBush.prototype.all = function() { - var values = []; - this.forEach( - /** - * @param {T} value Value. - */ - function(value) { - values.push(value); - }); - return values; -}; - - /** * @param {ol.structs.RBushNode.} node Node. * @param {function(ol.structs.RBushNode., ol.structs.RBushNode.): number} @@ -520,6 +504,22 @@ ol.structs.RBush.prototype.forEachNode = function(callback, opt_obj) { }; +/** + * @return {Array.} All. + */ +ol.structs.RBush.prototype.getAll = function() { + var values = []; + this.forEach( + /** + * @param {T} value Value. + */ + function(value) { + values.push(value); + }); + return values; +}; + + /** * @param {T} value Value. * @private diff --git a/test/spec/ol/structs/rbush.js b/test/spec/ol/structs/rbush.js index 473126ac09..579d4e1c54 100644 --- a/test/spec/ol/structs/rbush.js +++ b/test/spec/ol/structs/rbush.js @@ -10,10 +10,10 @@ describe('ol.structs.RBush', function() { describe('when empty', function() { - describe('#all', function() { + describe('#getAll', function() { it('returns the expected number of objects', function() { - expect(rBush.all()).to.be.empty(); + expect(rBush.getAll()).to.be.empty(); }); }); @@ -81,9 +81,9 @@ describe('ol.structs.RBush', function() { it('can remove each object', function() { var i, ii; for (i = 0, ii = objs.length; i < ii; ++i) { - expect(rBush.all()).to.contain(objs[i]); + expect(rBush.getAll()).to.contain(objs[i]); rBush.remove(objs[i]); - expect(rBush.all()).not.to.contain(objs[i]); + expect(rBush.getAll()).not.to.contain(objs[i]); } }); @@ -164,7 +164,7 @@ describe('ol.structs.RBush', function() { rBush.remove(objs[i]); expect(rBush.allInExtent(extents[i])).to.be.empty(); } - expect(rBush.all()).to.be.empty(); + expect(rBush.getAll()).to.be.empty(); }); it('can remove objects in random order', function() { @@ -182,7 +182,7 @@ describe('ol.structs.RBush', function() { rBush.remove(objs[index]); expect(rBush.allInExtent(extents[index])).to.be.empty(); } - expect(rBush.all()).to.be.empty(); + expect(rBush.getAll()).to.be.empty(); }); }); @@ -201,10 +201,10 @@ describe('ol.structs.RBush', function() { } }); - describe('#all', function() { + describe('#getAll', function() { it('returns the expected number of objects', function() { - expect(rBush.all().length).to.be(1000); + expect(rBush.getAll().length).to.be(1000); }); }); From f847b372619076698ae15cecf99b6cfa0febe27c Mon Sep 17 00:00:00 2001 From: Tom Payne Date: Wed, 27 Nov 2013 14:59:46 +0100 Subject: [PATCH 4/6] Rename ol.structs.RBush#allInExtent to getAllInExtent --- src/ol/structs/rbush.js | 34 +++++++++++++++++----------------- test/spec/ol/structs/rbush.js | 30 +++++++++++++++--------------- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/ol/structs/rbush.js b/src/ol/structs/rbush.js index 2ec4a192da..8dd8af4960 100644 --- a/src/ol/structs/rbush.js +++ b/src/ol/structs/rbush.js @@ -229,23 +229,6 @@ ol.structs.RBush.prototype.allDistMargin_ = function(node, compare) { }; -/** - * @param {ol.Extent} extent Extent. - * @return {Array.} All in extent. - */ -ol.structs.RBush.prototype.allInExtent = function(extent) { - var values = []; - this.forEachInExtent(extent, - /** - * @param {T} value Value. - */ - function(value) { - values.push(value); - }); - return values; -}; - - /** * FIXME empty description for jsdoc */ @@ -520,6 +503,23 @@ ol.structs.RBush.prototype.getAll = function() { }; +/** + * @param {ol.Extent} extent Extent. + * @return {Array.} All in extent. + */ +ol.structs.RBush.prototype.getAllInExtent = function(extent) { + var values = []; + this.forEachInExtent(extent, + /** + * @param {T} value Value. + */ + function(value) { + values.push(value); + }); + return values; +}; + + /** * @param {T} value Value. * @private diff --git a/test/spec/ol/structs/rbush.js b/test/spec/ol/structs/rbush.js index 579d4e1c54..cdf6b7167d 100644 --- a/test/spec/ol/structs/rbush.js +++ b/test/spec/ol/structs/rbush.js @@ -33,15 +33,15 @@ describe('ol.structs.RBush', function() { rBush.insert([-3, -3, -2, -2], objs[5]); }); - describe('#allInExtent', function() { + describe('#getAllInExtent', function() { it('returns the expected objects', function() { var result; - result = rBush.allInExtent([2, 2, 3, 3]); + result = rBush.getAllInExtent([2, 2, 3, 3]); expect(result).to.contain(objs[1]); expect(result).to.contain(objs[2]); expect(result.length).to.be(2); - result = rBush.allInExtent([-1, -1, 2, 2]); + result = rBush.getAllInExtent([-1, -1, 2, 2]); expect(result).to.contain(objs[0]); expect(result).to.contain(objs[1]); expect(result).to.contain(objs[2]); @@ -50,7 +50,7 @@ describe('ol.structs.RBush', function() { }); it('returns an empty array when given a disjoint extent', function() { - expect(rBush.allInExtent([5, 5, 6, 6]).length).to.be(0); + expect(rBush.getAllInExtent([5, 5, 6, 6]).length).to.be(0); }); }); @@ -144,12 +144,12 @@ describe('ol.structs.RBush', function() { } }); - describe('#allInExtent', function() { + describe('#getAllInExtent', function() { it('returns the expected objects', function() { var i, ii; for (i = 0, ii = objs.length; i < ii; ++i) { - expect(rBush.allInExtent(extents[i])).to.eql([objs[i]]); + expect(rBush.getAllInExtent(extents[i])).to.eql([objs[i]]); } }); @@ -160,9 +160,9 @@ describe('ol.structs.RBush', function() { it('can remove each object in turn', function() { var i, ii; for (i = 0, ii = objs.length; i < ii; ++i) { - expect(rBush.allInExtent(extents[i])).to.eql([objs[i]]); + expect(rBush.getAllInExtent(extents[i])).to.eql([objs[i]]); rBush.remove(objs[i]); - expect(rBush.allInExtent(extents[i])).to.be.empty(); + expect(rBush.getAllInExtent(extents[i])).to.be.empty(); } expect(rBush.getAll()).to.be.empty(); }); @@ -178,9 +178,9 @@ describe('ol.structs.RBush', function() { } for (i = 0, ii = objs.length; i < ii; ++i) { var index = indexes[i]; - expect(rBush.allInExtent(extents[index])).to.eql([objs[index]]); + expect(rBush.getAllInExtent(extents[index])).to.eql([objs[index]]); rBush.remove(objs[index]); - expect(rBush.allInExtent(extents[index])).to.be.empty(); + expect(rBush.getAllInExtent(extents[index])).to.be.empty(); } expect(rBush.getAll()).to.be.empty(); }); @@ -209,10 +209,10 @@ describe('ol.structs.RBush', function() { }); - describe('#allInExtent', function() { + describe('#getAllInExtent', function() { it('returns the expected number of objects', function() { - expect(rBush.allInExtent([0, 0, 10600, 10600]).length).to.be(1000); + expect(rBush.getAllInExtent([0, 0, 10600, 10600]).length).to.be(1000); }); it('can perform 1000 in-extent searches', function() { @@ -223,7 +223,7 @@ describe('ol.structs.RBush', function() { var max = [min[0] + Math.random() * 500, min[1] + Math.random() * 500]; var extent = [min[0], min[1], max[0], max[1]]; - n += rBush.allInExtent(extent).length; + n += rBush.getAllInExtent(extent).length; } expect(n).not.to.be(0); }); @@ -237,7 +237,7 @@ describe('ol.structs.RBush', function() { var max = [min[0] + Math.random() * 500, min[1] + Math.random() * 500]; var extent = [min[0], min[1], max[0], max[1]]; - n += rBush.allInExtent(extent).length; + n += rBush.getAllInExtent(extent).length; } expect(n).to.be(0); }); @@ -255,7 +255,7 @@ describe('ol.structs.RBush', function() { var extent = [min[0], min[1], max[0], max[1]]; rBush.insert(extent, {id: i}); } - expect(rBush.allInExtent([0, 0, 10600, 10600]).length).to.be(2000); + expect(rBush.getAllInExtent([0, 0, 10600, 10600]).length).to.be(2000); }); }); From 5d3a5ae68efce926994b309e710503abe9f28d4f Mon Sep 17 00:00:00 2001 From: Tom Payne Date: Wed, 27 Nov 2013 15:07:36 +0100 Subject: [PATCH 5/6] Fix infinite loop bug in ol.structs.RBush --- src/ol/structs/rbush.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ol/structs/rbush.js b/src/ol/structs/rbush.js index 8dd8af4960..549c3be3da 100644 --- a/src/ol/structs/rbush.js +++ b/src/ol/structs/rbush.js @@ -614,7 +614,8 @@ ol.structs.RBush.prototype.remove_ = function(extent, value) { return; } } - ++index; + node = path.pop(); + index = indexes.pop(); } else if (index < node.children.length) { child = node.children[index]; if (ol.extent.containsExtent(child.extent, extent)) { From c00d748384715af634ca5ec7ec137d5399afab62 Mon Sep 17 00:00:00 2001 From: Tom Payne Date: Wed, 27 Nov 2013 15:11:42 +0100 Subject: [PATCH 6/6] Update ol.interaction.Modify to use ol.structs.RTree#getAllInExtent --- src/ol/interaction/modifyinteraction.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ol/interaction/modifyinteraction.js b/src/ol/interaction/modifyinteraction.js index 6854a49856..e473be2f9c 100644 --- a/src/ol/interaction/modifyinteraction.js +++ b/src/ol/interaction/modifyinteraction.js @@ -454,7 +454,7 @@ ol.interaction.Modify.prototype.handleMouseMove_ = function(evt) { this.modifiable_ = false; var vertexFeature = this.vertexFeature_; var rBush = this.rBush_; - var nodes = rBush.allInExtent(box); + var nodes = rBush.getAllInExtent(box); var renderIntent = ol.layer.VectorLayerRenderIntent.HIDDEN; if (nodes.length > 0) { nodes.sort(sortByDistance);