[fix] Prevent Overlapping of Clusters in netjsongraph.js #171 #349

cestercian · 2025-03-12T07:37:25Z

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

- Remove redundant font-family declarations on the body and standardize to "sans-serif" - Update .iconfont declarations to include a serif fallback for better compatibility - Replace "0px" with "0" for margin properties for consistency - Improve overall formatting and structure for maintainability

This file is for testing purposes only and helps in identifying and reproducing the problem so that potential solutions can be explored.

This reverts commit f314caa.

This reverts commit c02b4ad.

This reverts commit 659b2d6.

nemesifier

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.

nemesifier

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 · 2025-03-19T20:20:19Z

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!

This reverts commit 61fe412.

examples/netjson-cluster-overlap.html

cestercian · 2025-03-19T20:47:23Z

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!

nemesifier

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.

cestercian · 2025-03-20T08:03:38Z

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!

nemesifier

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 · 2025-03-22T09:55:55Z

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 · 2025-03-22T15:30:33Z

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.

niteshsinha17

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)

examples/netjson-cluster-overlap.html

This reverts commit 71e004d.

cestercian · 2025-04-23T11:33:26Z

Screen.Recording.2025-04-23.at.4.54.24.PM.mov

Applied Node Coloring based on node status

nemesifier

Looks good @cestercian! The test coverage has went down considerably, can you please fix that? Look at the CI build configuration to see how to calculate test coverage locally so you can see which lines are not being executed during tests.

See also my comments below. Tomorrow I will do more code review, now I only had time to do a quick test.

lib/js/clusterUtils.js

This reverts commit 273f5ac.

lib/js/clusterUtils.js

src/js/netjsongraph.render.js

cestercian · 2025-05-05T09:41:41Z

Hey @niteshsinha17 ,
I moved overlap prevention logic from lib/js/clusterUtils.js to src/js/cluster-utils.js, updated imports, and removed the old file.
and optimized getNodeStyle() usage in netjsongraph.render.js by properly using all returned style configs.

let me know if any other changes are needed 😁

niteshsinha17 · 2025-05-05T09:44:17Z

@cestercian It's really good you are putting efforts. But I am interested in knowing your observations and reasoning behind your changes.

nemesifier

I would group all the clustering docs into a single section coming just before "Example Usage". The rest looks good to me but I'm trying to spare some time to do a complete round of testing before merging.

niteshsinha17 · 2025-05-06T20:37:17Z

