Skip to content

[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

Open
wants to merge 4 commits into
base: stramlining-map-rendering
Choose a base branch
from

Conversation

cestercian
Copy link
Member

@cestercian cestercian commented Mar 12, 2025

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

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:

  • Introduced spatial indexing using KDBush to optimize clustering performance.
  • Implemented grouping of nodes based on proximity and clustering attributes.
  • Modified cluster assignment logic to prevent overlapping.
  • Updated centroid calculations for accurate cluster placement.
  • Ensured better handling of non-clustered nodes.

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

image

@niteshsinha17 niteshsinha17 self-requested a review March 17, 2025 15:06
Copy link
Member

@nemesifier nemesifier left a 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.

Copy link
Member

@nemesifier nemesifier left a 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.

@cestercian
Copy link
Member Author

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!

@cestercian
Copy link
Member Author

Hi @nemesifier ,

I've made the requested changes:

✅ Moved netjson-cluster-overlap.html to /public/example_templates for consistency.
✅ Updated index.html to include a link to the new example, styled like the others.

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!

Copy link
Member

@nemesifier nemesifier left a 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 see when I try to open the example, do you see a different situation?

Cannot GET /example_templates/netjson-cluster-overlap.html.

Screenshot from 2025-03-19 19-01-01

@cestercian
Copy link
Member Author

hey @nemesifier

I’ve fixed the issue. The error occurred because the requested file was not being served correctly. I’ve updated netjson-cluster-overlap.html and changed the file location in index.html my latest commit, which should resolve it.

Let me know if you still encounter any issues!

@cestercian cestercian requested a review from nemesifier March 20, 2025 08:03
Copy link
Member

@nemesifier nemesifier left a 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?

@cestercian
Copy link
Member Author

image

It looks like the NetJSONGraph library isn't being recognized. This often happens when the project isn't started properly.

@nemesifier, are you running yarn start, or are you just opening the index.html file directly? If you're just opening the file, the dependencies might not be loading correctly. Try starting the project with yarn start and let me know if the issue persists.

@niteshsinha17
Copy link
Member

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?

@nemesifier I am getting this result. Seems something is wrong. I will look into PR in detail.

Screenshot 2025-03-22 at 8 58 06 PM

Copy link
Member

@niteshsinha17 niteshsinha17 left a 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)

@cestercian
Copy link
Member Author

Thank you, @niteshsinha17, for your review and valuable feedback!

I've made the following updates based on your suggestions:

  • Updated the CSS import paths to ensure they are loaded from the library, keeping it consistent with other examples.
  • Adjusted the styling of the legend to align with existing examples for a uniform look.
  • Retained the <script src="../dist/netjsongraph.min.js"></script> line, as it is essential for loading the NetJSONGraph library.
    • Without it, the new NetJSONGraph() constructor would be undefined, causing the graph rendering and clustering logic to fail.

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 example templates directory I don't know why this issue persists Im trying to find the bug, but the changes in the file examples/netjson-cluster-overlap.html doesn't reflect in the demo

@cestercian
Copy link
Member Author

image

this is output I'm getting, let me know if there are any other issues, Thanks again for the review !!

Copy link
Member

@nemesifier nemesifier left a 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:

Screenshot from 2025-03-24 18-20-37

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:

Screenshot from 2025-03-24 18-24-53

@cestercian
Copy link
Member Author

cestercian commented Mar 25, 2025

image
I've added more nodes (20-30) so that each status forms a distinct cluster when opening the example.

However, ensuring that the circles display the correct status color when zooming in requires modifying src/js/netjsongraph.config.js, specifically the nodeStyle.color property at line 172 (as seen in the attached screenshot). This change would affect the map rendering across the entire project.

Since this PR is already quite long, I think it would be best to handle this in a separate PR. Would you be open to creating a new issue for this, so we can track and work on it separately?

@cestercian cestercian requested a review from nemesifier March 25, 2025 21:47
Copy link
Member

@niteshsinha17 niteshsinha17 left a 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.

Comment on lines 378 to 385
const interval = setInterval(() => {
const markers = document.querySelectorAll('.marker-cluster');
if (markers.length > 0) {
preventClusterOverlap();
}

// Stop checking after 60 seconds
setTimeout(() => clearInterval(interval), 60000);
Copy link
Member

@niteshsinha17 niteshsinha17 Mar 26, 2025

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.

@cestercian
Copy link
Member Author

Moved cluster overlap prevention logic to lib/js/clusterUtils.js for better modularity.

Changes Made

  • Extracted preventClusterOverlap() into clusterUtils.js
  • Added setupClusterOverlapPrevention() for centralized event handling
  • Updated netjson-cluster-overlap.html to use ES module imports
  • Improved README.md with project structure and usage details

Copy link
Member

@nemesifier nemesifier left a 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.

@cestercian
Copy link
Member Author

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?

@cestercian
Copy link
Member Author

image

when Zoomed IN

image

When Zoomed OUT

I've made the necessary changes to improve the cluster separation and have included updated images for review. Let me know if there's anything else you'd like to tweak!

@cestercian cestercian marked this pull request as draft April 2, 2025 09:17
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there 3 red circles in the following screenshot?

image

We need to end up with something like the following (ignore the missing number of nodes in the cluster, as I have generated this with chatgpt):

image

@cestercian
Copy link
Member Author

Thanks for the review @nemesifier ,
I am currently working on it would let you when it's ready for review !!

Copy link
Member

@nemesifier nemesifier left a 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.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

denmark.webm

@cestercian cestercian marked this pull request as draft May 27, 2025 20:34
@cestercian
Copy link
Member Author

cluster.june.2.mov

Logic Changes for Cluster Separation

Enhanced Cluster Logic:

  • Implemented a mechanism to offset clusters of different attribute groups that originate from the same pixel location to prevent overlap.
  • Clusters are arranged in a circular pattern using angle-based separation (360° divided by the number of clusters) around the original position using a configurable clusterSeparation distance.
  • The offset is calculated in pixels and then converted back to geographical coordinates using Leaflet's containerPointToLatLng method.

cestercian

This comment was marked as resolved.

@cestercian cestercian marked this pull request as ready for review June 4, 2025 19:22
@cestercian cestercian requested a review from nemesifier June 4, 2025 20:52
Copy link
Member

@nemesifier nemesifier left a 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.

@cestercian
Copy link
Member Author

cluster june 6

Hey @nemesifier ,

I've updated the clustering logic to form clusters even for single nodes and added test cases to cover this change.

@cestercian cestercian requested a review from nemesifier June 5, 2025 19:30
@cestercian
Copy link
Member Author

9aed7264-f9c8-484e-a976-d0b1bb1ddebc.mov

made 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.

@cestercian cestercian requested a review from pandafy June 18, 2025 19:50
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please fix the following minor things?

  • avoid importing the javascript file of the library twice
  • move data to external JSON file (as with the other examples)
  • remove these annoying node labels
    Screenshot from 2025-07-03 21-51-23

if (node.properties && node.properties.status) {
const status = node.properties.status.toLowerCase();
if (
status === "ok" ||
Copy link
Member

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";
Copy link
Member

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?

Copy link
Member Author

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.

@cestercian
Copy link
Member Author

@nemesifier I've updated the PR with the requested changes.

Let me know if any other modification are required

@cestercian cestercian requested a review from nemesifier July 4, 2025 04:08
Copy link
Member

@nemesifier nemesifier left a 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.

  1. We need to make sure to test our example here looks very similar to what we'll use in OpenWISP.
  2. 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.

@cestercian cestercian changed the base branch from master to stramlining-map-rendering July 17, 2025 17:35
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash all the commits into one which will make managing both PRs at the same time easier (#396 and #171).

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.
@cestercian
Copy link
Member Author

cestercian commented Jul 17, 2025

Please squash all the commits into one which will make managing both PRs at the same time easier (#396 and #171).

Squashed in 4aaecad

Copy link
Member

@nemesifier nemesifier left a 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:

  1. 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.

  2. Overlap with geometries
    Screenshot from 2025-07-19 15-24-33
    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

[change] Prevent overlapping of clusters
4 participants