[change] Uniform geojson map rendering #396

cestercian · 2025-07-12T19:34:19Z

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 #395 .

Description of Changes

Streamed line map rendering logic for GeoJSON and NetJSON map data type

Screenshot

Please include any relevant screenshots.

Updated the Echarts rendering logic to wrok with

[fix] Updated the Echarts rendering

nemesifier

Seems to be moving in the right direction, I guess we can remove marker cluster now, right?

QA checks and CI builds are failing, please fix.

See my comments below.

nemesifier · 2025-07-14T15:40:07Z

src/js/netjsongraph.core.js

+          // clustering and ECharts overlays.
+          this.originalGeoJSON = JSON.parse(JSON.stringify(JSONData));
+          JSONData = this.utils.geojsonToNetjson(JSONData);
+          this.type = "netjson";


what's the point of keeping this stuff and keep all the if statements if the value is always netjson?

We should be able to simplify the code and remove the useless if statements. I wouldn't change the type of the object, it feels wrong to me.

I still see this.type = "netjson"; here, why?
Is it really necessary to change it?

[fix] Updated Test Cases

This reverts commit 59bf856.

Uniform geo json map rendering

Streamline map rendering

Simplifies the data pipeline by converting GeoJSON input to NetJSON early and treating all data as NetJSON internally. Removes legacy GeoJSON-specific branches, updates appendData logic, and adjusts tests to match the unified approach. This improves maintainability and consistency in map rendering and data updates.

nemesifier

Please squash the commits in this PR into one commit so that it will be easier to manage with #349.

…sp/netjsongraph.js into stramlining-map-rendering

Streamlines the map rendering logic for GeoJSON and NetJSON data types and removes the marker-cluster dependency. Closes #395.

…sp/netjsongraph.js into stramlining-map-rendering

cestercian · 2025-07-17T23:37:12Z

Please squash the commits in this PR into one commit so that it will be easier to manage with #349.

Squashed in 48b36ae

nemesifier

I have some questions below.

nemesifier · 2025-07-19T18:05:23Z

src/js/netjsongraph.render.js

+      self.originalGeoJSON = JSON.parse(JSON.stringify(JSONData));
+      JSONData = self.utils.geojsonToNetjson(JSONData);
+      // From this point forward we treat the data as NetJSON
+      self.type = "netjson";


I tried removing this and the geojson example continues working without errors, are you sure it's necessary? It feels wrong to me. You have already cleand up the code which was checking whether type is netjson, we assume there's only one way to load data now, why is this still necessary?

the code is not only checking if type is NetJSON it also preserve the original GeoJSON via self.originalGeoJSON to still render polygons.

I am asking to avoid changing the type here, but change any other code that relies on type being netjson so that we assume the type is always netjson.

src/js/netjsongraph.render.js

src/js/netjsongraph.util.js

src/js/netjsongraph.render.js

Extracted all GeoJSON-specific logic into src/js/netjsongraph.geojson.js for better maintainability and clarity. Updated the public API to forward geojsonToNetjson calls to the new helper. Refactored map rendering to use addPolygonOverlays for displaying Polygon and MultiPolygon features on Leaflet. Updated documentation to reflect these changes and improve clarity on GeoJSON support.

src/js/netjsongraph.geojson.js

cestercian

Addressed all the changes Kindly let me know if any other changes are required

nemesifier

I still see type being changed in a couple of places which I explained I feel it's wrong and we shouldn't do it, see my comments below.

nemesifier · 2025-07-21T23:00:31Z

src/js/netjsongraph.core.js

+          // clustering and ECharts overlays.
+          this.originalGeoJSON = JSON.parse(JSON.stringify(JSONData));
+          JSONData = this.utils.geojsonToNetjson(JSONData);
+          this.type = "netjson";


I still see this.type = "netjson"; here, why?
Is it really necessary to change it?

nemesifier · 2025-07-21T23:03:23Z

src/js/netjsongraph.render.js

+      self.originalGeoJSON = JSON.parse(JSON.stringify(JSONData));
+      JSONData = self.utils.geojsonToNetjson(JSONData);
+      // From this point forward we treat the data as NetJSON
+      self.type = "netjson";


I am asking to avoid changing the type here, but change any other code that relies on type being netjson so that we assume the type is always netjson.

Updates NetJSONGraph to retain 'geojson' as the public-facing type when loading GeoJSON data, instead of converting it to 'netjson'. Adjusts related rendering logic and tests to reflect this change.

cestercian · 2025-07-22T01:43:20Z

I still see type being changed in a couple of places which I explained I feel it's wrong and we shouldn't do it, see my comments below.

Noted and addressed in 5d63609 now we don't explicitly change the type to netJSON from geoJSON we just assume it's netJSON.

nemesifier

On master:

On this branch:

The Polygon is drawn in a way that makes it look as if there were additional points drawn on the map, which shouldn't happen.

Can you please edit the sample data to make sure we have at least one geojson geometry of each type supported?

This way we can make sure they all look as they should. Use a geojson visualizer (like the one here on github) to see how each standard geojson geometry should look like.

Added MultiPoint, LineString, MultiLineString, and MultiPolygon features to the GeoJSON sample data for broader geometry coverage. Refactored geometry handling in netjsongraph.geojson.js by removing unused Polygon and MultiPolygon processing code.

nemesifier

It's almost there but I still see a few details to adjust, let's compare the output of the github visualizer and https://geojson.io.

This is the current sample data visualized with the current implementation:

This is the github visualizer:

Which can be viewed by opening: https://github.com/openwisp/netjsongraph.js/blob/7cdb431162dfed76c5dac3eb39429efbaebbb067/public/assets/data/geojson-sample.json

This is the geojson.io visualizer (copy/paste the example data):

Now let's Observe the key differences:

The other visualizers don't use circles to highlight the coordinates, shapes are just drawn
The visualizers automatically center the view based on the data being shown, bounds are calculated and map center and zoom are automatically calculated so that the view is able to show everything when the map is loaded. This is very important, please make sure the library is able to behave identically

nemesifier

I noticed that some output values appear as [Object, Object]. Please add a few more sample properties and ensure all are shown correctly (please add an assertion in the tests for this).
Let's treat properties.name as a special case and convert it as netjson label (needs assertion in tests).
Can we convert lines and multi line strings as links in netjson? (needs assertion in tests)
Please add some id elements to the sample data and verify with an assertion that is converted in netjson format.
Where is the value Gjn_4 coming from? I don’t see any corresponding entry in the data sources. This random stuff shouldn't appear, make sure there's some test data which doesn't have any name, id or property defined and have an assertion to verify these random names are not shown.

cestercian added 4 commits July 10, 2025 21:00

[fix] Updated the Echarts rendering

a0cfae4

Updated the Echarts rendering logic to wrok with

[fix] Updated the Echarts rendering

597411a

Updated the Echarts rendering logic to wrok with

Merge Conflicts

77c295b

Merge pull request #394 from cestercian/Streamline-Map-Rendering

f3a763b

[fix] Updated the Echarts rendering

nemesifier changed the title ~~Stramlining map rendering~~ [change] Uniform geojson map rendering Jul 14, 2025

nemesifier assigned cestercian Jul 14, 2025

nemesifier added the enhancement label Jul 14, 2025

nemesifier requested changes Jul 14, 2025

View reviewed changes

cestercian added 13 commits July 14, 2025 23:52

[fix] Updated Test Cases

22097e4

[fix] Removed Marker-Cluster Dependency

59bf856

Merge pull request #399 from cestercian/Uniform-GeoJSON-map-rendering

af548a7

[fix] Updated Test Cases

[fix] Removed Marker-Cluster Dependency

ddfdfc3

This reverts commit 59bf856.

[fix] Uniform GeoJSON map rendering

292643e

[fix] Prettier fix

8f8215d

Merge pull request #400 from cestercian/Uniform-GeoJSON-map-rendering

cd8c137

Uniform geo json map rendering

Merge branch 'stramlining-map-rendering' into Streamline-Map-Rendering

3f4d8a4

Merge pull request #401 from cestercian/Streamline-Map-Rendering

860f7b8

Streamline map rendering

[fix] QA-checks and Test cases

33cf5cd

[fix] Qa-checks again

b99c37a

[fix] Test Coverage

ba95437

nemesifier requested changes Jul 17, 2025

View reviewed changes

nemesifier mentioned this pull request Jul 17, 2025

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

Open

4 tasks

cestercian added 4 commits July 18, 2025 04:41

[chores] Squashed all commits into one

adedbe8

Merge branch 'stramlining-map-rendering' of https://github.com/openwi…

bba47c5

…sp/netjsongraph.js into stramlining-map-rendering

[change] Uniform geojson map rendering

48b36ae

Streamlines the map rendering logic for GeoJSON and NetJSON data types and removes the marker-cluster dependency. Closes #395.

Merge branch 'stramlining-map-rendering' of https://github.com/openwi…

cbdd6b0

…sp/netjsongraph.js into stramlining-map-rendering

[fix] QA-checks + yarn.lock changes

f73f161

cestercian requested a review from nemesifier July 18, 2025 00:02

[fix] Changed Config to not show Label for test cases

08361a1

nemesifier reviewed Jul 19, 2025

View reviewed changes

cestercian added 2 commits July 21, 2025 22:56

[fix] Prettier

1f8c0bb

cestercian commented Jul 21, 2025

View reviewed changes

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

cestercian commented Jul 21, 2025

View reviewed changes

cestercian requested a review from nemesifier July 21, 2025 18:15

nemesifier requested changes Jul 21, 2025

View reviewed changes

[change] Preserve GeoJSON type in NetJSONGraph

5d63609

Updates NetJSONGraph to retain 'geojson' as the public-facing type when loading GeoJSON data, instead of converting it to 'netjson'. Adjusts related rendering logic and tests to reflect this change.

cestercian requested a review from nemesifier July 22, 2025 01:45

nemesifier requested changes Jul 22, 2025

View reviewed changes

cestercian requested a review from nemesifier July 22, 2025 16:03

nemesifier requested changes Jul 22, 2025

View reviewed changes

[change] Uniform geojson map rendering #396

Are you sure you want to change the base?

[change] Uniform geojson map rendering #396

Uh oh!

Conversation

cestercian commented Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Reference to Existing Issue

Description of Changes

Screenshot

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

cestercian commented Jul 17, 2025

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 19, 2025

Choose a reason for hiding this comment

Uh oh!

cestercian Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cestercian left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

nemesifier Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

cestercian commented Jul 22, 2025

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cestercian commented Jul 12, 2025 •

edited

Loading

nemesifier left a comment •

edited

Loading