-
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?
Add declusteredEvent to EntityCluster #12920
Conversation
Thank you for the pull request, @Oko-Tester! ✅ We can confirm we have a CLA on file for you. |
746701c
to
2d42fdc
Compare
this is ready |
|
||
if (entityCluster._declusteredEvent.numberOfListeners !== 0) { | ||
const allVisibleEntities = []; | ||
|
||
if (defined(entityCluster._labelCollection)) { | ||
for (let i = 0; i < entityCluster._labelCollection.length; i++) { | ||
const label = entityCluster._labelCollection.get(i); | ||
if (defined(label.id) && label.show) { | ||
allVisibleEntities.push(label.id); | ||
} | ||
} | ||
} | ||
|
||
if (defined(entityCluster._billboardCollection)) { | ||
for (let i = 0; i < entityCluster._billboardCollection.length; i++) { | ||
const billboard = entityCluster._billboardCollection.get(i); | ||
if (defined(billboard.id) && billboard.show) { | ||
allVisibleEntities.push(billboard.id); | ||
} | ||
} | ||
} | ||
|
||
if (defined(entityCluster._pointCollection)) { | ||
for (let i = 0; i < entityCluster._pointCollection.length; i++) { | ||
const point = entityCluster._pointCollection.get(i); | ||
if (defined(point.id) && point.show) { | ||
allVisibleEntities.push(point.id); | ||
} | ||
} | ||
} |
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.
This looks like a prime candidate for a function, so we don't repeat basically the same thing 3x. Then you could even early-return on the defined
check and save a level of indentation.
That said, these collections do not share an interface, so there's no guarantee they will continue to have these common methods and fields (show, id, get)
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.
Yea ur right. I´ll extract this into a helper function.
disableCollectionClustering(entityCluster._billboardCollection); | ||
disableCollectionClustering(entityCluster._pointCollection); | ||
|
||
if (entityCluster._declusteredEvent.numberOfListeners !== 0) { |
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.
It feels like the level of conditional branching here may be unnecessary. I'm always an advocate of trying to reduce the levels of nesting for clarity - usually be introducing new functions and then early-returning from them.
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 think that this was added in response to my comment at #12920 (comment) . And the behavior should be kept like that - it should not do that visible-stuff-computation when no listeners are present.
We can argue about details of the style. Some people strongly advocate for "One return
statement per function!". But I think that "watchdogs" and "early returns" can often increase readability. So when there is something like
if (condition) {
// 100 lines
} else {
// 2 lines
}
I'm also leaning towards the pattern of
// Don't do anything when not necessary
if (!condition) {
// 2 lines
return;
}
// Now do the real work
// 100 lines
(No strong opinion here)
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.
Yeah this was more of a comment on style than behavior.
Also, will say, I do have strong opinions about the early return pattern. I think by nature of being early, it's kind of an exception to the no-multiple-returns guideline. In my mind, the point of no-multiple-returns is to avoid missing a deeply nested return, which can drastically change the meaning of what comes after. Short functions, with early returns don't run into that issue.
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.
"Rules are for the obedience of fools and the guidance of wise men" (D. Bader)
When there's a rule that there shouldn't be multiple returns per function, then some people will follow that rule, no matter what. However, I'm also in the camp of "having to scroll to find an else
is a bad sign".
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.
If there are no specific requests to change the logic or further customise the style, I will keep the current implementation unchanged, provided that is acceptable to you.
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
9bd82e9
to
587e235
Compare
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 originalclusterEvent
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
declusteredEvent
: Fires with complete clustering information including both clustered and declustered entitiesgetLastClusteredEntities()
- Returns currently clustered entitiesgetLastDeclusteredEntities()
- Returns currently declustered entitiesgetAllProcessedEntities()
- Returns all entities processed during clusteringclusterEvent
remains unchanged and functionalIssue 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
declusteredEvent
fires with correctclustered
anddeclustered
arraysclusterEvent
still works as beforeAuthor checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change