-
Notifications
You must be signed in to change notification settings - Fork 3.7k
EntityCluster BUG Resolve the issue of incomplete aggregation. #13064
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
Conversation
|
Thank you for the pull request, @Duan971231! ✅ We can confirm we have a CLA on file for you. |
|
@lukemckinstry (You did the initial triage on the issue) : This PR fixes the issue, which also affects the currently deployed 'Clustering' sandcastle. Here is a comparison of the current
The state of this PR is the right one, literally. |
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.
Thanks for this PR and apologies for the slow follow up.
I see the improvement clearly in the example linked in #13062.
I actually do not see a difference between the current prod clustering sandcastle and this branch (I am unable to reproduce the left and right states above), but I also do not see any regressions and clustering appears to be working as expected.
A somewhat reliable way for reproducing the issue in https://sandcastle.cesium.com/index.html?id=clustering is to zoom in a bit (to the USA), move the westcoast just out of view, and then drag right, moving the westcoast back into the view. The newly appearing/visible cities/points will not be clustered (only when dragging even further). |
|
Thanks @javagl that makes it very clear so I see the improvement now. |
|
@lukemckinstry Hello, which changelog entry should I change? the CHANGELOG.md in the code hasn't been updated for quite some time. |
|
The file to be modified is https://github.com/CesiumGS/cesium/blob/main/CHANGES.md - it's just about adding a line similar to the one that was added at 6a6627f , saying something like
(adjust the wording as you see fit) |
|
@javagl Okay, the code has been submitted |

fixes: #13062
Based on this issue, I have reviewed the source code of ElementCluster.js and found that using kdBush after upgrading from v3.0.0 to v4.0.2, continue use Uint32Array will result in negative numbers being represented as 2 ^ 32. When using index. range in the code, there will be no expected results.
If not filled in here, the default will still be Float64Array
Expected adoption