Skip to content
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

Feature: get zoom level where a point gets unclustered #205

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

B0rk3
Copy link

@B0rk3 B0rk3 commented Sep 1, 2022

Hello everybody,

this PR is related to the issue #193.
It adds a new function that returns the lowest zoom level, where a point isn't clustered.

@karldanninger
Copy link

Hey @B0rk3 thank you for doing this!

I'm getting an error:

TypeError: Cannot read properties of undefined (reading 'points')
    at Supercluster.getPointUnclusterZoom (index.js:179:1)

Line 179 is: const unclustered = tree.points.some(

Any ideas?

index.js Outdated

const unclustered = tree.points.some(
treePoint => !treePoint.id && treePoint.x === pointX && treePoint.y === pointY
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This essentially loops through all clusters across all zooms until it reaches the unclustered point, in many instances iterating over nearly the whole dataset. I think we could implement this much more efficiently, utilizing the R-tree indices — e.g. only look at clusters within radius of the point.

Also, what if the point does have id? This filter doesn't look right...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review and advises!

I improved the code and used the within function of KDBush to filter all points that don't have the same coordinate as the provided point, by setting radius to 0.

I mistakenly assumed that only clusters have an id, so I wanted to filter them first. The value of the parentId should be better suited for this.

Additionally the expansionZoom now starts at the minZoom level, instead of 0, to avoid getting undefined trees entries.

If you have some other advises for improvements, please let me know.

@karldanninger
Copy link

Hey all, I've been using this fork for a couple days now, and it works as expected, testing with up to 3,000 locations!

My only feedback is when a marker that is already unclustered returns a value of 21. Not sure what else it should do, but I've put a guard in my code to not zoom into a marker if the result of this function returns 21, assuming it's unclustered.

I hope this feedback helps.

@B0rk3
Copy link
Author

B0rk3 commented Sep 16, 2022

Thank you for your feedback. Good to hear that you are using this extension.

I can't reproduce the behavior you described.
If I want to zoom to an already unclustered point, the function returns the min zoom level at which it will be unclustered. So it's actually zooming out of the map and not in.
I've no idea why the function is behaving differently, yet.

But I've found a other little bug. The max zoom level, which is set in the options, is exceeded by 1, if the specified point was not found in a tree. (your max zoom level is probably set to 20?)

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for getting back late — overall this is a very useful addition! Two remaining things:

  • The PR needs some unit tests to make sure it works as expected and doesn't regress in the future.
  • I'm not entirely sure how it works with the parentId !== -1 comparison — can you explain? Judging by the code, as soon as a point gets into a cluster on a zoom above, it gets a parentId assigned, and the only points that will have -1 at the end are those that were never clustered through the whole zoom range. But maybe I'm missing something.

@mourner
Copy link
Member

mourner commented Apr 26, 2023

Also, the PR will have to be updated slightly after #223 got merged, changing how Supercluster stores some data internally.

@B0rk3
Copy link
Author

B0rk3 commented Apr 26, 2023

Hello,

I have adapted the function according to the new changes and added a test function.

Regarding your question about comparing the parentId not equal to -1:
As you said I assume that for points that are not included in a cluster the parentId is set to -1.
The parentId for each point is initialized to -1 (line 64 in index.js).
When a cluster gets created its parentId is then set to a different id (see line 348 and 360).

I hope this helps you a little bit and I don't miss anything either.

const pointIdxs = tree.within(pointX, pointY, 0);

const unclustered = pointIdxs.some(
idx => tree.data[idx].parentId !== -1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong — tree.data layout is different now as I mentioned, it's a flat array of numbers. It should look something like this:

Suggested change
idx => tree.data[idx].parentId !== -1
idx => tree.data[this.stride * idx + OFFSET_PARENT] !== -1

But even that, this doesn't look right, because parentId will not be -1 for ALL points that eventually end up in a cluster regardless of zoom level, so this logic doesn't work here. Instead I'd suggest checking whether number of points equals 1 (tree.data[this.stride * idx + OFFSET_NUM] === 1).

const pointY = fround(latY(lat));

let expansionZoom = this.options.minZoom;
while (expansionZoom < this.options.maxZoom) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since points are more likely to get clustered on higher zooms, it would likely be faster to iterate starting from maxZoom and up until there are no points in the location or are all clusters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants