From a968a5ca663cc248d9e6827706591cf7395af32d Mon Sep 17 00:00:00 2001 From: Tim Schaub Date: Tue, 5 Mar 2013 11:40:08 +0100 Subject: [PATCH] The shared vertices structure now works with integer arrays only Previously, a lookup object containing start indexes was also used. This allowed us to remove arrays of vertices and properly update the start indexes for remaining coordinate values. In order to provide callers with stable identifiers and to work with arrays of integers alone, we cannot support a remove method. I think this is worth revisiting. Even if the array length cannot be changed in WebGL, we don't need to also impose the restriction outside. Instead, the WebGL renderer could be notified when array sizes change and update itself accordingly. I think there is more value in providing geometries with stable identifiers. This common structure is used to store vertices for all geometry types. A slight optimization could be made by creating yet another structure to store point vertices - since these don't need to have a counts array (always 1). Creating a new structure would mean saving memory at the expense of more lib code to transport. The coordinate value lookups are not negatively impacted by using the same structure for points and higher order geometries. Since the first change above goes against my instincts, I'm not making this second change (to add another structure for shared point vertices). --- src/ol/geom/linestring.js | 4 +- src/ol/geom/point.js | 4 +- src/ol/geom/sharedvertices.js | 108 ++++++++------------- test/spec/ol/filter/geometryfilter.test.js | 2 +- test/spec/ol/geom/sharedvertices.test.js | 97 +++++++++--------- 5 files changed, 95 insertions(+), 120 deletions(-) diff --git a/src/ol/geom/linestring.js b/src/ol/geom/linestring.js index 822af57277..1705954163 100644 --- a/src/ol/geom/linestring.js +++ b/src/ol/geom/linestring.js @@ -34,7 +34,7 @@ ol.geom.LineString = function(coordinates, opt_shared) { this.vertices = vertices; /** - * @type {string} + * @type {number} * @private */ this.sharedId_ = vertices.add(coordinates); @@ -142,7 +142,7 @@ ol.geom.LineString.prototype.getType = function() { /** * Get the identifier used to mark this line in the shared vertices structure. - * @return {string} The identifier. + * @return {number} The identifier. */ ol.geom.LineString.prototype.getSharedId = function() { return this.sharedId_; diff --git a/src/ol/geom/point.js b/src/ol/geom/point.js index 621a6e4f13..f593db0cb7 100644 --- a/src/ol/geom/point.js +++ b/src/ol/geom/point.js @@ -32,7 +32,7 @@ ol.geom.Point = function(coordinates, opt_shared) { this.vertices = vertices; /** - * @type {string} + * @type {number} * @private */ this.sharedId_ = vertices.add([coordinates]); @@ -98,7 +98,7 @@ ol.geom.Point.prototype.getType = function() { /** * Get the identifier used to mark this point in the shared vertices structure. - * @return {string} The identifier. + * @return {number} The identifier. */ ol.geom.Point.prototype.getSharedId = function() { return this.sharedId_; diff --git a/src/ol/geom/sharedvertices.js b/src/ol/geom/sharedvertices.js index 39ff922027..3941b8f0e7 100644 --- a/src/ol/geom/sharedvertices.js +++ b/src/ol/geom/sharedvertices.js @@ -20,19 +20,25 @@ ol.geom.SharedVerticesOptions; * @param {ol.geom.SharedVerticesOptions=} opt_options Shared vertices options. */ ol.geom.SharedVertices = function(opt_options) { - var options = goog.isDef(opt_options) ? opt_options : {}; - - /** - * @type {number} - * @private - */ - this.counter_ = 0; + var options = opt_options ? opt_options : {}; /** * @type {Array.} */ this.coordinates = []; + /** + * @type {Array.} + * @private + */ + this.starts_ = []; + + /** + * @type {Array.} + * @private + */ + this.counts_ = []; + /** * Number of dimensions per vertex. Default is 2. * @type {number} @@ -49,25 +55,13 @@ ol.geom.SharedVertices = function(opt_options) { goog.asserts.assert(goog.isNull(this.offset_) || this.offset_.length === this.dimension_); - /** - * @type {Object} - * @private - */ - this.lookup_ = {}; - - /** - * @type {Array.} - * @private - */ - this.ids_ = []; - }; /** * Adds a vertex array to the shared coordinate array. * @param {ol.geom.VertexArray} vertices Array of vertices. - * @return {string} Index used to reference the added vertex array. + * @return {number} Index used to reference the added vertex array. */ ol.geom.SharedVertices.prototype.add = function(vertices) { var start = this.coordinates.length; @@ -87,28 +81,23 @@ ol.geom.SharedVertices.prototype.add = function(vertices) { } } } - var id = this.getId_(); - var idIndex = this.ids_.push(id) - 1; - this.lookup_[id] = { - idIndex: idIndex, - start: start, - count: count - }; - return id; + var length = this.starts_.push(start); + this.counts_.push(count); + return length - 1; }; /** - * @param {string} id The vertex array identifier (returned by add). + * @param {number} id The vertex array identifier (returned by add). * @param {number} index The vertex index. * @param {number} dim The coordinate dimension. * @return {number} The coordinate value. */ ol.geom.SharedVertices.prototype.get = function(id, index, dim) { + goog.asserts.assert(id < this.starts_.length); goog.asserts.assert(dim <= this.dimension_); - goog.asserts.assert(this.lookup_.hasOwnProperty(id)); - goog.asserts.assert(index < this.lookup_[id].count); - var start = this.lookup_[id].start; + goog.asserts.assert(index < this.counts_[id]); + var start = this.starts_[id]; var value = this.coordinates[start + (index * this.dimension_) + dim]; if (this.offset_) { value += this.offset_[dim]; @@ -118,22 +107,23 @@ ol.geom.SharedVertices.prototype.get = function(id, index, dim) { /** - * @param {string} id The vertex array identifier (returned by add). + * @param {number} id The vertex array identifier (returned by add). * @return {number} The number of vertices in the referenced array. */ ol.geom.SharedVertices.prototype.getCount = function(id) { - goog.asserts.assert(this.lookup_.hasOwnProperty(id)); - return this.lookup_[id].count; + goog.asserts.assert(id < this.counts_.length); + return this.counts_[id]; }; /** - * Gets an identifier that is unique for this instance. - * @return {string} Identifier. - * @private + * Get the array of counts. The index returned by the add method can be used + * to look up the number of vertices. + * + * @return {Array.} The counts array. */ -ol.geom.SharedVertices.prototype.getId_ = function() { - return String(++this.counter_); +ol.geom.SharedVertices.prototype.getCounts = function() { + return this.counts_; }; @@ -154,42 +144,20 @@ ol.geom.SharedVertices.prototype.getOffset = function() { /** - * @param {string} id The vertex array identifier (returned by add). + * @param {number} id The vertex array identifier (returned by add). * @return {number} The start index in the shared vertices array. */ ol.geom.SharedVertices.prototype.getStart = function(id) { - goog.asserts.assert(this.lookup_.hasOwnProperty(id)); - return this.lookup_[id].start; + goog.asserts.assert(id < this.starts_.length); + return this.starts_[id]; }; /** - * @param {number} id The vertex array identifier (returned by add). - * @return {ol.geom.VertexArray} The removed vertex array. + * Get the array of start indexes. + * @return {Array.} The starts array. */ -ol.geom.SharedVertices.prototype.remove = function(id) { - goog.asserts.assert(this.lookup_.hasOwnProperty(id)); - var info = this.lookup_[id]; - var dimension = this.dimension_; - var length = info.count * dimension; - var removed = this.coordinates.splice(info.start, length); - var offset = this.offset_; - var array = new Array(info.count); - var vertex; - for (var i = 0; i < info.count; ++i) { - vertex = new Array(dimension); - for (var j = 0; j < dimension; ++j) { - vertex[j] = removed[(i * dimension) + j] + (offset ? offset[j] : 0); - } - array[i] = vertex; - } - delete this.lookup_[id]; - this.ids_.splice(info.idIndex, 1); - var afterInfo; - for (var k = info.idIndex, kk = this.ids_.length; k < kk; ++k) { - afterInfo = this.lookup_[this.ids_[k]]; - afterInfo.idIndex -= 1; - afterInfo.start -= length; - } - return array; +ol.geom.SharedVertices.prototype.getStarts = function() { + return this.starts_; }; + diff --git a/test/spec/ol/filter/geometryfilter.test.js b/test/spec/ol/filter/geometryfilter.test.js index b9c41ef7ae..dc7bb6dc03 100644 --- a/test/spec/ol/filter/geometryfilter.test.js +++ b/test/spec/ol/filter/geometryfilter.test.js @@ -34,7 +34,7 @@ describe('ol.filter.Geometry', function() { it('works for multi-linestring', function() { var filter = new ol.filter.Geometry( - ol.filter.GeometryType.MULTILINESTRING); + ol.filter.GeometryType.MULTILINESTRING); expect(filter.getType()).toBe(ol.filter.GeometryType.MULTILINESTRING); }); diff --git a/test/spec/ol/geom/sharedvertices.test.js b/test/spec/ol/geom/sharedvertices.test.js index c007c39205..06b15bfea5 100644 --- a/test/spec/ol/geom/sharedvertices.test.js +++ b/test/spec/ol/geom/sharedvertices.test.js @@ -43,8 +43,24 @@ describe('ol.geom.SharedVertices', function() { it('returns an identifier for coordinate access', function() { var vertices = new ol.geom.SharedVertices(); var id = vertices.add([[1, 2], [3, 4]]); - expect(typeof id).toBe('string'); + expect(typeof id).toBe('number'); }); + + it('returns the index of the added vertices', function() { + var vertices = new ol.geom.SharedVertices(); + + var first = vertices.add([[1, 2]]); + var second = vertices.add([[3, 4], [5, 6]]); + var third = vertices.add([[7, 8], [9, 10], [11, 12]]); + + expect(vertices.coordinates).toEqual( + [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]); + + expect(first).toBe(0); + expect(second).toBe(1); + expect(third).toBe(2); + }); + }); describe('#get()', function() { @@ -98,6 +114,17 @@ describe('ol.geom.SharedVertices', function() { }); }); + describe('#getCounts()', function() { + it('returns the counts array', function() { + var vertices = new ol.geom.SharedVertices(); + var first = vertices.add([[2, 3], [3, 4], [4, 5]]); + var second = vertices.add([[5, 6], [6, 6]]); + var third = vertices.add([[7, 8]]); + + expect(vertices.getCounts()).toEqual([3, 2, 1]); + }); + }); + describe('#getDimension()', function() { it('returns 2 by default', function() { var vertices = new ol.geom.SharedVertices(); @@ -123,57 +150,43 @@ describe('ol.geom.SharedVertices', function() { }); describe('#getStart()', function() { - it('returns the start of the identified vertex array', function() { + it('returns the start index of an identified vertex array', function() { var vertices = new ol.geom.SharedVertices(); - var first = vertices.add([[1, 2]]); - var second = vertices.add([[3, 4], [5, 6]]); - var third = vertices.add([[7, 8], [9, 10], [11, 12]]); + var first = vertices.add([[2, 3], [4, 5], [6, 7]]); + var second = vertices.add([[8, 9], [10, 11]]); + var third = vertices.add([[12, 13]]); expect(vertices.coordinates).toEqual( - [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]); + [2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]); + // 0 1 2 3 4 5 6 7 8 9 10 11 + expect(vertices.getStart(first)).toBe(0); - expect(vertices.getStart(second)).toBe(2); - expect(vertices.getStart(third)).toBe(6); + expect(vertices.getStart(second)).toBe(6); + expect(vertices.getStart(third)).toBe(10); }); }); - describe('#remove()', function() { - it('removes a vertex array', function() { + describe('#getStarts()', function() { + it('returns the counts array', function() { + var vertices = new ol.geom.SharedVertices(); + var first = vertices.add([[2, 3], [3, 4], [4, 5]]); + var second = vertices.add([[5, 6], [6, 6]]); + var third = vertices.add([[7, 8]]); + + expect(vertices.getStarts()).toEqual([0, 6, 10]); + }); + }); + + describe('#coordinates', function() { + it('is a flat array of all coordinate values', function() { var vertices = new ol.geom.SharedVertices(); var first = vertices.add([[1, 2], [3, 4]]); var second = vertices.add([[5, 6]]); var third = vertices.add([[7, 8], [9, 10], [11, 12]]); - - expect(vertices.remove(second)).toEqual([[5, 6]]); - expect(vertices.coordinates).toEqual([1, 2, 3, 4, 7, 8, 9, 10, 11, 12]); - - expect(vertices.remove(first)).toEqual([[1, 2], [3, 4]]); - expect(vertices.coordinates).toEqual([7, 8, 9, 10, 11, 12]); - - expect(vertices.remove(third)).toEqual([[7, 8], [9, 10], [11, 12]]); - expect(vertices.coordinates).toEqual([]); + expect(vertices.coordinates).toEqual( + [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]); }); - it('adjusts returned vertices by offset', function() { - - var vertices = new ol.geom.SharedVertices({offset: [10, 20]}); - var first = vertices.add([[1, 2]]); - var second = vertices.add([[3, 4]]); - var third = vertices.add([[5, 6]]); - - expect(vertices.remove(second)).toEqual([[3, 4]]); - expect(vertices.coordinates).toEqual([-9, -18, -5, -14]); - - expect(vertices.remove(third)).toEqual([[5, 6]]); - expect(vertices.coordinates).toEqual([-9, -18]); - - expect(vertices.remove(first)).toEqual([[1, 2]]); - expect(vertices.coordinates).toEqual([]); - }); - - }); - - describe('#coordinates', function() { it('is not reassigned', function() { var vertices = new ol.geom.SharedVertices(); var first = vertices.add([[1, 2], [3, 4]]); @@ -181,12 +194,6 @@ describe('ol.geom.SharedVertices', function() { var second = vertices.add([[5, 6]]); expect(vertices.coordinates).toBe(coordinates); - - vertices.remove(first); - expect(vertices.coordinates).toBe(coordinates); - - vertices.remove(second); - expect(vertices.coordinates).toBe(coordinates); }); });