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

Label/Billboard/Point clustering #4240

Merged
merged 88 commits into from
Sep 29, 2016
Merged

Label/Billboard/Point clustering #4240

merged 88 commits into from
Sep 29, 2016

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Aug 26, 2016

Adds label, billboard, and point clustering. There is a new object on each data source called clustering. This has properties enabled, pixelRange and minimumClusterSize. There is also clusterEvent that is raised for every new cluster. The parameters to the callback are the array of entities being clustered and an entity that will be drawn for the cluster. The label and billboard properties for the second entity parameter are the only things considered when drawing the cluster.

TODO:

  • Fix existing tests
  • Add new tests
  • Update doc
  • Update CHANGES.md
  • Update LICENSE.md for kdbush
  • Submit issues for deprecated function parameters

bagnell added 28 commits August 18, 2016 17:35
… were clustered all of the billboards would be rendered.
…illboard images are impossible to clone without reading from the texture atlas.
…asources share a cluster to be clustered together.
@bagnell
Copy link
Contributor Author

bagnell commented Aug 26, 2016

If you run the Sandcastle and switch to 2D/CV, all of the labels and billboards are in the wrong position. That isn't a problem with the new code. The same problem exists in master with the KML Sandcastle example. I think the problem is in the clamp-to-ground code.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 27, 2016

Note that this is being merged into cluster-master as we don't anticipate this making 1.25 despite thinking otherwise offline.

@colek42
Copy link

colek42 commented Sep 27, 2016

We don't mind the clusters being out of sync. Most of the movement in our time dynamic data is sub pixel at the zoom levels we will be using it at. We would appreciate a recluster() function that we can call to ensure that the clusters are updated once in a while. Clustering is one of the biggest feature requests that we get. Thanks for working on it.

@mramato
Copy link
Contributor

mramato commented Sep 27, 2016

Have we thought about allowing cluster selection to see what's in the cluster? Not saying it needs to be part of this PR, but no default handling for a cluster seems incomplete. We've talked about multiselection in the past and having multiple tabs in the InfoBox to do it. (or we could show the clusters description, which if set, currently does nothing).

</tr>
</tbody>
</table>
<div id="toggleClustering"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make these checkboxes, the toggle button is confusing because you don't know what state it's in.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 27, 2016

@mramato We discussed the issues with how clustering works offline. I submitted changes to fix time-dynamic data like we discussed as well and fixed the issue when removing points. This is ready for another review.

@mramato
Copy link
Contributor

mramato commented Sep 28, 2016

Using the cluster demo:

  1. Switch to 2D (you'll get some weird billboard flashing)
  2. Disable clustering
  3. Everything is broken

image

Similar problems in Columbus View

@mramato
Copy link
Contributor

mramato commented Sep 28, 2016

To experiment with custom styling, I tried making a point where the size depends on the size of the cluster (instead of a billboard). Is there a reason this doesn't work?

removeListener = dataSource.clustering.clusterEvent.addEventListener(function(cluster, entity) {
    entity.label = undefined;
    entity.point = {
        pixelSize : cluster.length
    };
});

…Coordinates

Fix off-by-one in compressing/decompressing texture coordinates
@bagnell
Copy link
Contributor Author

bagnell commented Sep 28, 2016

Using the cluster demo:

  1. Switch to 2D (you'll get some weird billboard flashing)
  2. Disable clustering
  3. Everything is broken

This was mentioned above. The labels are clamped to the ground and its broken. Open the KML example that uses the same KML file and you'll see the same behavior.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 28, 2016

To experiment with custom styling, I tried making a point where the size depends on the size of the cluster (instead of a billboard). Is there a reason this doesn't work?

The code that adds the cluster is here: https://github.com/AnalyticalGraphicsInc/cesium/pull/4240/files#diff-3ca360d29d7d55bfdc7322d4e18721e7R157

Its pretty limited because I wanted to keep it close to the cluster position and limit the size so it doesn't overlap other clusters. I'm open to any suggestions here.

@mramato
Copy link
Contributor

mramato commented Sep 28, 2016

This was mentioned above. The labels are clamped to the ground and its broken. Open the KML example that uses the same KML file and you'll see the same behavior.

That breakage is worst than I thought then. We should fix this ASAP. I'll mark those issues as next release.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 28, 2016

@mramato I updated the custom styling like we discussed offline. This is ready for another look.

@mramato
Copy link
Contributor

mramato commented Sep 29, 2016

The doc needs to be updated, but then I think this is good to go.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 29, 2016

@mramato The doc is updated. This is ready.

@mramato
Copy link
Contributor

mramato commented Sep 29, 2016

Thanks @bagnell!

@mramato mramato merged commit 0b0ed14 into cluster-master Sep 29, 2016
@mramato mramato deleted the cluster branch September 29, 2016 18:15
@pjcozzi pjcozzi mentioned this pull request Nov 3, 2016
4 tasks
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.

7 participants