Add declusteredEvent to EntityCluster #12920

Oko-Tester · 2025-09-25T07:31:30Z

Fixes #5760 by adding declusteredEvent that provides both clustered and declustered entities, plus new API methods for accessing clustering state. Maintains backward compatibility.

Description

This PR adds comprehensive clustering state information to EntityCluster by introducing a new declusteredEvent and supporting API methods. The original clusterEvent only provided information about entities that were successfully clustered, but developers also needed access to entities that were processed but not clustered (declustered entities).

Key Changes

New declusteredEvent: Fires with complete clustering information including both clustered and declustered entities
New API methods:
- getLastClusteredEntities() - Returns currently clustered entities
- getLastDeclusteredEntities() - Returns currently declustered entities
- getAllProcessedEntities() - Returns all entities processed during clustering
Enhanced event coverage: Events now fire when clustering is disabled or clusters become empty
Backward compatibility: Original clusterEvent remains unchanged and functional

Issue number and link

Fixes #5760 - Include declustered entities in cluster callback

This addresses multiple user requests from the forum and GitHub comments spanning several years.

Testing plan

Manual Testing

Create CustomDataSource with clustering enabled
Add entities at various distances to create both clustered and declustered entities
Verify declusteredEvent fires with correct clustered and declustered arrays
Disable clustering and verify event fires with all entities as declustered
Test API methods return correct entity lists
Verify original clusterEvent still works as before

Author checklist

I have submitted a Contributor License Agreement
I have added my name to CONTRIBUTORS.md
I have updated CHANGES.md with a short summary of my change
I have added or updated unit tests to ensure consistent code coverage
I have updated the inline documentation, and included code examples where relevant
I have performed a self-review of my code

github-actions · 2025-09-25T07:31:58Z

Thank you for the pull request, @Oko-Tester!

✅ We can confirm we have a CLA on file for you.

packages/engine/Source/DataSources/EntityCluster.js

Oko-Tester · 2025-09-26T12:09:36Z

this is ready

packages/engine/Source/DataSources/EntityCluster.js

Fixes CesiumGS#5760 by adding declusteredEvent that provides both clustered and declustered entities, plus new API methods for accessing clustering state. Maintains backward compatibility.

… skipping unused declustering computations

Oko-Tester · 2025-10-04T12:54:16Z

@mzschwartz5 Is there any news regarding the approval?

mzschwartz5 · 2025-10-06T16:25:04Z

Hi @Oko-Tester, I haven't had a chance to review this yet. It might not happen until Thursday as I'm out tomorrow and Wednesday.

mzschwartz5

Hey @Oko-Tester,

Thanks for the PR. I have a number of mostly minor comments about code style and performance, but those might be irrelevant because I'm a little unsure of the overall approach. It's a lot of extra state that we're tracking now, which means more memory consumed, more bookkeeping to sync arrays, more room for potential edge cases.

In the linked issue you said "clusterEvent does not fire when a cluster becomes empty, which makes it impossible to detect de-clustered entities." My intuition is that there's a better way to deal with this than to track more state and manually fire the decluster event accordingly. I haven't actually looked at the code closely to tell, but that's my instinct.

mzschwartz5 · 2025-10-17T01:10:49Z

packages/engine/Source/DataSources/EntityCluster.js