src/js/netjsongraph.render.js

    const series = [
-      Object.assign(configs.mapOptions.nodeConfig, {
+      {
        type:
          configs.mapOptions.nodeConfig.type === "effectScatter"
            ? "effectScatter"
            : "scatter",
+        name: "nodes",
        coordinateSystem: "leaflet",
        data: nodesData,
        animationDuration: 1000,
-      }),
+        label: configs.mapOptions.nodeConfig.label,


@cestercian By looking at your code and spending some time in debugging. I think now I know the reason why u are adding an extra object in the series array. You have added this because getNodeStyle is returning {} as size for some nodes (may be because of config) and you wanted to provide your custom functions so that nodes get correct styles? Please correct if I am wrong.

cestercian added 8 commits March 1, 2025 13:43

Merge branch 'master' into master

5ad2983

Create netjson-cluster-overlap.html

f314caa

This file is for testing purposes only and helps in identifying and reproducing the problem so that potential solutions can be explored.

Update netjsongraph.util.js

09005af

Merge branch 'master' of https://github.com/cestercian/netjsongraph.js

02682ae

Revert "Create netjson-cluster-overlap.html"

c02b4ad

This reverts commit f314caa.

Reapply "Create netjson-cluster-overlap.html"

659b2d6

This reverts commit c02b4ad.

Revert "Reapply "Create netjson-cluster-overlap.html""

61fe412

This reverts commit 659b2d6.

niteshsinha17 self-requested a review March 17, 2025 15:06

nemesifier reviewed Mar 19, 2025

View reviewed changes

Reapply "Reapply "Create netjson-cluster-overlap.html""

ff949d7

This reverts commit 61fe412.

nemesifier requested changes Mar 19, 2025

View reviewed changes

examples/netjson-cluster-overlap.html Outdated Show resolved Hide resolved

cestercian added 2 commits March 20, 2025 02:14

Update index.html

d62d97b

Create netjson-cluster-overlap.html

a84b1fe

nemesifier reviewed Mar 19, 2025

View reviewed changes

cestercian added 2 commits March 20, 2025 13:12

Update netjson-cluster-overlap.html

cc0722d

Update index.html

ffc2a0c

cestercian requested a review from nemesifier March 20, 2025 08:03

nemesifier reviewed Mar 21, 2025

View reviewed changes

cestercian added 2 commits March 22, 2025 15:30

Update index.html

61a9d91

Merge branch 'master' into master

87e82af

niteshsinha17 requested changes Mar 22, 2025

View reviewed changes

examples/netjson-cluster-overlap.html Outdated Show resolved Hide resolved

examples/netjson-cluster-overlap.html Outdated Show resolved Hide resolved

examples/netjson-cluster-overlap.html Outdated Show resolved Hide resolved

cestercian added 2 commits March 23, 2025 02:17

Update netjson-cluster-overlap.html

d3c5c82

Update netjson-cluster-overlap.html

882b22c

cestercian added 4 commits April 23, 2025 16:00

Revert "[fix] Netjsongraph.render.js"

be2e4c6

This reverts commit 71e004d.

[fix] Netjsongraph.render.js

572155f

[change] Netjsongraph.js

fd5b529

[fix] Formatting

46390b0

nemesifier changed the title ~~[Fix] Prevent Overlapping of Clusters in netjsongraph.js (#171)~~ [Fix] Prevent Overlapping of Clusters in netjsongraph.js #171 Apr 23, 2025

cestercian added 4 commits April 25, 2025 02:40

[fix] Netjson-clustering.html

d8e7ec6

[fix] Netjson-clustering.html

6b60948

[change] Removed Comments

ab8af74

[fix] Prettier

3a8fd56

nemesifier assigned cestercian Apr 25, 2025

nemesifier requested changes Apr 25, 2025

View reviewed changes

lib/js/clusterUtils.js Outdated Show resolved Hide resolved

lib/js/clusterUtils.js Outdated Show resolved Hide resolved

cestercian added 2 commits April 26, 2025 15:44

[fix] Netjsongraph.config.js

401de7f

[fix] Prettier

09bc435

cestercian requested a review from nemesifier April 26, 2025 14:18

cestercian added 5 commits April 26, 2025 20:30

[fix] Netjsongraph.render.test.js

f7acfcc

[fix] Netjsongraph.render.test.js

273f5ac

Revert "[fix] Netjsongraph.render.test.js"

fd0e1bb

This reverts commit 273f5ac.

[fix] Netjsongraph.render.test.js

8369683

[fix] Eslint

f37ac79

niteshsinha17 requested changes Apr 28, 2025

View reviewed changes

lib/js/clusterUtils.js Outdated Show resolved Hide resolved

src/js/netjsongraph.render.js Outdated Show resolved Hide resolved

cestercian added 3 commits May 4, 2025 01:16

Merge branch 'master' into master

08681ff

[fix] Cluster-utlis.js

a79eeec

[fix] Netjsongraph.render.js

12b3437

cestercian requested a review from niteshsinha17 May 5, 2025 09:36

nemesifier changed the title ~~[Fix] Prevent Overlapping of Clusters in netjsongraph.js #171~~ [fix] Prevent Overlapping of Clusters in netjsongraph.js #171 May 6, 2025

nemesifier reviewed May 6, 2025

View reviewed changes

niteshsinha17 reviewed May 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Prevent Overlapping of Clusters in netjsongraph.js #171 #349

[fix] Prevent Overlapping of Clusters in netjsongraph.js #171 #349

cestercian commented Mar 12, 2025 •

edited

Loading

nemesifier left a comment

nemesifier left a comment

cestercian commented Mar 19, 2025

cestercian commented Mar 19, 2025

nemesifier left a comment

cestercian commented Mar 20, 2025

nemesifier left a comment

cestercian commented Mar 22, 2025

niteshsinha17 commented Mar 22, 2025

niteshsinha17 left a comment •

edited

Loading

cestercian commented Apr 23, 2025

nemesifier left a comment

cestercian commented May 5, 2025

niteshsinha17 commented May 5, 2025

nemesifier left a comment

niteshsinha17 May 6, 2025 •

edited

Loading

[fix] Prevent Overlapping of Clusters in netjsongraph.js #171 #349

Are you sure you want to change the base?

[fix] Prevent Overlapping of Clusters in netjsongraph.js #171 #349

Conversation

cestercian commented Mar 12, 2025 • edited Loading

Checklist

Reference to Existing Issue

Description of Changes

Changes Implemented:

Testing

Screenshot

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

cestercian commented Mar 19, 2025

cestercian commented Mar 19, 2025

nemesifier left a comment

Choose a reason for hiding this comment

cestercian commented Mar 20, 2025

nemesifier left a comment

Choose a reason for hiding this comment

cestercian commented Mar 22, 2025

niteshsinha17 commented Mar 22, 2025

niteshsinha17 left a comment • edited Loading

Choose a reason for hiding this comment

cestercian commented Apr 23, 2025

nemesifier left a comment

Choose a reason for hiding this comment

cestercian commented May 5, 2025

niteshsinha17 commented May 5, 2025

nemesifier left a comment

Choose a reason for hiding this comment

niteshsinha17 May 6, 2025 • edited Loading

Choose a reason for hiding this comment

cestercian commented Mar 12, 2025 •

edited

Loading

niteshsinha17 left a comment •

edited

Loading

niteshsinha17 May 6, 2025 •

edited

Loading