-
Notifications
You must be signed in to change notification settings - Fork 502
GeoPoint -> LatLng #466
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
GeoPoint -> LatLng #466
Conversation
…date" This reverts commit 25cb2c3.
@isaacbrodsky, I'm not sure why the website was breaking and this fixed it, as I'm not really sure what would have changed to break things... Also, I updated the |
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.
These names are a huge improvement IMO. One nice thing too is that it makes it easy for the bindings to additionally offer lngLatToCell
or cellToLngLat
in case the caller prefers.
/** @brief icosahedron face centers in lat/lon radians */ | ||
const GeoPoint faceCenterGeo[NUM_ICOSA_FACES] = { | ||
/** @brief icosahedron face centers in lat/lng radians */ | ||
const LatLng faceCenterGeo[NUM_ICOSA_FACES] = { |
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 one's super helpful, since we declare the data below positionally
List<Long> polyfill(List<GeoCoord> points, List<List<GeoCoord>> holes, int res); | ||
List<String> polyfillAddress(List<GeoCoord> points, List<List<GeoCoord>> holes, int res); | ||
List<Long> polyfill(List<LatLng> points, List<List<LatLng>> holes, int res); | ||
List<String> polyfillAddress(List<LatLng> points, List<List<LatLng>> holes, int res); |
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.
These Java ones are anticipating that the binding will change, not sure if that makes sense at this point?
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 use consistent terms in the bindings as well, so this makes sense to me.
To keep the number of changes under control, I've tried to focus this PR around the renaming of
GeoPoint
toLatLng
and generally usinglng
instead oflon
throughout the codebase. I list a few other potential related changes that we might also want to consider, but I'm proposing we do those in a separate PR.Renaming
GeoPoint
toLatLng
LinkedGeoPoint
toLinkedLatLng
(is this one a bit weird?)pointToCell
tolatLngToCell
cellToPoint
tocellToLatLng
vertexToPoint
tovertexToLatLng
lng
instead oflon
throughout the codebaseWork for follow-up PRs
LinkedGeoPolygon
?LinkedGeoLoop
?GeoLoop
?GeoPolygon
?GeoMultiPolygon
?pointDist*
tosphereDist*
?