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

Show photos taken in the visible area of the map #376

Merged
merged 12 commits into from
Feb 9, 2023

Conversation

RaymondHuang210129
Copy link
Contributor

First of all, Thanks for your great project! This app allows me to migrate my photos from Google to my self-hosted NAS without compromising the user experience.

I like the location feature in Google Photos because I can locate my travel photos by dragging the map to specific places.
Thus I would like to contribute some codes to make this feature available in this app.

I added another page called 'Locations' which shows the map on the top, and the timeline shows the photos taken within the visible range in the map.

I guess the only way to change schema automatically is to have the app updated and run the migration script, so I manually added two columns (latitude, longitude) to the table oc_memories and runs the below query to fill each row:

ALTER TABLE oc_memories ADD latitude double DEFAULT NULL;
ALTER TABLE oc_memories ADD longitude double DEFAULT NULL;
UPDATE IGNORE oc_memories SET longitude = IF(exif REGEXP '(?<=GPSLatitude\":).*?(?=,)' AND exif REGEXP '(?<=GPSLongitude\":).*?(?=,)', REGEXP_SUBSTR(exif, '(?<=GPSLongitude\":).*?(?=,)') + 0.0, NULL);
UPDATE IGNORE oc_memories SET latitude = IF(exif REGEXP '(?<=GPSLatitude\":).*?(?=,)' AND exif REGEXP '(?<=GPSLongitude\":).*?(?=,)', REGEXP_SUBSTR(exif, '(?<=GPSLatitude\":).*?(?=,)') + 0.0, NULL);

The IGNORE statement results from the photos with malformed GPSLatitude/Longitude values taken by some devices (my old HTC phone treats it as string type instead of rational64u when the value is close to 0).

In my test bed with 13k photos, The webpage does not slow down after applying the geometric filter.

I did not implement the marker clustering feature on the map yet, since client-side clustering requires all marker information sent to the frontend and it could cause bad performance (as discussed in #121 ). I didn't find any server-side clustering library for PHP yet, so I think I should implement this feature in the next step.

BTW, My VSCode linter doesn't seem to match your coding style. Could you provide such configurations in README? Thanks a lot.

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.

Hi @RaymondHuang210129, sorry for the delay in reviewing this.

I added another page called 'Locations' which shows the map on the top, and the timeline shows the photos taken within the visible range in the map.

Overall this seems like a good start for implementing the maps feature. I'd like to note here that this will have to be hidden behind a "experimental" flag until the rest of the features are completed: otherwise it'll be confusing UX.

I guess the only way to change schema automatically is to have the app updated and run the migration script, so I manually added two columns (latitude, longitude) to the table oc_memories and runs the below query to fill each row:

ALTER TABLE oc_memories ADD latitude double DEFAULT NULL;
ALTER TABLE oc_memories ADD longitude double DEFAULT NULL;
UPDATE IGNORE oc_memories SET longitude = IF(exif REGEXP '(?<=GPSLatitude\":).*?(?=,)' AND exif REGEXP '(?<=GPSLongitude\":).*?(?=,)', REGEXP_SUBSTR(exif, '(?<=GPSLongitude\":).*?(?=,)') + 0.0, NULL);
UPDATE IGNORE oc_memories SET latitude = IF(exif REGEXP '(?<=GPSLatitude\":).*?(?=,)' AND exif REGEXP '(?<=GPSLongitude\":).*?(?=,)', REGEXP_SUBSTR(exif, '(?<=GPSLatitude\":).*?(?=,)') + 0.0, NULL);

The IGNORE statement results from the photos with malformed GPSLatitude/Longitude values taken by some devices (my old HTC phone treats it as string type instead of rational64u when the value is close to 0).

I would not worry about this. Asking users to re-run indexing after a major upgrade is fine.

In my test bed with 13k photos, The webpage does not slow down after applying the geometric filter.

I don't expect it to. One thing to check would be the query response times (compared to the plain timeline queries). If everything is okay they should be very similar.

I did not implement the marker clustering feature on the map yet, since client-side clustering requires all marker information sent to the frontend and it could cause bad performance (as discussed in #121 ). I didn't find any server-side clustering library for PHP yet, so I think I should implement this feature in the next step.

Sounds good.

BTW, My VSCode linter doesn't seem to match your coding style. Could you provide such configurations in README? Thanks a lot.

Just use Prettier for Vue and TS. For PHP, you need to run make php-lint

lib/Controller/LocationsController.php Outdated Show resolved Hide resolved
lib/Db/TimelineQueryDays.php Outdated Show resolved Hide resolved
@@ -5,7 +5,7 @@
:class="{ 'icon-loading': loading > 0 }"
>
<!-- Static top matter -->
<TopMatter ref="topmatter" />
<TopMatter ref="topmatter" @updateBoundary="updateBoundary" />
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is supposed to be generic. Maybe there's some better way to achieve this.

On a side note, I'm thinking a three column view might be better UX on desktop. That might need more thinking implementation-wise.

@pulsejet pulsejet added the feature New feature or request label Jan 31, 2023
@RaymondHuang210129
Copy link
Contributor Author

Refactored the code as a transform filter.

@pulsejet
Copy link
Owner

pulsejet commented Feb 8, 2023

@RaymondHuang210129 I should've dropped a note earlier: I'm working on something similar right now for map clustering. Thankfully I think there's not a lot of conflict yet so I'm going to try merging in your code locally and continue from there. But I'll suggest putting this on hold for the time being to avoid duplicate efforts.

@RaymondHuang210129
Copy link
Contributor Author

@pulsejet Got it, thanks! I will wait for your merging process. Sorry for knowing this message late.

@pulsejet pulsejet mentioned this pull request Feb 8, 2023
9 tasks
@pulsejet pulsejet closed this in #396 Feb 9, 2023
@pulsejet pulsejet merged commit 4d94353 into pulsejet:master Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants