-
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
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 |
---|---|---|
|
@@ -67,6 +67,11 @@ function EntityCluster(options) { | |
|
||
this._clusterEvent = new Event(); | ||
|
||
this._declusteredEvent = new Event(); | ||
this._allProcessedEntities = []; | ||
this._lastClusteredEntities = []; | ||
this._lastDeclusteredEntities = []; | ||
|
||
/** | ||
* Determines if entities in this collection will be shown. | ||
* | ||
|
@@ -127,6 +132,10 @@ function getBoundingBox(item, coord, pixelRange, entityCluster, result) { | |
function addNonClusteredItem(item, entityCluster) { | ||
item.clusterShow = true; | ||
|
||
if (defined(item.id)) { | ||
entityCluster._lastDeclusteredEntities.push(item.id); | ||
} | ||
|
||
if ( | ||
!defined(item._labelCollection) && | ||
defined(item.id) && | ||
|
@@ -157,7 +166,16 @@ function addCluster(position, numPoints, ids, entityCluster) { | |
cluster.point.position = | ||
position; | ||
|
||
entityCluster._lastClusteredEntities = | ||
entityCluster._lastClusteredEntities.concat(ids); | ||
|
||
entityCluster._clusterEvent.raiseEvent(ids, cluster); | ||
|
||
entityCluster._declusteredEvent.raiseEvent({ | ||
clustered: ids, | ||
declustered: entityCluster._lastDeclusteredEntities.slice(), | ||
cluster: cluster, | ||
}); | ||
} | ||
|
||
function hasLabelIndex(entityCluster, entityId) { | ||
|
@@ -207,6 +225,10 @@ function getScreenSpacePositions( | |
continue; | ||
} | ||
|
||
if (defined(item.id)) { | ||
entityCluster._allProcessedEntities.push(item.id); | ||
} | ||
|
||
points.push({ | ||
index: i, | ||
collection: collection, | ||
|
@@ -216,7 +238,7 @@ function getScreenSpacePositions( | |
} | ||
} | ||
|
||
const pointBoundinRectangleScratch = new BoundingRectangle(); | ||
const pointBoundingRectangleScratch = new BoundingRectangle(); | ||
const totalBoundingRectangleScratch = new BoundingRectangle(); | ||
const neighborBoundingRectangleScratch = new BoundingRectangle(); | ||
|
||
|
@@ -226,6 +248,10 @@ function createDeclutterCallback(entityCluster) { | |
return; | ||
} | ||
|
||
entityCluster._allProcessedEntities = []; | ||
entityCluster._lastClusteredEntities = []; | ||
entityCluster._lastDeclusteredEntities = []; | ||
|
||
const scene = entityCluster._scene; | ||
|
||
const labelCollection = entityCluster._labelCollection; | ||
|
@@ -240,6 +266,11 @@ function createDeclutterCallback(entityCluster) { | |
!entityCluster._clusterLabels && | ||
!entityCluster._clusterPoints) | ||
) { | ||
entityCluster._declusteredEvent.raiseEvent({ | ||
clustered: [], | ||
declustered: [], | ||
cluster: null, | ||
}); | ||
return; | ||
} | ||
|
||
|
@@ -414,7 +445,7 @@ function createDeclutterCallback(entityCluster) { | |
point.coord, | ||
pixelRange, | ||
entityCluster, | ||
pointBoundinRectangleScratch, | ||
pointBoundingRectangleScratch, | ||
); | ||
const totalBBox = BoundingRectangle.clone( | ||
bbox, | ||
|
@@ -485,6 +516,18 @@ function createDeclutterCallback(entityCluster) { | |
} | ||
} | ||
|
||
if ( | ||
entityCluster._lastClusteredEntities.length > 0 || | ||
entityCluster._lastDeclusteredEntities.length > 0 | ||
) { | ||
entityCluster._declusteredEvent.raiseEvent({ | ||
clustered: entityCluster._lastClusteredEntities.slice(), | ||
declustered: entityCluster._lastDeclusteredEntities.slice(), | ||
cluster: null, | ||
allProcessed: entityCluster._allProcessedEntities.slice(), | ||
}); | ||
} | ||
|
||
if (clusteredLabelCollection.length === 0) { | ||
clusteredLabelCollection.destroy(); | ||
entityCluster._clusterLabelCollection = undefined; | ||
|
@@ -567,6 +610,16 @@ Object.defineProperties(EntityCluster.prototype, { | |
return this._clusterEvent; | ||
}, | ||
}, | ||
/** | ||
* Gets the event that will be raised when clustering is processed, including both clustered and declustered entities. | ||
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. So does this event do everything the pre-existing |
||
* @memberof EntityCluster.prototype | ||
* @type {Event} | ||
*/ | ||
declusteredEvent: { | ||
get: function () { | ||
return this._declusteredEvent; | ||
}, | ||
}, | ||
/** | ||
* Gets or sets whether clustering billboard entities is enabled. | ||
* @memberof EntityCluster.prototype | ||
|
@@ -847,6 +900,58 @@ function disableCollectionClustering(collection) { | |
} | ||
} | ||
|
||
function getVisibleEntitiesFromCollection(collection) { | ||
if (!defined(collection)) { | ||
return []; | ||
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 style nit - define before this, and return it instead of a literal |
||
} | ||
|
||
const visibleEntities = []; | ||
for (let i = 0; i < collection.length; i++) { | ||
const item = collection.get(i); | ||
if (defined(item.id) && item.show) { | ||
visibleEntities.push(item.id); | ||
} | ||
} | ||
return visibleEntities; | ||
} | ||
|
||
function handleDeclusteredEvent(entityCluster) { | ||
if (entityCluster._declusteredEvent.numberOfListeners === 0) { | ||
return; | ||
} | ||
const allVisibleEntities = [ | ||
...getVisibleEntitiesFromCollection(entityCluster._labelCollection), | ||
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 know there was previous discussion about the use of the spread ( |
||
...getVisibleEntitiesFromCollection(entityCluster._billboardCollection), | ||
...getVisibleEntitiesFromCollection(entityCluster._pointCollection), | ||
]; | ||
|
||
if (allVisibleEntities.length > 0) { | ||
const uniqueEntities = Array.from(new Set(allVisibleEntities)); | ||
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. Why is it possible for |
||
|
||
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 = []; | ||
} | ||
} | ||
|
||
Comment on lines
+928
to
+954
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 don't believe this need any conditional logic. Just set and the rest handles itself whether |
||
function updateEnable(entityCluster) { | ||
if (entityCluster.enabled) { | ||
return; | ||
|
@@ -869,6 +974,8 @@ function updateEnable(entityCluster) { | |
disableCollectionClustering(entityCluster._labelCollection); | ||
disableCollectionClustering(entityCluster._billboardCollection); | ||
disableCollectionClustering(entityCluster._pointCollection); | ||
|
||
handleDeclusteredEvent(entityCluster); | ||
} | ||
|
||
/** | ||
|
@@ -998,9 +1105,37 @@ EntityCluster.prototype.destroy = function () { | |
this._pixelRangeDirty = false; | ||
this._minimumClusterSizeDirty = false; | ||
|
||
this._allProcessedEntities = []; | ||
this._lastClusteredEntities = []; | ||
this._lastDeclusteredEntities = []; | ||
|
||
return undefined; | ||
}; | ||
|
||
/** | ||
* Returns the last set of clustered entities from the most recent clustering operation. | ||
* @returns {Entity[]} Array of entities that were clustered | ||
*/ | ||
EntityCluster.prototype.getLastClusteredEntities = function () { | ||
return this._lastClusteredEntities.slice(); | ||
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. Is there any reason to return a shallow copy of the internal array (via 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. 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:
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 ( 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. Hmm... I see. I would still rather we protect against undefined in In a typed language, you would just have a compile-time contract here, marking this function as returning something 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) |
||
}; | ||
|
||
/** | ||
* Returns the last set of declustered entities from the most recent clustering operation. | ||
* @returns {Entity[]} Array of entities that were not clustered | ||
*/ | ||
EntityCluster.prototype.getLastDeclusteredEntities = function () { | ||
return this._lastDeclusteredEntities.slice(); | ||
}; | ||
|
||
/** | ||
* Returns all entities that were processed in the most recent clustering operation. | ||
* @returns {Entity[]} Array of all processed entities | ||
*/ | ||
EntityCluster.prototype.getAllProcessedEntities = function () { | ||
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. 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? 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. (And similarly rename the internal array) |
||
return this._allProcessedEntities.slice(); | ||
}; | ||
|
||
/** | ||
* A event listener function used to style clusters. | ||
* @callback EntityCluster.newClusterCallback | ||
|
@@ -1019,4 +1154,26 @@ EntityCluster.prototype.destroy = function () { | |
* cluster.label.text = entities.length.toLocaleString(); | ||
* }); | ||
*/ | ||
|
||
/** | ||
* A event listener function for enhanced clustering information. | ||
* @callback EntityCluster.declusteredCallback | ||
* | ||
* @param {object} clusteringData An object containing clustering information. | ||
* @param {Entity[]} clusteringData.clustered An array of entities that were clustered. | ||
* @param {Entity[]} clusteringData.declustered An array of entities that were not clustered. | ||
* @param {object|null} clusteringData.cluster The cluster object (if this event is for a specific cluster) or null for summary events. | ||
* @param {Entity[]} [clusteringData.allProcessed] An array of all entities processed (only in summary events). | ||
* | ||
* @example | ||
* // Using the enhanced declusteredEvent to access both clustered and declustered entities | ||
* dataSource.clustering.declusteredEvent.addEventListener(function(data) { | ||
* console.log('Clustered entities:', data.clustered.length); | ||
* console.log('Declustered entities:', data.declustered.length); | ||
* if (data.allProcessed) { | ||
* console.log('Total processed entities:', data.allProcessed.length); | ||
* } | ||
* }); | ||
*/ | ||
|
||
export default EntityCluster; |
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.
Similar to my other comment - do we want / need to be creating a shallow copy of these arrays here, via
.slice()
?