Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tacruc
Copy link

@tacruc tacruc commented Sep 6, 2023

Add's a heatmap layer to show the location of the image on the map. This was suggested in #765.

Screenshot_20230906_234018-1

Signed-off-by: Arne Hamann <git@arne.email>
Signed-off-by: Arne Hamann <git@arne.email>
Copy link
Owner

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

  1. Separate user-specific settings to enable / disable the heatmap and the clusters shown over it
  2. Performance needs to be benchmarked; can't load everything.
  3. 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)

Comment on lines +263 to +267
const zoom = "18";
let minLat = -90;
let maxLat = 90;
let minLon = -180;
let maxLon = 180;
Copy link
Owner

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.

Copy link
Author

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

Comment on lines +285 to +287
this.points = res.data.map((c) => {
return [c.center[0], c.center[1], 1]
});
Copy link
Owner

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

Copy link
Author

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.

@tacruc
Copy link
Author

tacruc commented Sep 9, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants