-
Notifications
You must be signed in to change notification settings - Fork 246
feat(photo-processing): implement location processing and geocoding system #95
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
base: main
Are you sure you want to change the base?
Conversation
…ystem Add comprehensive location processing pipeline including EXIF GPS extraction, coordinate validation, and Nominatim reverse geocoding with caching support. Enhance photo metadata with location data and improve ExifPanel display. Add geocode cache file to .gitignore to exclude generated data from tracking.
- Updated Nominatim reverse geocoding to include a display name in the response. - Modified the ExifPanel to conditionally show detailed location based on the new environment variable. - Adjusted location formatting logic to accommodate East Asian language order. - Added displayName field to relevant types and processing functions for improved location representation.
|
|
||
| NOMINATIM_ACCEPT_LANGUAGE=en | ||
| VITE_SHOW_DETAILED_LOCATION=true |
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.
consider using config.json instead of env? https://github.com/Afilmory/afilmory/blob/main/config.example.json
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.
or builder.config.json
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.
we should remove this since we already use config.json
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.
use builder.config.json instead
| @@ -0,0 +1,36 @@ | |||
| import type { PickedExif } from '../../types/photo.js' | |||
|
|
|||
| export function convertExifGPSToDecimal( | |||
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.
i remember there is already a function for this
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.
Can we use convertExifGPSToDecimal in apps/web/src/lib/map-utils.ts ?
| if (existing) return existing | ||
|
|
||
| const p = schedule(async () => { | ||
| const url = `https://nominatim.openstreetmap.org/reverse?lat=${encodeURIComponent( |
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.
https://operations.osmfoundation.org/policies/nominatim/ nominatim requires a 1 qps limit for its api
- Introduced geocoding settings in builder and site configurations, allowing for customizable language preferences. - Updated ExifPanel to utilize site configuration for displaying detailed location based on user settings. - Enhanced example configuration files to reflect new options for geocoding and UI display preferences.
|
I have fixed the above issues. Also, the frequency of requests to the Nominatim API has been strictly controlled. Please check the code again, thank you. |
|
|
||
| NOMINATIM_ACCEPT_LANGUAGE=en | ||
| VITE_SHOW_DETAILED_LOCATION=true |
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.
we should remove this since we already use config.json
| const isEastAsiaOrder = | ||
| activeLang.startsWith('zh') || | ||
| activeLang.startsWith('ja') || | ||
| activeLang === 'jp' |
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.
how about ko?
| const isEastAsiaOrder = | ||
| activeLang.startsWith('zh') || | ||
| activeLang.startsWith('ja') || | ||
| activeLang === 'jp' |
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.
ja or jp?
| return isEastAsiaOrder ? parts.join(' ') : parts.join(', ') | ||
| }, [currentPhoto]) |
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.
iirc in China we don't use space to separate address. like 辽宁省沈阳市. But i guess depends on the result of the api call. I'm open to discussion.
| address?: Record<string, string> | ||
| } | ||
|
|
||
| const CACHE_FILE = path.join(workdir, 'src/data/.geocode-cache.json') |
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.
TODO for reviewer: why do we need a new cache file. Any recommendation? Would it be possible to directly use manifest
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.
cc @Innei
|
|
||
| type NominatimResponse = { | ||
| display_name?: string | ||
| address?: Record<string, string> |
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.
nit: consider using a more accurate type hint for this?
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.
I think it's okay to use manifest, scanning the corresponding fields in builder can detect whether cache data exists.
| } | ||
| } | ||
|
|
||
| const roundCoord = (n: number, decimals = 3) => Number(n.toFixed(decimals)) |
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.
why 3?
| location: await processLocation( | ||
| exifData, | ||
| photoKey, | ||
| existingItem, | ||
| options, | ||
| ), |
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.
Please add an option to disable reverse geoencoding. Due to the rate limit, it would be extremely slow to build manifest for users with many photos
|
@codex review code and summary this pr changes, what feature want to implement? |
For now, I can only help with PRs you've created. |
|
@copilot review code and summary this pr changes, what feature want to implement? |
|
|
||
| NOMINATIM_ACCEPT_LANGUAGE=en | ||
| VITE_SHOW_DETAILED_LOCATION=true |
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.
use builder.config.json instead
| apps/web/assets-git | ||
| apps/web/public/thumbnails | ||
| apps/web/src/data/photos-manifest.json | ||
| apps/web/src/data/.geocode-cache.json |
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.
.geocode-cache.json should upload to remote repo cache
|
|
||
| type NominatimResponse = { | ||
| display_name?: string | ||
| address?: Record<string, string> |
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.
I think it's okay to use manifest, scanning the corresponding fields in builder can detect whether cache data exists.
Summary
This PR enhances the display of location information by showing the geographical name instead of just raw latitude and longitude coordinates. It integrates the free [Nominatim](https://nominatim.org/) API to perform reverse geocoding.
For a detailed preview of the changes, please refer to: https://vincent.ac/IMG_0306
Changes
Feature: Use the Nominatim API to fetch human-readable geographical names based on latitude and longitude.
UI Update: Locations are now displayed as descriptive names when available, improving readability and user experience.
New Environment Variables:
NOMINATIM_ACCEPT_LANGUAGE→ Sets the preferred language for location names.enVITE_SHOW_DETAILED_LOCATION→ Controls the granularity of the displayed location:true, shows detailed location names (e.g., street, neighborhood, etc.).false, only shows the city-level location.false