-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Add declusteredEvent to EntityCluster #12920
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
base: main
Are you sure you want to change the base?
Changes from all commits
04fcc53
2c9215f
5d851e7
126337f
5f40635
587e235
5224ff1
a11b0b8
f0b1458
5ec4ced
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,8 @@ function EntityCluster(options) { | |
| this._removeEventListener = undefined; | ||
|
|
||
| this._clusterEvent = new Event(); | ||
| this._declusterEvent = new Event(); | ||
| this._previouslyClusteredEntities = []; | ||
|
|
||
| /** | ||
| * Determines if entities in this collection will be shown. | ||
|
|
@@ -216,7 +218,7 @@ function getScreenSpacePositions( | |
| } | ||
| } | ||
|
|
||
| const pointBoundinRectangleScratch = new BoundingRectangle(); | ||
| const pointBoundingRectangleScratch = new BoundingRectangle(); | ||
| const totalBoundingRectangleScratch = new BoundingRectangle(); | ||
| const neighborBoundingRectangleScratch = new BoundingRectangle(); | ||
|
|
||
|
|
@@ -414,7 +416,7 @@ function createDeclutterCallback(entityCluster) { | |
| point.coord, | ||
| pixelRange, | ||
| entityCluster, | ||
| pointBoundinRectangleScratch, | ||
| pointBoundingRectangleScratch, | ||
| ); | ||
| const totalBBox = BoundingRectangle.clone( | ||
| bbox, | ||
|
|
@@ -500,8 +502,28 @@ function createDeclutterCallback(entityCluster) { | |
| entityCluster._clusterPointCollection = undefined; | ||
| } | ||
|
|
||
| entityCluster._previousClusters = newClusters; | ||
| entityCluster._previousHeight = currentHeight; | ||
| const currentlyClusteredIds = []; | ||
|
|
||
| if (newClusters.length > 0 && defined(clusteredLabelCollection)) { | ||
| for (let c = 0; c < clusteredLabelCollection.length; ++c) { | ||
| const clusterLabel = clusteredLabelCollection.get(c); | ||
| if (defined(clusterLabel) && defined(clusterLabel.id)) { | ||
| currentlyClusteredIds.push(...clusterLabel.id); | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+507
to
+514
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Random drive-by comment:
When something should never be I know, there are different philosophies. But there's an important difference between being "defensive"/"resilient", and what I refer to as "fail silent".
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I didn’t consider that. I think it was mentioned earlier in this PR, but you’re right, better to fail visibly than silently.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay so sounds like we can address points 1 and 2 easily. For point 3 (the spread operator) - are we copying the all currently clustered IDs (i.e. every ID of every entity that is clustered) on every declutter callback? That seems like it could be bad for performance. It also doubles the memory we consume in tracking which entities are clustered (since we always have a copy of the Side note: does this only fire a declustered event for |
||
|
|
||
| const hasActiveClusters = currentlyClusteredIds.length > 0; | ||
| const hadPreviouslyClusters = | ||
| entityCluster._previouslyClusteredEntities.length > 0; | ||
|
|
||
| if (!hasActiveClusters && hadPreviouslyClusters) { | ||
| entityCluster._declusterEvent.raiseEvent( | ||
| entityCluster._previouslyClusteredEntities, | ||
| ); | ||
| } | ||
|
|
||
| entityCluster._previouslyClusteredEntities = currentlyClusteredIds; | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -567,6 +589,16 @@ Object.defineProperties(EntityCluster.prototype, { | |
| return this._clusterEvent; | ||
| }, | ||
| }, | ||
| /** | ||
| * Gets the event that will be raised when all entities have been declustered. | ||
| * @memberof EntityCluster.prototype | ||
| * @type {Event<EntityCluster.declusterCallback>} | ||
| */ | ||
| declusterEvent: { | ||
| get: function () { | ||
| return this._declusterEvent; | ||
| }, | ||
| }, | ||
| /** | ||
| * Gets or sets whether clustering billboard entities is enabled. | ||
| * @memberof EntityCluster.prototype | ||
|
|
@@ -993,6 +1025,7 @@ EntityCluster.prototype.destroy = function () { | |
|
|
||
| this._previousClusters = []; | ||
| this._previousHeight = undefined; | ||
| this._previouslyClusteredEntities = []; | ||
|
|
||
| this._enabledDirty = false; | ||
| this._pixelRangeDirty = false; | ||
|
|
@@ -1019,4 +1052,17 @@ EntityCluster.prototype.destroy = function () { | |
| * cluster.label.text = entities.length.toLocaleString(); | ||
| * }); | ||
| */ | ||
| /** | ||
| * A event listener function used when all entities have been declustered. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small typo- "A event" --> "An event" |
||
| * @callback EntityCluster.declusterCallback | ||
| * | ||
| * @param {Entity[]} declusteredEntities An array of the entities that were in the last | ||
| * remaining clusters and are now displayed individually. | ||
| * | ||
| * @example | ||
| * // Listen for decluster events | ||
| * dataSource.clustering.declusterEvent.addEventListener(function(entities) { | ||
| * console.log('All clusters removed. Last entities declustered:', entities); | ||
| * }); | ||
| */ | ||
| export default EntityCluster; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
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.
I feel like this is a bit misleading.
clusterEventfires whenever a new cluster is added, right? ButdeclusterEventonly fires when all clusters are removed. (If I've understood this PR correctly)It seems like
declusterEventshould also fire whenever a cluster is removed. (I don't know if that's feasible to implement, though)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.
You are correct about the asymmetry. The behavior is as follows:
clusterEvent: Triggered every time addCluster is called, which happens in the following cases:
When new clusters are created
When existing clusters are reused (when the camera zooms in and they still meet the minimumClusterSize threshold)
declusterEvent: Only fires when transitioning from "has clusters" to "no clusters at all". It does NOT fire when individual clusters are removed.
However, users can still detect which clusters have been removed by tracking the entity IDs themselves. Since clusterEvent is triggered every frame with the current cluster composition, you can compare frame by frame to determine which entities were declustered.
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.
A wild guess is that this question may have to do with the name.
When you have methods like
addSomething/removeSomethingorcreateSomething/destroySomethingthen you'd also expect a certain "symmetry".
(And when you have
addSomething,removeSomething, anddeleteSomething, you'll wonder: Wait what...?)In this case, one could expect this event to be raises when ... the opposite of that happened what causes a
clusterEvent. Yes, there is the documentation that says that this event "... will be raised when all entities have been declustered.", but still.@mzschwartz5 It's up to you, but I think that options could be to
declusteredAllEventorallDeclusteredEventor ... whatever is appropriate here)declusterEventthat indeed is the opposite of whatclusterEventis doingUh oh!
There was an error while loading. Please reload this page.
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.
Yes, you’re right about the naming confusion. I think we should rename the event to lastDeclusterEvent (or allDeclusteredEvent, whatever you prefer) so it’s clear that it only fires when the last cluster is removed. But there’s no real need for a new event that tracks every declustering — users can already detect removed clusters by comparing the cluster composition from the clusterEvent. So I don’t think an additional event for every declustering is necessary.
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.
I'm not deeply involved here - it was just a comment while looking at my GitHub notifications. So I don't know in how far it makes sense to define an event for individual removed clusters. But... (again: a bit shallow)...:
Users can do a lot of stuff. Users can track this and that and store this and that and compute some difference here and there. The question is whether there's a convincing use case for being informed about individual clusters. If there is, then uers should not be forced to write 200 lines of (possibly buggy) code to derive the information they need, when they could just listen for a single, predefined event.
Maybe there is no such use case. And in doubt, this new event could be added later, when the demand arises. And following the mantra: "When in doubt, leave it out!", it's probably OK to omit it for now (and certainly in the context of this PR)
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.
I generally agree with @javagl, I and I think it would be sufficient for this PR to simply rename this event. I liked the proposed "allDeclusteredEvent"