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

Templatize Point2/PointLL #2429

Merged
merged 28 commits into from
Jun 30, 2020
Merged

Templatize Point2/PointLL #2429

merged 28 commits into from
Jun 30, 2020

Conversation

danpat
Copy link
Member

@danpat danpat commented Jun 18, 2020

Issue

The Point2 class, (and PointLL that is subclassed from it) is hard-coded to use float.

Most of the time, this is fine. But sometimes, you want to do something that requires a bit more precision. In my case, I'm splitting geometry using midgard::trim_polyline.

This leads to creating new points along existing lines. When using float, practical precision is limited to about 0.5m wrt geospatial coordinates. Most of the time this is fine.

However, in my case, I'm rendering a map with the original geometry, plus some newly injected intermediate points created by midgard::trim_polyline.

When float precision is used, sometimes the error margin becomes visible in weird ways, like in the circled section in the following screenshot:

Screen Shot 2020-06-10 at 7 58 31 am

Using double for the newly created types has enough precision that they are "on the line" for all human-practical detail levels - float does not, and half-metre errors do become visible in some real-world contexts.

Long story short - this PR turns Point2 into a template class, and makes all the necessary changes to support that. Most code remains unchanged, and float precision remains the default, but this refactor allows you to optionally choose PointXY<double> or GeoPoint<double> (for spherical Distance, etc) if you need the extra detail for some reason.

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Generally use squash merge to rebase and clean comments before merging
  • Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@danpat danpat changed the title Templatize Point2 WIP: Templatize Point2 Jun 18, 2020
@danpat danpat changed the title WIP: Templatize Point2 Templatize Point2/PointLL Jun 26, 2020
@danpat danpat marked this pull request as ready for review June 26, 2020 08:27
@danpat
Copy link
Member Author

danpat commented Jun 26, 2020

@kevinkreiser @purew This is ready for a round of 👀

src/midgard/polyline2.cc Outdated Show resolved Hide resolved
purew
purew previously approved these changes Jun 30, 2020
Copy link
Contributor

@purew purew left a comment

Choose a reason for hiding this comment

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

LGTM 🍰

@kevinkreiser kevinkreiser merged commit 8686efe into master Jun 30, 2020
@purew purew deleted the danpat_point2_refactor branch June 30, 2020 21:04
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.

3 participants