+ * Returns all entities that were processed in the most recent clustering operation.
+ * @returns {Entity[]} Array of all processed entities
+ */
+EntityCluster.prototype.getAllProcessedEntities = function () {


This is an opinion, but I feel like the name of this API could use some work shopping - before I even suggest an alternative, though, let me ask: does "all" entities mean all entities that are in clusters after the most recent operation, or also entities that are declustered?

(And similarly rename the internal array)

mzschwartz5 · 2025-10-17T01:12:32Z

packages/engine/Source/DataSources/EntityCluster.js

+ * @returns {Entity[]} Array of entities that were clustered
+ */
+EntityCluster.prototype.getLastClusteredEntities = function () {
+  return this._lastClusteredEntities.slice();


Is there any reason to return a shallow copy of the internal array (via slice())? Why not just make a property / standard getter for these arrays?

With zero context, I'll throw in the baseline comment: Minimize mutablilty and make ownership clear. It at least serves the purpose of avoiding the nearly-impossible-to-debug glitches that may result from a client doing this:

const entities = entityCluster.getLastClusteredEntities(); // (French accent:) A few moments later... entities[42] = undefined; // Even later entityCluster.updateSomething(); // Crashes due to `undefined` in array...

It's a thin protection layer, but better than nothing. In other environments, this would/could/should return a read-only view on the internal state (Collections.unmodifiableList et al).

Hmm... I see. I would still rather we protect against undefined in updateSomething.

In a typed language, you would just have a compile-time contract here, marking this function as returning something const or read-only. But even there, a user can generally find a way to shoot themselves in the foot at run-time, if they really want to.

So, here too, a user could do something crazy with the array, but we can't guard against everything. (But we should still guard against the basic things)

mzschwartz5 · 2025-10-17T01:15:14Z

packages/engine/Source/DataSources/EntityCluster.js

+    return;
+  }
+  const allVisibleEntities = [
+    ...getVisibleEntitiesFromCollection(entityCluster._labelCollection),


I know there was previous discussion about the use of the spread (...) operator on large collections. Were these instances overlooked or intentionally left as-is because we know they're not large?

mzschwartz5 · 2025-10-17T01:19:54Z

packages/engine/Source/DataSources/EntityCluster.js

+  ];
+
+  if (allVisibleEntities.length > 0) {
+    const uniqueEntities = Array.from(new Set(allVisibleEntities));


Why is it possible for allVisibleEntities to contain duplicates?

mzschwartz5 · 2025-10-17T01:23:43Z

packages/engine/Source/DataSources/EntityCluster.js

+  if (allVisibleEntities.length > 0) {
+    const uniqueEntities = Array.from(new Set(allVisibleEntities));
+
+    entityCluster._declusteredEvent.raiseEvent({
+      clustered: [],
+      declustered: uniqueEntities,
+      cluster: null,
+      allProcessed: uniqueEntities,
+    });
+
+    entityCluster._lastClusteredEntities = [];
+    entityCluster._lastDeclusteredEntities = uniqueEntities.slice();
+    entityCluster._allProcessedEntities = uniqueEntities.slice();
+  } else {
+    entityCluster._declusteredEvent.raiseEvent({
+      clustered: [],
+      declustered: [],
+      cluster: null,
+      allProcessed: [],
+    });
+
+    entityCluster._lastClusteredEntities = [];
+    entityCluster._lastDeclusteredEntities = [];
+    entityCluster._allProcessedEntities = [];
+  }
+}
+


I don't believe this need any conditional logic. Just set
const uniqueEntities = Array.from(new Set(allVisibleEntities));

and the rest handles itself whether allVisibleEntities is empty or not.

mzschwartz5 · 2025-10-17T01:25:18Z

packages/engine/Source/DataSources/EntityCluster.js


+function getVisibleEntitiesFromCollection(collection) {
+  if (!defined(collection)) {
+    return [];


small style nit - define
const visibleEntities = [];

before this, and return it instead of a literal []

mzschwartz5 · 2025-10-17T01:32:50Z

packages/engine/Source/DataSources/EntityCluster.js

    },
  },
+  /**
+   * Gets the event that will be raised when clustering is processed, including both clustered and declustered entities.


So does this event do everything the pre-existing clusteredEvent did, and more? Because the comment says in includes clustered entities. So it's not just an event for declustering but also for clustering?

mzschwartz5 · 2025-10-17T01:36:34Z

packages/engine/Source/DataSources/EntityCluster.js

+        clustered: entityCluster._lastClusteredEntities.slice(),
+        declustered: entityCluster._lastDeclusteredEntities.slice(),
+        cluster: null,
+        allProcessed: entityCluster._allProcessedEntities.slice(),


Similar to my other comment - do we want / need to be creating a shallow copy of these arrays here, via .slice()?

mzschwartz5 · 2025-10-17T01:39:18Z

packages/engine/Specs/DataSources/EntityClusterSpec.js

I'm not an expert on unit testing philosphy, so maybe this is a bad suggestion, but could we maybe have unit test(s) that check that clustering generally works for all three of: points, billboards, and labels (collections)?

I'm just a little wary given the discussion about using the item.show property as if these classes all have a shared abstract interface (which they don't). I don't expect that to be an issue, but if someone were to ever change that property name on one of the classes, at least having a failing unit test here would prevent the issue.

Oko-Tester · 2025-10-17T10:25:01Z

Hey @mzschwartz5,
Thanks for your feedback. I'll rethink my current implementation and maybe find a better way. But I won't be able to deal with this MR for a few days.

As for your comment on unit tests, I am not an expert in this field either, but it seems sensible to me to use them to avoid potential future problems.

javagl reviewed Sep 25, 2025

View reviewed changes

packages/engine/Source/DataSources/EntityCluster.js Outdated Show resolved Hide resolved

Oko-Tester force-pushed the fix-issue-5760-declustered-entities branch from 746701c to 2d42fdc Compare September 26, 2025 12:07

Oko-Tester marked this pull request as ready for review September 26, 2025 12:07

mzschwartz5 reviewed Sep 26, 2025

View reviewed changes

packages/engine/Source/DataSources/EntityCluster.js Outdated Show resolved Hide resolved

mzschwartz5 reviewed Sep 26, 2025

View reviewed changes

packages/engine/Source/DataSources/EntityCluster.js Outdated Show resolved Hide resolved

javagl and others added 6 commits September 29, 2025 10:42

Cache rectangle in ClippingPolygon

04fcc53

Add declusteredEvent to EntityCluster

2c9215f

Fixes CesiumGS#5760 by adding declusteredEvent that provides both clustered and declustered entities, plus new API methods for accessing clustering state. Maintains backward compatibility.

perf: optimize entity clustering by avoiding spread on large sets and…

5d851e7

… skipping unused declustering computations

docs: updated CHANGES.md

126337f

test: extend EntityClusterSpec to cover declusteredEvent

5f40635

refactor: reduce nesting and extract duplicate collection logic

587e235

Oko-Tester force-pushed the fix-issue-5760-declustered-entities branch from 9bd82e9 to 587e235 Compare September 29, 2025 08:43

Merge branch 'main' into fix-issue-5760-declustered-entities

5224ff1

ggetz assigned mzschwartz5 Sep 29, 2025

Oko-Tester added 2 commits October 1, 2025 11:39

Merge branch 'main' into fix-issue-5760-declustered-entities

a11b0b8

Merge branch 'main' into fix-issue-5760-declustered-entities

f0b1458

mzschwartz5 requested changes Oct 17, 2025

View reviewed changes

Add declusteredEvent to EntityCluster #12920

Are you sure you want to change the base?

Add declusteredEvent to EntityCluster #12920

Conversation

Oko-Tester commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key Changes

Issue number and link

Testing plan

Manual Testing

Author checklist

Uh oh!

github-actions bot commented Sep 25, 2025

Uh oh!

Uh oh!

Oko-Tester commented Sep 26, 2025

Uh oh!

Uh oh!

Uh oh!

Oko-Tester commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzschwartz5 commented Oct 6, 2025

Uh oh!

mzschwartz5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javagl Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oko-Tester commented Oct 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Oko-Tester commented Sep 25, 2025 •

edited

Loading

Oko-Tester commented Oct 4, 2025 •

edited

Loading

mzschwartz5 left a comment •

edited

Loading

javagl Oct 17, 2025 •

edited

Loading