-
Notifications
You must be signed in to change notification settings - Fork 299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ignore features with null geometry #48
Conversation
This should pass once #49 is merged and this is rebased against that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid having to allocate the array twice — when you call filter
, it copies the original array even if there's nothing to filter.
We should probably replace both map
and filter
with a simple loop and skip null
geometries in place.
94711cd
to
6fc9f98
Compare
@mourner updated, is that what you meant? |
index.js
Outdated
@@ -265,6 +271,10 @@ function createCluster(x, y, id, numPoints, properties) { | |||
} | |||
|
|||
function createPointCluster(p, id) { | |||
if (!p.geometry) { | |||
return null; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move this part inside the loop above.
index.js
Outdated
@@ -43,7 +43,13 @@ SuperCluster.prototype = { | |||
this.points = points; | |||
|
|||
// generate a cluster object for each point and index input points into a KD-tree | |||
var clusters = points.map(createPointCluster); | |||
var clusters = []; | |||
points.forEach(function (point, i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using forEach
in performance-critical code. for (var i = 0; i < points.length; i++)
is much faster.
@mourner how's this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
It is valid for GeoJSON features to have a null geometry, this PR aims to ignores such features rather than crashing.