-
Notifications
You must be signed in to change notification settings - Fork 93
[fix] Prevent Overlapping of Clusters in netjsongraph.js #171 #349
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: stramlining-map-rendering
Are you sure you want to change the base?
Conversation
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 has slipped my attention, the notification for this was buried among many other notifications. That's why it's good to let us know about your work in the dev chat.
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.
In the PR description you mention a test file called cluster_overlap_test.html
, which I see you have removed. What type of problem where you having with this? I think such a test file to compare the behavior of this branch with master and verify whether the situation really improves is quite useful.
Thanks for the review @nemesifier! The cluster_overlap_test.html file was initially created to visualize the issue, but I removed it since I thought it wasn't necessary after fixing the overlap problem. However, I see your point—it could be useful for verifying improvements. I can reintroduce it and ensure it effectively compares the branch behavior with master. Let me know if you'd like any specific tests included! |
Hi @nemesifier , I've made the requested changes: ✅ Moved netjson-cluster-overlap.html to /public/example_templates for consistency. Let me know if you need any further modifications or if you'd like me to proceed with replacing the current clustering example with this one. Thanks for your feedback! |
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.
hey @nemesifier I’ve fixed the issue. The error occurred because the requested file was not being served correctly. I’ve updated Let me know if you still encounter any issues! |
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 don't know if I am doing something wrong but this isn't working for me, here's what I see:
netjsongraphjs-clustering-pr.webm
@niteshsinha17 do you see a different situation or do you get the same result I get?
![]() It looks like the @nemesifier, are you running |
@nemesifier I am getting this result. Seems something is wrong. I will look into PR in detail. |
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 have done a very basic review for now. The graph is not rendering properly.
Check this comment, here @nemesifier
has mentioned about an example that needs to be added. #239 (review)
Thank you, @niteshsinha17, for your review and valuable feedback! I've made the following updates based on your suggestions:
Let me know if there's anything else you'd like me to refine. Thanks again for your insights! and FYI 😊 the changes that reflect in projects are made to file in |
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.
Now I can see the example, thank you for fixing that!
Can you please create more nodes (eg: 20-30) so that we end up seeing 1 cluster for each status when opening up the example?
Can you also make sure that when zooming in, we can see the circle of the same color of its status? Right now it shows as blue, but in the OpenWISP Dashboard each circle is of the color of its status, see the following screenshot from the OpenWISP Demo:
We need to improve the clustering feature to avoid the overlap because on bigger instances with many nodes this is how the map shows up:
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.
Thanks for adding nodes in the example. I suggest to look into other js files to understand how everything runs.
const interval = setInterval(() => { | ||
const markers = document.querySelectorAll('.marker-cluster'); | ||
if (markers.length > 0) { | ||
preventClusterOverlap(); | ||
} | ||
|
||
// Stop checking after 60 seconds | ||
setTimeout(() => clearInterval(interval), 60000); |
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 is not the correct way to fix this issue. Do not use setTimeout or setIntervals. Instead some type of event like zoom, can be better place to handle this.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Moved cluster overlap prevention logic to Changes Made
|
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 is what I am seeing:
Peek.2025-03-27.16-56.webm
When zooming in, the circles should appear of the color of their status, not blue.
When zooming out, I expect to see a few clusters of different colors, but I see they're overlapping.
Please, in the next iteration post a similar GIF/Webp recording of the result, I am using a Linux program called Peek which is very handy.
Thanks for the feedback, @nemesifier ! I already mentioned the need to modify nodeStyle.color in src/js/netjsongraph.config.js (line 172) in my earlier comment. Since this change affects the entire project’s map rendering, I suggested handling it in a separate PR, since this PR is getting to long already. Would you be open to creating a new issue for this so we can track it separately? |
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.
Thanks for the review @nemesifier , |
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.
While testing openwisp/openwisp-monitoring#668 on demo I see that there's overlapping clusters:
demo.webm
Will DM you to show you this.
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.
denmark.webm
cluster.june.2.movLogic Changes for Cluster SeparationEnhanced Cluster Logic:
|
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.
Great progress @cestercian! 👍
Problem found during testing: the green cluster overlapped the non clustered point.
clustering-04-june-2025.webm
When calculating whether we should move the cluster, can you take into account also non clustered points to avoid this? I guess if there were 2 yellow points we would have got a cluster and it would have worked normally, but we won't always have clusters, some times as in this case we will have only 1 point of a specific color and we don't want to hide it below the clusters.
Hey @nemesifier , I've updated the clustering logic to form clusters even for single nodes and added test cases to cover this change. |
9aed7264-f9c8-484e-a976-d0b1bb1ddebc.movmade cluster separation robust for all attribute clusters at a location, and then applied a repulsion function to guarantee that all clusters on the map repel each other and never overlap, regardless of their initial positions or sizes. |
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 (node.properties && node.properties.status) { | ||
const status = node.properties.status.toLowerCase(); | ||
if ( | ||
status === "ok" || |
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 seems we're putting the logic of OpenWISP monitoring in the library, which is wrong.
Once we overcome the issues we're facing, we'll have to clean this up, for the moment it's not a priority, we need to make it work properly first.
(configs.mapOptions.nodeConfig && | ||
configs.mapOptions.nodeConfig.nodeStyle && | ||
configs.mapOptions.nodeConfig.nodeStyle.color) || | ||
"#6c757d"; |
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.
where is this color coming from?
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.
The node color is picked from the config if it’s defined under nodeStyle.color. If nothing’s set there, it just falls back to a default gray so that nodes always have a visible color on the map.
@nemesifier I've updated the PR with the requested changes. Let me know if any other modification are required |
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.
The API of OpenWISP Monitoring returns data in GeoJSON.
The data in this example is not GeoJSON and that's why we are having all this problems.
- We need to make sure to test our example here looks very similar to what we'll use in OpenWISP.
- Please start investigating why loading data in GeoJSON format triggers different rendering, we need to fix this issue as it's preventing us from testing this solution in real world scenarios and hence we are not sure the solution is really working.
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 PR introduces a new feature that enables node clustering in the map view. This helps to improve the readability of the map when there are a large number of nodes in a small area. This PR also includes a new example that demonstrates how to use the new feature.
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.
During testing, I noticed these quirks:
-
Interactions with element on the map is broken
I can't see the details or any element on the map or drill down to it. -
Overlap with geometries
Can we consider the center of any geojson geometry as a point to include in the cluster and when the cluster breaks we show the geometry?
If there's other shapes we can't include in the clusters like lines, let's make sure clusters are shown on top.
Checklist
Reference to Existing Issue
Closes #171
Description of Changes
This PR addresses issue #171 by preventing the overlapping of clusters in the
netjsongraph.js
visualization.Changes Implemented:
KDBush
to optimize clustering performance.Testing
A test file (
cluster_overlap_test.html
) was created to visualize and verify the clustering behavior. The test file simulates overlapping nodes with different statuses and ensures they are properly clustered without overlaps.Screenshot