Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

too many coordinates / bounds classes #3785

Closed
3 tasks
jfirebaugh opened this issue Feb 2, 2016 · 3 comments
Closed
3 tasks

too many coordinates / bounds classes #3785

jfirebaugh opened this issue Feb 2, 2016 · 3 comments
Assignees
Labels

Comments

@jfirebaugh
Copy link
Contributor

We represent coordinates using no less than 8 different types in different parts of the code:

  • LatLng
  • ProjectedMeters
  • PrecisionPoint
  • TileCoordinate
  • TileID
  • std::array<float, 2>
  • vec2
  • Coordinate

This is three more than we should have. We should have:

  • One type for spherical geographic coordinates (LatLng)
  • One type for projected cartesian coordinates (ProjectedMeters, but the naming is off -- this class should be unit-less)
  • One type for screen pixel coordinates (PrecisionPoint, but should be renamed to something like ScreenCoordinate)
  • One type for tile z/x/y coordinates (TileID)
  • One type for normalized vector tile coordinates (Coordinate)

So the ones to eliminate are:

  • std::array<float, 2>
  • vec2
  • TileCoordinate

For bounds, we have:

  • LatLngBounds
  • MetersBounds
  • box (I'm in the process of removing this.)
@jfirebaugh
Copy link
Contributor Author

We also have at least three duplicated and inconsistent copies of projection math:

  • In projection.hpp
  • In transform_state.cpp
  • In geo.cpp

@1ec5
Copy link
Contributor

1ec5 commented Feb 2, 2016

The projection math in TransformState is often dependent on the current scale, but it would be great to factor some of that to be based on Projection, which is more readily available to code at the SDK level. That would hep to eliminate some of the additional redundancy at the SDK level.

@brunoabinader
Copy link
Member

Good point @jfirebaugh 👍

I believe we shouldn't use vec2<T> directly (we could enforce this by e.g. enforcing a private ctor). Instead, we'd use meaningful aliases like PrecisionPoint. It seems to me that Coordinate and PrecisionPoint are correlates, so we might get rid of Coordinate. From what I see, std::array<float, 2> can also be easily transformed into a PrecisionPoint. In sum, we'd get rid of vec2<T> (directly), Coordinate and std::array<float, 2>.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants