Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Replace ol.Coordinate with Array.<number> #465

Merged
merged 5 commits into from

3 participants

@twpayne

This issue is a question:

Should we replace ol.Coordinate with Array.<number>?

This is quite a major change, but has many advantages:

  • Quicker to type ([0, 0] instead of new ol.Coordinate(0, 0))
  • Easily extensible to 3 or more dimensions
  • Arrays are generally more efficiently implemented by JS runtimes than Objects
  • Increased opportunity for object re-use, leading to reduced GC load
  • Easier interoperability with the packed arrays of vertices used by WebGL (see the webgl-vector branch)
@twpayne

If the build passes, then this is ready for review.

Combined with #463, this makes the API much lighter for simple cases.

@fredj fredj commented on the diff
src/ol/view2d.js
@@ -171,7 +171,7 @@ ol.View2D.prototype.getView2DState = function() {
var resolution = /** @type {number} */ (this.getResolution());
var rotation = /** @type {number} */ (this.getRotation());
return {
- center: new ol.Coordinate(center.x, center.y),
+ center: center.slice(),
@fredj Owner
fredj added a note

center.slice(0, 2) instead?

@fredj Owner
fredj added a note

forget it. (note to self: don't review code before the third cup of coffee)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fredj
Owner

is it worth doing the same for ol.Pixel?

@twpayne

is it worth doing the same for ol.Pixel?

Good question. I'm not sure. I like the fact that ol.Coordinate, ol.Pixel and ol.TileCoord are all distinct types: this can help us catch errors. If we make ol.Pixel and Array.<number> then it becomes the same type as ol.Coordinate. Currently, we don't use ol.Pixel very much, so I have a slight preference to keep ol.Pixel as a distinct type, but I can be persuaded otherwise.

@fredj
Owner
@twpayne

Thanks @fredj!

Does anyone else have feedback on this PR? Personally, I think this is a useful improvement and that it should be merged. Since it's a wide-ranging change, I'd like to see it go in as soon as possible.

@elemoine
Owner

Looks good to me. Please merge. It's a large change, so I think it should not stay pending too long. Thanks!

@twpayne

OK, thanks for the reviews @fredj and @elemoine.

@twpayne twpayne merged commit 3b416c4 into openlayers:master
@twpayne twpayne deleted the twpayne:coordinate-as-array branch
@twpayne

@fredj, I think it's worth turning your pixel-as-array branch into a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.