Reading-eScience-Centre / leaflet-coverage

A Leaflet plugin for visualizing coverages
https://covjson.org/playground/
Other
10 stars 6 forks source link

Feature/polygon series #13

Closed guygriffiths closed 2 years ago

guygriffiths commented 2 years ago

Fixes issue where PolygonSeries was not correctly plotting the colour corresponding to the selected time value.

@letmaik - Any comments on this? I'm going to keep developing on this branch to implement a MultiPolygonSeries type, since we're planning on using that in a project, but I ran into this issue with PolygonSeries and thought I should fix that first.

guygriffiths commented 2 years ago

Also, I'm having trouble getting the latest change to appear when running npm run playground. The updated code appears to be in node_modules, but it's never getting run (easily tested by putting in console log statements which I can grep for in the minified JS, but which never appear in the console). Any thoughts?

letmaik commented 2 years ago

Also, I'm having trouble getting the latest change to appear when running npm run playground. The updated code appears to be in node_modules, but it's never getting run (easily tested by putting in console log statements which I can grep for in the minified JS, but which never appear in the console). Any thoughts?

Yes, good point, this won't work without my changes to the playground from my dev branch. I haven't PR'd that yet but you can use it like that: npm run playground -- letmaik/2021. Don't forget to revert your change to run-playground.js ("dev" -> "start"). If you get a git error, just delete the existing temporary playground folder.

guygriffiths commented 2 years ago

I've modified the set time methods of the (Multi)PolygonSeries to change the fillColor of the GeoJSON directly - I was finding that polygons were disappearing with a full redraw(), and this more direct method seems to work fine.

I'm unable to get PointSeries data to work though. I'm getting a leaflet error when the circleMarker gets added to the map. This is with no changes to the code, using the PointSeries example from the CovJSON playground. Any thoughts? It looks like it's a similar error to this: https://github.com/Leaflet/Leaflet/issues/7334 but my specific call stack is:

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'x')
    at Proxy.intersects (Bounds.js:135)
    at Proxy._empty (CircleMarker.js:95)
    at Proxy._updateCircle (SVG.js:189)
    at Proxy._updatePath (CircleMarker.js:91)
    at Proxy._update (CircleMarker.js:86)
    at Proxy._reset (Path.js:140)
    at Proxy.onAdd (Path.js:85)
    at Proxy._layerAdd (Layer.js:114)
    at Proxy.whenReady (Map.js:1465)
    at Proxy.addLayer (Layer.js:176)

(all of the Proxy stuff is Vue-related, and in reality those methods are on various different classes)

I also haven't been able to get the playground working with updated changes, even with your latest branch, but that's not really a problem, since I'm wrapping the leaflet-coverage code in a Vue component for another project, and that's now working fine for live updates.

letmaik commented 2 years ago

I'm unable to get PointSeries data to work though. I'm getting a leaflet error when the circleMarker gets added to the map. This is with no changes to the code, using the PointSeries example from the CovJSON playground. Any thoughts?

I just tried your branch in the playground and it works fine, not sure. The playground branch is now merged into master, so give this another try and let me know what exactly doesn't work with npm run playground.

guygriffiths commented 2 years ago

I've made those changes as suggested. When I do npm run playground, it appears to work, and will plot MultiPolygonSeries data, but will colour all of the polygons black, and not display a time selector. The same is true for PolygonSeries data, as well as PointSeries data (both the clickable examples in the playground, and my own JSON files). I've managed to get the latest version of the playground working, and I think it's because those examples are not setting the time option in the playground code. I wonder if perhaps the default behaviour should be to select the final time in the axis if it's not specified?

I'm still getting the same issue displaying PointSeries with my Vue plugin though. It sounds like it could be related to multiple versions of leaflet getting pulled in, but as far as I can tell that's not the case. I'll investigate it further, it doesn't sound like it's a problem with leaflet-coverage, anyway.

letmaik commented 2 years ago

When I do npm run playground, it appears to work, and will plot MultiPolygonSeries data, but will colour all of the polygons black, and not display a time selector.

That's intended, since the data is displayed in a C3 time series plot when clicking on the polygon. Having a time selector without plot but colored polygon is a different visualization mechanism that's not implemented in the playground but is supported by leaflet-coverage.

guygriffiths commented 2 years ago

Ok, great, so this is the desired behaviour and it all appears to be working correctly now. Shall I go ahead and merge and release it? I'm assuming 0.8.1 but can bump it up to 0.9.0 or something first if you like.