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

Labels don't show for individual entities in datasources when clustering is enabled #6087

Closed
thw0rted opened this issue Jan 4, 2018 · 19 comments

Comments

@thw0rted
Copy link
Contributor

thw0rted commented Jan 4, 2018

Issue

Entities won't show a label if their owning datasource has clustering enabled

Reproduction

Open this Sandcastle example, slightly modified from the basic Clustering demo, then toggle the "Enabled" checkbox. Be sure to zoom in enough that some entities are not clustered. Observe that individual (non-clustered) entities show a label when clustering is disabled, but the label disappears when clustering = true.


I'm not sure if this is a recent regression or not, as I've updated Cesium a number of times in the last 2 months and just started noticing the problem recently. In the example, the entities are created from a KML source, but in my application I observe the same problem with entities I create manually and control with a CustomDataSource.

@ggetz
Copy link
Contributor

ggetz commented Jan 4, 2018

Thanks @thw0rted for the report! I bevel this may be a regression, I see the labels were applied correctly during the initial PR. If you have the bandwidth to look at this, we'd greatly appreciate if you could contribute a fix or track down the cause.

@thw0rted
Copy link
Contributor Author

thw0rted commented Jan 5, 2018

Have you guys considered hosting old versions of Sandcastle on the public site? It would make it a lot easier to track down version-specific errors like this and as a side benefit you could make the "share" button hard-coded to a single library version so that user code doesn't break in future updates. (It's pretty likely that some issue here has an old link to Sandcastle that uses a deprecated features no longer supported in the latest...)

@ggetz
Copy link
Contributor

ggetz commented Jan 5, 2018

We already do: http://cesiumjs.org/releases/1.39/index.html

I believe the share button will export a URL that uses the same build as well.

@thw0rted
Copy link
Contributor Author

thw0rted commented Jan 5, 2018

Holy cow, this has been around for a while. I didn't think to check, but the points did in fact have labels in the Sandcastle example as it was originally implemented, so you can just load the "stock" Clustering demo and toggle Enabled to test. I tried as far back as 1.26, which worked as shown in the PR screenshot you linked. Then I tried 1.27, and it's already broken (!). So, I guess it "never" worked except for the one original release.

I guess clustering must not be too popular since I seem to be the first to notice this? That, or nobody uses labels and clustering for the same data...

@ggetz
Copy link
Contributor

ggetz commented Jan 5, 2018

Thanks for tracking this release down @thw0rted! As far as I've seen, clustering is used mostly just billboards which is why it may have gone unnoticed.

@bagnell Do you know what may have happened between these releases?

@emackey
Copy link
Contributor

emackey commented Jan 8, 2018

#4919 (comment)

The Label Clustering demo has been broken for a very long time. If you zoom in to where the individual entities de-cluster, you will never see any labels while clustering is enabled. This is a serious bug that has flown under the radar for too long. I suspect it is caused by bad logic in EntityCluster, in particular the calculation of labelIndex is sometimes wrong. This needs more investigation.

@thw0rted
Copy link
Contributor Author

thw0rted commented Jan 9, 2018

Thanks @emackey , I saw that issue but didn't notice your comment about missing un-clustered labels further down the thread. Up to you if you want to track both problems under the same issue # or keep this one open.

@emackey
Copy link
Contributor

emackey commented Jan 9, 2018

Let's keep this one open, it's much clearer that this is one specific issue. I'll close the old one.

@hpinkos
Copy link
Contributor

hpinkos commented Aug 8, 2018

Also reported by @ollazarev in #6897 (see #6897 for a great minimal code example)

EntityCluster might need a sizable overhaul to get this to work. The main problem is that it's working too much at the primitive level and incorrectly assumes that each entity has either a label, a billboard, or a point (not a label and a point).

@OmarShehata
Copy link
Contributor

I think this is the same issue described in this forum thread:

https://groups.google.com/forum/#!topic/cesium-dev/urW44wpGJZk

@thw0rted
Copy link
Contributor Author

thw0rted commented Jan 7, 2019

First, @OmarShehata , is this maybe related to #7446? Second, I haven't messed with clustering in a while but now find myself needing it, and need the labels to work properly -- is there any chance that this will get addressed in an upcoming release? If it isn't fixed soon, I will have to look at "burning in" the labels as part of the billboard image, which is pretty limiting.

@OmarShehata
Copy link
Contributor

@thw0rted it seems like that's the issue. When applying the suggested fix from that issue I can see the labels in your original example. Here's my diff:

a809398

I don't know that we have someone actively working on this, but I think if there's a community PR open I'm sure it'll make it in for release.

@thw0rted
Copy link
Contributor Author

In case it helps I can confirm that the linked patch resolves the issue. I've been begging my management for approval to sign a CLA for well over a year now, but I'll go ahead and ask again in case they forgot :-/

@emstrabala
Copy link

Is this issue going to make a build anytime soon?

@hpinkos
Copy link
Contributor

hpinkos commented Apr 11, 2019

@emstrabala sorry, this hasn't been a priority for us. We would be more than happy to review a pull request though if anyone has time to look into the issue

@bbehling-trimble
Copy link

Are we waiting on a merge conflict?

We need to have this feature working too.

@OmarShehata
Copy link
Contributor

@bbehling-trimble I think the PR #7744 just hasn't had a reviewer yet. Can you test that branch and confirm that it fixes the issue? That'll help a lot in getting it out by the next release!

@hpinkos
Copy link
Contributor

hpinkos commented Sep 19, 2019

Fixed in #7744

@hpinkos hpinkos closed this as completed Sep 19, 2019
@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/urW44wpGJZk

If this issue affects any of these threads, please post a comment like the following:

The issue at #6087 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

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

No branches or pull requests

8 participants