-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
[Proof of principle] Photos heat map #807
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Arne Hamann <[email protected]>
Signed-off-by: Arne Hamann <[email protected]>
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.
Overall this is nice, but will need some touch up.
- Separate user-specific settings to enable / disable the heatmap and the clusters shown over it
- Performance needs to be benchmarked; can't load everything.
- I don't prefer the current color scheme, it seems a bit out of place
I can take care of the above but it will take a while (out travelling for the next few weeks)
const zoom = "18"; | ||
let minLat = -90; | ||
let maxLat = 90; | ||
let minLon = -180; | ||
let maxLon = 180; |
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 will kill performance on slower servers. It might make sense to change the granularity of what is fetched when the heatmap is enabled, but can't load everything.
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.
Yes I was thinking of implementing an own endpoint, which just delivers the point array of the appropriate size to cover the World. This response might additionally be cached for some time (Or until a certain number of images was changed.)
this.points = res.data.map((c) => { | ||
return [c.center[0], c.center[1], 1] | ||
}); |
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 we are loading this current screen only, this might as well be a computed prop derived from this.clusters
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.
As said in the long response. From my experience the heatmap is capable to handle much more Datapoints than the markers.
If this observation holds I think it makes sense to seperate this two data sources.
@pulsejet thanks for your fast response and good review. I agree with your comments and enjoy your holiday. I would like to add some performance observations. On the devices I tested with the heatmap was fasty able to handle on the order of 10k-100k points. This is significant more then the marker plugin. Therefore the traidoff on how much data to load might be different. And it should be sufficient to cover the World, if we can get the request to be fast on small servers. Additional getting the points in one request makes the heatmap, work smoothly on tilling and zooming, as no data has to be loaded. Still I agree that the request should respond fast and not load millions of points. Additional I agree that we should test on a raspberry pi server and a low-end smartphone. On the other hand on high latancy connection like bad mobile connection one request on initialisation might be preferable to reloading on panning the map. |
Add's a heatmap layer to show the location of the image on the map. This was suggested in #765.