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

Add location information to timeline #59

Merged
merged 8 commits into from
Aug 12, 2023
Merged

Conversation

zach-capalbo
Copy link
Contributor

Photofield is great! But I sometimes forget where I've taken my photos, so I've added some code to extract the GPS coordinates from the EXIF data, and use rgeo to lookup the location. Coordinates and location are stored in the database. Unique locations are displayed on the timeline events next to the date.

image

I'm submitting this PR in case this is useful or helpful. Feel free to accept it or not if it aligns with the goals of the photofield project. Just some notes about this implementation:

  • I've added the location to the event header for timeline view (since that's the one I usually use), but not to any of the others. Should be easy enough to do though, following the same pattern.
  • rgeo is very fast and does not seem to add a noticeable delay in indexing metadata, however I have not benchmarked it
  • The lookups provided by the builtin rgeo databases are not terribly precise
  • The size of the photofield executable jumps to about 60MB (Windows x64) with these changes, probably due to the added geographic lookup data in rgeo. (Either that, or I'm missing some optimization or other in my build; I'm not a heavy go user)

Anyway, hope this is helpful in some way!

@SmilyOrg
Copy link
Owner

Hey, thanks a lot for contributing! I've been meaning to add something like this, just haven't gotten around to it yet!

I'll take a look at the code when I get the chance, but it looks good from a first glance.

One interesting synergy might also be with the tagging system I've been trialing in a branch. The reverse geo could be used to create tags like "city:Dublin", "country:Ireland", etc. Would make it neat for search.

Copy link
Owner

@SmilyOrg SmilyOrg left a comment

Choose a reason for hiding this comment

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

Hey, thanks again for looking into this! I left some comments if you'd like to work on it more, otherwise I can also help once I have some other pieces in place :)

@@ -0,0 +1,3 @@
ALTER TABLE infos DROP COLUMN "longitude" text;
ALTER TABLE infos DROP COLUMN "latitude" text;
ALTER TABLE infos DROP COLUMN "location" text;
Copy link
Owner

Choose a reason for hiding this comment

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

rgeo seems to be fast enough that the location text itself might not even need to be cached? In my quick profiling the reverse geocoding almost doesn't show up. Instead it could happen at "layout-time".

@@ -161,6 +164,15 @@ func NewSource(config Config, migrations embed.FS, migrationsThumbs embed.FS) *S
source.database = NewDatabase(filepath.Join(config.DataDir, "photofield.cache.db"), migrations)
source.imageInfoCache = newInfoCache()
source.pathCache = newPathCache()

r, err := rgeo.New(rgeo.Provinces10, rgeo.Cities10)
Copy link
Owner

Choose a reason for hiding this comment

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

This takes about half a minute to init on my (admittedly quite old) PC.

Maybe a good idea would be to load it async (e.g. in a goroutine) just so that restarts don't take that long?

Alternatively rgeo could be extended to use a sqlite file instead of JSON for the loading, or maybe even for the queries itself. Should be way faster :)


if err != nil {
// Handle error
fmt.Println("RGEO ERR", err)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
fmt.Println("RGEO ERR", err)
log.Fatalf("failed to init rgeo: %s", err)

@@ -103,6 +105,8 @@ func LayoutTimeline(layout Layout, collection collection.Collection, scene *rend
Interval: 1 * time.Second,
}

locations := make(map[string]bool)
Copy link
Owner

Choose a reason for hiding this comment

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

The dedup here is pretty good already, but it can still be a bit verbose at times. One idea might also be to only list the different cities if they are all in the same country, or different countries or regions if not. That might take a bit to figure out though, maybe better left for v2 :)

Comment on lines 1 to 3
ALTER TABLE infos DROP COLUMN "longitude" text;
ALTER TABLE infos DROP COLUMN "latitude" text;
ALTER TABLE infos DROP COLUMN "location" text;
Copy link
Owner

@SmilyOrg SmilyOrg May 1, 2023

Choose a reason for hiding this comment

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

Types are not needed while dropping columns.

Suggested change
ALTER TABLE infos DROP COLUMN "longitude" text;
ALTER TABLE infos DROP COLUMN "latitude" text;
ALTER TABLE infos DROP COLUMN "location" text;
ALTER TABLE infos DROP COLUMN "longitude";
ALTER TABLE infos DROP COLUMN "latitude";
ALTER TABLE infos DROP COLUMN "location";

@SmilyOrg SmilyOrg mentioned this pull request May 3, 2023
@zach-capalbo
Copy link
Contributor Author

Thanks for taking a look!! I'm not sure that I'll have much time in the next few weeks to get back to this, so please feel free to take it over and combine it with other features that you're working on. If I get some time before then, I'll make the changes to address your comments

* Use standard LatLng for storing geolocation
* Fix migrations to take into account latest main
* Better location text construction
@SmilyOrg
Copy link
Owner

SmilyOrg commented Aug 3, 2023

@zach-capalbo I finally took some time to make the changes above! I still think the rgeo initialization time is a bit of a non-starter to be able to merge this as is, but it should be fine if it's behind a feature flag. In fact, lemme do that...

@SmilyOrg SmilyOrg merged commit 013ee69 into SmilyOrg:main Aug 12, 2023
1 check passed
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.

None yet

2 participants