Skip to content

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

Merged
merged 77 commits into from
Jun 5, 2021
Merged

GeoPoint -> LatLng #466

merged 77 commits into from
Jun 5, 2021

Conversation

ajfriend
Copy link
Contributor

@ajfriend ajfriend commented May 19, 2021

To keep the number of changes under control, I've tried to focus this PR around the renaming of GeoPoint to LatLng and generally using lng instead of lon 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 to LatLng
  • LinkedGeoPoint to LinkedLatLng (is this one a bit weird?)
  • pointToCell to latLngToCell
  • cellToPoint to cellToLatLng
  • vertexToPoint to vertexToLatLng
  • generally use lng instead of lon throughout the codebase

Work for follow-up PRs

  • LinkedGeoPolygon?
  • LinkedGeoLoop?
  • GeoLoop?
  • GeoPolygon?
  • GeoMultiPolygon?
  • pointDist* to sphereDist*?
  • other various uses of "point" and "geo" throughout the codebase?

@ajfriend ajfriend marked this pull request as draft May 19, 2021 02:46
@coveralls
Copy link

coveralls commented May 19, 2021

Coverage Status

Coverage decreased (-0.0003%) to 98.991% when pulling b8e018e on ajfriend:latlng_code into 9ab1053 on uber:master.

@ajfriend
Copy link
Contributor Author

@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 js live examples to use the 4.0 names, but those will be broken on the website until we release 4.0 for JS.

@ajfriend ajfriend requested a review from isaacbrodsky May 31, 2021 02:34
Copy link
Collaborator

@nrabinowitz nrabinowitz left a 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] = {
Copy link
Collaborator

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);
Copy link
Collaborator

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?

Copy link
Collaborator

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.

@ajfriend ajfriend merged commit 32b6e07 into uber:master Jun 5, 2021
@ajfriend ajfriend deleted the latlng_code branch June 5, 2021 02:21
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.

5 participants