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

fix: allowed LatLngBounds.center to work across world boundary & added simpleCenter #1860

Merged
merged 6 commits into from
May 26, 2024

Conversation

monsieurtanuki
Copy link
Contributor

What

  • The formula referenced for the LatLngBounds.center mentioned a [-180, +180] fix - now that fix is implemented.
  • A more intuitive LatLngBounds.simpleCenter is now implemented.

Impacted files

  • latlng_bounds.dart: fixed the computation of center according to referenced link; added a more intuitive simpleCenter method.
  • latlng_bounds_test.dart: added the tests that failed in the initial issue

Impacted files:
* `latlng_bounds.dart`: fixed the computation of `center` according to referenced link; added a more intuitive `simpleCenter` method.
* `latlng_bounds_test.dart`: added the tests that failed in the initial issue
@josxha josxha linked an issue Mar 27, 2024 that may be closed by this pull request
@JaffaKetchup
Copy link
Member

JaffaKetchup commented May 22, 2024

@josxha Did you have any other concerns about this?

One thing maybe we should consider is writing a small doc snippet to compare them and which is better for what situation.

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM, although maybe a little bit of better documentation would be good? Maybe a diagram of where the two centers would calculate different values, and which is correct to use for which situation?
Will wait for @josxha to approve before merge.

@JaffaKetchup JaffaKetchup changed the title fix: 1689 - fixed LatLngBounds.center and added simpleCenter fix: allowed LatLngBounds.center to work across world boundary & added simpleCenter May 23, 2024
@josxha
Copy link
Contributor

josxha commented May 23, 2024

The changes in this pull request are straight forward. The only two things I struggle with are the name simpleCenter. I'd prefer a more descriptive way but haven't found one ever since. And the other thing is that we should use this new center calculation for the camera, shouldn't we?

@monsieurtanuki
Copy link
Contributor Author

Regarding the naming: in the original code transformed here into dart, the description of the "complex" center is

Midpoint - This is the half-way point along a great circle path between the two points".

Copy link
Contributor

@josxha josxha left a comment

Choose a reason for hiding this comment

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

Alright 👍

@JaffaKetchup JaffaKetchup merged commit 8114a16 into fleaflet:master May 26, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG] Failure to calculate LatLngBounds.center for some points
3 participants